Re: [PATCH v3 1/8] coresight: tmc-etr: Don't stop Perf cleanup for active sysfs reads

On Tue, Jul 28, 2026 at 04:00:13PM +0100, James Clark wrote:
> The linked fixes commit deliberately allows reads of an old sysfs buffer
> while in Perf mode because they are separate software buffers. However
> it didn't modify tmc_disable_etr_sink() to match this relaxation. The
> result is that when a Perf event ends while the sysfs buffer is being
> read, clean up will be skipped.
> 
> Fix it by ignoring the sysfs_reading flag unless the active session is
> a sysfs one.

I am not clear why this is relevant to per-thread mode, maybe it would
be clear for me after reading other patches.

For this patch self, I think the issue comes from the sink buffer's life
time - SYSFS mode's buffer has longer life time than a sysfs session,
it is designed to allow reading sysfs trace data after the sysfs
session, or even during a perf session. So the code gets complex for
handling these cases.

I really think we should have a correct life time (or state machine)
for sysfs buffer, something like:

  Operations                        |  ETR sysfs buffer state machine
  ----------------------------------+-----------------------------------------
                                    |  INVALID: Init state, buffer unallocated
  echo 1 > .../tmc_etr0/enable_sink |  READY: user can read zeroed data
  echo 1 > .../etm0/enable_source   |  BUSY: user cannot read as hardware is
                                    |        writing data to buffer
  echo 0 > .../etm0/enable_source   |  READY: trace is stopped, user can read
                                    |         trace data
  echo 0 > .../tmc_etr0/enable_sink |  INVALID: buffer has been released

With the state machine's help, we might even don't need to bother what
the sink device's mode is.

> When sysfs and Perf share the same memory in ETR_MODE_RESRV mode, a new
> Perf session needs to overwrite an old inactive sysfs session by zeroing
> len. This avoids sysfs from reading stale data because it has a separate
> set of offsets in its etr_buf struct, even if that's backed by the same
> memory as the Perf one.

This can be naturally resolved if we have a state machine above?

[...]

> +static bool tmc_perf_sysfs_shared(struct tmc_drvdata *drvdata,
> +				  struct etr_buf *perf_buf)
> +{
> +	/* In ETR_MODE_RESRV mode, sysfs and Perf share the same memory. */
> +	return perf_buf &&
> +	       drvdata->sysfs_buf &&
> +	       drvdata->sysfs_buf->mode == ETR_MODE_RESRV &&
> +	       perf_buf->mode == ETR_MODE_RESRV;
> +}
> +
>  static int tmc_enable_etr_sink_perf(struct coresight_device *csdev,
>  				    struct coresight_path *path)
>  {
> @@ -1772,6 +1782,18 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev,
>  		goto unlock_out;
>  	}
>  
> +	/*
> +	 * Don't use if it's shared and being read by sysfs. Sysfs may only
> +	 * start reading (the cleared zero length buffer) after the first
> +	 * tmc_enable_etr_sink_perf(), which changes the result of this check,
> +	 * so it should only be done once.
> +	 */
> +	if ((drvdata->sysfs_reading &&
> +	     tmc_perf_sysfs_shared(drvdata, etr_perf->etr_buf))) {
> +		rc = -EBUSY;
> +		goto unlock_out;
> +	}

This is quite tricky. If someone reads the sysfs entry in the middle of
a perf session, and the traced task is migrated to a different CPU or
wakes up, the sink device may need to be re-enabled, however the sink
enabling could fail due to the concurrent sysfs read here.

Thanks,
Leo