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

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

[...]

...

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

.