[v4] coresight: catu: Introduce refcount and spinlock for enabling/disabling

[PATCH v4 1/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling

Posted by Yabin Cui 8 months, 1 week ago

When tracing ETM data on multiple CPUs concurrently via the
perf interface, the CATU device is shared across different CPU
paths. This can lead to race conditions when multiple CPUs attempt
to enable or disable the CATU device simultaneously.

To address these race conditions, this patch introduces the
following changes:

1. The enable and disable operations for the CATU device are not
   reentrant. Therefore, a spinlock is added to ensure that only
   one CPU can enable or disable a given CATU device at any point
   in time.

2. A reference counter is used to manage the enable/disable state
   of the CATU device. The device is enabled when the first CPU
   requires it and is only disabled when the last CPU finishes
   using it. This ensures the device remains active as long as at
   least one CPU needs it.

Signed-off-by: Yabin Cui <yabinc@google.com>
---
 drivers/hwtracing/coresight/coresight-catu.c | 25 +++++++++++++-------
 drivers/hwtracing/coresight/coresight-catu.h |  1 +
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index fa170c966bc3..3909b562b077 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -458,12 +458,17 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
 static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
 		       void *data)
 {
-	int rc;
+	int rc = 0;
 	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
 
-	CS_UNLOCK(catu_drvdata->base);
-	rc = catu_enable_hw(catu_drvdata, mode, data);
-	CS_LOCK(catu_drvdata->base);
+	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
+	if (csdev->refcnt == 0) {
+		CS_UNLOCK(catu_drvdata->base);
+		rc = catu_enable_hw(catu_drvdata, mode, data);
+		CS_LOCK(catu_drvdata->base);
+	}
+	if (!rc)
+		csdev->refcnt++;
 	return rc;
 }
 
@@ -486,12 +491,15 @@ static int catu_disable_hw(struct catu_drvdata *drvdata)
 
 static int catu_disable(struct coresight_device *csdev, void *__unused)
 {
-	int rc;
+	int rc = 0;
 	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
 
-	CS_UNLOCK(catu_drvdata->base);
-	rc = catu_disable_hw(catu_drvdata);
-	CS_LOCK(catu_drvdata->base);
+	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
+	if (--csdev->refcnt == 0) {
+		CS_UNLOCK(catu_drvdata->base);
+		rc = catu_disable_hw(catu_drvdata);
+		CS_LOCK(catu_drvdata->base);
+	}
 	return rc;
 }
 
@@ -550,6 +558,7 @@ static int __catu_probe(struct device *dev, struct resource *res)
 	dev->platform_data = pdata;
 
 	drvdata->base = base;
+	raw_spin_lock_init(&drvdata->spinlock);
 	catu_desc.access = CSDEV_ACCESS_IOMEM(base);
 	catu_desc.pdata = pdata;
 	catu_desc.dev = dev;
diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
index 141feac1c14b..755776cd19c5 100644
--- a/drivers/hwtracing/coresight/coresight-catu.h
+++ b/drivers/hwtracing/coresight/coresight-catu.h
@@ -65,6 +65,7 @@ struct catu_drvdata {
 	void __iomem *base;
 	struct coresight_device *csdev;
 	int irq;
+	raw_spinlock_t spinlock;
 };
 
 #define CATU_REG32(name, offset)					\
-- 
2.49.0.604.gff1f9ca942-goog

