Re: [PATCH v2 2/3] coresight: tmc: refactor the tmc-etr mode setting to avoid race conditions

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

On Fri, Jun 20, 2025 at 03:54:11PM +0800, Junhao He wrote:

[...]

> To fix this, configure the tmc-etr mode before invoking enable_etr_perf()
> or enable_etr_sysfs(), explicitly check if the tmc-etr sink is already
> enabled in a different mode, and simplily the setup and checks for "mode".
> To prevent race conditions between mode transitions.

I have a similiar fixing for CTI driver, see:
https://lore.kernel.org/linux-arm-kernel/20250701-arm_cs_pm_fix_v3-v2-0-23ebb864fcc1@arm.com/T/#maccd68b460fb8d74ccdd3504163d8f486f04178b

[...]

> @@ -1781,14 +1756,52 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>  static int tmc_enable_etr_sink(struct coresight_device *csdev,
>  			       enum cs_mode mode, void *data)
>  {
> +	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	enum cs_mode old_mode;
> +	int rc = -EINVAL;
> +
> +	scoped_guard(spinlock_irqsave, &drvdata->spinlock) {

scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

I am concerned that the spinlock is acquired and released for several
times in a single flow. It is possible to hit some corner case, e.g.,

- CPU0 acquires the lock, set sink as SYSFS mode, and releases the lock;
- CPU1 acquires the lock, detects the SYSFS mode has been set,
  directly bail out, then enable ETM.

The problem is the sink has not been enabled yet. This leads to CPU1
to enable the tracer but prior to sink's enabling.

The key piont is we should ensure the mode is consistent to the
hardware state. I will take a bit time for if we can have a better idea
for this.

> +		old_mode = coresight_get_mode(csdev);
> +		if (old_mode != CS_MODE_DISABLED && old_mode != mode)
> +			return -EBUSY;
> +
> +		if (drvdata->reading)
> +			return -EBUSY;
> +
> +		/* In sysFS mode we can have multiple writers per sink. */
> +		if (old_mode == CS_MODE_SYSFS) {
> +			csdev->refcnt++;

I am just wandering if we can unify the "refcnt" code for both SYSFS
mode and Perf mode, and as a result, we can consolidate the code cross
different drivers.

Something like:

                if (!csdev->refcnt) {
                        coresight_set_mode(csdev, mode);
                } else {
                        /* The device has been configured with an incompatible mode */
                        if (coresight_get_mode(csdev) != mode)
                                return -EBUSY;

                        csdev->refcnt++;
                }

Then when disable the device:

                csdev->refcnt--;
                if (!csdev->refcnt)
                        coresight_set_mode(csdev, CS_MODE_DISABLED);

We should not check "drvdata->reading" when enable or disable sink, as
it is a flag to track if reading the trace buffer, it is irrelevant to
the sink device's enabling or disabling.

Please verify perf mode (especially for system wide session) to avoid
anything missed.

Thanks,
Leo

> +			return 0;
> +		}
> +
> +		/*
> +		 * minor note: In sysFS mode, the task1 get locked first, it setup
> +		 * etr mode to SYSFS. Then task2 get locked，it will directly return
> +		 * success even when the tmc-etr is not enabled at this moment.
> +		 * Ultimately, task1 will still successfully enable tmc-etr.
> +		 * This is a transient state and does not cause an anomaly.
> +		 */
> +		coresight_set_mode(csdev, mode);
> +	}
> +
>  	switch (mode) {
>  	case CS_MODE_SYSFS:
> -		return tmc_enable_etr_sink_sysfs(csdev);
> +		rc = tmc_enable_etr_sink_sysfs(csdev);
> +		break;
>  	case CS_MODE_PERF:
> -		return tmc_enable_etr_sink_perf(csdev, data);
> +		rc = tmc_enable_etr_sink_perf(csdev, data);
> +		break;
>  	default:
> -		return -EINVAL;
> +		rc = -EINVAL;
>  	}
> +
> +	if (rc && old_mode != mode) {
> +		scoped_guard(spinlock_irqsave, &drvdata->spinlock) {
> +			coresight_set_mode(csdev, old_mode);
> +		}
> +	}
> +
> +	return rc;
>  }
>  
>  static int tmc_disable_etr_sink(struct coresight_device *csdev)
> -- 
> 2.33.0
>