Hi Yingchao,
On Wed, Apr 15, 2026 at 11:22:49AM +0800, Yingchao Deng (Consultant) wrote:
[...]
> Gentle reminder.
This series would be on Mike's radar.
I will also look into details once I finish Levi's series review.
Thanks,
Leo
On Wed, Apr 15, 2026 at 09:21:21AM +0800, Jie Gan wrote:
[...]
> > > > @@ -918,8 +918,10 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
> > > > cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset);
> > > > if (cfg_hash) {
> > > > ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
> > > > - if (ret)
> > > > + if (ret) {
> > > > + etm4_release_trace_id(drvdata);
> > >
> > > If so, even an ID is reserved for failures, and the ID map is big enough
> > > for each CPU, we don't need to worry memory leak or ID used out issue ?
> >
> > However, in theory, this could lead to an ID leak,
> > so it would be better to release it in error cases.
>
> What I am thinking is as SoCs continue to grow more complex with an
> increasing number of subsystems, trace IDs may be exhausted in the near
> future. (that's why we have dynamic trace ID allocation/release).
Thanks for the input.
I am wandering if we can use "dev->devt" as the trace ID. A device's
major/minor number is unique in kernel and dev_t is defined as u32:
typedef u32 __kernel_dev_t;
And we can consolidate this for both SYSFS and PERF modes.
Thanks,
Leo
On Mon, Apr 13, 2026 at 03:19:56PM +0100, Yeoreum Yun wrote:
> If etm4_enable_sysfs() fails in cscfg_csdev_enable_active_config(),
> the trace ID may be leaked because it is not released.
>
> To address this, call etm4_release_trace_id() when etm4_enable_sysfs()
> fails in cscfg_csdev_enable_active_config().
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com>
> ---
> drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 8ebfd3924143..1bc9f13e33f7 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -918,8 +918,10 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
> cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset);
> if (cfg_hash) {
> ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
> - if (ret)
> + if (ret) {
> + etm4_release_trace_id(drvdata);
I am not familiar with the trace ID, seems to me, it just allocate a ID
for each tracer from the ID map and then always use this cached ID for
the tracers.
If so, even an ID is reserved for failures, and the ID map is big enough
for each CPU, we don't need to worry memory leak or ID used out issue ?
Thanks,
Leo
> return ret;
> + }
> }
>
> raw_spin_lock(&drvdata->spinlock);
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>
On 4/14/2026 12:15 PM, Jie Gan wrote:
>
> I dont think you have looped relevant maintainers and reviewers for the Coresight subsystem.
>
> Thanks,
> Jie
I will send it to all relevant maintainers and reviewers for the Coresight subsystem in next revision.
Thanks,
Zane
On Tue, Apr 14, 2026 at 11:30:39AM +0800, Jie Gan wrote:
[...]
> > > Tested-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
> >
> > Just heads up: since Sashiko [1] pointed out a corner case where an SMP call
> > may fail when disabling the source device, the per-CPU path pointer
> > might not be cleared. If the ETMv4 device is then removed (e.g. if the
> > user unloads the ETMv4 module), CPU PM notifier might access the stale
> > path pointer. Though this is a rare case, we should handle it safely.
> > This is why the series was not picked for the v7.1 merge window.
> >
> > Thanks a lot for the testing, Jie! It's very helpful, and I will add
> > your test tags in the next spin.
> >
> > Anyway, please expect more iterations.
>
> Noted, will run the test cases on new iterations.
Thank you!
On Mon, Apr 13, 2026 at 03:19:54PM +0100, Yeoreum Yun wrote:
> Introduce struct etm4_caps to describe ETMv4 capabilities
> and move capabilities information into it.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com>
LGTM:
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
FWIW, two comments from Sashiko are valuable for me, please see below.
> ---
> .../coresight/coresight-etm4x-core.c | 234 +++++++++---------
> .../coresight/coresight-etm4x-sysfs.c | 190 ++++++++------
> drivers/hwtracing/coresight/coresight-etm4x.h | 176 ++++++-------
> 3 files changed, 328 insertions(+), 272 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index d565a73f0042..6443f3717b37 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -88,8 +88,9 @@ static int etm4_probe_cpu(unsigned int cpu);
> */
> static bool etm4x_sspcicrn_present(struct etmv4_drvdata *drvdata, int n)
> {
> - return (n < drvdata->nr_ss_cmp) &&
> - drvdata->nr_pe &&
> + const struct etmv4_caps *caps = &drvdata->caps;
> +
> + return (n < caps->nr_ss_cmp) && caps->nr_pe &&
> (drvdata->config.ss_status[n] & TRCSSCSRn_PC);
As Sashiko suggests:
"This isn't a regression introduced by this patch, but should this be
checking caps->nr_pe_cmp instead of caps->nr_pe?"
I confirmed the ETMv4 specification (ARM IHI0064H.b), the comment
above is valid as the we should check caps->nr_pe_cmp instead.
Could you first use a patch to fix the typo and then apply
capabilities afterwards? This is helpful for porting to stable
kernels.
[...]
> @@ -525,14 +530,14 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1))
> dev_err(etm_dev,
> "timeout while waiting for Idle Trace Status\n");
> - if (drvdata->nr_pe)
> + if (caps->nr_pe)
> etm4x_relaxed_write32(csa, config->pe_sel, TRCPROCSELR);
> etm4x_relaxed_write32(csa, config->cfg, TRCCONFIGR);
> /* nothing specific implemented */
> etm4x_relaxed_write32(csa, 0x0, TRCAUXCTLR);
> etm4x_relaxed_write32(csa, config->eventctrl0, TRCEVENTCTL0R);
> etm4x_relaxed_write32(csa, config->eventctrl1, TRCEVENTCTL1R);
> - if (drvdata->stallctl)
> + if (caps->stallctl)
> etm4x_relaxed_write32(csa, config->stall_ctrl, TRCSTALLCTLR);
> etm4x_relaxed_write32(csa, config->ts_ctrl, TRCTSCTLR);
> etm4x_relaxed_write32(csa, config->syncfreq, TRCSYNCPR);
> @@ -542,17 +547,17 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> etm4x_relaxed_write32(csa, config->vinst_ctrl, TRCVICTLR);
> etm4x_relaxed_write32(csa, config->viiectlr, TRCVIIECTLR);
> etm4x_relaxed_write32(csa, config->vissctlr, TRCVISSCTLR);
> - if (drvdata->nr_pe_cmp)
> + if (caps->nr_pe_cmp)
> etm4x_relaxed_write32(csa, config->vipcssctlr, TRCVIPCSSCTLR);
> - for (i = 0; i < drvdata->nrseqstate - 1; i++)
> + for (i = 0; i < caps->nrseqstate - 1; i++)
> etm4x_relaxed_write32(csa, config->seq_ctrl[i], TRCSEQEVRn(i));
Sashiko's comment:
"If the hardware does not implement a sequencer, caps->nrseqstate (a u8)
will be 0. Does 0 - 1 evaluate to -1 as an int, which then gets promoted
to ULONG_MAX against val (an unsigned long)?"
This is a good catch. The condition check should be:
for (i = 0; i < caps->nrseqstate; i++)
...;
The issue is irrelevant to your patch, but could you use a patch to fix
"nrseqstate - 1" first and then apply the cap refactoring on it? This
would be friendly for porting to stable kernel.
Thanks,
Leo
On Mon, Apr 13, 2026 at 06:30:18PM +0800, Jie Gan wrote:
[...]
> tested on QCOM sa8775-ride:
>
> === 1. Sysfs mode: basic enable/disable ===
> PASS: Sink tmc_etr0 enabled
> PASS: Source etm0 enabled
> PASS: Source etm0 disabled cleanly
> PASS: Sink tmc_etr0 disabled cleanly
>
> === 2. Sysfs mode: repeated enable/disable cycles (10x) ===
> PASS: 10 enable/disable cycles completed without error
>
> === 3. Sysfs mode: enable source with no active sink ===
> PASS: Enable without sink returned error (expected)
>
> === 4. Sysfs mode: enable/disable all per-CPU sources ===
> etm0 (cpu0): enabled OK
> etm1 (cpu1): enabled OK
> etm2 (cpu2): enabled OK
> etm3 (cpu3): enabled OK
> etm4 (cpu4): enabled OK
> etm5 (cpu5): enabled OK
> etm6 (cpu6): enabled OK
> etm7 (cpu7): enabled OK
> PASS: All online per-CPU sources enabled/disabled successfully
>
> === 5. CPU hotplug: offline CPU while sysfs tracing active ===
> Using source etm1 on cpu1
> Tracing active on cpu1, offlining CPU...
> [ 82.805359] psci: CPU1 killed (polled 0 ms)
> PASS: Source auto-disabled on CPU offline
> [ 83.346033] Detected PIPT I-cache on CPU1
> [ 83.346114] GICv3: CPU1: found redistributor 100 region
> 0:0x0000000017a80000
> [ 83.346283] CPU1: Booted secondary processor 0x0000000100 [0x410fd4b2]
> PASS: Source re-enabled after CPU re-online
>
> === 6. Sysfs: enable source on offline CPU (expect ENODEV) ===
> [ 84.013788] psci: CPU1 killed (polled 0 ms)
> PASS: Enable on offline cpu1 rejected (enable_source=0)
> [ 84.349558] Detected PIPT I-cache on CPU1
> [ 84.349640] GICv3: CPU1: found redistributor 100 region
> 0:0x0000000017a80000
> [ 84.349811] CPU1: Booted secondary processor 0x0000000100 [0x410fd4b2]
>
> === 7. CPU PM: trace survives CPU idle entry/exit ===
> Sleeping 3s to allow CPU idle entry...
> Idle entries on cpu0 during test: 35
> PASS: Source still enabled after idle (PM save/restore working)
>
> === 8. Perf mode: basic cs_etm recording ===
> SKIP: perf not found in PATH
>
> === 11. TRBE: check save/restore sysfs nodes (if present) ===
> SKIP: No TRBE devices found
>
> Tested-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
Just heads up: since Sashiko [1] pointed out a corner case where an SMP call
may fail when disabling the source device, the per-CPU path pointer
might not be cleared. If the ETMv4 device is then removed (e.g. if the
user unloads the ETMv4 module), CPU PM notifier might access the stale
path pointer. Though this is a rare case, we should handle it safely.
This is why the series was not picked for the v7.1 merge window.
Thanks a lot for the testing, Jie! It's very helpful, and I will add
your test tags in the next spin.
Anyway, please expect more iterations.
Thanks,
Leo
[1] https://sashiko.dev/#/patchset/20260405-arm_coresight_path_power_management…