[PATCH v2 0/3] coresight: catu: Introduce refcount and spinlock for enabling/disabling

[PATCH v2 0/3] 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 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 (3):
  coresight: catu: Introduce refcount and spinlock for
    enabling/disabling
  coresight: catu: Prevent concurrent PERF and SYSFS mode enablement
  coresight: core: Disable helpers for devices that fail to enable

 drivers/hwtracing/coresight/coresight-catu.c | 33 ++++++++++++++------
 drivers/hwtracing/coresight/coresight-catu.h |  2 ++
 drivers/hwtracing/coresight/coresight-core.c | 12 +++----
 3 files changed, 30 insertions(+), 17 deletions(-)

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v2 1/3] 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 | 27 ++++++++++++++------
 drivers/hwtracing/coresight/coresight-catu.h |  1 +
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index fa170c966bc3..b1d490dd7249 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,17 @@ 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);
+	} else {
+		rc = -EBUSY;
+	}
 	return rc;
 }
 
@@ -550,6 +560,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.472.ge94155a9ec-goog

[PATCH v2 2/3] coresight: catu: Prevent concurrent PERF and SYSFS mode enablement

[Yabin Cui]

A CATU device can be enabled for either PERF mode or SYSFS mode, but
not both simultaneously. Consider the following race condition:

CPU 1 enables CATU for PERF mode.
CPU 2 enables CATU for SYSFS mode. It only increases refcnt.
CPU 2 enables ETR for SYSFS mode.
CPU 1 fails to enable ETR for PERF mode.

This results in a CATU being enabled in PERF mode and an ETR enabled
in SYSFS mode, leading to unknown behavior.

To prevent this situation, this patch checks enabled mode of a
CATU device before increasing its refcnt.

Signed-off-by: Yabin Cui <yabinc@google.com>
Suggested-by: James Clark <james.clark@linaro.org>
Suggested-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-catu.c | 6 +++++-
 drivers/hwtracing/coresight/coresight-catu.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index b1d490dd7249..0caf3a3e75ce 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -466,7 +466,10 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
 		CS_UNLOCK(catu_drvdata->base);
 		rc = catu_enable_hw(catu_drvdata, mode, data);
 		CS_LOCK(catu_drvdata->base);
-	}
+		if (!rc)
+			catu_drvdata->mode = mode;
+	} else if (catu_drvdata->mode != mode)
+		rc = -EBUSY;
 	if (!rc)
 		csdev->refcnt++;
 	return rc;
@@ -499,6 +502,7 @@ static int catu_disable(struct coresight_device *csdev, void *__unused)
 		CS_UNLOCK(catu_drvdata->base);
 		rc = catu_disable_hw(catu_drvdata);
 		CS_LOCK(catu_drvdata->base);
+		catu_drvdata->mode = CS_MODE_DISABLED;
 	} else {
 		rc = -EBUSY;
 	}
diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
index 755776cd19c5..ea406464f0a8 100644
--- a/drivers/hwtracing/coresight/coresight-catu.h
+++ b/drivers/hwtracing/coresight/coresight-catu.h
@@ -66,6 +66,7 @@ struct catu_drvdata {
 	struct coresight_device *csdev;
 	int irq;
 	raw_spinlock_t spinlock;
+	enum cs_mode mode;
 };
 
 #define CATU_REG32(name, offset)					\
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v2 3/3] 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 | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index fb43ef6a3b1f..e3270d9b46c9 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 		/* Enable all helpers adjacent to the path first */
 		ret = coresight_enable_helpers(csdev, mode, path);
 		if (ret)
-			goto err;
+			goto err_helper;
 		/*
 		 * ETF devices are tricky... They can be a link or a sink,
 		 * depending on how they are configured.  If an ETF has been
@@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 		switch (type) {
 		case CORESIGHT_DEV_TYPE_SINK:
 			ret = coresight_enable_sink(csdev, mode, sink_data);
-			/*
-			 * Sink is the first component turned on. If we
-			 * failed to enable the sink, there are no components
-			 * that need disabling. Disabling the path here
-			 * would mean we could disrupt an existing session.
-			 */
 			if (ret)
