Concurrent per-thread events results in a WARN on N1SDP which leads to the realization that per-thread events shouldn't have been sharing sinks in the first place.
This slips through because different per-thread events will have the same PID if owned by the same process, and we only check the PID and nothing else. That results in unexpected WARNs because it looks like we assumed it couldn't happen (although exclusive PMU rules allow it). But even if it was supported it would result in trace from the wrong thread in another event's per-thread buffer, so we should disallow it.
Fix it everywhere the same PID checking logic was copy pasted. Then the PIDs can be dropped from a few structs as they are now unused.
Signed-off-by: James Clark james.clark@linaro.org --- Changes in v3: - Storing and accessing event owners at runtime causes problems due to various scenarios of: events (and sibling events) exiting, children inheriting event FDs, PID reuse, CPU affine events that also have a target process set but different inherit settings. Fix it by creating a session ID in etm_setup_aux() and holding the references in it for the duration of the whole session. (Leo) - Make the ETR buffer allocator consistent with sink sharing rules by not doing numeric PID comparisons there either. - Fix up some Sashiko reports that it sees after interacting with cscfg and taking extra references to tasks and PIDs. - Link to v2: https://lore.kernel.org/r/20260709-james-cs-multiple-per-threads-v2-0-10ac7a...
Changes in v2: - Fix inherited events by following event->parent - Link to v1: https://lore.kernel.org/r/20260709-james-cs-multiple-per-threads-v1-0-d384e6...
--- James Clark (8): coresight: tmc-etr: Don't stop Perf cleanup for active sysfs reads coresight: configfs: Don't assume active until cscfg_mgr is set coresight: etm-perf: Flush workqueue before unloading module coresight: tmc-etr: Prevent per-thread events from sharing a sink coresight: tmc-etr: Use session ID for buffer ownership coresight: tmc-etf: Prevent per-thread events from sharing a sink coresight: etb10: Prevent per-thread events from sharing a sink coresight: ultrasoc-smb: Prevent per-thread events from sharing a sink
drivers/hwtracing/coresight/coresight-core.c | 28 +-- drivers/hwtracing/coresight/coresight-etb10.c | 33 ++-- drivers/hwtracing/coresight/coresight-etm-perf.c | 79 ++++++++- drivers/hwtracing/coresight/coresight-etm-perf.h | 15 ++ drivers/hwtracing/coresight/coresight-priv.h | 2 - drivers/hwtracing/coresight/coresight-syscfg.c | 6 +- drivers/hwtracing/coresight/coresight-tmc-core.c | 6 +- drivers/hwtracing/coresight/coresight-tmc-etf.c | 44 ++--- drivers/hwtracing/coresight/coresight-tmc-etr.c | 207 +++++++++++++---------- drivers/hwtracing/coresight/coresight-tmc.h | 30 ++-- drivers/hwtracing/coresight/coresight-trbe.c | 3 +- drivers/hwtracing/coresight/ultrasoc-smb.c | 25 +-- drivers/hwtracing/coresight/ultrasoc-smb.h | 6 +- include/linux/coresight.h | 5 +- 14 files changed, 294 insertions(+), 195 deletions(-) --- base-commit: 98495b5a4d77dd22e106f462b76e1093a55b29a7 change-id: 20260708-james-cs-multiple-per-threads-ed1d25ed1734
Best regards,
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.
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.
Reported-by: sashiko-bot sashiko-bot@kernel.org Fixes: cad5f8d399bb ("coresight: tmc-etr: Relax collection of trace from sysfs mode") Signed-off-by: James Clark james.clark@linaro.org --- drivers/hwtracing/coresight/coresight-tmc-etf.c | 16 ++++---- drivers/hwtracing/coresight/coresight-tmc-etr.c | 50 +++++++++++++++++++++---- drivers/hwtracing/coresight/coresight-tmc.h | 7 +++- 3 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 8882b1c4cdc0..3836063031d7 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -198,7 +198,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev) raw_spin_lock_irqsave(&drvdata->spinlock, flags); }
- if (drvdata->reading) { + if (drvdata->sysfs_reading) { ret = -EBUSY; goto out; } @@ -259,7 +259,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, raw_spin_lock_irqsave(&drvdata->spinlock, flags); do { ret = -EINVAL; - if (drvdata->reading) + if (drvdata->sysfs_reading) break; /* * No need to continue if the ETB/ETF is already operated @@ -337,7 +337,7 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
raw_spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->reading) { + if (drvdata->sysfs_reading) { raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); return -EBUSY; } @@ -371,7 +371,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev, bool first_enable = false;
raw_spin_lock_irqsave(&drvdata->spinlock, flags); - if (drvdata->reading) { + if (drvdata->sysfs_reading) { raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); return -EBUSY; } @@ -401,7 +401,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev, bool last_disable = false;
raw_spin_lock_irqsave(&drvdata->spinlock, flags); - if (drvdata->reading) { + if (drvdata->sysfs_reading) { raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); return; } @@ -718,7 +718,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
raw_spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->reading) { + if (drvdata->sysfs_reading) { ret = -EBUSY; goto out; } @@ -746,7 +746,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata) __tmc_etb_disable_hw(drvdata); }
- drvdata->reading = true; + drvdata->sysfs_reading = true; out: raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -797,7 +797,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata) drvdata->buf = NULL; }
- drvdata->reading = false; + drvdata->sysfs_reading = false; raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
/* diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 361a433e6f0c..be0bbe036d02 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1151,7 +1151,7 @@ static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata, * starts at anywhere in the buffer, depending on the RRP, we adjust the * @len returned to handle buffer wrapping around. * - * We are protected here by drvdata->reading != 0, which ensures the + * We are protected here by drvdata->sysfs_reading != 0, which ensures the * sysfs_buf stays alive. */ ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata, @@ -1268,7 +1268,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) raw_spin_lock_irqsave(&drvdata->spinlock, flags); }
- if (drvdata->reading || coresight_get_mode(csdev) == CS_MODE_PERF) { + if (drvdata->sysfs_reading || coresight_get_mode(csdev) == CS_MODE_PERF) { ret = -EBUSY; goto out; } @@ -1732,6 +1732,16 @@ tmc_update_etr_buffer(struct coresight_device *csdev, return size; }
+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; + } + rc = tmc_etr_enable_hw(drvdata, etr_perf->etr_buf); if (!rc) { /* Associate with monitored process. */ @@ -1779,6 +1801,10 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, coresight_set_mode(csdev, CS_MODE_PERF); drvdata->perf_buf = etr_perf->etr_buf; csdev->refcnt++; + + /* A new Perf session clears an old sysfs one if the buffer is shared */ + if (tmc_perf_sysfs_shared(drvdata, etr_perf->etr_buf)) + drvdata->sysfs_buf->len = 0; }
unlock_out: @@ -1807,7 +1833,13 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
raw_spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->reading) { + /* + * In SYSFS mode an active read is responsible for disabling and + * enabling HW. Otherwise in Perf mode, an old inactive sysfs session + * may be read which Perf should ignore. + */ + if (drvdata->sysfs_reading && + coresight_get_mode(csdev) == CS_MODE_SYSFS) { raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); return -EBUSY; } @@ -1928,14 +1960,16 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) return -EINVAL;
raw_spin_lock_irqsave(&drvdata->spinlock, flags); - if (drvdata->reading) { + if (drvdata->sysfs_reading) { ret = -EBUSY; goto out; }
/* - * We can safely allow reads even if the ETR is operating in PERF mode, - * since the sysfs session is captured in mode specific data. + * We can safely allow reads even if the ETR is operating in PERF mode + * since sysfs has it's own buffer. For ETR_MODE_RESRV the buffers are + * shared but Perf discards sysfs data before starting a session to + * avoid corruption. * If drvdata::sysfs_data is NULL the trace data has been read already. */ if (!drvdata->sysfs_buf) { @@ -1947,7 +1981,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata) if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) __tmc_etr_disable_hw(drvdata);
- drvdata->reading = true; + drvdata->sysfs_reading = true; out: raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -1982,7 +2016,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata) drvdata->sysfs_buf = NULL; }
- drvdata->reading = false; + drvdata->sysfs_reading = false; raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
/* Free allocated memory out side of the spinlock */ diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 319a354ede9f..dc1a57ab8011 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -221,7 +221,10 @@ struct tmc_resrv_buf { * @pid: Process ID of the process that owns the session that is using * this component. For example this would be the pid of the Perf * process. - * @reading: buffer's in the reading through "/dev/xyz.tmc" entry + * @sysfs_reading: Sysfs mode buffer is being read through "/dev/xyz.tmc" entry. + * Note: ETR has a separate software buffer for the two modes so + * the device can still be read while in Perf mode if there is a + * previous inactive sysfs session. * @stop_on_flush: Stop on flush trigger user configuration. * @buf: Snapshot of the trace data for ETF/ETB. * @etr_buf: details of buffer used in TMC-ETR @@ -255,7 +258,7 @@ struct tmc_drvdata { struct miscdevice crashdev; raw_spinlock_t spinlock; pid_t pid; - bool reading; + bool sysfs_reading; bool stop_on_flush; union { char *buf; /* TMC ETB */
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
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
Perf's etm_setup_aux() calls cscfg_activate_config() and dereferences cscfg_mgr, which can be done after coresight_init() has initialized the Perf PMU but before cscfg_init() has finished while the driver is still loading.
Fix it by only assuming cscfg is initialized if cscfg_mgr is set.
Reported-by: sashiko-bot sashiko-bot@kernel.org Fixes: a0114b4740dd ("coresight: etm-perf: Update to activate selected configuration") Signed-off-by: James Clark james.clark@linaro.org --- drivers/hwtracing/coresight/coresight-syscfg.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index 2bfdd7b45e49..475e8fefa100 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -581,7 +581,7 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs, int err = 0;
mutex_lock(&cscfg_mutex); - if (cscfg_mgr->load_state != CSCFG_NONE) { + if (!cscfg_mgr || cscfg_mgr->load_state != CSCFG_NONE) { mutex_unlock(&cscfg_mutex); return -EBUSY; } @@ -662,7 +662,7 @@ int cscfg_unload_config_sets(struct cscfg_load_owner_info *owner_info) struct cscfg_load_owner_info *load_list_item = NULL;
mutex_lock(&cscfg_mutex); - if (cscfg_mgr->load_state != CSCFG_NONE) { + if (!cscfg_mgr || cscfg_mgr->load_state != CSCFG_NONE) { mutex_unlock(&cscfg_mutex); return -EBUSY; } @@ -902,7 +902,7 @@ static int _cscfg_activate_config(unsigned long cfg_hash) struct cscfg_config_desc *config_desc; int err = -EINVAL;
- if (cscfg_mgr->load_state == CSCFG_UNLOAD) + if (!cscfg_mgr || cscfg_mgr->load_state == CSCFG_UNLOAD) return -EBUSY;
list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
On Tue, Jul 28, 2026 at 04:00:14PM +0100, James Clark wrote:
Perf's etm_setup_aux() calls cscfg_activate_config() and dereferences cscfg_mgr, which can be done after coresight_init() has initialized the Perf PMU but before cscfg_init() has finished while the driver is still loading.
For this patch, if Perf PMU is dependent on cscfg, why we cannot adjust the init sequence that first call cscfg_init(), then register cs_etm PMU event?
In _cscfg_activate_config(), I can see it dereferences cscfg_mgr:
if (cscfg_mgr->load_state == CSCFG_UNLOAD)
This patch does not check if cscfg_mgr is set. If we try to check cscfg_mgr every time before dereferences it, this approach is quite fragile.
Fix it by only assuming cscfg is initialized if cscfg_mgr is set.
Reported-by: sashiko-bot sashiko-bot@kernel.org Fixes: a0114b4740dd ("coresight: etm-perf: Update to activate selected configuration") Signed-off-by: James Clark james.clark@linaro.org
drivers/hwtracing/coresight/coresight-syscfg.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index 2bfdd7b45e49..475e8fefa100 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -581,7 +581,7 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs, int err = 0; mutex_lock(&cscfg_mutex);
- if (cscfg_mgr->load_state != CSCFG_NONE) {
- if (!cscfg_mgr || cscfg_mgr->load_state != CSCFG_NONE) { mutex_unlock(&cscfg_mutex); return -EBUSY;
If cscfg_mgr is NULL, would directly bail out? This can avoid adding check for every dereference.
Thanks, Leo
P.S. For me, this kind of rare race condition reported by Sashiko can sometimes distract from the main issue. Trying to address it in the same series may mix different issues together. I miss the simpler approach where issues could be resolved with clear boundary.
On 13/08/2026 10:09, Leo Yan wrote:
On Tue, Jul 28, 2026 at 04:00:14PM +0100, James Clark wrote:
Perf's etm_setup_aux() calls cscfg_activate_config() and dereferences cscfg_mgr, which can be done after coresight_init() has initialized the Perf PMU but before cscfg_init() has finished while the driver is still loading.
For this patch, if Perf PMU is dependent on cscfg, why we cannot adjust the init sequence that first call cscfg_init(), then register cs_etm PMU event?
cscfg_init() relies on perf making some files in sysfs. I think I tried that first but it doesn't work:
/* add config to perf fs to allow selection */ err = etm_perf_add_symlink_cscfg(cscfg_device(), config_desc);
One solution would be to combine the modules into one. Not sure if there is any functional requirement for them being separate. They're both "software" parts of the set of Coresight modules, rather than being associated with any hardware. Having them separate is obviously creating some bugs and needless complexity.
In _cscfg_activate_config(), I can see it dereferences cscfg_mgr:
if (cscfg_mgr->load_state == CSCFG_UNLOAD)
This patch does not check if cscfg_mgr is set. If we try to check cscfg_mgr every time before dereferences it, this approach is quite fragile.
Maybe we can have a "cscfg_get_state()" function which also checks for NULL and returns CSCFG_UNLOAD in that case?
Fix it by only assuming cscfg is initialized if cscfg_mgr is set.
Reported-by: sashiko-bot sashiko-bot@kernel.org Fixes: a0114b4740dd ("coresight: etm-perf: Update to activate selected configuration") Signed-off-by: James Clark james.clark@linaro.org
drivers/hwtracing/coresight/coresight-syscfg.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index 2bfdd7b45e49..475e8fefa100 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -581,7 +581,7 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs, int err = 0; mutex_lock(&cscfg_mutex);
- if (cscfg_mgr->load_state != CSCFG_NONE) {
- if (!cscfg_mgr || cscfg_mgr->load_state != CSCFG_NONE) { mutex_unlock(&cscfg_mutex); return -EBUSY;
If cscfg_mgr is NULL, would directly bail out? This can avoid adding check for every dereference.
Not sure what you mean by this?
Thanks, Leo
P.S. For me, this kind of rare race condition reported by Sashiko can sometimes distract from the main issue. Trying to address it in the same series may mix different issues together. I miss the simpler approach where issues could be resolved with clear boundary.
It's a bit like the other fix, we're holding new refcounts to the task_struct and PID, so we should probably make sure we're going to release them. Getting this wrong prevents free_event_data() from being called.
Don't worry I didn't fix all of the edge cases either.
The work queue is never flushed on module unload, so pending work items can run after coresight-config is cleaned up or the .text section is unloaded. The global workqueue also can't be flushed manually, so change it to a local workqueue and flush it on exit.
The init function needs to be rearranged to fix a similar problem. The Perf PMU can be used as soon as registration succeeds, so move it as late as possible so that the only failure that can follow is coresight- config. Failing to register coresight-config means no flush dependency between coresight-config and the Perf PMU will exist, so it doesn't need to be done.
Assisted-by: Codex:GPT-5.6-Sol Reported-by: sashiko-bot sashiko-bot@kernel.org Fixes: 0bcbf2e30ff2 ("coresight: etm-perf: new PMU driver for ETM tracers") Signed-off-by: James Clark james.clark@linaro.org --- drivers/hwtracing/coresight/coresight-core.c | 28 ++++++++++++++---------- drivers/hwtracing/coresight/coresight-etm-perf.c | 22 +++++++++++++++---- drivers/hwtracing/coresight/coresight-etm-perf.h | 1 + 3 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index f7b1308a759c..5e653b08a83d 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -2094,31 +2094,31 @@ static int __init coresight_init(void) if (ret) return ret;
- ret = etm_perf_init(); - if (ret) - goto exit_bus_unregister; - /* Register function to be called for panic */ ret = atomic_notifier_chain_register(&panic_notifier_list, &coresight_notifier); if (ret) - goto exit_perf; + goto exit_bus_unregister;
- /* initialise the coresight syscfg API */ - ret = cscfg_init(); + ret = coresight_pm_setup(); if (ret) goto exit_notifier;
- ret = coresight_pm_setup(); + ret = etm_perf_init(); + if (ret) + goto exit_pm; + + /* initialise the coresight syscfg API */ + ret = cscfg_init(); if (!ret) return 0;
- cscfg_exit(); + etm_perf_exit(); +exit_pm: + coresight_pm_cleanup(); exit_notifier: atomic_notifier_chain_unregister(&panic_notifier_list, &coresight_notifier); -exit_perf: - etm_perf_exit(); exit_bus_unregister: bus_unregister(&coresight_bustype); return ret; @@ -2127,6 +2127,12 @@ static int __init coresight_init(void) static void __exit coresight_exit(void) { coresight_pm_cleanup(); + /* + * Flush Perf workqueue before the rest of Coresight is torn down + * because work items touch coresight-config and also require the .text + * to remain loaded. + */ + etm_perf_flush_workqueue(); cscfg_exit(); atomic_notifier_chain_unregister(&panic_notifier_list, &coresight_notifier); diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 09b21a711a87..7fb5c3d18bd5 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -27,6 +27,7 @@ #include "coresight-trace-id.h"
static struct pmu etm_pmu; +static struct workqueue_struct *etm_free_wq; static bool etm_perf_up;
/* @@ -292,7 +293,7 @@ static void etm_free_aux(void *data) { struct etm_event_data *event_data = data;
- schedule_work(&event_data->work); + queue_work(etm_free_wq, &event_data->work); }
/* @@ -1034,6 +1035,10 @@ int __init etm_perf_init(void) { int ret;
+ etm_free_wq = alloc_workqueue("coresight_etm_free", WQ_UNBOUND, 0); + if (!etm_free_wq) + return -ENOMEM; + etm_pmu.capabilities = (PERF_PMU_CAP_EXCLUSIVE | PERF_PMU_CAP_ITRACE | PERF_PMU_CAP_AUX_PAUSE); @@ -1054,13 +1059,22 @@ int __init etm_perf_init(void) etm_pmu.module = THIS_MODULE;
ret = perf_pmu_register(&etm_pmu, CORESIGHT_ETM_PMU_NAME, -1); - if (ret == 0) - etm_perf_up = true; + if (ret) { + destroy_workqueue(etm_free_wq); + return ret; + }
- return ret; + etm_perf_up = true; + return 0; }
void etm_perf_exit(void) { perf_pmu_unregister(&etm_pmu); + destroy_workqueue(etm_free_wq); +} + +void etm_perf_flush_workqueue(void) +{ + flush_workqueue(etm_free_wq); } diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 24d929428633..86e259cc1adf 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -116,5 +116,6 @@ int etm_perf_add_symlink_cscfg(struct device *dev, void etm_perf_del_symlink_cscfg(struct cscfg_config_desc *config_desc); int __init etm_perf_init(void); void etm_perf_exit(void); +void etm_perf_flush_workqueue(void);
#endif
On Tue, Jul 28, 2026 at 04:00:15PM +0100, James Clark wrote:
The work queue is never flushed on module unload, so pending work items can run after coresight-config is cleaned up or the .text section is unloaded. The global workqueue also can't be flushed manually, so change it to a local workqueue and flush it on exit.
Why can't we drop the workqueue if it introduces race conditions and complexity? In particular, I don't see much benefit from deferring the resource release.
According to the comment in __rb_free_aux(), the free_aux() callback should never be called from atomic context. It should be safe to release the resources directly from the free_aux() callback, without going through a workqueue.
Thanks, Leo
On 13/08/2026 15:19, Leo Yan wrote:
On Tue, Jul 28, 2026 at 04:00:15PM +0100, James Clark wrote:
The work queue is never flushed on module unload, so pending work items can run after coresight-config is cleaned up or the .text section is unloaded. The global workqueue also can't be flushed manually, so change it to a local workqueue and flush it on exit.
Why can't we drop the workqueue if it introduces race conditions and complexity? In particular, I don't see much benefit from deferring the resource release.
According to the comment in __rb_free_aux(), the free_aux() callback should never be called from atomic context. It should be safe to release the resources directly from the free_aux() callback, without going through a workqueue.
Thanks, Leo
Nice catch. This was fixed in Perf core in 95ff4ca26c49, very shortly after the Coresight Perf mode was merged. I'll change this commit to drop the workqueue completely.
Sinks are potentially shared between multiple ETMs, and even with Coresight being an exclusive PMU, multiple PERF_ATTACH_TASK events in parallel are supported. If both of the events are owned by the same PID then the sink becomes shared resulting in trace from the wrong thread appearing in a per-thread buffer and WARNs in the driver being hit:
$ perf test -w named_threads 2 10 & $ perf record -e cs_etm//u --per-thread --pid 984
WARNING: drivers/hwtracing/coresight/coresight-tmc-etr.c:1654 at tmc_update_etr_buffer+0x328/0x348 [coresight_tmc], CPU#1: perf/686 [...] Call trace: tmc_update_etr_buffer+0x328/0x348 [coresight_tmc] (P) etm_event_stop+0x1c8/0x260 [coresight] etm_event_del+0x20/0x38 [coresight] event_sched_out+0xc4/0x1c8 group_sched_out+0x5c/0x170 __pmu_ctx_sched_out+0xe8/0x148 ctx_sched_out+0x12c/0x178 perf_event_exit_task_context+0xb0/0x3a8 perf_event_exit_task+0xa4/0x138 do_exit+0x1cc/0x808 do_group_exit+0x7c/0xb0 get_signal+0x628/0x678 arch_do_signal_or_restart+0x98/0x1b28 exit_to_user_mode_loop+0x90/0x188 el0_svc+0x1cc/0x260 el0t_64_sync_handler+0x78/0x130 el0t_64_sync+0x198/0x1a0
Fix it by returning -EBUSY if an incompatible event tries to use the same sink. Users will only be able to use per-thread mode reliably with multiple threads on a system that has a dedicated sink per ETM. This limitation hasn't changed since this fix, it just makes the behavior more consistent.
The identity and compatibility of a session are determined by the owning process, the target process, and the inheritance settings together.
"Owning process" is a reference to the TGID of the caller of etm_setup_aux(), so any thread in that group can access the events. It has to be pinned at this point rather than checked at runtime via event->owner so that we can handle scenarios like event FDs being passed to child threads and original owners exiting.
The "target process" is determined by taking a reference to event->hw.target in etm_setup_aux(), which is always called on the root event, so is unaffected by inheritance. Perf holds a reference to that task struct as long as the event exists, so it can be safely taken even if the task isn't live by the time mmap is called.
The remaining inheritance settings are used to determine if a PERF_ATTACH_TASK event tracks the same child threads as another event with the same target on a different CPU. If not, then sinks can't be shared to avoid unexpected trace appearing from a child when inherit=false on one but not another.
Fixes: 8d03cfd16a72 ("coresight: tmc-etr: Add support for CPU-wide trace scenarios") Signed-off-by: James Clark james.clark@linaro.org --- drivers/hwtracing/coresight/coresight-etm-perf.c | 53 ++++++++++++++++++++++++ drivers/hwtracing/coresight/coresight-etm-perf.h | 14 +++++++ drivers/hwtracing/coresight/coresight-tmc-etr.c | 20 +++------ drivers/hwtracing/coresight/coresight-tmc.h | 3 ++ include/linux/coresight.h | 1 + 5 files changed, 77 insertions(+), 14 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 7fb5c3d18bd5..28cf319a39da 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -251,6 +251,9 @@ static void free_event_data(struct work_struct *work) }
free_percpu(event_data->path); + put_pid(event_data->session_id.owner); + if (event_data->session_id.target) + put_task_struct(event_data->session_id.target); kfree(event_data); }
@@ -452,6 +455,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, event_data->cfg_hash = cfg_hash; }
+ /* + * Save an identity that determines if sessions are equivalent enough + * for shared sinks to receive trace from other events. + */ + event_data->session_id.owner = get_task_pid(current, PIDTYPE_TGID); + if (event->attach_state & PERF_ATTACH_TASK) { + event_data->session_id.target = get_task_struct(event->hw.target); + event_data->session_id.inherit = event->attr.inherit; + event_data->session_id.inherit_thread = event->attr.inherit_thread; + } mask = &event_data->mask;
/* @@ -1078,3 +1091,43 @@ void etm_perf_flush_workqueue(void) { flush_workqueue(etm_free_wq); } + +bool etm_perf_compare_session(struct etm_session_id *a, + struct etm_session_id *b) +{ + return a->owner == b->owner && + a->target == b->target && + a->inherit == b->inherit && + a->inherit_thread == b->inherit_thread; +} +EXPORT_SYMBOL_GPL(etm_perf_compare_session); + +/* + * Returns true if a sink that's potentially connected to multiple ETMs can be + * used in parallel with another event. + */ +bool etm_perf_sink_can_share(struct coresight_device *csdev, + struct etm_session_id *owner, + struct perf_output_handle *new) +{ + struct etm_event_data *new_event_data = perf_get_aux(new); + struct etm_session_id *new_id = &new_event_data->session_id; + + /* First user can always use the sink */ + if (csdev->refcnt == 0) + return true; + + /* + * Only allow sharing if both events have the same owner and are + * expected to follow the exact same set of processes. For CPU wide + * events, both new and current 'target's are NULL so sharing between + * all events is allowed. + * + * When 'target' mismatches, trace from two different PERF_ATTACH_TASK + * events is prevented from appearing in a buffer targeting another + * process. Inherit flags change which set of processes are followed, + * even for events with the same 'target', so they must also be checked. + */ + return etm_perf_compare_session(owner, new_id); +} +EXPORT_SYMBOL_GPL(etm_perf_sink_can_share); diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 86e259cc1adf..87b3ba72d07c 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -82,6 +82,13 @@ struct etm_filters { bool ssstatus; };
+struct etm_session_id { + struct pid *owner; + struct task_struct *target; + bool inherit; + bool inherit_thread; +}; + /** * struct etm_event_data - Coresight specifics associated to an event * @work: Handle to free allocated memory outside IRQ context. @@ -90,6 +97,7 @@ struct etm_filters { * @snk_config: The sink configuration. * @cfg_hash: The hash id of any coresight config selected. * @path: An array of path, each slot for one CPU. + * @session_id: Reference to the session that called setup_aux() */ struct etm_event_data { struct work_struct work; @@ -98,6 +106,7 @@ struct etm_event_data { void *snk_config; u32 cfg_hash; struct coresight_path * __percpu *path; + struct etm_session_id session_id; };
int etm_perf_symlink(struct coresight_device *csdev, bool link); @@ -117,5 +126,10 @@ void etm_perf_del_symlink_cscfg(struct cscfg_config_desc *config_desc); int __init etm_perf_init(void); void etm_perf_exit(void); void etm_perf_flush_workqueue(void); +bool etm_perf_compare_session(struct etm_session_id *a, + struct etm_session_id *b); +bool etm_perf_sink_can_share(struct coresight_device *csdev, + struct etm_session_id *owner, + struct perf_output_handle *new);
#endif diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index be0bbe036d02..be5ed04a554f 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1746,10 +1746,10 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, struct coresight_path *path) { int rc = 0; - pid_t pid; unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct perf_output_handle *handle = path->handle; + struct etm_event_data *event_data = perf_get_aux(handle); struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
raw_spin_lock_irqsave(&drvdata->spinlock, flags); @@ -1764,20 +1764,14 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, goto unlock_out; }
- /* Get a handle on the pid of the session owner */ - pid = etr_perf->pid; - /* Do not proceed if this device is associated with another session */ - if (drvdata->pid != -1 && drvdata->pid != pid) { + if (!etm_perf_sink_can_share(csdev, &drvdata->perf_session, handle)) { rc = -EBUSY; goto unlock_out; }
- /* - * No HW configuration is needed if the sink is already in - * use for this session. - */ - if (drvdata->pid == pid) { + /* No HW configuration is needed if the sink is already in use. */ + if (csdev->refcnt) { csdev->refcnt++; goto unlock_out; } @@ -1796,10 +1790,9 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev,
rc = tmc_etr_enable_hw(drvdata, etr_perf->etr_buf); if (!rc) { - /* Associate with monitored process. */ - drvdata->pid = pid; coresight_set_mode(csdev, CS_MODE_PERF); drvdata->perf_buf = etr_perf->etr_buf; + drvdata->perf_session = event_data->session_id; csdev->refcnt++;
/* A new Perf session clears an old sysfs one if the buffer is shared */ @@ -1853,11 +1846,10 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) /* Complain if we (somehow) got out of sync */ WARN_ON_ONCE(coresight_get_mode(csdev) == CS_MODE_DISABLED); tmc_etr_disable_hw(drvdata); - /* Dissociate from monitored process. */ - drvdata->pid = -1; coresight_set_mode(csdev, CS_MODE_DISABLED); /* Reset perf specific data */ drvdata->perf_buf = NULL; + drvdata->perf_session = (struct etm_session_id) {};
raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index dc1a57ab8011..9b153b7910fb 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -13,6 +13,7 @@ #include <linux/mutex.h> #include <linux/refcount.h> #include <linux/crc32.h> +#include "coresight-etm-perf.h"
#define TMC_RSZ 0x004 #define TMC_STS 0x00c @@ -248,6 +249,7 @@ struct tmc_resrv_buf { * (after crash) by default. * @crash_mdata: Reserved memory for storing tmc crash metadata. * Used by ETR/ETF. + * @perf_session: Session identity of the Perf event currently using this sink. */ struct tmc_drvdata { struct clk *atclk; @@ -278,6 +280,7 @@ struct tmc_drvdata { struct etr_buf *perf_buf; struct tmc_resrv_buf resrv_buf; struct tmc_resrv_buf crash_mdata; + struct etm_session_id perf_session; };
struct etr_buf_operations { diff --git a/include/linux/coresight.h b/include/linux/coresight.h index add0579cad88..1c06a9800cfe 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -715,4 +715,5 @@ int coresight_etm_get_trace_id(struct coresight_device *csdev, enum cs_mode mode struct coresight_device *sink); int coresight_get_enable_clocks(struct device *dev, struct clk **pclk, struct clk **atclk); + #endif /* _LINUX_COREISGHT_H */
On Tue, Jul 28, 2026 at 04:00:16PM +0100, James Clark wrote:
[...]
The identity and compatibility of a session are determined by the owning process, the target process, and the inheritance settings together.
Could you elaborate a bit on the session model you are trying to support here?
My understanding of a legacy sink such as ETR/ETF is that it can be owned by only one perf session at a time. Multiple events belonging to that session may use the sink, but an attempt from another perf session should be rejected with -EBUSY.
For per-thread mode, I wonder if we could support _only_ non-inherited events. In that case, we would not need to determine compatibility based on the target process and inheritance settings.
From the patch, it seems trying to support a more flexible model, where an already active legacy sink can also be shared by another perf session and also support inherit mode for child events?
Thanks, Leo
On 13/08/2026 17:05, Leo Yan wrote:
On Tue, Jul 28, 2026 at 04:00:16PM +0100, James Clark wrote:
[...]
The identity and compatibility of a session are determined by the owning process, the target process, and the inheritance settings together.
Could you elaborate a bit on the session model you are trying to support here?
My understanding of a legacy sink such as ETR/ETF is that it can be owned by only one perf session at a time. Multiple events belonging to that session may use the sink, but an attempt from another perf session should be rejected with -EBUSY.
For per-thread mode, I wonder if we could support _only_ non-inherited
non-inheritance is enforced for per-thread mode in the perf core. So we already don't support it.
events. In that case, we would not need to determine compatibility based on the target process and inheritance settings.
From the patch, it seems trying to support a more flexible model, where
I don't think so. It's actually less flexible than what exists currently. Currently we only check the PID of the owner, and all sharing is allowed. That means there are a lot of combinations of things that result in sharing that are invalid. For example PID re-use and incompatible per-thread events, like the original bug.
an already active legacy sink can also be shared by another perf session
There isn't any sharing with "another perf session", unless there is a mistake somewhere? Checking that the owners are equivalent enforces this. Or do you mean another event owned by the same process?
and also support inherit mode for child events?
Thanks, Leo
We need to fix the bugs but still support one process owning multiple PERF_ATTACH_TASK events, because that's what you get when you do this:
perf record -e cs_etm// -- my_task
Perf doesn't do one per-thread event here, it's is still PERF_ATTACH_TASK, but with a separate event opened for each CPU. So we need a model that still supports this to not cause a regression, but doesn't have any sharing bugs.
I'm not sure the exact model you had in mind was that still supports this and fixes the bugs? The one in this change is pretty complete and only does 4 comparisons, which seems quite simple to me. It only has one shortcoming that I'm aware of, that it doesn't take into account the cgroup settings, but I don't see a use case where that needs to be solved. Even if different events with different cgroup settings should would follow different processes, the owning process would have to do something quite crazy to get there.
On Fri, Aug 14, 2026 at 10:09:01AM +0100, James Clark wrote:
[...]
There isn't any sharing with "another perf session", unless there is a mistake somewhere? Checking that the owners are equivalent enforces this. Or do you mean another event owned by the same process?
Now I understand that the problem is constrained to different events within the same session.
I'm not sure the exact model you had in mind was that still supports this and fixes the bugs?
Let me try to describe my understanding of the problem.
./perf test -w named_threads 2 1000000 & ./perf record -e cs_etm//u --per-thread --pid $!
We can simplify the flow as:
| T1 | CPU0 ------------------------------ | T2 | CPU1 ------------------------------ `> T2 stops and the driver reports the warning when trying to sync ETR_BUF(T1), while T2 is associated with ETR_BUF(T2).
AUX_BUF(T1) | | ETR_BUF(T1) | Bounce buf0 | -> Used by H/W trace
AUX_BUF(T2) | | ETR_BUF(T2) | Bounce buf1 | -> Not used by H/W trace
With `--per-thread --pid $PID`, perf creates separate events for the child threads, say T1 and T2. Perf allocates a separate AUX buffer for each event, and the ETR driver also allocates a separate bounce buffer for each event. However, because there is only one shared ETR sink, only one of those bounce buffers can actually be used by the hardware at a time.
If T1 stops while T2 is still running, the ETR remains enabled. Later, when T2 stops, the ETR is still using ETR_BUF(T1). This mismatch triggers the warning and prevents the data from being copied.
I am just wandering if we can improve the sink driver to only allocate a single bounce buffer that is independent of any threads (and any associated events).
| T1 | CPU0 ------------------------------ | T2 | CPU1 ------------------------------ `> T2 stops and can sync trace from the shared bounce buffer to AUX_BUF(T2).
AUX_BUF(T1) | | AUX_BUF(T2) | |
ETR_BUF | Bounce buf | -> Used by H/W trace
This might also simplify the CPU-wide case. Each CPU would still have its own AUX buffer, but the ETR driver would maintain only one bounce buffer for the shared sink. A reference count could track how many events are using the sink, with the final event responsible for stopping the sink and copying the trace data from bounce buffer to aux buffer.
The one in this change is pretty complete and only does 4 comparisons, which seems quite simple to me.
Before going further with the heavily sink buffer refactoring, perhaps a more pragmatic solution would be to reject the problematic case for now. Can we do something like below?
+void coresight_trace_id_is_perf_started(struct coresight_trace_id_map *id_map) +{ + PERF_SESSION(atomic_read(&id_map->perf_cs_etm_session_active)); +}
@@ -399,6 +399,15 @@ etm_event_build_path(struct perf_event *event, int cpu, goto out; }
+ if (!coresight_trace_id_is_perf_started(&sink->perf_sink_id_map)) { + sink->perf_owner = event->owner; + sink->perf_target = event->hw.target; + } else { + if (sink->perf_owner != event->owner || + sink->perf_target != event->hw.target) + goto out; + } +
We use a central place etm_event_build_path() to record and compare event's owner and target process, then we don't need to spread the check into sink drivers. We only care about if owner and target must be consistent.
Regard of the inherit/inherit_thread, I always see they are consistent within the same session. Should we ignore them?
Thanks, Leo
On 19/08/2026 09:45, Leo Yan wrote:
On Fri, Aug 14, 2026 at 10:09:01AM +0100, James Clark wrote:
[...]
There isn't any sharing with "another perf session", unless there is a mistake somewhere? Checking that the owners are equivalent enforces this. Or do you mean another event owned by the same process?
Now I understand that the problem is constrained to different events within the same session.
I'm not sure the exact model you had in mind was that still supports this and fixes the bugs?
Let me try to describe my understanding of the problem.
./perf test -w named_threads 2 1000000 & ./perf record -e cs_etm//u --per-thread --pid $!
We can simplify the flow as:
| T1 |CPU0 ------------------------------ | T2 | CPU1 ------------------------------ `> T2 stops and the driver reports the warning when trying to sync ETR_BUF(T1), while T2 is associated with ETR_BUF(T2).
AUX_BUF(T1) | | ETR_BUF(T1) | Bounce buf0 | -> Used by H/W trace
AUX_BUF(T2) | | ETR_BUF(T2) | Bounce buf1 | -> Not used by H/W trace
With `--per-thread --pid $PID`, perf creates separate events for the child threads, say T1 and T2. Perf allocates a separate AUX buffer for each event, and the ETR driver also allocates a separate bounce buffer for each event. However, because there is only one shared ETR sink, only one of those bounce buffers can actually be used by the hardware at a time.
If T1 stops while T2 is still running, the ETR remains enabled. Later, when T2 stops, the ETR is still using ETR_BUF(T1). This mismatch triggers the warning and prevents the data from being copied.
I am just wandering if we can improve the sink driver to only allocate a single bounce buffer that is independent of any threads (and any associated events).
| T1 |CPU0 ------------------------------ | T2 | CPU1 ------------------------------ `> T2 stops and can sync trace from the shared bounce buffer to AUX_BUF(T2).
AUX_BUF(T1) | | AUX_BUF(T2) | |
ETR_BUF | Bounce buf | -> Used by H/W trace
This might also simplify the CPU-wide case. Each CPU would still have its own AUX buffer, but the ETR driver would maintain only one bounce buffer for the shared sink. A reference count could track how many events are using the sink, with the final event responsible for
Isn't this how it's already working? get_perf_etr_buf_cpu_wide() allocates a single shared buffer with a refcount. I didn't change this, I only changed the rules about what is considered shared or not so that it matches the semantics of the perf events that back the tracing session.
stopping the sink and copying the trace data from bounce buffer to aux buffer.
The one in this change is pretty complete and only does 4 comparisons, which seems quite simple to me.
Before going further with the heavily sink buffer refactoring, perhaps a more pragmatic solution would be to reject the problematic case for now. Can we do something like below?
+void coresight_trace_id_is_perf_started(struct coresight_trace_id_map *id_map) +{
- PERF_SESSION(atomic_read(&id_map->perf_cs_etm_session_active));
+}
@@ -399,6 +399,15 @@ etm_event_build_path(struct perf_event *event, int cpu, goto out; }
- if (!coresight_trace_id_is_perf_started(&sink->perf_sink_id_map)) {
sink->perf_owner = event->owner;sink->perf_target = event->hw.target;- } else {
if (sink->perf_owner != event->owner ||sink->perf_target != event->hw.target)goto out;- }
We use a central place etm_event_build_path() to record and compare event's owner and target process, then we don't need to spread the check into sink drivers. We only care about if owner and target must be consistent.
But we don't know where the target will run when the event is created. That's why the check is delayed until etm_event_start() and the process has been scheduled. Where it runs needs to be taken into account to calculate if this sink can be shared.
Moving the check to event creation time would cause a regression for two users that plan to trace two different threads (or different CPUs where the processes are known to never run on a shared sink at the same time). With your example the second user is completely prohibited from opening per-thread events, but with the existing driver and this change it works. I think that's quite a significant change in functionality, what's the justification for taking those use cases away from users?
Regard of the inherit/inherit_thread, I always see they are consistent within the same session. Should we ignore them?
Do you mean they are always consistent in Perf? I don't think the driver can afford to bend the rules just because Perf promises to never do it. It might not always do that, and any tool can do perf_event_open(), not just Perf.
It's quite easy to imagine the bug report being: "I opened one event with inherit set, and one event without. Why do I get trace from other threads in my event without inherit set? I expect to see only trace from one process".
Thanks, Leo
On 20/08/2026 12:09, James Clark wrote:
On 19/08/2026 09:45, Leo Yan wrote:
On Fri, Aug 14, 2026 at 10:09:01AM +0100, James Clark wrote:
[...]
There isn't any sharing with "another perf session", unless there is a mistake somewhere? Checking that the owners are equivalent enforces this. Or do you mean another event owned by the same process?
Now I understand that the problem is constrained to different events within the same session.
I'm not sure the exact model you had in mind was that still supports this and fixes the bugs?
Let me try to describe my understanding of the problem.
./perf test -w named_threads 2 1000000 & ./perf record -e cs_etm//u --per-thread --pid $!
We can simplify the flow as:
| T1 | CPU0 ------------------------------ | T2 | CPU1 ------------------------------ `> T2 stops and the driver reports the warning when trying to sync ETR_BUF(T1), while T2 is associated with ETR_BUF(T2).
AUX_BUF(T1) | | ETR_BUF(T1) | Bounce buf0 | -> Used by H/W trace
AUX_BUF(T2) | | ETR_BUF(T2) | Bounce buf1 | -> Not used by H/W trace
With `--per-thread --pid $PID`, perf creates separate events for the child threads, say T1 and T2. Perf allocates a separate AUX buffer for each event, and the ETR driver also allocates a separate bounce buffer for each event. However, because there is only one shared ETR sink, only one of those bounce buffers can actually be used by the hardware at a time.
If T1 stops while T2 is still running, the ETR remains enabled. Later, when T2 stops, the ETR is still using ETR_BUF(T1). This mismatch triggers the warning and prevents the data from being copied.
I am just wandering if we can improve the sink driver to only allocate a single bounce buffer that is independent of any threads (and any associated events).
| T1 | CPU0 ------------------------------ | T2 | CPU1 ------------------------------ `> T2 stops and can sync trace from the shared bounce buffer to AUX_BUF(T2).
AUX_BUF(T1) | | AUX_BUF(T2) | |
ETR_BUF | Bounce buf | -> Used by H/W trace
This might also simplify the CPU-wide case. Each CPU would still have its own AUX buffer, but the ETR driver would maintain only one bounce buffer for the shared sink. A reference count could track how many events are using the sink, with the final event responsible for
Isn't this how it's already working? get_perf_etr_buf_cpu_wide() allocates a single shared buffer with a refcount. I didn't change this, I only changed the rules about what is considered shared or not so that it matches the semantics of the perf events that back the tracing session.
stopping the sink and copying the trace data from bounce buffer to aux buffer.
The one in this change is pretty complete and only does 4 comparisons, which seems quite simple to me.
Before going further with the heavily sink buffer refactoring, perhaps a more pragmatic solution would be to reject the problematic case for now. Can we do something like below?
+void coresight_trace_id_is_perf_started(struct coresight_trace_id_map *id_map) +{ + PERF_SESSION(atomic_read(&id_map->perf_cs_etm_session_active)); +}
@@ -399,6 +399,15 @@ etm_event_build_path(struct perf_event *event, int cpu, goto out; } + if (!coresight_trace_id_is_perf_started(&sink->perf_sink_id_map)) { + sink->perf_owner = event->owner; + sink->perf_target = event->hw.target; + } else { + if (sink->perf_owner != event->owner || + sink->perf_target != event->hw.target) + goto out; + }
We use a central place etm_event_build_path() to record and compare event's owner and target process, then we don't need to spread the check into sink drivers. We only care about if owner and target must be consistent.
But we don't know where the target will run when the event is created. That's why the check is delayed until etm_event_start() and the process has been scheduled. Where it runs needs to be taken into account to calculate if this sink can be shared.
Moving the check to event creation time would cause a regression for two users that plan to trace two different threads (or different CPUs where the processes are known to never run on a shared sink at the same time). With your example the second user is completely prohibited from opening per-thread events, but with the existing driver and this change it works. I think that's quite a significant change in functionality, what's the justification for taking those use cases away from users?
Another way to put it is that the current code checks the PID, and I made it also check the target PID in the same place. That's the extent of the change.
Everything else are minor fixes to things that were already broken like we shouldn't be comparing PIDs numerically because they can be re-used. I can't see how adding one more condition to an existing comparison is complicated or is too big of a change.
Regard of the inherit/inherit_thread, I always see they are consistent within the same session. Should we ignore them?
Do you mean they are always consistent in Perf? I don't think the driver can afford to bend the rules just because Perf promises to never do it. It might not always do that, and any tool can do perf_event_open(), not just Perf.
It's quite easy to imagine the bug report being: "I opened one event with inherit set, and one event without. Why do I get trace from other threads in my event without inherit set? I expect to see only trace from one process".
Thanks, Leo
Comparing numeric PIDs for ownership no longer matches the sharing rules in coresight_sink_can_share(), change it to compare the session identity instead.
Now that we're not using a numeric PID for ownership we can't use the IDR map. Change it to a simple linear realloc'd array, it's unlikely the performance difference is measurable, systems wouldn't have more than a handful of shared sources for one sink, and this is only accessed once at setup.
Assisted-by: Codex:GPT-5.6-Sol Fixes: 8d03cfd16a72 ("coresight: tmc-etr: Add support for CPU-wide trace scenarios") Signed-off-by: James Clark james.clark@linaro.org --- drivers/hwtracing/coresight/coresight-etb10.c | 3 +- drivers/hwtracing/coresight/coresight-etm-perf.c | 4 +- drivers/hwtracing/coresight/coresight-tmc-core.c | 4 +- drivers/hwtracing/coresight/coresight-tmc-etf.c | 5 +- drivers/hwtracing/coresight/coresight-tmc-etr.c | 139 ++++++++++++----------- drivers/hwtracing/coresight/coresight-tmc.h | 16 ++- drivers/hwtracing/coresight/coresight-trbe.c | 3 +- drivers/hwtracing/coresight/ultrasoc-smb.c | 3 +- include/linux/coresight.h | 4 +- 9 files changed, 100 insertions(+), 81 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index a827f76b8144..6a4a1edf46ba 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -371,7 +371,8 @@ static int etb_disable(struct coresight_device *csdev) }
static void *etb_alloc_buffer(struct coresight_device *csdev, - struct perf_event *event, void **pages, + struct perf_event *event, + struct etm_session_id *owner, void **pages, int nr_pages, bool overwrite) { int node; diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 28cf319a39da..652beee2edbd 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -517,7 +517,9 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, * sinks. */ event_data->snk_config = - sink_ops(sink)->alloc_buffer(sink, event, pages, + sink_ops(sink)->alloc_buffer(sink, event, + &event_data->session_id, + pages, nr_pages, overwrite); if (!event_data->snk_config) goto err; diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index c89fe996af23..873868718b14 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -9,7 +9,6 @@ #include <linux/init.h> #include <linux/types.h> #include <linux/device.h> -#include <linux/idr.h> #include <linux/io.h> #include <linux/iommu.h> #include <linux/err.h> @@ -832,8 +831,7 @@ static int __tmc_probe(struct device *dev, struct resource *res) ret = tmc_etr_setup_caps(dev, devid, &desc.access); if (ret) goto out; - idr_init(&drvdata->idr); - mutex_init(&drvdata->idr_mutex); + mutex_init(&drvdata->perf_bufs_mutex); dev_list = "tmc_etr"; break; case TMC_CONFIG_TYPE_ETF: diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 3836063031d7..d90090b846ab 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -419,8 +419,9 @@ static void tmc_disable_etf_link(struct coresight_device *csdev, }
static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, - struct perf_event *event, void **pages, - int nr_pages, bool overwrite) + struct perf_event *event, + struct etm_session_id *owner, + void **pages, int nr_pages, bool overwrite) { int node; struct cs_buffers *buf; diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index be5ed04a554f..324d94e6d7e1 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -8,7 +8,6 @@ #include <linux/coresight.h> #include <linux/dma-mapping.h> #include <linux/iommu.h> -#include <linux/idr.h> #include <linux/mutex.h> #include <linux/refcount.h> #include <linux/slab.h> @@ -37,8 +36,6 @@ struct etr_buf_hw { * etr_perf_buffer - Perf buffer used for ETR * @drvdata - The ETR drvdaga this buffer has been allocated for. * @etr_buf - Actual buffer used by the ETR - * @pid - The PID of the session owner that etr_perf_buffer - * belongs to. * @snaphost - Perf session mode * @nr_pages - Number of pages in the ring buffer. * @pages - Array of Pages in the ring buffer. @@ -46,7 +43,6 @@ struct etr_buf_hw { struct etr_perf_buffer { struct tmc_drvdata *drvdata; struct etr_buf *etr_buf; - pid_t pid; bool snapshot; int nr_pages; void **pages; @@ -1396,16 +1392,16 @@ alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event, return ERR_PTR(-ENOMEM); }
-static struct etr_buf * -get_perf_etr_buf_cpu_wide(struct tmc_drvdata *drvdata, - struct perf_event *event, int nr_pages, - void **pages, bool snapshot) +static struct etr_buf *get_perf_etr_buf_cpu_wide(struct tmc_drvdata *drvdata, + struct perf_event *event, + struct etm_session_id *owner, + int nr_pages, void **pages, + bool snapshot) { - int ret; - pid_t pid = task_pid_nr(event->owner); - struct etr_buf *etr_buf; + struct etr_buf_mapping *perf_bufs; + struct etr_buf *etr_buf, *existing; + unsigned int i;
-retry: /* * An etr_perf_buffer is associated with an event and holds a reference * to the AUX ring buffer that was created for that event. In CPU-wide @@ -1424,69 +1420,75 @@ get_perf_etr_buf_cpu_wide(struct tmc_drvdata *drvdata, * allocated for this session. If so it is shared with this event, * otherwise it is created. */ - mutex_lock(&drvdata->idr_mutex); - etr_buf = idr_find(&drvdata->idr, pid); - if (etr_buf) { - refcount_inc(&etr_buf->refcount); - mutex_unlock(&drvdata->idr_mutex); - return etr_buf; + mutex_lock(&drvdata->perf_bufs_mutex); + for (i = 0; i < drvdata->nr_perf_bufs; i++) { + if (etm_perf_compare_session(&drvdata->perf_bufs[i].owner, owner)) { + etr_buf = drvdata->perf_bufs[i].buf; + refcount_inc(&etr_buf->refcount); + mutex_unlock(&drvdata->perf_bufs_mutex); + return etr_buf; + } }
/* If we made it here no buffer has been allocated, do so now. */ - mutex_unlock(&drvdata->idr_mutex); + mutex_unlock(&drvdata->perf_bufs_mutex);
etr_buf = alloc_etr_buf(drvdata, event, nr_pages, pages, snapshot); if (IS_ERR(etr_buf)) return etr_buf;
- /* Now that we have a buffer, add it to the IDR. */ - mutex_lock(&drvdata->idr_mutex); - ret = idr_alloc(&drvdata->idr, etr_buf, pid, pid + 1, GFP_KERNEL); - mutex_unlock(&drvdata->idr_mutex); - - /* Another event with this session ID has allocated this buffer. */ - if (ret == -ENOSPC) { - tmc_free_etr_buf(etr_buf); - goto retry; + mutex_lock(&drvdata->perf_bufs_mutex); + for (i = 0; i < drvdata->nr_perf_bufs; i++) { + if (etm_perf_compare_session(&drvdata->perf_bufs[i].owner, owner)) { + existing = drvdata->perf_bufs[i].buf; + refcount_inc(&existing->refcount); + mutex_unlock(&drvdata->perf_bufs_mutex); + tmc_free_etr_buf(etr_buf); + return existing; + } }
- /* The IDR can't allocate room for a new session, abandon ship. */ - if (ret == -ENOMEM) { + perf_bufs = krealloc_array(drvdata->perf_bufs, + drvdata->nr_perf_bufs + 1, + sizeof(*perf_bufs), GFP_KERNEL); + if (!perf_bufs) { + mutex_unlock(&drvdata->perf_bufs_mutex); tmc_free_etr_buf(etr_buf); - return ERR_PTR(ret); + return ERR_PTR(-ENOMEM); }
+ drvdata->perf_bufs = perf_bufs; + perf_bufs[drvdata->nr_perf_bufs].owner = *owner; + perf_bufs[drvdata->nr_perf_bufs].buf = etr_buf; + drvdata->nr_perf_bufs++; + mutex_unlock(&drvdata->perf_bufs_mutex);
return etr_buf; }
-static struct etr_buf * -get_perf_etr_buf_per_thread(struct tmc_drvdata *drvdata, - struct perf_event *event, int nr_pages, - void **pages, bool snapshot) +static struct etr_buf *get_perf_etr_buf(struct tmc_drvdata *drvdata, + struct perf_event *event, + struct etm_session_id *owner, + int nr_pages, void **pages, + bool snapshot) { /* - * In per-thread mode the etr_buf isn't shared, so just go ahead - * with memory allocation. + * Events without a CPU must have inherit=false so always target a + * single process. etm_perf_sink_can_share() prevents other single + * process events from sharing sinks, so take a shortcut and go ahead + * with memory allocation without using shared drvdata->perf_bufs. */ - return alloc_etr_buf(drvdata, event, nr_pages, pages, snapshot); -} - -static struct etr_buf * -get_perf_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event, - int nr_pages, void **pages, bool snapshot) -{ if (event->cpu == -1) - return get_perf_etr_buf_per_thread(drvdata, event, nr_pages, - pages, snapshot); + return alloc_etr_buf(drvdata, event, nr_pages, pages, snapshot);
- return get_perf_etr_buf_cpu_wide(drvdata, event, nr_pages, + return get_perf_etr_buf_cpu_wide(drvdata, event, owner, nr_pages, pages, snapshot); }
static struct etr_perf_buffer * tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, struct perf_event *event, - int nr_pages, void **pages, bool snapshot) + struct etm_session_id *owner, int nr_pages, void **pages, + bool snapshot) { int node; struct etr_buf *etr_buf; @@ -1498,7 +1500,8 @@ tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, struct perf_event *event, if (!etr_perf) return ERR_PTR(-ENOMEM);
- etr_buf = get_perf_etr_buf(drvdata, event, nr_pages, pages, snapshot); + etr_buf = get_perf_etr_buf(drvdata, event, owner, nr_pages, pages, + snapshot); if (!IS_ERR(etr_buf)) goto done;
@@ -1508,7 +1511,7 @@ tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, struct perf_event *event, done: /* * Keep a reference to the ETR this buffer has been allocated for - * in order to have access to the IDR in tmc_free_etr_buffer(). + * in order to have access to the buffer array in tmc_free_etr_buffer(). */ etr_perf->drvdata = drvdata; etr_perf->etr_buf = etr_buf; @@ -1516,22 +1519,21 @@ tmc_etr_setup_perf_buf(struct tmc_drvdata *drvdata, struct perf_event *event, return etr_perf; }
- static void *tmc_alloc_etr_buffer(struct coresight_device *csdev, - struct perf_event *event, void **pages, + struct perf_event *event, + struct etm_session_id *owner, void **pages, int nr_pages, bool snapshot) { struct etr_perf_buffer *etr_perf; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- etr_perf = tmc_etr_setup_perf_buf(drvdata, event, + etr_perf = tmc_etr_setup_perf_buf(drvdata, event, owner, nr_pages, pages, snapshot); if (IS_ERR(etr_perf)) { dev_dbg(&csdev->dev, "Unable to allocate ETR buffer\n"); return NULL; }
- etr_perf->pid = task_pid_nr(event->owner); etr_perf->snapshot = snapshot; etr_perf->nr_pages = nr_pages; etr_perf->pages = pages; @@ -1543,28 +1545,33 @@ static void tmc_free_etr_buffer(void *config) { struct etr_perf_buffer *etr_perf = config; struct tmc_drvdata *drvdata = etr_perf->drvdata; - struct etr_buf *buf, *etr_buf = etr_perf->etr_buf; + struct etr_buf *etr_buf = etr_perf->etr_buf; + unsigned int i;
if (!etr_buf) goto free_etr_perf_buffer;
- mutex_lock(&drvdata->idr_mutex); + mutex_lock(&drvdata->perf_bufs_mutex); /* If we are not the last one to use the buffer, don't touch it. */ if (!refcount_dec_and_test(&etr_buf->refcount)) { - mutex_unlock(&drvdata->idr_mutex); + mutex_unlock(&drvdata->perf_bufs_mutex); goto free_etr_perf_buffer; }
- /* We are the last one, remove from the IDR and free the buffer. */ - buf = idr_remove(&drvdata->idr, etr_perf->pid); - mutex_unlock(&drvdata->idr_mutex); + for (i = 0; i < drvdata->nr_perf_bufs; i++) { + if (drvdata->perf_bufs[i].buf != etr_buf) + continue;
- /* - * Something went very wrong if the buffer associated with this ID - * is not the same in the IDR. Leak to avoid use after free. - */ - if (buf && WARN_ON(buf != etr_buf)) - goto free_etr_perf_buffer; + drvdata->nr_perf_bufs--; + drvdata->perf_bufs[i] = + drvdata->perf_bufs[drvdata->nr_perf_bufs]; + if (!drvdata->nr_perf_bufs) { + kfree(drvdata->perf_bufs); + drvdata->perf_bufs = NULL; + } + break; + } + mutex_unlock(&drvdata->perf_bufs_mutex);
tmc_free_etr_buf(etr_perf->etr_buf);
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 9b153b7910fb..0125f2ad78ed 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -8,7 +8,6 @@ #define _CORESIGHT_TMC_H
#include <linux/dma-mapping.h> -#include <linux/idr.h> #include <linux/miscdevice.h> #include <linux/mutex.h> #include <linux/refcount.h> @@ -192,6 +191,11 @@ struct etr_buf { void *private; };
+struct etr_buf_mapping { + struct etm_session_id owner; + struct etr_buf *buf; +}; + /** * @paddr : Start address of reserved memory region. * @vaddr : Corresponding CPU virtual address. @@ -239,8 +243,9 @@ struct tmc_resrv_buf { * @etr_caps: Bitmask of capabilities of the TMC ETR, inferred from the * device configuration register (DEVID) * @etr_mode: User preferred mode of the ETR device, default auto mode. - * @idr: Holds etr_bufs allocated for this ETR. - * @idr_mutex: Access serialisation for idr. + * @perf_bufs: CPU-wide Perf buffers and their owners. + * @nr_perf_bufs: Number of entries in @perf_bufs. + * @perf_bufs_mutex: Access serialisation for @perf_bufs. * @sysfs_buf: SYSFS buffer for ETR. * @perf_buf: PERF buffer for ETR. * @resrv_buf: Used by ETR as hardware trace buffer and for trace data @@ -274,8 +279,9 @@ struct tmc_drvdata { u32 trigger_cntr; u32 etr_caps; enum etr_mode etr_mode; - struct idr idr; - struct mutex idr_mutex; + struct etr_buf_mapping *perf_bufs; + unsigned int nr_perf_bufs; + struct mutex perf_bufs_mutex; struct etr_buf *sysfs_buf; struct etr_buf *perf_buf; struct tmc_resrv_buf resrv_buf; diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c index c7cbca45f2de..81841dc95e0c 100644 --- a/drivers/hwtracing/coresight/coresight-trbe.c +++ b/drivers/hwtracing/coresight/coresight-trbe.c @@ -746,7 +746,8 @@ static unsigned long trbe_get_trace_size(struct perf_output_handle *handle, }
static void *arm_trbe_alloc_buffer(struct coresight_device *csdev, - struct perf_event *event, void **pages, + struct perf_event *event, + struct etm_session_id *owner, void **pages, int nr_pages, bool snapshot) { struct trbe_buf *buf; diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c index 20a950b9dd4f..f66ec53070d7 100644 --- a/drivers/hwtracing/coresight/ultrasoc-smb.c +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c @@ -302,7 +302,8 @@ static int smb_disable(struct coresight_device *csdev) }
static void *smb_alloc_buffer(struct coresight_device *csdev, - struct perf_event *event, void **pages, + struct perf_event *event, + struct etm_session_id *owner, void **pages, int nr_pages, bool overwrite) { struct cs_buffers *buf; diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 1c06a9800cfe..9e29f2cbf557 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -37,6 +37,7 @@ #define CORESIGHT_UNLOCK 0xc5acce55
extern const struct bus_type coresight_bustype; +struct etm_session_id;
enum coresight_dev_type { CORESIGHT_DEV_TYPE_SINK, @@ -371,7 +372,8 @@ struct coresight_ops_sink { struct coresight_path *path); int (*disable)(struct coresight_device *csdev); void *(*alloc_buffer)(struct coresight_device *csdev, - struct perf_event *event, void **pages, + struct perf_event *event, + struct etm_session_id *owner, void **pages, int nr_pages, bool overwrite); void (*free_buffer)(void *config); unsigned long (*update_buffer)(struct coresight_device *csdev,
Like the previous commit to ETR, ETF sinks shouldn't share per-thread events. Fix it by returning -EBUSY if a second per-thread event tries to use the same sink.
Fixes: 880af782c6e8 ("coresight: tmc-etf: Add support for CPU-wide trace scenarios") Signed-off-by: James Clark james.clark@linaro.org --- drivers/hwtracing/coresight/coresight-tmc-core.c | 2 -- drivers/hwtracing/coresight/coresight-tmc-etf.c | 23 +++++++---------------- drivers/hwtracing/coresight/coresight-tmc.h | 4 ---- 3 files changed, 7 insertions(+), 22 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index 873868718b14..2c2f2f312e8e 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -800,8 +800,6 @@ static int __tmc_probe(struct device *dev, struct resource *res) devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID); drvdata->config_type = BMVAL(devid, 6, 7); drvdata->memwidth = tmc_get_memwidth(devid); - /* This device is not associated with a session */ - drvdata->pid = -1; drvdata->etr_mode = ETR_MODE_AUTO;
if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d90090b846ab..c451f1da3cdd 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -250,11 +250,10 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, struct coresight_path *path) { int ret = 0; - pid_t pid; unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct perf_output_handle *handle = path->handle; - struct cs_buffers *buf = etm_perf_sink_config(handle); + struct etm_event_data *event_data = perf_get_aux(handle);
raw_spin_lock_irqsave(&drvdata->spinlock, flags); do { @@ -270,10 +269,8 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, break; }
- /* Get a handle on the pid of the process to monitor */ - pid = buf->pid; - - if (drvdata->pid != -1 && drvdata->pid != pid) { + if (!etm_perf_sink_can_share(csdev, &drvdata->perf_session, + handle)) { ret = -EBUSY; break; } @@ -282,19 +279,15 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, if (ret) break;
- /* - * No HW configuration is needed if the sink is already in - * use for this session. - */ - if (drvdata->pid == pid) { + /* No HW configuration is needed if the sink is already in use. */ + if (csdev->refcnt) { csdev->refcnt++; break; }
ret = tmc_etb_enable_hw(drvdata); if (!ret) { - /* Associate with monitored process. */ - drvdata->pid = pid; + drvdata->perf_session = event_data->session_id; coresight_set_mode(csdev, CS_MODE_PERF); csdev->refcnt++; } @@ -351,9 +344,8 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev) /* Complain if we (somehow) got out of sync */ WARN_ON_ONCE(coresight_get_mode(csdev) == CS_MODE_DISABLED); tmc_etb_disable_hw(drvdata); - /* Dissociate from monitored process. */ - drvdata->pid = -1; coresight_set_mode(csdev, CS_MODE_DISABLED); + drvdata->perf_session = (struct etm_session_id) {};
raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -433,7 +425,6 @@ static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, if (!buf) return NULL;
- buf->pid = task_pid_nr(event->owner); buf->snapshot = overwrite; buf->nr_pages = nr_pages; buf->data_pages = pages; diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index 0125f2ad78ed..99601ac9179d 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -223,9 +223,6 @@ struct tmc_resrv_buf { * @crashdev: specifics to handle "/dev/crash_tmc_xyz" entry for reading * crash tracedata. * @spinlock: only one at a time pls. - * @pid: Process ID of the process that owns the session that is using - * this component. For example this would be the pid of the Perf - * process. * @sysfs_reading: Sysfs mode buffer is being read through "/dev/xyz.tmc" entry. * Note: ETR has a separate software buffer for the two modes so * the device can still be read while in Perf mode if there is a @@ -264,7 +261,6 @@ struct tmc_drvdata { struct miscdevice miscdev; struct miscdevice crashdev; raw_spinlock_t spinlock; - pid_t pid; bool sysfs_reading; bool stop_on_flush; union {
Like the previous commit to ETR, ETB10 sinks shouldn't share per-thread events. Fix it by returning -EBUSY if a second per-thread event tries to use the same sink.
Fixes: 75d7dbd38824 ("coresight: etb10: Add support for CPU-wide trace scenarios") Signed-off-by: James Clark james.clark@linaro.org --- drivers/hwtracing/coresight/coresight-etb10.c | 30 +++++++-------------------- 1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 6a4a1edf46ba..ee0f20c30153 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -71,8 +71,7 @@ * @miscdev: specifics to handle "/dev/xyz.etb" entry. * @spinlock: only one at a time pls. * @reading: synchronise user space access to etb buffer. - * @pid: Process ID of the process being monitored by the session - * that is using this component. + * @perf_session: Session identity of the Perf event currently using this sink. * @buf: area of memory where ETB buffer content gets sent. * @buffer_depth: size of @buf. * @trigger_cntr: amount of words to store after a trigger. @@ -84,7 +83,7 @@ struct etb_drvdata { struct miscdevice miscdev; raw_spinlock_t spinlock; atomic_t reading; - pid_t pid; + struct etm_session_id perf_session; u8 *buf; u32 buffer_depth; u32 trigger_cntr; @@ -168,11 +167,10 @@ static int etb_enable_sysfs(struct coresight_device *csdev) static int etb_enable_perf(struct coresight_device *csdev, struct coresight_path *path) { int ret = 0; - pid_t pid; unsigned long flags; struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct perf_output_handle *handle = path->handle; - struct cs_buffers *buf = etm_perf_sink_config(handle); + struct etm_event_data *event_data = perf_get_aux(handle);
raw_spin_lock_irqsave(&drvdata->spinlock, flags);
@@ -182,19 +180,13 @@ static int etb_enable_perf(struct coresight_device *csdev, struct coresight_path goto out; }
- /* Get a handle on the pid of the process to monitor */ - pid = buf->pid; - - if (drvdata->pid != -1 && drvdata->pid != pid) { + if (!etm_perf_sink_can_share(csdev, &drvdata->perf_session, handle)) { ret = -EBUSY; goto out; }
- /* - * No HW configuration is needed if the sink is already in - * use for this session. - */ - if (drvdata->pid == pid) { + /* No HW configuration is needed if the sink is already in use. */ + if (csdev->refcnt) { csdev->refcnt++; goto out; } @@ -210,8 +202,7 @@ static int etb_enable_perf(struct coresight_device *csdev, struct coresight_path
ret = etb_enable_hw(drvdata); if (!ret) { - /* Associate with monitored process. */ - drvdata->pid = pid; + drvdata->perf_session = event_data->session_id; coresight_set_mode(drvdata->csdev, CS_MODE_PERF); csdev->refcnt++; } @@ -361,8 +352,7 @@ static int etb_disable(struct coresight_device *csdev) /* Complain if we (somehow) got out of sync */ WARN_ON_ONCE(coresight_get_mode(csdev) == CS_MODE_DISABLED); etb_disable_hw(drvdata); - /* Dissociate from monitored process. */ - drvdata->pid = -1; + drvdata->perf_session = (struct etm_session_id) {}; coresight_set_mode(csdev, CS_MODE_DISABLED); raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -384,7 +374,6 @@ static void *etb_alloc_buffer(struct coresight_device *csdev, if (!buf) return NULL;
- buf->pid = task_pid_nr(event->owner); buf->snapshot = overwrite; buf->nr_pages = nr_pages; buf->data_pages = pages; @@ -755,9 +744,6 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id) if (!drvdata->buf) return -ENOMEM;
- /* This device is not associated with a session */ - drvdata->pid = -1; - pdata = coresight_get_platform_data(dev); if (IS_ERR(pdata)) return PTR_ERR(pdata);
Like the previous commit to ETR, SMB sinks shouldn't share per-thread events. Fix it by returning -EBUSY if a second per-thread event tries to use the same sink.
Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver") Signed-off-by: James Clark james.clark@linaro.org --- drivers/hwtracing/coresight/coresight-priv.h | 2 -- drivers/hwtracing/coresight/ultrasoc-smb.c | 22 ++++++---------------- drivers/hwtracing/coresight/ultrasoc-smb.h | 6 +++--- 3 files changed, 9 insertions(+), 21 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index dddac946659f..492f050eb698 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -94,7 +94,6 @@ enum etm_addr_type { * struct cs_buffer - keep track of a recording session' specifics * @cur: index of the current buffer * @nr_pages: max number of pages granted to us - * @pid: PID this cs_buffer belongs to * @offset: offset within the current buffer * @data_size: how much we collected in this run * @snapshot: is this run in snapshot mode @@ -103,7 +102,6 @@ enum etm_addr_type { struct cs_buffers { unsigned int cur; unsigned int nr_pages; - pid_t pid; unsigned long offset; local_t data_size; bool snapshot; diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c index f66ec53070d7..ba4a7a8f6f96 100644 --- a/drivers/hwtracing/coresight/ultrasoc-smb.c +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c @@ -216,22 +216,15 @@ static int smb_enable_perf(struct coresight_device *csdev, { struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent); struct perf_output_handle *handle = path->handle; - struct cs_buffers *buf = etm_perf_sink_config(handle); - pid_t pid; + struct etm_event_data *event_data = perf_get_aux(handle);
- if (!buf) - return -EINVAL; - - /* Get a handle on the pid of the target process */ - pid = buf->pid; - - /* Device is already in used by other session */ - if (drvdata->pid != -1 && drvdata->pid != pid) + /* Do not proceed if this device is associated with another session */ + if (!etm_perf_sink_can_share(csdev, &drvdata->perf_session, handle)) return -EBUSY;
- if (drvdata->pid == -1) { + if (!csdev->refcnt) { smb_enable_hw(drvdata); - drvdata->pid = pid; + drvdata->perf_session = event_data->session_id; coresight_set_mode(csdev, CS_MODE_PERF); }
@@ -293,8 +286,7 @@ static int smb_disable(struct coresight_device *csdev)
smb_disable_hw(drvdata);
- /* Dissociate from the target process. */ - drvdata->pid = -1; + drvdata->perf_session = (struct etm_session_id) {}; coresight_set_mode(csdev, CS_MODE_DISABLED); dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
@@ -317,7 +309,6 @@ static void *smb_alloc_buffer(struct coresight_device *csdev, buf->snapshot = overwrite; buf->nr_pages = nr_pages; buf->data_pages = pages; - buf->pid = task_pid_nr(event->owner);
return buf; } @@ -565,7 +556,6 @@ static int smb_probe(struct platform_device *pdev) smb_reset_buffer(drvdata); platform_set_drvdata(pdev, drvdata); raw_spin_lock_init(&drvdata->spinlock); - drvdata->pid = -1;
ret = smb_register_sink(pdev, drvdata); if (ret) { diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h index 323f0ccb6878..491c2716b835 100644 --- a/drivers/hwtracing/coresight/ultrasoc-smb.h +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h @@ -10,6 +10,7 @@ #include <linux/bitfield.h> #include <linux/miscdevice.h> #include <linux/spinlock.h> +#include "coresight-etm-perf.h"
/* Offset of SMB global registers */ #define SMB_GLB_CFG_REG 0x00 @@ -108,8 +109,7 @@ struct smb_data_buffer { * @miscdev: Specifics to handle "/dev/xyz.smb" entry. * @spinlock: Control data access to one at a time. * @reading: Synchronise user space access to SMB buffer. - * @pid: Process ID of the process being monitored by the - * session that is using this component. + * @perf_session: Session identity of the Perf event currently using this sink. */ struct smb_drv_data { void __iomem *base; @@ -118,7 +118,7 @@ struct smb_drv_data { struct miscdevice miscdev; raw_spinlock_t spinlock; bool reading; - pid_t pid; + struct etm_session_id perf_session; };
#endif
-
James Clark -
Leo Yan