CoreSight

coresight@lists.linaro.org

3 participants
2708 discussions

Re: [PATCH v3] coresight: fix missing error code when trace ID is invalid

by Leo Yan

On Mon, May 11, 2026 at 05:27:10PM +0800, Jie Gan wrote: [...] > > > @@ -755,10 +755,16 @@ void coresight_path_assign_trace_id(struct coresight_path *path, > > > * Non 0 is either success or fail. > > > */ > > > if (trace_id != 0) { > > > - path->trace_id = trace_id; > > > - return; > > > + if (IS_VALID_CS_TRACE_ID(trace_id)) { > > > + path->trace_id = trace_id; > > > + return 0; > > > + } > > > + > > > + return -EINVAL; I'd advocate a bit early exit style, like: /* 0 means the device has no ID assignment, so keep searching */ if (trace_id == 0) continue; if (!IS_VALID_CS_TRACE_ID(trace_id)) return -EINVAL; path->trace_id = trace_id; return 0; Early exit can reduce indentation depth, and it handles simple cases first and then the complex logic. In some cases (maye not this case), we may benefit a bit from compiler optimization [1]. [1] https://xania.org/202512/18-partial-inlining [...] > The return value has been ignored in perf mode. It will introduce noisy by > adding __must_check. So I think its better without __must_check? Wouldn't it need to update perf mode as well? Regarding __must_check, I searched Documentation but didn't find guidance on when it should be used. I don't want to use this annotation randomly (some functions use it and some not), this will be hard for everyone to follow up. IMO, it's fine not to use __must_check here. I would leave this to Suzuki and other maintainers if have different opinions. Thanks, Leo

5 months, 4 weeks

Re: [PATCH] coresight: fix missing error code when trace ID is invalid

by James Clark

On 08/05/2026 6:45 am, Jie Gan wrote: > When coresight_path_assign_trace_id() fails to allocate a valid trace > ID, the code jumps to err_path without setting ret to an error value. > This causes coresight_enable_sysfs() to return 0 (success) to the > caller even though no trace session was started. > > Set ret = -EINVAL before the goto so that callers receive a proper > error code. > > Fixes: d87d76d823d1 ("Coresight: Allocate trace ID after building the path") > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > --- > drivers/hwtracing/coresight/coresight-sysfs.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c > index d2a6ed8bcc74..c9338c783540 100644 > --- a/drivers/hwtracing/coresight/coresight-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-sysfs.c > @@ -195,42 +195,44 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > */ > if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE) > csdev->refcnt++; > goto out; > } > > sink = coresight_find_activated_sysfs_sink(csdev); > if (!sink) { > ret = -EINVAL; > goto out; > } > > path = coresight_build_path(csdev, sink); > if (IS_ERR(path)) { > pr_err("building path(s) failed\n"); > ret = PTR_ERR(path); > goto out; > } > > coresight_path_assign_trace_id(path, CS_MODE_SYSFS); > - if (!IS_VALID_CS_TRACE_ID(path->trace_id)) > + if (!IS_VALID_CS_TRACE_ID(path->trace_id)) { > + ret = -EINVAL; > goto err_path; > + } > > ret = coresight_enable_path(path, CS_MODE_SYSFS); > if (ret) > goto err_path; > > ret = coresight_enable_source_sysfs(csdev, CS_MODE_SYSFS, path); > if (ret) > goto err_source; > > switch (subtype) { > case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: > /* > * When working from sysFS it is important to keep track > * of the paths that were created so that they can be > * undone in 'coresight_disable()'. Since there can only > * be a single session per tracer (when working from sysFS) > * a per-cpu variable will do just fine. > */ > cpu = source_ops(csdev)->cpu_id(csdev); > per_cpu(tracer_path, cpu) = path; > > --- > base-commit: 17c7841d09ee7d33557fd075562d9289b6018c90 > change-id: 20260508-fix-trace-id-error-dbfdd4d8f2d1 > > Best regards, Reviewed-by: James Clark <james.clark(a)linaro.org>

6 months

Re: [PATCH] coresight: fix source not disabled on idr_alloc_u32 failure

by Leo Yan

On Mon, May 11, 2026 at 05:28:37PM +0800, Jie Gan wrote: > Hi Leo, > > On 5/11/2026 5:24 PM, Leo Yan wrote: > > On Mon, May 11, 2026 at 05:04:44PM +0800, Jie Gan wrote: > > > In coresight_enable_sysfs(), for non-CPU sources (SOFTWARE, TPDM, > > > OTHERS), the source device is enabled via coresight_enable_source_sysfs() > > > before idr_alloc_u32() maps the path. If idr_alloc_u32() fails, the > > > original code jumped directly to err_source, which only calls > > > coresight_disable_path() and coresight_release_path(). The source device > > > was left enabled with an incremented refcnt but no path tracked for it, > > > leaving the device in an inconsistent state. > > > > > > Disable the source before jumping to err_source so the enable and path > > > operations are fully unwound. > > > > > > Fixes: 1f5149c7751c ("coresight: Move all sysfs code to sysfs file") > > > Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com> > > > > Actually I have noticed this. Since my PM series will remove IDR things, > > and I don't think anyone really hit idr alloc error, this is why I > > didn't send fix for this. > > > > Anyway, this is a reasonable fix. I will send out my PM series later > > in today, I will pick this patch into my series and rebase on it, hope > > this is easier for all of us. > > Well noted. Please feel free to pick it into your series. Thanks! Just note, I updated the Fixes tag as: Fixes: 5c0016d7b343 ("coresight: core: Use IDR for non-cpu bound sources' paths.") Which is the original patch for the issue. Thanks, Leo

6 months

Re: [PATCH] coresight: fix source not disabled on idr_alloc_u32 failure

by Leo Yan

On Mon, May 11, 2026 at 05:04:44PM +0800, Jie Gan wrote: > In coresight_enable_sysfs(), for non-CPU sources (SOFTWARE, TPDM, > OTHERS), the source device is enabled via coresight_enable_source_sysfs() > before idr_alloc_u32() maps the path. If idr_alloc_u32() fails, the > original code jumped directly to err_source, which only calls > coresight_disable_path() and coresight_release_path(). The source device > was left enabled with an incremented refcnt but no path tracked for it, > leaving the device in an inconsistent state. > > Disable the source before jumping to err_source so the enable and path > operations are fully unwound. > > Fixes: 1f5149c7751c ("coresight: Move all sysfs code to sysfs file") > Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com> Actually I have noticed this. Since my PM series will remove IDR things, and I don't think anyone really hit idr alloc error, this is why I didn't send fix for this. Anyway, this is a reasonable fix. I will send out my PM series later in today, I will pick this patch into my series and rebase on it, hope this is easier for all of us. Thanks, Leo

6 months

Re: [PATCH] coresight: fix missing error code when trace ID is invalid

by Leo Yan

On Fri, May 08, 2026 at 01:45:35PM +0800, Jie Gan wrote: [...] > coresight_path_assign_trace_id(path, CS_MODE_SYSFS); > - if (!IS_VALID_CS_TRACE_ID(path->trace_id)) > + if (!IS_VALID_CS_TRACE_ID(path->trace_id)) { > + ret = -EINVAL; > goto err_path; > + } On the top of this patch, could we do a further improvement? Move IS_VALID_CS_TRACE_ID() into coresight_path_assign_trace_id() and return 0 for success and < 0 for failures. As result, callers only need to check the returned value. For this patch: Reviewed-by: Leo Yan <leo.yan(a)arm.com>

6 months

Re: [PATCH v11 07/27] coresight: Take a reference on csdev

by Leo Yan

On Thu, May 07, 2026 at 03:55:26PM +0100, James Clark wrote: [...] > Hi Leo, > > Testing on the Orion O6 board was all good, and so was stress testing > concurrent sysfs mode and hotplug on Juno. Thanks a lot for test! > However, I was trying to stress test sysfs mode and rmmod on Juno and > ran into an issue, although a similar issue is present without your > patchset. I don't think CPU PM introduces additional complexity for the above cases. The reason is that CPU PM notifiers _only_ apply to active sessions, and once a device is enabled, the module cannot be removed. If the race conditions between enabling/disabling sessions and module load/unload are properly handled, CPU PM should be safe. If we have bug in these race conditions, the high frequency data access in CPU PM may expose the issues - I don't expect CPU PM is the culprit. > If you run an rmmod on all the coresight devices at the same time as an > enable_source / disable loop you always get this: > > WARNING: possible circular locking dependency detected > 7.0.0-rc1+ #713 Tainted: G N > ------------------------------------------------------ > rmmod/1361 is trying to acquire lock: > ffff0008042f69a8 (kn->active#144){++++}-{0:0}, at: > __kernfs_remove+0x1b8/0x2c8 kn->active is not a lock but an active reference of sysfs node, but it use lockdep annotation to detect lock dependency. > Possible unsafe locking scenario: > CPU0 CPU1 > ---- ---- > lock(coresight_mutex); > lock(cpu_hotplug_lock); > lock(coresight_mutex); > lock(kn->active#144); > *** DEADLOCK *** The potential deadlock sequence could be: kernfs_fop_write_iter() `> kernfs_get_active_of() => acquire kn->active `> coresight_enable_sysfs() => acquire coresight_mutex coresight_unregister() => acquire coresight_mutex `> device_unregister() `> __kernfs_remove() `> kernfs_drain() => acquire kn->active > I think the issue can be fixed by releasing the coresight_mutex before > device_unregister(): > > diff --git a/drivers/hwtracing/coresight/coresight-core.c > b/drivers/hwtracing/coresight/coresight-core.c > index 015363da12fa..620560880f12 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -1639,8 +1639,8 @@ void coresight_unregister(struct coresight_device > *csdev) > coresight_remove_conns(csdev); > coresight_clear_default_sink(csdev); > coresight_release_platform_data(csdev->dev.parent, csdev->pdata); > - device_unregister(&csdev->dev); > mutex_unlock(&coresight_mutex); > + device_unregister(&csdev->dev); > } > EXPORT_SYMBOL_GPL(coresight_unregister); If so, we also need to move device_register() out of the mutex scope. That said, I still think we should dive a bit if can use smaller locking granluarity (combining with bus management provided by device model). > Although I didn't think too hard about the implications, but it might be ok > because once all the connections are removed the device can't be used so > releasing the coresight_mutex isn't an issue. > > But then testing that I ran into some kind of refleak where I couldn't > unload modules anymore, even though I'd disabled everything. But that could > be a different issue: > > rmmod: ERROR: Module coresight_funnel is in use > rmmod: ERROR: Module coresight_replicator is in use > rmmod: ERROR: Module coresight_etm4x is in use > rmmod: ERROR: Module coresight_tmc is in use > rmmod: ERROR: Module coresight_cti is in use > rmmod: ERROR: Module coresight is in use by: coresight_tmc coresight_cti > coresight_etm4x coresight_replicator coresight_funnel I suspect this is due to module references are not properly released, or the entire CS path is not properly disabled. After the issue occurs, can the ETM sysfs knob still be accessed? I am curious whether this is caused by the sysfs knob disappearing so no way to disable the path or the sysfs knob still exists but the driver internally misses to disable the path. > Anyway I don't think your patches make this worse, so we can probably ignore > it, but it would be good to be able to stress test the new modifications > around the same area. As no regression in test, I agree that we should not defer this series. We can fix the race with module load/unload as a separate task: - sysfs mode + module load/unload - perf mode + module load/unload Then we can combine stress test with CPU idle/hotplug. Thanks, Leo

6 months

Re: [PATCH v2] coresight: fix missing error code when trace ID is invalid

by James Clark

On 09/05/2026 1:58 am, Jie Gan wrote: > When coresight_path_assign_trace_id() cannot assign a valid trace ID, > coresight_enable_sysfs() takes the err_path goto with ret still 0, > returning success to the caller despite no trace session being started. > > Fix this by changing coresight_path_assign_trace_id() to return int. > Move the IS_VALID_CS_TRACE_ID() check inside the function so it returns > -EINVAL on failure and 0 on success. Update coresight_enable_sysfs() to > check the return value directly instead of inspecting path->trace_id > after the call. > > The other caller in coresight-etm-perf.c discards the return value and > continues to check path->trace_id via IS_VALID_CS_TRACE_ID() directly. > This is unaffected: on failure path->trace_id is no longer written, so > it remains 0, which IS_VALID_CS_TRACE_ID() rejects the same as before. > > Fixes: d87d76d823d1 ("Coresight: Allocate trace ID after building the path") > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > --- > Changes in v2: > - Refactor the coresight_path_assign_trace_id function. > - Link to v1: https://lore.kernel.org/r/20260508-fix-trace-id-error-v1-1-5f11a5456fdf@oss… > --- > drivers/hwtracing/coresight/coresight-core.c | 14 ++++++++++---- > drivers/hwtracing/coresight/coresight-priv.h | 2 +- > drivers/hwtracing/coresight/coresight-sysfs.c | 4 ++-- > 3 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 46f247f73cf6..cabdc0c72f38 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -739,12 +739,12 @@ static int coresight_get_trace_id(struct coresight_device *csdev, > * Call this after creating the path and before enabling it. This leaves > * the trace ID set on the path, or it remains 0 if it couldn't be assigned. > */ > -void coresight_path_assign_trace_id(struct coresight_path *path, > +int coresight_path_assign_trace_id(struct coresight_path *path, > enum cs_mode mode) > { > struct coresight_device *sink = coresight_get_sink(path); > struct coresight_node *nd; > - int trace_id; > + int trace_id, ret = -EINVAL; > > list_for_each_entry(nd, &path->path_list, link) { > /* Assign a trace ID to the path for the first device that wants to do it */ > @@ -755,10 +755,16 @@ void coresight_path_assign_trace_id(struct coresight_path *path, > * Non 0 is either success or fail. > */ > if (trace_id != 0) { > - path->trace_id = trace_id; > - return; > + if (IS_VALID_CS_TRACE_ID(trace_id)) { > + path->trace_id = trace_id; > + ret = 0; Reviewed-by: James Clark <james.clark(a)linaro.org> Minor nit, but just "return 0" here is a bit simpler. > + } > + > + return ret; And then "return -EINVAL" for these ones. > } > } > + > + return ret; > } > > /** > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h > index 1ea882dffd70..34c7e792adbd 100644 > --- a/drivers/hwtracing/coresight/coresight-priv.h > +++ b/drivers/hwtracing/coresight/coresight-priv.h > @@ -153,7 +153,7 @@ int coresight_make_links(struct coresight_device *orig, > void coresight_remove_links(struct coresight_device *orig, > struct coresight_connection *conn); > u32 coresight_get_sink_id(struct coresight_device *csdev); > -void coresight_path_assign_trace_id(struct coresight_path *path, > +int coresight_path_assign_trace_id(struct coresight_path *path, > enum cs_mode mode); > > #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X) > diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c > index d2a6ed8bcc74..b6a870399e83 100644 > --- a/drivers/hwtracing/coresight/coresight-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-sysfs.c > @@ -211,8 +211,8 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > goto out; > } > > - coresight_path_assign_trace_id(path, CS_MODE_SYSFS); > - if (!IS_VALID_CS_TRACE_ID(path->trace_id)) > + ret = coresight_path_assign_trace_id(path, CS_MODE_SYSFS); > + if (ret) > goto err_path; > > ret = coresight_enable_path(path, CS_MODE_SYSFS); > > --- > base-commit: 17c7841d09ee7d33557fd075562d9289b6018c90 > change-id: 20260508-fix-trace-id-error-dbfdd4d8f2d1 > > Best regards,

6 months

Re: [PATCH v6 04/13] coresight: etm4x: exclude ss_status from drvdata->config

by Leo Yan

On Wed, Apr 22, 2026 at 02:21:54PM +0100, Yeoreum Yun wrote: [...] > @@ -573,11 +573,9 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) > etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i)); > > for (i = 0; i < caps->nr_ss_cmp; i++) { > - /* always clear status bit on restart if using single-shot */ > - if (config->ss_ctrl[i] || config->ss_pe_cmp[i]) > - config->ss_status[i] &= ~TRCSSCSRn_STATUS; > etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i)); > - etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i)); > + /* always clear status and pending bits on restart if using single-shot */ > + etm4x_relaxed_write32(csa, 0x0, TRCSSCSRn(i)); After confirmed with hardware team, we should preserve status bits (including STATUS and PENDING bits) during a session. So here we should set drvdata->ss_status to TRCSSCSRn. > @@ -1503,8 +1501,9 @@ static void etm4_init_arch_data(void *info) > */ > caps->nr_ss_cmp = FIELD_GET(TRCIDR4_NUMSSCC_MASK, etmidr4); > for (i = 0; i < caps->nr_ss_cmp; i++) { > - drvdata->config.ss_status[i] = > - etm4x_relaxed_read32(csa, TRCSSCSRn(i)); > + drvdata->ss_status[i] = etm4x_relaxed_read32(csa, TRCSSCSRn(i)); > + drvdata->ss_status[i] &= (TRCSSCSRn_PC | TRCSSCSRn_DV | > + TRCSSCSRn_DA | TRCSSCSRn_INST); It is fine for read these capacity bits when probe, but we need to clear status when a session is starting to avoid the stale value left from previous session: drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING); We can do this in etm4_parse_event_config() for perf mode, and might create a new function (say etm4_parse_sysfs_config()) for preparing config for sysfs mode? The rest looks good to me. Thanks, Leo

6 months

[PATCH] coresight: tmc: Fix overflow when calculating is bigger than 2GiB

by Leo Yan

When specifying a 2GB AUX buffer, the ETR driver ends up allocating only a 1MB buffer instead: # echo 'file coresight-tmc-etr.c +p' > \ /sys/kernel/debug/dynamic_debug/control # perf record -e cs_etm/@tmc_etr0,timestamp=0/u -C 0 -m ,2G -- test coresight tmc_etr0: allocated buffer of size 1024KB in mode 0 The page index is an 'int' type, and shifting it by PAGE_SHIFT overflows when the resulting value exceeds 2GB. This produces a negative value, causing the driver to fall back to the minimum buffer size (1MB). Cast the page index to a wider type to accommodate large buffer sizes. Also fix a similar issue in the buffer offset calculation. Reported-by: Michiel van Tol <michiel.vantol(a)arm.com> Fixes: 99443ea19e8b ("coresight: Add generic TMC sg table framework") Fixes: eebe8dbd8630 ("coresight: tmc: Decouple the perf buffer allocation from sysfs mode") Signed-off-by: Leo Yan <leo.yan(a)arm.com> --- drivers/hwtracing/coresight/coresight-tmc-etr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index cee82e52c4ea96b035f1db71b2d9a006bfc1c51e..990bbb721e1d712d7b93f1e36087fdaf9d3baa3b 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -154,7 +154,7 @@ tmc_pages_get_offset(struct tmc_pages *tmc_pages, dma_addr_t addr) for (i = 0; i < tmc_pages->nr_pages; i++) { page_start = tmc_pages->daddrs[i]; if (addr >= page_start && addr < (page_start + PAGE_SIZE)) - return i * PAGE_SIZE + (addr - page_start); + return (long)i * PAGE_SIZE + (addr - page_start); } return -EINVAL; @@ -1381,7 +1381,7 @@ alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event, node = (event->cpu == -1) ? NUMA_NO_NODE : cpu_to_node(event->cpu); /* Use the minimum limit if the required size is smaller */ - size = nr_pages << PAGE_SHIFT; + size = (ssize_t)nr_pages << PAGE_SHIFT; size = max_t(ssize_t, size, TMC_ETR_PERF_MIN_BUF_SIZE); /* --- base-commit: eebe8dbd8630f51cf70b1f68a440cd3d7f7a914d change-id: 20260217-arm_coresight_fix_big_buffer_size-a8a41298369d Best regards, -- Leo Yan <leo.yan(a)arm.com>