-				goto out;
+				goto err;
 			break;
 		case CORESIGHT_DEV_TYPE_SOURCE:
 			/* sources are enabled from either sysFS or Perf */
@@ -507,6 +501,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 out:
 	return ret;
 err:
+	coresight_disable_helpers(csdev);
+err_helper:
 	coresight_disable_path_from(path, nd);
 	goto out;
 }
-- 
2.49.0.472.ge94155a9ec-goog

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

[Yabin Cui]

On Tue, Apr 1, 2025 at 6:19 PM Yabin Cui <yabinc@google.com> 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 | 27 ++++++++++++++------
>  drivers/hwtracing/coresight/coresight-catu.h |  1 +
>  2 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index fa170c966bc3..b1d490dd7249 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,17 @@ 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);
> +       } else {
> +               rc = -EBUSY;
> +       }
>         return rc;
>  }
>
> @@ -550,6 +560,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.472.ge94155a9ec-goog
>

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

[Yabin Cui]

On Tue, Apr 1, 2025 at 6:19 PM Yabin Cui <yabinc@google.com> 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 | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fb43ef6a3b1f..e3270d9b46c9 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>                 /* Enable all helpers adjacent to the path first */
>                 ret = coresight_enable_helpers(csdev, mode, path);
>                 if (ret)
> -                       goto err;
> +                       goto err_helper;
>                 /*
>                  * ETF devices are tricky... They can be a link or a sink,
>                  * depending on how they are configured.  If an ETF has been
> @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>                 switch (type) {
>                 case CORESIGHT_DEV_TYPE_SINK:
>                         ret = coresight_enable_sink(csdev, mode, sink_data);
> -                       /*
> -                        * Sink is the first component turned on. If we
> -                        * failed to enable the sink, there are no components
> -                        * that need disabling. Disabling the path here
> -                        * would mean we could disrupt an existing session.
> -                        */
>                         if (ret)
> -                               goto out;
> +                               goto err;
>                         break;
>                 case CORESIGHT_DEV_TYPE_SOURCE:
>                         /* sources are enabled from either sysFS or Perf */
> @@ -507,6 +501,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>  out:
>         return ret;
>  err:
> +       coresight_disable_helpers(csdev);
> +err_helper:
>         coresight_disable_path_from(path, nd);
>         goto out;
>  }
> --
> 2.49.0.472.ge94155a9ec-goog
>

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

[Leo Yan]

Hi Yabin,

On Tue, Apr 01, 2025 at 06:21:59PM -0700, Yabin Cui wrote:

[...]

> > @@ -486,12 +491,17 @@ 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);
> > +       } else {
> > +               rc = -EBUSY;

This is not an error if the decremented reference counter is not zero.
It should return 0.  Otherwise, the change looks good to me.

Thanks,
Leo

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

[Leo Yan]

On Tue, Apr 01, 2025 at 06:22:46PM -0700, Yabin Cui wrote:
> On Tue, Apr 1, 2025 at 6:19 PM Yabin Cui <yabinc@google.com> 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 | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index fb43ef6a3b1f..e3270d9b46c9 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >                 /* Enable all helpers adjacent to the path first */
> >                 ret = coresight_enable_helpers(csdev, mode, path);
> >                 if (ret)
> > -                       goto err;
> > +                       goto err_helper;
> >                 /*
> >                  * ETF devices are tricky... They can be a link or a sink,
> >                  * depending on how they are configured.  If an ETF has been
> > @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >                 switch (type) {
> >                 case CORESIGHT_DEV_TYPE_SINK:
> >                         ret = coresight_enable_sink(csdev, mode, sink_data);
> > -                       /*
> > -                        * Sink is the first component turned on. If we
> > -                        * failed to enable the sink, there are no components
> > -                        * that need disabling. Disabling the path here
> > -                        * would mean we could disrupt an existing session.
> > -                        */
> >                         if (ret)
> > -                               goto out;
> > +                               goto err;
> >                         break;
> >                 case CORESIGHT_DEV_TYPE_SOURCE:
> >                         /* sources are enabled from either sysFS or Perf */
> > @@ -507,6 +501,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >  out:
> >         return ret;
> >  err:
> > +       coresight_disable_helpers(csdev);

Given it is unconditionally to enable helpers, I would suggest we
disable helper unconditionally.  Thus, we don't need to add a new
'err_helper' tag, simply reuse the 'err' tag would be fine.

I would like to know if mainatiners have different opinions.

Thanks,
Leo

> > +err_helper:
> >         coresight_disable_path_from(path, nd);
> >         goto out;
> >  }
> > --
> > 2.49.0.472.ge94155a9ec-goog
> >

