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

[PATCH v4 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 v3:
 - Add newlines between variable definition and guard().
 - Add path parameter when calling coresight_disable_helpers.
 - Use "goto err_disable_helpers" in coresight_enable_path().

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 | 10 +++++---
 3 files changed, 25 insertions(+), 11 deletions(-)

-- 
2.49.0.604.gff1f9ca942-goog

[PATCH v4 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..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

[PATCH v4 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 | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index fb43ef6a3b1f..d9fcea69d221 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, path);
 				goto out;
+			}
 			break;
 		case CORESIGHT_DEV_TYPE_SOURCE:
 			/* sources are enabled from either sysFS or Perf */
@@ -497,15 +499,17 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 			child = list_next_entry(nd, link)->csdev;
 			ret = coresight_enable_link(csdev, parent, child, source);
 			if (ret)
-				goto err;
+				goto err_disable_helpers;
 			break;
 		default:
-			goto err;
+			goto err_disable_helpers;
 		}
 	}
 
 out:
 	return ret;
+err_disable_helpers:
+	coresight_disable_helpers(csdev, path);
 err:
 	coresight_disable_path_from(path, nd);
 	goto out;
-- 
2.49.0.604.gff1f9ca942-goog

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

[Jie Gan]

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)					\

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

[Leo Yan]

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 2/2] coresight: core: Disable helpers for devices that fail to enable

[Leo Yan]

On Tue, Apr 15, 2025 at 11:46:49AM -0700, 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 | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fb43ef6a3b1f..d9fcea69d221 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, path);

I think we can do better for code consolidation - we can use a central
place for error handling.  I will give details below.

>  				goto out;
> +			}
>  			break;
>  		case CORESIGHT_DEV_TYPE_SOURCE:
>  			/* sources are enabled from either sysFS or Perf */
> @@ -497,15 +499,17 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>  			child = list_next_entry(nd, link)->csdev;
>  			ret = coresight_enable_link(csdev, parent, child, source);
>  			if (ret)
> -				goto err;
> +				goto err_disable_helpers;
>  			break;
>  		default:
> -			goto err;
> +			goto err_disable_helpers;

I know this is not a problem introduced by your patch - for an error
case, it returns 0.  This will hide unexpected issues.  I would like
to suggest to return -EINVAL for unknown types.

>  		}
>  	}
>  
>  out:
>  	return ret;
> +err_disable_helpers:
> +	coresight_disable_helpers(csdev, path);
>  err:
>  	coresight_disable_path_from(path, nd);
>  	goto out;

I am just wandering if we can handle errors in a unified way and
without using goto.  I would change the code as below.

The point is to use a general flow for error handling, include a
sink error.  For sink error, we still invoke
coresight_disable_path_from() for an empty operation.

Also, I think we need an additional patch for error handling in
coresight_enable_helpers(). If any errors are detected while enabling
a helper, we should disable the helpers that have already been
enabled.

Please let me know if you have any questions.

Thanks,
Leo

---8<---

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index fb43ef6a3b1f..cf2a3708a05e 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_disable_path;
 		/*
 		 * ETF devices are tricky... They can be a link or a sink,
 		 * depending on how they are configured.  If an ETF has been
@@ -487,7 +487,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 			 * would mean we could disrupt an existing session.
 			 */
 			if (ret)
-				goto out;
+				goto err_disable_helpers;
 			break;
 		case CORESIGHT_DEV_TYPE_SOURCE:
 			/* sources are enabled from either sysFS or Perf */
@@ -497,18 +497,21 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 			child = list_next_entry(nd, link)->csdev;
 			ret = coresight_enable_link(csdev, parent, child, source);
 			if (ret)
-				goto err;
+				goto err_disable_helpers;
 			break;
 		default:
-			goto err;
+			ret = -EINVAL;
+			goto err_disable_helpers;
 		}
 	}
 
