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