Re: [PATCH v2 2/3] coresight: catu: Prevent concurrent PERF and SYSFS mode enablement

[Leo Yan]

On Tue, Apr 01, 2025 at 06:18:31PM -0700, Yabin Cui wrote:
> A CATU device can be enabled for either PERF mode or SYSFS mode, but
> not both simultaneously. Consider the following race condition:
> 
> CPU 1 enables CATU for PERF mode.
> CPU 2 enables CATU for SYSFS mode. It only increases refcnt.
> CPU 2 enables ETR for SYSFS mode.
> CPU 1 fails to enable ETR for PERF mode.
> 
> This results in a CATU being enabled in PERF mode and an ETR enabled
> in SYSFS mode, leading to unknown behavior.
> 
> To prevent this situation, this patch checks enabled mode of a
> CATU device before increasing its refcnt.

Yes.  We have also observed the same issue.  I am currently working on
refactoring the Arm CoreSight locking, my plan is to use coresight_path
to maintain mode.

Given it is uncommon case to use two modes concurrently in Arm
CoreSight, I assume this is not an urgent issue.  Could we defer this
patch?  My understanding is that this issue will be prevented with
checking the path mode, rather than checking modes in seperate modules.

To be clear, I am fine with other two patches in the series.

Thanks,
Leo

> Signed-off-by: Yabin Cui <yabinc@google.com>
> Suggested-by: James Clark <james.clark@linaro.org>
> Suggested-by: Leo Yan <leo.yan@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-catu.c | 6 +++++-
>  drivers/hwtracing/coresight/coresight-catu.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index b1d490dd7249..0caf3a3e75ce 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -466,7 +466,10 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
>  		CS_UNLOCK(catu_drvdata->base);
>  		rc = catu_enable_hw(catu_drvdata, mode, data);
>  		CS_LOCK(catu_drvdata->base);
> -	}
> +		if (!rc)
> +			catu_drvdata->mode = mode;
> +	} else if (catu_drvdata->mode != mode)
> +		rc = -EBUSY;
>  	if (!rc)
>  		csdev->refcnt++;
>  	return rc;
> @@ -499,6 +502,7 @@ static int catu_disable(struct coresight_device *csdev, void *__unused)
>  		CS_UNLOCK(catu_drvdata->base);
>  		rc = catu_disable_hw(catu_drvdata);
>  		CS_LOCK(catu_drvdata->base);
> +		catu_drvdata->mode = CS_MODE_DISABLED;
>  	} else {
>  		rc = -EBUSY;
>  	}
> diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
> index 755776cd19c5..ea406464f0a8 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.h
> +++ b/drivers/hwtracing/coresight/coresight-catu.h
> @@ -66,6 +66,7 @@ struct catu_drvdata {
>  	struct coresight_device *csdev;
>  	int irq;
>  	raw_spinlock_t spinlock;
> +	enum cs_mode mode;
>  };
>  
>  #define CATU_REG32(name, offset)					\
> -- 
> 2.49.0.472.ge94155a9ec-goog
> 

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

[Mike Leach]

Hi,