Re: [PATCH v4 1/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling

Posted by Leo Yan 8 months ago

Hi Yabin,

Sorry for late reply as I was on vacation for the past two weeks.

On Tue, Apr 15, 2025 at 11:46:48AM -0700, Yabin Cui wrote:
> When tracing ETM data on multiple CPUs concurrently via the
> perf interface, the CATU device is shared across different CPU
> paths. This can lead to race conditions when multiple CPUs attempt
> to enable or disable the CATU device simultaneously.
> 
> To address these race conditions, this patch introduces the
> following changes:
> 
> 1. The enable and disable operations for the CATU device are not
>    reentrant. Therefore, a spinlock is added to ensure that only
>    one CPU can enable or disable a given CATU device at any point
>    in time.
> 
> 2. A reference counter is used to manage the enable/disable state
>    of the CATU device. The device is enabled when the first CPU
>    requires it and is only disabled when the last CPU finishes
>    using it. This ensures the device remains active as long as at
>    least one CPU needs it.
> 
> Signed-off-by: Yabin Cui <yabinc@google.com>

LGTM:

Reviewed-by: Leo Yan <leo.yan@arm.com>

As Jie reminded, please add James' review tag in next spin.

Thanks,
Leo

> ---
>  drivers/hwtracing/coresight/coresight-catu.c | 25 +++++++++++++-------
>  drivers/hwtracing/coresight/coresight-catu.h |  1 +
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index fa170c966bc3..3909b562b077 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -458,12 +458,17 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
>  static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
>  		       void *data)
>  {
> -	int rc;
> +	int rc = 0;
>  	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
>  
> -	CS_UNLOCK(catu_drvdata->base);
> -	rc = catu_enable_hw(catu_drvdata, mode, data);
> -	CS_LOCK(catu_drvdata->base);
> +	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
> +	if (csdev->refcnt == 0) {
> +		CS_UNLOCK(catu_drvdata->base);
> +		rc = catu_enable_hw(catu_drvdata, mode, data);
> +		CS_LOCK(catu_drvdata->base);
> +	}
> +	if (!rc)
> +		csdev->refcnt++;
>  	return rc;
>  }
>  
> @@ -486,12 +491,15 @@ static int catu_disable_hw(struct catu_drvdata *drvdata)
>  
>  static int catu_disable(struct coresight_device *csdev, void *__unused)
>  {
> -	int rc;
> +	int rc = 0;
>  	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
>  
> -	CS_UNLOCK(catu_drvdata->base);
> -	rc = catu_disable_hw(catu_drvdata);
> -	CS_LOCK(catu_drvdata->base);
> +	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
> +	if (--csdev->refcnt == 0) {
> +		CS_UNLOCK(catu_drvdata->base);
> +		rc = catu_disable_hw(catu_drvdata);
> +		CS_LOCK(catu_drvdata->base);
> +	}
>  	return rc;
>  }
>  
> @@ -550,6 +558,7 @@ static int __catu_probe(struct device *dev, struct resource *res)
>  	dev->platform_data = pdata;
>  
>  	drvdata->base = base;
> +	raw_spin_lock_init(&drvdata->spinlock);
>  	catu_desc.access = CSDEV_ACCESS_IOMEM(base);
>  	catu_desc.pdata = pdata;
>  	catu_desc.dev = dev;
> diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
> index 141feac1c14b..755776cd19c5 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.h
> +++ b/drivers/hwtracing/coresight/coresight-catu.h
> @@ -65,6 +65,7 @@ struct catu_drvdata {
>  	void __iomem *base;
>  	struct coresight_device *csdev;
>  	int irq;
> +	raw_spinlock_t spinlock;
>  };
>  
>  #define CATU_REG32(name, offset)					\
> -- 
> 2.49.0.604.gff1f9ca942-goog
>

Re: [PATCH v4 1/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling

Posted by Yabin Cui 8 months ago

On Wed, Apr 23, 2025 at 1:16 AM Leo Yan <leo.yan@arm.com> wrote:
>
> Hi Yabin,
>
> Sorry for late reply as I was on vacation for the past two weeks.
>
> On Tue, Apr 15, 2025 at 11:46:48AM -0700, Yabin Cui wrote:
> > When tracing ETM data on multiple CPUs concurrently via the
> > perf interface, the CATU device is shared across different CPU
> > paths. This can lead to race conditions when multiple CPUs attempt
> > to enable or disable the CATU device simultaneously.
> >
> > To address these race conditions, this patch introduces the
> > following changes:
> >
> > 1. The enable and disable operations for the CATU device are not
> >    reentrant. Therefore, a spinlock is added to ensure that only
> >    one CPU can enable or disable a given CATU device at any point
> >    in time.
> >
> > 2. A reference counter is used to manage the enable/disable state
> >    of the CATU device. The device is enabled when the first CPU
> >    requires it and is only disabled when the last CPU finishes
> >    using it. This ensures the device remains active as long as at
> >    least one CPU needs it.
> >
> > Signed-off-by: Yabin Cui <yabinc@google.com>
>
> LGTM:
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
>
> As Jie reminded, please add James' review tag in next spin.

Sorry for forgetting to add Reviewed-by. Will add it in next spin.

>
> Thanks,
> Leo
>
> > ---
> >  drivers/hwtracing/coresight/coresight-catu.c | 25 +++++++++++++-------
> >  drivers/hwtracing/coresight/coresight-catu.h |  1 +
> >  2 files changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> > index fa170c966bc3..3909b562b077 100644
> > --- a/drivers/hwtracing/coresight/coresight-catu.c
> > +++ b/drivers/hwtracing/coresight/coresight-catu.c
> > @@ -458,12 +458,17 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
> >  static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
> >                      void *data)
> >  {
> > -     int rc;
> > +     int rc = 0;
> >       struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
> >
> > -     CS_UNLOCK(catu_drvdata->base);
> > -     rc = catu_enable_hw(catu_drvdata, mode, data);
> > -     CS_LOCK(catu_drvdata->base);
> > +     guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
> > +     if (csdev->refcnt == 0) {
> > +             CS_UNLOCK(catu_drvdata->base);
> > +             rc = catu_enable_hw(catu_drvdata, mode, data);
> > +             CS_LOCK(catu_drvdata->base);
> > +     }
> > +     if (!rc)
> > +             csdev->refcnt++;
> >       return rc;
> >  }
> >
> > @@ -486,12 +491,15 @@ static int catu_disable_hw(struct catu_drvdata *drvdata)
> >
> >  static int catu_disable(struct coresight_device *csdev, void *__unused)
> >  {
> > -     int rc;
> > +     int rc = 0;
> >       struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
> >
> > -     CS_UNLOCK(catu_drvdata->base);
> > -     rc = catu_disable_hw(catu_drvdata);
> > -     CS_LOCK(catu_drvdata->base);
> > +     guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
> > +     if (--csdev->refcnt == 0) {
> > +             CS_UNLOCK(catu_drvdata->base);
> > +             rc = catu_disable_hw(catu_drvdata);
> > +             CS_LOCK(catu_drvdata->base);
> > +     }
> >       return rc;
> >  }
> >
> > @@ -550,6 +558,7 @@ static int __catu_probe(struct device *dev, struct resource *res)
> >       dev->platform_data = pdata;
> >
> >       drvdata->base = base;
> > +     raw_spin_lock_init(&drvdata->spinlock);
> >       catu_desc.access = CSDEV_ACCESS_IOMEM(base);
> >       catu_desc.pdata = pdata;
> >       catu_desc.dev = dev;
> > diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
> > index 141feac1c14b..755776cd19c5 100644
> > --- a/drivers/hwtracing/coresight/coresight-catu.h
> > +++ b/drivers/hwtracing/coresight/coresight-catu.h
> > @@ -65,6 +65,7 @@ struct catu_drvdata {
> >       void __iomem *base;
> >       struct coresight_device *csdev;
> >       int irq;
> > +     raw_spinlock_t spinlock;
> >  };
> >
> >  #define CATU_REG32(name, offset)                                     \
> > --
> > 2.49.0.604.gff1f9ca942-goog
> >

