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

[Leo Yan <leo.yan@arm.com>]

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
>