CoreSight

coresight@lists.linaro.org

1 participants
2835 discussions

Re: [PATCH v6 09/13] coresight: etm4x: missing cscfg_csdev_disable_active_config() in perf enable

by Leo Yan

On Wed, Apr 22, 2026 at 02:21:59PM +0100, Yeoreum Yun wrote: [...] > @@ -895,6 +895,8 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > * Missing BB support could cause silent decode errors > * so fail to open if it's not supported. > */ > + if (cfg_hash) > + cscfg_csdev_disable_active_config(csdev); I prefer do a bit refactoring for this. we just save cfg_hash and cfg_preset into drvdata in etm4_parse_event_config(): drvdata->cfg_hash = ATTR_CFG_GET_FLD(attr, configid); if (drvdata->cfg_hash) drvdata->preset = ATTR_CFG_GET_FLD(attr, preset); Then create two helpers: etm4_cscfg_enable(csdev) { struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); return cscfg_csdev_enable_active_config(csdev, drvdata->cfg_hash, drvdata->preset); } etm4_cscfg_disable(csdev) { cscfg_csdev_disable_active_config(csdev); } These helpers will be used by etm4_{enable|disable}_perf() and etm4_{enable|disable}_sysfs(). This might benefit the future cscfg refactoring. Thanks, Leo P.s. I will stop review at here. I assume ETMv3 changes just mirror ETMv4's. So all ETMv4's comments would apply on ETMv3 patches. If I miss anything, please let me know.

3 months, 3 weeks

Re: [PATCH v6 08/13] coresight: etm4x: remove redundant call etm4_enable_hw() with hotplug

by Leo Yan

On Wed, Apr 22, 2026 at 02:21:58PM +0100, Yeoreum Yun wrote: [...] > + /* > + * Take the hotplug lock to prevent redundant calls to etm4_enable_hw(). > + * > + * The cpu_online_mask is set at the CPUHP_BRINGUP_CPU step. > + * In other words, if etm4_enable_sysfs() is called between > + * CPUHP_BRINGUP_CPU and CPUHP_AP_ARM_CORESIGHT_STARTING, > + * etm4_enable_hw() may be invoked in etm4_enable_sysfs_smp_call() > + * and then executed again in etm4_starting_cpu(). > + */ > + cpus_read_lock(); > ret = smp_call_function_single(drvdata->cpu, > etm4_enable_sysfs_smp_call, &arg, 1); > + cpus_read_unlock(); This will cause double deadlock with the patch: https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_… I think we need to drop this one. Thanks, Leo

3 months, 3 weeks

Re: [PATCH v6 07/13] coresight: etm4x: fix inconsistencies with sysfs configuration

by Leo Yan

On Wed, Apr 22, 2026 at 02:21:57PM +0100, Yeoreum Yun wrote: [...] > - Since active_config and related fields are accessed only by the local CPU > in etm4_enable/disable_sysfs_smp_call() (similar to perf enable/disable), > remove the lock/unlock from the sysfs enable/disable path and > startup/dying_cpu except when to access config fields. Thanks for writing this up, which is helpful for understanding. [...] > @@ -918,40 +948,29 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa > > /* enable any config activated by configfs */ > cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset); With the patch [1], we can move cscfg_config_sysfs_get_active_cfg() to smp call. As a result, all things for enabling cscfg can be in the same place. [1] https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_… > - if (cfg_hash) { > - ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); > - if (ret) { > - etm4_release_trace_id(drvdata); > - return ret; > - } > - } > - > - raw_spin_lock(&drvdata->spinlock); > - > - drvdata->trcid = path->trace_id; > - > - /* Tracer will never be paused in sysfs mode */ > - drvdata->paused = false; > > /* > * Executing etm4_enable_hw on the cpu whose ETM is being enabled > * ensures that register writes occur when cpu is powered. > */ > arg.drvdata = drvdata; > + arg.path = path; > + arg.cfg_hash = cfg_hash; > + arg.preset = preset; Connect with the comment above , don't need to pass cfg_hash/preset anymore. > + raw_spin_lock(&drvdata->spinlock); > + arg.config = drvdata->config; > + raw_spin_unlock(&drvdata->spinlock); Seems to me, this is right way for locking - here we simply use spinlock for exclusive access config from sysfs knobs. However, we avoid the config copy and directly access in SMP call? we still can use the raw spinlock in SMP call. My suggestion is: - First use a patch to move the drvdata assignment to SMP call and remove spinlock; - Then, rebase this patch for moving cscfg into SMP call. If so, we only need add a new field "arg->path", right? > @@ -1857,13 +1875,11 @@ static int etm4_starting_cpu(unsigned int cpu) > if (!etmdrvdata[cpu]) > return 0; > > - raw_spin_lock(&etmdrvdata[cpu]->spinlock); With the change [2], the starting and dying functions have been removed. If you rebase patches on the top PM series, then you will not bother this. Anyway, this is right to remove spinlock for hotplug notifiers. [2] https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_…

3 months, 3 weeks

Re: [PATCH] coresight: trbe: Hide enable_sink sysfs file

by Leo Yan

On Thu, May 07, 2026 at 11:53:45AM +0100, James Clark wrote: [...] > +static umode_t coresight_attr_is_visible(struct kobject *kobj, > + struct attribute *attr, int n) > { > struct device *dev = kobj_to_dev(kobj); > + struct coresight_device *csdev = to_coresight_device(dev); > > if (attr == &dev_attr_label.attr) { > if (fwnode_property_present(dev_fwnode(dev), "label")) > return attr->mode; > else > return 0; > + } else if (attr == &dev_attr_enable_sink.attr || > + attr == &dev_attr_enable_source.attr) { > + if (csdev->no_sysfs_mode) > + return 0; > + else > + return attr->mode; I'd prefer no_sysfs_mode to work as a general flag rather than being limited to sink/source devices only. Otherwise, LGTM. Thanks, Leo

3 months, 3 weeks

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

by Suzuki K Poulose

On Tue, 12 May 2026 09:56:07 +0800, 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. > > Change coresight_path_assign_trace_id() to return int, moving the > IS_VALID_CS_TRACE_ID() check inside it so it returns -EINVAL on failure > and 0 on success. Update both callers to propagate this return value > directly instead of inspecting path->trace_id after the call. > > [...] Applied, thanks! [1/1] coresight: fix missing error code when trace ID is invalid https://git.kernel.org/coresight/c/f4526ffee6ff Best regards, -- Suzuki K Poulose <suzuki.poulose(a)arm.com>

3 months, 4 weeks

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

by Leo Yan

On Tue, May 12, 2026 at 09:56:07AM +0800, 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. > > Change coresight_path_assign_trace_id() to return int, moving the > IS_VALID_CS_TRACE_ID() check inside it so it returns -EINVAL on failure > and 0 on success. Update both callers to propagate this return value > directly instead of inspecting path->trace_id after the call. > > Fixes: d87d76d823d1 ("Coresight: Allocate trace ID after building the path") > Reviewed-by: James Clark <james.clark(a)linaro.org> > Reviewed-by: Richard Cheng <icheng(a)nvidia.com> > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> Reviewed-by: Leo Yan <leo.yan(a)arm.com>

3 months, 4 weeks

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

3 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>

3 months, 4 weeks

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

3 months, 4 weeks

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

3 months, 4 weeks

← Newer
1
...
9
10
11
12
13
14
15
...
284
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight