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

[James Clark]

On 12/08/2026 17:45, Leo Yan wrote:
> 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.
> 

It's because we have to track the enable and disable state accurately so 
that we can drop the reference to the session ID's task and PID on 
disable. Sashiko sees that we already don't track it properly so we 
don't handle the newly added references properly either.

I can mention that in the commit message.

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

Yep, plus the fact that we added ETR_MODE_RESRV which also shares the 
same backing memory between sysfs and Perf and didn't take any of this 
into account, pre-sashiko :)

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

I think this is basically the same as now but more explicit. At the 
moment the same state is represented by the refcount, mode, 
sysfs_reading, and len fields. I can try the state machine and see if it 
improves things. There was one complication to work around where open() 
and read() are separate calls, which is what lead me to use the len = 0 
hack in the first place, but it might be fine, I just have to make 
read() check the INVALID state too.

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

Naturally resolved in what way though? There are only two options: you 
either block new Perf sessions indefinitely until sysfs has disabled 
_and_ read. Or, you erase the sysfs data when a Perf session starts and 
don't block. I chose the second one.

This will always be a special case for ETR_MODE_RESRV over the other modes.

> [...]
> 
>> +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.

I think that's fine? There's no other way to do it is there? We do the 
same -EBUSY for incompatible per-thread sessions that migrate to a CPU 
with a shared sink. This only impacts ETR_MODE_RESRV anyway, are we 
really worried about concurrent use in that niche mode?

> 
> Thanks,
> Leo