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

[hejunhao]

On 2025/7/3 0:47, Leo Yan wrote:
> 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) {

Ok, will fix it.

> 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.

Yes, you are right. So I added a minor note for that. The impact of this 
corner case may not have been fully considered.

> 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++;

we need to check the mode, cannot directly increase the reference count 
In performance mode.

>                  }
>
> 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.

In sysfs mode, it is necessary to check drvdata->reading.

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

I will refactor this code and upload a new version that includes all of 
your suggested solutions.

Thanks,
Junhao

> 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
>>
> .
>