Re: [PATCH v4 1/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling

Posted by Jie Gan 8 months, 1 week ago


On 4/16/2025 2:46 AM, Yabin Cui wrote:
> When tracing ETM data on multiple CPUs concurrently via the
> perf interface, the CATU device is shared across different CPU
> paths. This can lead to race conditions when multiple CPUs attempt
> to enable or disable the CATU device simultaneously.
> 
> To address these race conditions, this patch introduces the
> following changes:
> 
> 1. The enable and disable operations for the CATU device are not
>     reentrant. Therefore, a spinlock is added to ensure that only
>     one CPU can enable or disable a given CATU device at any point
>     in time.
> 
> 2. A reference counter is used to manage the enable/disable state
>     of the CATU device. The device is enabled when the first CPU
>     requires it and is only disabled when the last CPU finishes
>     using it. This ensures the device remains active as long as at
>     least one CPU needs it.
> 
> Signed-off-by: Yabin Cui <yabinc@google.com>

Hi Yabin,

You missed the reviewed-by tag. James gave his tag in V3.

As well as another patchset.

Thanks,
Jie

> ---
>   drivers/hwtracing/coresight/coresight-catu.c | 25 +++++++++++++-------
>   drivers/hwtracing/coresight/coresight-catu.h |  1 +
>   2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index fa170c966bc3..3909b562b077 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -458,12 +458,17 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
>   static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
>   		       void *data)
>   {
> -	int rc;
> +	int rc = 0;
>   	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
>   
> -	CS_UNLOCK(catu_drvdata->base);
> -	rc = catu_enable_hw(catu_drvdata, mode, data);
> -	CS_LOCK(catu_drvdata->base);
> +	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
> +	if (csdev->refcnt == 0) {
> +		CS_UNLOCK(catu_drvdata->base);
> +		rc = catu_enable_hw(catu_drvdata, mode, data);
> +		CS_LOCK(catu_drvdata->base);
> +	}
> +	if (!rc)
> +		csdev->refcnt++;
>   	return rc;
>   }
>   
> @@ -486,12 +491,15 @@ static int catu_disable_hw(struct catu_drvdata *drvdata)
>   
>   static int catu_disable(struct coresight_device *csdev, void *__unused)
>   {
> -	int rc;
> +	int rc = 0;
>   	struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
>   
> -	CS_UNLOCK(catu_drvdata->base);
> -	rc = catu_disable_hw(catu_drvdata);
> -	CS_LOCK(catu_drvdata->base);
> +	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
> +	if (--csdev->refcnt == 0) {
> +		CS_UNLOCK(catu_drvdata->base);
> +		rc = catu_disable_hw(catu_drvdata);
> +		CS_LOCK(catu_drvdata->base);
> +	}
>   	return rc;
>   }
>   
> @@ -550,6 +558,7 @@ static int __catu_probe(struct device *dev, struct resource *res)
>   	dev->platform_data = pdata;
>   
>   	drvdata->base = base;
> +	raw_spin_lock_init(&drvdata->spinlock);
>   	catu_desc.access = CSDEV_ACCESS_IOMEM(base);
>   	catu_desc.pdata = pdata;
>   	catu_desc.dev = dev;
> diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
> index 141feac1c14b..755776cd19c5 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.h
> +++ b/drivers/hwtracing/coresight/coresight-catu.h
> @@ -65,6 +65,7 @@ struct catu_drvdata {
>   	void __iomem *base;
>   	struct coresight_device *csdev;
>   	int irq;
> +	raw_spinlock_t spinlock;
>   };
>   
>   #define CATU_REG32(name, offset)					\

[PATCH v4 1/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling
[PATCH v4 2/2] coresight: core: Disable helpers for devices that fail to enable