On Wed, 2 Apr 2025 at 14:32, Leo Yan <leo.yan@arm.com> wrote:
>
> On Tue, Apr 01, 2025 at 06:22:46PM -0700, Yabin Cui wrote:
> > On Tue, Apr 1, 2025 at 6:19 PM Yabin Cui <yabinc@google.com> 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 | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > > index fb43ef6a3b1f..e3270d9b46c9 100644
> > > --- a/drivers/hwtracing/coresight/coresight-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > > @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > >                 /* Enable all helpers adjacent to the path first */
> > >                 ret = coresight_enable_helpers(csdev, mode, path);
> > >                 if (ret)
> > > -                       goto err;
> > > +                       goto err_helper;
> > >                 /*
> > >                  * ETF devices are tricky... They can be a link or a sink,
> > >                  * depending on how they are configured.  If an ETF has been
> > > @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > >                 switch (type) {
> > >                 case CORESIGHT_DEV_TYPE_SINK:
> > >                         ret = coresight_enable_sink(csdev, mode, sink_data);
> > > -                       /*
> > > -                        * Sink is the first component turned on. If we
> > > -                        * failed to enable the sink, there are no components
> > > -                        * that need disabling. Disabling the path here
> > > -                        * would mean we could disrupt an existing session.
> > > -                        */
> > >                         if (ret)
> > > -                               goto out;
> > > +                               goto err;

Going to err here is wrong. The comment above specifically states that
we do _not_ want to disable the path, yet the new code flow disables
helpers. then falls through to coresight_disable_path_from() - which
the original code avoided and which also disables helpers a second
time.

Please rework to get the correct program flow.

> > >                         break;
> > >                 case CORESIGHT_DEV_TYPE_SOURCE:
> > >                         /* sources are enabled from either sysFS or Perf */
> > > @@ -507,6 +501,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > >  out:
> > >         return ret;
> > >  err:
> > > +       coresight_disable_helpers(csdev);

Drop this - put it where it is needed  in the flow

Regards

Mike


>
> Given it is unconditionally to enable helpers, I would suggest we
> disable helper unconditionally.  Thus, we don't need to add a new
> 'err_helper' tag, simply reuse the 'err' tag would be fine.
>
> I would like to know if mainatiners have different opinions.
>
> Thanks,
> Leo
>
> > > +err_helper:
> > >         coresight_disable_path_from(path, nd);
> > >         goto out;
> > >  }
> > > --
> > > 2.49.0.472.ge94155a9ec-goog
> > >

coresight_disable_path_from() also disables helpers.

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

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

[Leo Yan]

On Wed, Apr 02, 2025 at 02:50:22PM +0100, Mike Leach wrote:

[...]

> > > > @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > > >                 /* Enable all helpers adjacent to the path first */
> > > >                 ret = coresight_enable_helpers(csdev, mode, path);
> > > >                 if (ret)
> > > > -                       goto err;
> > > > +                       goto err_helper;
> > > >                 /*
> > > >                  * ETF devices are tricky... They can be a link or a sink,
> > > >                  * depending on how they are configured.  If an ETF has been
> > > > @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > > >                 switch (type) {
> > > >                 case CORESIGHT_DEV_TYPE_SINK:
> > > >                         ret = coresight_enable_sink(csdev, mode, sink_data);
> > > > -                       /*
> > > > -                        * Sink is the first component turned on. If we
> > > > -                        * failed to enable the sink, there are no components
> > > > -                        * that need disabling. Disabling the path here
> > > > -                        * would mean we could disrupt an existing session.
> > > > -                        */
> > > >                         if (ret)
> > > > -                               goto out;
> > > > +                               goto err;
> 
> Going to err here is wrong. The comment above specifically states that
> we do _not_ want to disable the path, yet the new code flow disables
> helpers.

Okay, now I understand here avoids to disable source and links for a
sink error.

> then falls through to coresight_disable_path_from() - which
> the original code avoided and which also disables helpers a second
> time.

Seems to me, the conclusion for "disables helpers a second time" is
incorrect.

I checked the coresight_disable_path_from() function, when the current
'nd' is passed to it, it will iterate from the _next_ node after 'nd'.

   /* Here 'nd' will be skipped and start from the next node */
   list_for_each_entry_continue(nd, &path->path_list, link) {
       ...

       coresight_disable_helpers(csdev, path);
   }

This means the _current_ coresight device (here is sink device) will
not disable its helpers.  Could you confirm for this?

Thanks,
Leo

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

[Jie Gan]