6 months

[PATCH] coresight: etm4x: Correct TRCVMIDCCTLR1 save and restore

by Leo Yan

It is a typo to use trcvmidcctlr0 to save and restore TRCVMIDCCTLR1. Use trcvmidcctlr1 instead. Fixes: f5bd523690d2 ("coresight: etm4x: Convert all register accesses") Signed-off-by: Leo Yan <leo.yan(a)arm.com> --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index d565a73f0042e3e0b21fcf9cb94009cc25834d3d..c370b6bf45ef26aff29ee71630f86eae2487069a 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1979,7 +1979,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR0); if (drvdata->numvmidc > 4) - state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR1); + state->trcvmidcctlr1 = etm4x_read32(csa, TRCVMIDCCTLR1); state->trcclaimset = etm4x_read32(csa, TRCCLAIMCLR); @@ -2102,7 +2102,7 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR0); if (drvdata->numvmidc > 4) - etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR1); + etm4x_relaxed_write32(csa, state->trcvmidcctlr1, TRCVMIDCCTLR1); etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET); --- base-commit: 971f3474f8898ae8bbab19a9b547819a5e6fbcf1 change-id: 20260408-arm_cs_fix_trcvmidcctlr1_typo-3382c27d1c28 Best regards, -- Leo Yan <leo.yan(a)arm.com>

6 months

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight