[PATCH v3 0/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling

[PATCH v3 0/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling

[Yabin Cui]

Hi Coresight maintainers,

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. This patchset
is to fix race conditions when enabling/disabling a CATU device.

Changes since v2:
- In catu_disable(), return 0 when refcnt > 0.
- Remove the patch checking enabled mode.
- Disable helpers at the places where a coresight device fails to
  enable.

Changes since v1:
- Use raw_spinlock_t and guard().
- Add a patch to check enabled mode.
- Add a patch to disable helpers when fails to enable a device.

Yabin Cui (2):
  coresight: catu: Introduce refcount and spinlock for
    enabling/disabling
  coresight: core: Disable helpers for devices that fail to enable

 drivers/hwtracing/coresight/coresight-catu.c | 25 +++++++++++++-------
 drivers/hwtracing/coresight/coresight-catu.h |  1 +
 drivers/hwtracing/coresight/coresight-core.c |  9 +++++--
 3 files changed, 25 insertions(+), 10 deletions(-)

-- 
2.49.0.504.g3bcea36a83-goog

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

[Yabin Cui]

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..30b78b2f8adb 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);
+	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
 
-	CS_UNLOCK(catu_drvdata->base);
-	rc = catu_enable_hw(catu_drvdata, mode, data);
-	CS_LOCK(catu_drvdata->base);
+	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);
+	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
 
-	CS_UNLOCK(catu_drvdata->base);
-	rc = catu_disable_hw(catu_drvdata);
-	CS_LOCK(catu_drvdata->base);
+	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.504.g3bcea36a83-goog

[PATCH v3 2/2] coresight: core: Disable helpers for devices that fail to enable

[Yabin Cui]

When enabling a SINK or LINK type coresight device fails, the
associated helpers should be disabled.

Signed-off-by: Yabin Cui <yabinc@google.com>
Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index fb43ef6a3b1f..a56ba9087538 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -486,8 +486,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 			 * that need disabling. Disabling the path here
 			 * would mean we could disrupt an existing session.
 			 */
-			if (ret)
+			if (ret) {
+				coresight_disable_helpers(csdev);
 				goto out;
+			}
 			break;
 		case CORESIGHT_DEV_TYPE_SOURCE:
 			/* sources are enabled from either sysFS or Perf */
@@ -496,10 +498,13 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 			parent = list_prev_entry(nd, link)->csdev;
 			child = list_next_entry(nd, link)->csdev;
 			ret = coresight_enable_link(csdev, parent, child, source);
-			if (ret)
+			if (ret) {
+				coresight_disable_helpers(csdev);
 				goto err;
+			}
 			break;
 		default:
+			coresight_disable_helpers(csdev);
 			goto err;
 		}
 	}
-- 
2.49.0.504.g3bcea36a83-goog

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

[James Clark]

On 08/04/2025 8:59 pm, 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>
> ---
>   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..30b78b2f8adb 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);
> +	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
>   

Very minor nit only because you need to resend anyway, but there should 
be a newline between the variable definitions and the code. Not sure why 
checkpatch doesn't warn here.

> -	CS_UNLOCK(catu_drvdata->base);
> -	rc = catu_enable_hw(catu_drvdata, mode, data);
> -	CS_LOCK(catu_drvdata->base);
> +	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);
> +	guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock);
>   
> -	CS_UNLOCK(catu_drvdata->base);
> -	rc = catu_disable_hw(catu_drvdata);
> -	CS_LOCK(catu_drvdata->base);
> +	if (--csdev->refcnt == 0) {

Hopefully this never underflows if disable is called again after a 
failed enable. We could add a WARN_ON() but I think this is a general 
case and not specific to these patches so is probably better to do later 
as separate change.

Reviewed-by: James Clark <james.clark@linaro.org>

Re: [PATCH v3 2/2] coresight: core: Disable helpers for devices that fail to enable

[James Clark]

On 08/04/2025 8:59 pm, Yabin Cui wrote:
> When enabling a SINK or LINK type coresight device fails, the
> associated helpers should be disabled.
> 
> Signed-off-by: Yabin Cui <yabinc@google.com>
> Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fb43ef6a3b1f..a56ba9087538 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -486,8 +486,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>   			 * that need disabling. Disabling the path here
>   			 * would mean we could disrupt an existing session.
>   			 */
> -			if (ret)
> +			if (ret) {
> +				coresight_disable_helpers(csdev);

Hi Yabin,

Unfortunately coresight_disable_helpers() takes a path pointer now so 
this needs to be updated.

I tested with that change made and it works ok.

>   				goto out;
> +			}
>   			break;
>   		case CORESIGHT_DEV_TYPE_SOURCE:
>   			/* sources are enabled from either sysFS or Perf */
> @@ -496,10 +498,13 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>   			parent = list_prev_entry(nd, link)->csdev;
>   			child = list_next_entry(nd, link)->csdev;
>   			ret = coresight_enable_link(csdev, parent, child, source);
> -			if (ret)
> +			if (ret) {
> +				coresight_disable_helpers(csdev);
>   				goto err;
> +			}
>   			break;
>   		default:
> +			coresight_disable_helpers(csdev);

Minor nit, you could collapse these last two into "goto 
err_disable_helpers" and add another label before err: that disables 
helpers before falling through to err:.

Other than that:

Reviewed-by: James Clark <james.clark@linaro.org>

>   			goto err;
>   		}
>   	}