On 4/2/2025 10:12 PM, Leo Yan wrote:
> On Wed, Apr 02, 2025 at 02:50:22PM +0100, Mike Leach wrote:
> 
> [...]
> 
>>>>> @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>>>>>                  /* Enable all helpers adjacent to the path first */
>>>>>                  ret = coresight_enable_helpers(csdev, mode, path);
>>>>>                  if (ret)
>>>>> -                       goto err;
>>>>> +                       goto err_helper;
>>>>>                  /*
>>>>>                   * ETF devices are tricky... They can be a link or a sink,
>>>>>                   * depending on how they are configured.  If an ETF has been
>>>>> @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>>>>>                  switch (type) {
>>>>>                  case CORESIGHT_DEV_TYPE_SINK:
>>>>>                          ret = coresight_enable_sink(csdev, mode, sink_data);
>>>>> -                       /*
>>>>> -                        * Sink is the first component turned on. If we
>>>>> -                        * failed to enable the sink, there are no components
>>>>> -                        * that need disabling. Disabling the path here
>>>>> -                        * would mean we could disrupt an existing session.
>>>>> -                        */
>>>>>                          if (ret)
>>>>> -                               goto out;
>>>>> +                               goto err;
>>
>> Going to err here is wrong. The comment above specifically states that
>> we do _not_ want to disable the path, yet the new code flow disables
>> helpers.
> 
> Okay, now I understand here avoids to disable source and links for a
> sink error.
> 
>> then falls through to coresight_disable_path_from() - which
>> the original code avoided and which also disables helpers a second
>> time.
> 
> Seems to me, the conclusion for "disables helpers a second time" is
> incorrect.
> 
> I checked the coresight_disable_path_from() function, when the current
> 'nd' is passed to it, it will iterate from the _next_ node after 'nd'.
> 
>     /* Here 'nd' will be skipped and start from the next node */
>     list_for_each_entry_continue(nd, &path->path_list, link) {
>         ...
> 
>         coresight_disable_helpers(csdev, path);
>     }
> 
> This means the _current_ coresight device (here is sink device) will
> not disable its helpers.  Could you confirm for this?

It seems there is an exception that the helper devices connected to the 
sink? The sink device may not the first the device to be enabled in the 
path if the sink device has a helper device.