-out:
-	return ret;
-err:
+	return 0;
+
+err_disable_helpers:
+	coresight_disable_helpers(csdev, path);
+err_disable_path:
 	coresight_disable_path_from(path, nd);
-	goto out;
+	return ret;
 }
 
> -- 
> 2.49.0.604.gff1f9ca942-goog
> 

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

[Yabin Cui]

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 2/2] coresight: core: Disable helpers for devices that fail to enable

[Yabin Cui]

On Wed, Apr 23, 2025 at 2:32 AM Leo Yan <leo.yan@arm.com> wrote:
>
> On Tue, Apr 15, 2025 at 11:46:49AM -0700, 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 | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index fb43ef6a3b1f..d9fcea69d221 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, path);
>
> I think we can do better for code consolidation - we can use a central
> place for error handling.  I will give details below.
>
> >                               goto out;
> > +                     }
> >                       break;
> >               case CORESIGHT_DEV_TYPE_SOURCE:
> >                       /* sources are enabled from either sysFS or Perf */
> > @@ -497,15 +499,17 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >                       child = list_next_entry(nd, link)->csdev;
> >                       ret = coresight_enable_link(csdev, parent, child, source);
> >                       if (ret)
> > -                             goto err;
> > +                             goto err_disable_helpers;
> >                       break;
> >               default:
> > -                     goto err;
> > +                     goto err_disable_helpers;
>
> I know this is not a problem introduced by your patch - for an error
> case, it returns 0.  This will hide unexpected issues.  I would like
> to suggest to return -EINVAL for unknown types.
>
> >               }
> >       }
> >
> >  out:
> >       return ret;
> > +err_disable_helpers:
> > +     coresight_disable_helpers(csdev, path);
> >  err:
> >       coresight_disable_path_from(path, nd);
> >       goto out;
>
> I am just wandering if we can handle errors in a unified way and
> without using goto.  I would change the code as below.
>
> The point is to use a general flow for error handling, include a
> sink error.  For sink error, we still invoke
> coresight_disable_path_from() for an empty operation.
>
> Also, I think we need an additional patch for error handling in
> coresight_enable_helpers(). If any errors are detected while enabling
> a helper, we should disable the helpers that have already been
> enabled.
>
> Please let me know if you have any questions.

I'm fine with these changes. However, there's one spot below that's
controversial in the v2 patch.

>
> Thanks,
> Leo
>
> ---8<---
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fb43ef6a3b1f..cf2a3708a05e 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_disable_path;
>                 /*
>                  * ETF devices are tricky... They can be a link or a sink,
>                  * depending on how they are configured.  If an ETF has been
> @@ -487,7 +487,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>                          * would mean we could disrupt an existing session.
>                          */
>                         if (ret)
> -                               goto out;
> +                               goto err_disable_helpers;

I made a similar change in this area in the v2 patch, which
contradicts the original comment here.
Mike had concerns about it. I explained my reasoning, but haven't
heard back. Could we get confirmation
that it's safe to remove the comment and proceed to err_disable_helpers here?

>                         break;
>                 case CORESIGHT_DEV_TYPE_SOURCE:
>                         /* sources are enabled from either sysFS or Perf */
> @@ -497,18 +497,21 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>                         child = list_next_entry(nd, link)->csdev;
>                         ret = coresight_enable_link(csdev, parent, child, source);
>                         if (ret)
> -                               goto err;
> +                               goto err_disable_helpers;
>                         break;
>                 default:
> -                       goto err;
> +                       ret = -EINVAL;
> +                       goto err_disable_helpers;
>                 }
>         }
>
> -out:
> -       return ret;
> -err:
> +       return 0;
> +
> +err_disable_helpers:
> +       coresight_disable_helpers(csdev, path);
> +err_disable_path:
>         coresight_disable_path_from(path, nd);
> -       goto out;
> +       return ret;
>  }
>
> > --
> > 2.49.0.604.gff1f9ca942-goog
> >