Re: [PATCH v3 1/8] coresight: tmc-etr: Don't stop Perf cleanup for active sysfs reads
From: James Clark
Date: Thu Aug 13 2026 - 05:05:22 EST
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