So I think we should add following logic before return?

                 switch (type) {
                 case CORESIGHT_DEV_TYPE_SINK:
                         ret = coresight_enable_sink(csdev, mode, 
sink_data);
                         /*
                          * Sink is the first component turned on. If we
                          * failed to enable the sink, there are no 
components
                          * that need disabling. Disabling the path here
                          * would mean we could disrupt an existing session.
                          */
                         if (ret) {
				/* disable the helpers which connected to sink */
				coresight_disable_helpers(csdev, path);
                                 goto out;
			}
                         break;


Thanks,
Jie

> 
> Thanks,
> Leo
> 

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

[Yabin Cui]

On Wed, Apr 2, 2025 at 6:01 AM Leo Yan <leo.yan@arm.com> wrote:
>
> Hi Yabin,
>
> On Tue, Apr 01, 2025 at 06:21:59PM -0700, Yabin Cui wrote:
>
> [...]
>
> > > @@ -486,12 +491,17 @@ 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);
> > > +       } else {
> > > +               rc = -EBUSY;
>
> This is not an error if the decremented reference counter is not zero.
> It should return 0.  Otherwise, the change looks good to me.

In coresight_disable_helpers(), the return value of catu_disable()
isn't checked.
The -EBUSY return was used for consistency with other refcounted
disable functions
like tmc_disable_etf_sink() and tmc_disable_etr_sink(). I'm happy to
change it back
to 0 if you believe that would be the more accurate return value here.

>
> Thanks,
> Leo

Re: [PATCH v2 2/3] coresight: catu: Prevent concurrent PERF and SYSFS mode enablement

[Yabin Cui]

On Wed, Apr 2, 2025 at 6:47 AM Leo Yan <leo.yan@arm.com> wrote:
>
> On Tue, Apr 01, 2025 at 06:18:31PM -0700, Yabin Cui wrote:
> > A CATU device can be enabled for either PERF mode or SYSFS mode, but
> > not both simultaneously. Consider the following race condition:
> >
> > CPU 1 enables CATU for PERF mode.
> > CPU 2 enables CATU for SYSFS mode. It only increases refcnt.
> > CPU 2 enables ETR for SYSFS mode.
> > CPU 1 fails to enable ETR for PERF mode.
> >
> > This results in a CATU being enabled in PERF mode and an ETR enabled
> > in SYSFS mode, leading to unknown behavior.
> >
> > To prevent this situation, this patch checks enabled mode of a
> > CATU device before increasing its refcnt.
>
> Yes.  We have also observed the same issue.  I am currently working on
> refactoring the Arm CoreSight locking, my plan is to use coresight_path
> to maintain mode.
>
> Given it is uncommon case to use two modes concurrently in Arm
> CoreSight, I assume this is not an urgent issue.  Could we defer this
> patch?  My understanding is that this issue will be prevented with
> checking the path mode, rather than checking modes in seperate modules.

Sure, I am fine with withdrawing this patch, as there are other plans to address
the issue.

>
> To be clear, I am fine with other two patches in the series.
>
> Thanks,
> Leo
>
> > Signed-off-by: Yabin Cui <yabinc@google.com>
> > Suggested-by: James Clark <james.clark@linaro.org>
> > Suggested-by: Leo Yan <leo.yan@arm.com>
> > ---
> >  drivers/hwtracing/coresight/coresight-catu.c | 6 +++++-
> >  drivers/hwtracing/coresight/coresight-catu.h | 1 +
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> > index b1d490dd7249..0caf3a3e75ce 100644
> > --- a/drivers/hwtracing/coresight/coresight-catu.c
> > +++ b/drivers/hwtracing/coresight/coresight-catu.c
> > @@ -466,7 +466,10 @@ static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
> >               CS_UNLOCK(catu_drvdata->base);
> >               rc = catu_enable_hw(catu_drvdata, mode, data);
> >               CS_LOCK(catu_drvdata->base);
> > -     }
> > +             if (!rc)
> > +                     catu_drvdata->mode = mode;
> > +     } else if (catu_drvdata->mode != mode)
> > +             rc = -EBUSY;
> >       if (!rc)
> >               csdev->refcnt++;
> >       return rc;
> > @@ -499,6 +502,7 @@ static int catu_disable(struct coresight_device *csdev, void *__unused)
> >               CS_UNLOCK(catu_drvdata->base);
> >               rc = catu_disable_hw(catu_drvdata);
> >               CS_LOCK(catu_drvdata->base);
> > +             catu_drvdata->mode = CS_MODE_DISABLED;
> >       } else {
> >               rc = -EBUSY;
> >       }
> > diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
> > index 755776cd19c5..ea406464f0a8 100644
> > --- a/drivers/hwtracing/coresight/coresight-catu.h
> > +++ b/drivers/hwtracing/coresight/coresight-catu.h
> > @@ -66,6 +66,7 @@ struct catu_drvdata {
> >       struct coresight_device *csdev;
> >       int irq;
> >       raw_spinlock_t spinlock;
> > +     enum cs_mode mode;
> >  };
> >
> >  #define CATU_REG32(name, offset)                                     \
> > --
> > 2.49.0.472.ge94155a9ec-goog
> >

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

[Yabin Cui]

On Wed, Apr 2, 2025 at 5:54 PM Jie Gan <quic_jiegan@quicinc.com> wrote:
>
>
>
> On 4/2/2025 10:12 PM, Leo Yan wrote:
> > On Wed, Apr 02, 2025 at 02:50:22PM +0100, Mike Leach wrote:
> >
> > [...]
> >
> >>>>> @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >>>>>                  /* Enable all helpers adjacent to the path first */
> >>>>>                  ret = coresight_enable_helpers(csdev, mode, path);
> >>>>>                  if (ret)
> >>>>> -                       goto err;
> >>>>> +                       goto err_helper;
> >>>>>                  /*
> >>>>>                   * ETF devices are tricky... They can be a link or a sink,
> >>>>>                   * depending on how they are configured.  If an ETF has been
> >>>>> @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >>>>>                  switch (type) {
> >>>>>                  case CORESIGHT_DEV_TYPE_SINK:
> >>>>>                          ret = coresight_enable_sink(csdev, mode, sink_data);
> >>>>> -                       /*
> >>>>> -                        * Sink is the first component turned on. If we
> >>>>> -                        * failed to enable the sink, there are no components
> >>>>> -                        * that need disabling. Disabling the path here
> >>>>> -                        * would mean we could disrupt an existing session.
> >>>>> -                        */
> >>>>>                          if (ret)
> >>>>> -                               goto out;
> >>>>> +                               goto err;
> >>
> >> Going to err here is wrong. The comment above specifically states that
> >> we do _not_ want to disable the path, yet the new code flow disables
> >> helpers.
> >
> > Okay, now I understand here avoids to disable source and links for a
> > sink error.
> >
> >> then falls through to coresight_disable_path_from() - which
> >> the original code avoided and which also disables helpers a second
> >> time.
> >
> > Seems to me, the conclusion for "disables helpers a second time" is
> > incorrect.
> >
> > I checked the coresight_disable_path_from() function, when the current
> > 'nd' is passed to it, it will iterate from the _next_ node after 'nd'.
> >
> >     /* Here 'nd' will be skipped and start from the next node */
> >     list_for_each_entry_continue(nd, &path->path_list, link) {
> >         ...
> >
> >         coresight_disable_helpers(csdev, path);
> >     }
> >
> > This means the _current_ coresight device (here is sink device) will
> > not disable its helpers.  Could you confirm for this?

Yes, without this patch:
If the current coresight device fails to enable, the code calls
coresight_disable_path_from() to disable devices and helpers after this
device. But it doesn't disable helpers for the current device.

With this patch:
1) When the helpers of the current device fails to enable, call
   coresight_disable_path_from() to disable following devices along the
   path.
2) When the current device fails to enable, first disable the helpers for
  the current devices, then call coresight_disable_path_from() to disable
  following devices and helpers.

Regarding the comment about not calling coresight_disable_path_from()
if a sink device fails to enable, I feel that's a case of it not being
necessary,
rather than something that causes logic error. So this patch calls it to avoid
duplicating coresight_disable_helpers(). If my understanding is wrong, I am
happy to change to duplicate coresight_disable_helpers() (in three places).

>
> It seems there is an exception that the helper devices connected to the
> sink? The sink device may not the first the device to be enabled in the
> path if the sink device has a helper device.
>
> So I think we should add following logic before return?
>
>                  switch (type) {
>                  case CORESIGHT_DEV_TYPE_SINK:
>                          ret = coresight_enable_sink(csdev, mode,
> sink_data);
>                          /*
>                           * Sink is the first component turned on. If we
>                           * failed to enable the sink, there are no
> components
>                           * that need disabling. Disabling the path here
>                           * would mean we could disrupt an existing session.
>                           */
>                          if (ret) {
>                                 /* disable the helpers which connected to sink */
>                                 coresight_disable_helpers(csdev, path);
>                                  goto out;
>                         }
>                          break;

This is the change I initially considered. But there are two other cases
(case CORESIGHT_DEV_TYPE_LINK and case default) that may need
to call coresight_disable_helpers() before returning an error.

>
>
> Thanks,
> Jie
>
> >
> > Thanks,
> > Leo
> >
>

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

[Suzuki K Poulose]

On 03/04/2025 18:53, Yabin Cui wrote:
> On Wed, Apr 2, 2025 at 6:01 AM Leo Yan <leo.yan@arm.com> wrote:
>>
>> Hi Yabin,
>>
>> On Tue, Apr 01, 2025 at 06:21:59PM -0700, Yabin Cui wrote:
>>
>> [...]
>>
>>>> @@ -486,12 +491,17 @@ 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);
>>>> +       } else {
>>>> +               rc = -EBUSY;
>>
>> This is not an error if the decremented reference counter is not zero.
>> It should return 0.  Otherwise, the change looks good to me.
> 
> In coresight_disable_helpers(), the return value of catu_disable()
> isn't checked.
> The -EBUSY return was used for consistency with other refcounted
> disable functions
> like tmc_disable_etf_sink() and tmc_disable_etr_sink(). I'm happy to
> change it back
> to 0 if you believe that would be the more accurate return value here.

Please stick to 0 here. This indicates there was no errors w.r.t the 
current session. This is similar to what we do for TMC ETR for e.g.

Suzuki


> 
>>
>> Thanks,
>> Leo