Re: [PATCH v3 2/2] coresight: core: Disable helpers for devices that fail to enable

[Mike Leach]

On Tue, 15 Apr 2025 at 14:51, James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 08/04/2025 8:59 pm, Yabin Cui wrote:
> > When enabling a SINK or LINK type coresight device fails, the
> > associated helpers should be disabled.
> >
> > Signed-off-by: Yabin Cui <yabinc@google.com>
> > Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > ---
> >   drivers/hwtracing/coresight/coresight-core.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index fb43ef6a3b1f..a56ba9087538 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -486,8 +486,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >                        * that need disabling. Disabling the path here
> >                        * would mean we could disrupt an existing session.
> >                        */
> > -                     if (ret)
> > +                     if (ret) {
> > +                             coresight_disable_helpers(csdev);
>
> Hi Yabin,
>
> Unfortunately coresight_disable_helpers() takes a path pointer now so
> this needs to be updated.
>
> I tested with that change made and it works ok.
>
> >                               goto out;
> > +                     }
> >                       break;
> >               case CORESIGHT_DEV_TYPE_SOURCE:
> >                       /* sources are enabled from either sysFS or Perf */
> > @@ -496,10 +498,13 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >                       parent = list_prev_entry(nd, link)->csdev;
> >                       child = list_next_entry(nd, link)->csdev;
> >                       ret = coresight_enable_link(csdev, parent, child, source);
> > -                     if (ret)
> > +                     if (ret) {
> > +                             coresight_disable_helpers(csdev);
> >                               goto err;
> > +                     }
> >                       break;
> >               default:
> > +                     coresight_disable_helpers(csdev);
>
> Minor nit, you could collapse these last two into "goto
> err_disable_helpers" and add another label before err: that disables
> helpers before falling through to err:.
>
> Other than that:
>
> Reviewed-by: James Clark <james.clark@linaro.org>
>
> >                       goto err;
> >               }
> >       }
>

Subject to James' comments -

Reviewed-by: Mike Leach <mike.leach@linaro.org>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v3 2/2] coresight: core: Disable helpers for devices that fail to enable

[Yabin Cui]

On Fri, Apr 25, 2025 at 8:25 AM Mike Leach <mike.leach@linaro.org> wrote:
>
> On Tue, 15 Apr 2025 at 14:51, James Clark <james.clark@linaro.org> wrote:
> >
> >
> >
> > On 08/04/2025 8:59 pm, Yabin Cui wrote:
> > > When enabling a SINK or LINK type coresight device fails, the
> > > associated helpers should be disabled.
> > >
> > > Signed-off-by: Yabin Cui <yabinc@google.com>
> > > Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > ---
> > >   drivers/hwtracing/coresight/coresight-core.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > > index fb43ef6a3b1f..a56ba9087538 100644
> > > --- a/drivers/hwtracing/coresight/coresight-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > > @@ -486,8 +486,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > >                        * that need disabling. Disabling the path here
> > >                        * would mean we could disrupt an existing session.
> > >                        */
> > > -                     if (ret)
> > > +                     if (ret) {
> > > +                             coresight_disable_helpers(csdev);
> >
> > Hi Yabin,
> >
> > Unfortunately coresight_disable_helpers() takes a path pointer now so
> > this needs to be updated.
> >
> > I tested with that change made and it works ok.
> >
> > >                               goto out;
> > > +                     }
> > >                       break;
> > >               case CORESIGHT_DEV_TYPE_SOURCE:
> > >                       /* sources are enabled from either sysFS or Perf */
> > > @@ -496,10 +498,13 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > >                       parent = list_prev_entry(nd, link)->csdev;
> > >                       child = list_next_entry(nd, link)->csdev;
> > >                       ret = coresight_enable_link(csdev, parent, child, source);
> > > -                     if (ret)
> > > +                     if (ret) {
> > > +                             coresight_disable_helpers(csdev);
> > >                               goto err;
> > > +                     }
> > >                       break;
> > >               default:
> > > +                     coresight_disable_helpers(csdev);
> >
> > Minor nit, you could collapse these last two into "goto
> > err_disable_helpers" and add another label before err: that disables
> > helpers before falling through to err:.
> >
> > Other than that:
> >
> > Reviewed-by: James Clark <james.clark@linaro.org>
> >
> > >                       goto err;
> > >               }
> > >       }
> >
>
> Subject to James' comments -
>
> Reviewed-by: Mike Leach <mike.leach@linaro.org>
>

Hi Mike, I have uploaded the v4 patch as suggested by James, in
"[PATCH v4 2/2] coresight: core: Disable helpers for devices that fail
to enable".
Could you help review the v4 patch? In that patch, Leo suggested consolidating
error handling. But you expressed concern about it when reviewing the v2 patch.


>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK