CoreSight

coresight@lists.linaro.org

3 participants
2734 discussions

Re: [PATCH v5 07/12] coresight: etm4x: fix inconsistencies with sysfs configuration

by Leo Yan

On Wed, Apr 15, 2026 at 05:55:23PM +0100, Yeoreum Yun wrote: > The current ETM4x configuration via sysfs can lead to > several inconsistencies: > > - If the configuration is modified via sysfs while a perf session is > active, the running configuration may differ before a sched-out and > after a subsequent sched-in. > > - If a perf session and a sysfs session enable tracing concurrently, > the configuration from configfs may become corrupted. I think this happens because the sysfs path calls cscfg_csdev_enable_active_config() without first acquiring the mode, allowing a perf session to acquire the mode and call the same function concurrently. > - There is a risk of corrupting drvdata->config if a perf session enables > tracing while cscfg_csdev_disable_active_config() is being handled in > etm4_disable_sysfs(). Similiar to above, cscfg_csdev_disable_active_config() is not protected in etm4_disable_sysfs(). > To resolve these issues, separate the configuration into: > > - active_config: the configuration applied to the current session > - config: the configuration set via sysfs > > Additionally: > > - Apply the configuration from configfs after taking the appropriate mode. > > - 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. > > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> > --- > .../hwtracing/coresight/coresight-etm4x-cfg.c | 2 +- > .../coresight/coresight-etm4x-core.c | 107 ++++++++++-------- > drivers/hwtracing/coresight/coresight-etm4x.h | 2 + > 3 files changed, 63 insertions(+), 48 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c > index d14d7c8a23e5..0553771d04e7 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c > @@ -47,7 +47,7 @@ static int etm4_cfg_map_reg_offset(struct etmv4_drvdata *drvdata, > struct cscfg_regval_csdev *reg_csdev, u32 offset) > { > int err = -EINVAL, idx; > - struct etmv4_config *drvcfg = &drvdata->config; > + struct etmv4_config *drvcfg = &drvdata->active_config; > u32 off_mask; > > if (((offset >= TRCEVENTCTL0R) && (offset <= TRCVIPCSSCTLR)) || > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index b199aebbdb60..15aaf4a898e1 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -245,6 +245,10 @@ void etm4_release_trace_id(struct etmv4_drvdata *drvdata) > > struct etm4_enable_arg { > struct etmv4_drvdata *drvdata; > + unsigned long cfg_hash; > + int preset; Since smp call cannot directly call cscfg_config_sysfs_get_active_cfg() due to it runs in atomic context but ...active_cfg() tries to acquire mutex. So we firstly retrieve pass 'cfg_hash' and 'preset' in sleepable context and deliver them to the SMP call. After coresight cfg refactoring, we should remove cfg_hash/preset from ETM driver, the ETM driver only needs to retrieve register list and can do this in smp call. Before we finish cfg refactoring, it is fine for me to add these two parameters into etm4_enable_arg. > + u8 trace_id; Can we add 'path' instead ? The SMP call can retrieve path->trace_id. This can benefit for future clean up (e.g., we can store config into path so we can retrieve config from path pointer), and this allows us for further refactoring to unify etm4_enable_sysfs_smp_call() and etm4_enable_perf(). > + struct etmv4_config config; We don't need this. We can defer to get drvdata->config in SMP call. > int rc; > }; [...] > @@ -918,40 +946,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); > - 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.cfg_hash = cfg_hash; > + arg.preset = preset; > + arg.trace_id = path->trace_id; > + > + raw_spin_lock(&drvdata->spinlock); > + arg.config = drvdata->config; > + raw_spin_unlock(&drvdata->spinlock); Can we defer this in smp call ? And we can consolidate a bit configurations, we can consider to use a separate patch for this. etm4x_apply_config(drvdata, event, hash, preset) { /* perf mode */ if (event) { etm4_parse_event_config(drvdata->csdev, event); } else if (mode == CS_MODE_PERF) { scoped_guard(raw_spinlock, &drvdata->spinlock) &drvdata->active_config = drvdata->config; } /* At the end, we always apply the config */ cscfg_csdev_enable_active_config(drvdata->csdev, hash, preset); } > + > ret = smp_call_function_single(drvdata->cpu, > etm4_enable_sysfs_smp_call, &arg, 1); > if (!ret) > ret = arg.rc; > if (!ret) > - drvdata->sticky_enable = true; > - > - if (ret) > + dev_dbg(&csdev->dev, "ETM tracing enabled\n"); > + else > etm4_release_trace_id(drvdata); > > - raw_spin_unlock(&drvdata->spinlock); > - > - if (!ret) > - dev_dbg(&csdev->dev, "ETM tracing enabled\n"); > return ret; > } > > @@ -1038,7 +1055,7 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) > { > u32 control; > const struct etmv4_caps *caps = &drvdata->caps; > - struct etmv4_config *config = &drvdata->config; > + struct etmv4_config *config = &drvdata->active_config; > struct coresight_device *csdev = drvdata->csdev; > struct csdev_access *csa = &csdev->access; > int i; > @@ -1074,6 +1091,8 @@ static void etm4_disable_sysfs_smp_call(void *info) > > etm4_disable_hw(drvdata); > > + cscfg_csdev_disable_active_config(drvdata->csdev); > + > coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED); > } > > @@ -1124,7 +1143,6 @@ static void etm4_disable_sysfs(struct coresight_device *csdev) > * DYING hotplug callback is serviced by the ETM driver. > */ > cpus_read_lock(); > - raw_spin_lock(&drvdata->spinlock); > > /* > * Executing etm4_disable_hw on the cpu whose ETM is being disabled > @@ -1133,10 +1151,6 @@ static void etm4_disable_sysfs(struct coresight_device *csdev) > smp_call_function_single(drvdata->cpu, etm4_disable_sysfs_smp_call, > drvdata, 1); > > - raw_spin_unlock(&drvdata->spinlock); > - > - cscfg_csdev_disable_active_config(csdev); > - > cpus_read_unlock(); > > /* > @@ -1379,6 +1393,7 @@ static void etm4_init_arch_data(void *info) > struct etm4_init_arg *init_arg = info; > struct etmv4_drvdata *drvdata; > struct etmv4_caps *caps; > + struct etmv4_config *config; > struct csdev_access *csa; > struct device *dev = init_arg->dev; > int i; > @@ -1386,6 +1401,7 @@ static void etm4_init_arch_data(void *info) > drvdata = dev_get_drvdata(init_arg->dev); > caps = &drvdata->caps; > csa = init_arg->csa; > + config = &drvdata->active_config; Should we init drvdata->config instead so make it has sane values ? In other words, drvdata->active_config are always set at the runtime, so don't need to init it at all, right? Thanks, Leo

4 weeks

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

by Leo Yan

On Wed, Apr 15, 2026 at 05:55:20PM +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)); In Arm ARM, D24.4.60 TRCSSCSR<n>, bits[0..3] are RO. I think it is fine for directly clear the regiser with zero (means it will only clear status / pending bits). [...] > @@ -1841,10 +1839,11 @@ static ssize_t sshot_status_show(struct device *dev, > { > unsigned long val; > struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); > + const struct etmv4_caps *caps = &drvdata->caps; > struct etmv4_config *config = &drvdata->config; > > raw_spin_lock(&drvdata->spinlock); > - val = config->ss_status[config->ss_idx]; > + val = caps->ss_cmp[config->ss_idx]; > raw_spin_unlock(&drvdata->spinlock); > return scnprintf(buf, PAGE_SIZE, "%#lx\n", val); > } This sysfs knob never can print out a realtime status for sshot, I am fine for only printing caps->ss_cmp, this can avoid any misleading. @Suzuki, @Mike, do you agree with the change above? If maintainers agree with this, as Jie suggested, it is good to add a comment in the code and update the document: Documentation/trace/coresight/coresight-etm4x-reference.rst Thanks, Leo

4 weeks

Re: [PATCH v5 02/12] coresight: etm4x: fix underflow for nrseqstate

by Leo Yan

On Wed, Apr 15, 2026 at 05:55:18PM +0100, Yeoreum Yun wrote: > TCRSEQEVR<n> is implemented only when TCRIDR5.NUMSEQSTATE is 0b100, > in which case n ranges from 0 to 2; otherwise, TCRIDR5.NUMSEQSTATE is 0b000. My suggestion in previous version is not quite right, thanks for making this correct. [...] > @@ -1395,6 +1395,8 @@ static ssize_t seq_idx_store(struct device *dev, > struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct etmv4_config *config = &drvdata->config; > > + if (!drvdata->nrseqstate) > + return -EINVAL; For "nrseqstate = 0" case, would it return -EOPNOTSUPP instead? Otherwise, LGTM: Reviewed-by: Leo Yan <leo.yan(a)arm.com>

4 weeks

Re: [PATCH v5 01/12] coresight: etm4x: fix wrong check of etm4x_sspcicrn_present()

by Suzuki K Poulose

On 15/04/2026 17:55, Yeoreum Yun wrote: > According to Embedded Trace Macrocell Architecture Specification > ETMv4.0 to ETM4.6 [0], TRCSSPCICR<n> is present only if all of > the following are true: > > - TRCIDR4.NUMSSCC > n. > - TRCIDR4.NUMPC > 0b0000. > - TRCSSCSR<n>.PC == 0b1. > > Comment for etm4x_sspcicrn_present() is align with the specification. > However, the check should use drvdata->nr_pe_cmp to check TRCIDR4.NUMPC > not nr_pe. > > Link: https://developer.arm.com/documentation/ihi0064/latest/ [0] Fixes: f6a18f354c58 ("coresight: etm4x: Handle access to TRCSSPCICRn") With that looks good to me Suzuki > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> > --- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index d565a73f0042..74b7063d130e 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -89,7 +89,7 @@ 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 && > + drvdata->nr_pe_cmp && > (drvdata->config.ss_status[n] & TRCSSCSRn_PC); > } >

4 weeks

Re: [PATCH v5 05/12] coresight: etm4x: remove redundant fields in etmv4_save_state

by Leo Yan

On Wed, Apr 15, 2026 at 05:55:21PM +0100, Yeoreum Yun wrote: > Some of fields are redundant in etmv4_save_state and never used: > > ss_status => trcsscsr > seq_state => trcseqstr > cntr_val => trccntvr > vinst_ctrl => trcvictlr > > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> Reviewed-by: Leo Yan <leo.yan(a)arm.com>

4 weeks

Re: [PATCH v5 06/12] coresight: etm4x: fix leaked trace id

by Leo Yan

Hi Jie, On Fri, Apr 17, 2026 at 09:01:17AM +0800, Jie Gan wrote: [...] > ... we still need support static trace ID allocation in parallel for > the dummy sources and we should not break this logic in future refactor. Just confirm what is the reason you need to use static trace ID for the dummy sources? I am wandering if we could use dev->devt as trace ID for dummy devices. Since the device's MAJOR number is non-zero and occupies the upper bits (see MINORBITS), it is naturally separated from the hardware trace ID range. If so, we even don't need to bother ID alloc/release. Thanks, Leo

1 month

Re: [PATCH v5 06/12] coresight: etm4x: fix leaked trace id

by Leo Yan

Hi Levi, On Thu, Apr 16, 2026 at 06:06:41PM +0100, Yeoreum Yun wrote: [...] > > We should use paired way for allocation and release. For example: > > > > coresight_enable_sysfs() > > { > > ... > > coresight_path_assign_trace_id(path); > > > > failed: > > coresight_path_unassign_trace_id(path); > > } > > > > coresight_disable_sysfs() > > { > > coresight_path_unassign_trace_id(path); > > } > > > > But this requires broader refactoring. E.g., the STM driver currently > > allocates system trace IDs statically during probe, we might need to > > consolidate for all modules to use dynamic allocation. > > So IIUC, Do we want to "map" per "session" and save this map information > in the "sink" driver? or just use "global" map but locate it in sink > driver? I prefer to save map in the sink's driver data, this is more scalable as the trace ID is allocated within a session rather than system wide. > I totally agree for above suggestion -- unsigned trace id > in the coresight_XXX function -- (but we need to add another callback > for this) but I think we don't need to sustain map per session > and it seems enough to use current storage for trace_id not move to > sink driver. > > Anyway It would be better to refactorying wiht another patchset... Yeah, we can come back to these ideas when work on it. Thanks, Leo

1 month

Re: [PATCH v5 06/12] coresight: etm4x: fix leaked trace id

by Leo Yan

On Wed, Apr 15, 2026 at 05:55:22PM +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(). > > Reviewed-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > 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 f55338a4989d..b199aebbdb60 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -920,8 +920,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); > return ret; > + } LGTM: Reviewed-by: Leo Yan <leo.yan(a)arm.com> Just recording a bit thoughts. As Suzuki mentioned, it would be better to allocate trace IDs within a session. We might consider maintaining the trace ID map in the sink driver data, since the sink driver is unique within a session so it is a central place to allocate trace ID. We should use paired way for allocation and release. For example: coresight_enable_sysfs() { ... coresight_path_assign_trace_id(path); failed: coresight_path_unassign_trace_id(path); } coresight_disable_sysfs() { coresight_path_unassign_trace_id(path); } But this requires broader refactoring. E.g., the STM driver currently allocates system trace IDs statically during probe, we might need to consolidate for all modules to use dynamic allocation. Thanks, Leo

1 month

Re: [PATCH v5 01/12] coresight: etm4x: fix wrong check of etm4x_sspcicrn_present()

by Leo Yan

On Wed, Apr 15, 2026 at 05:55:17PM +0100, Yeoreum Yun wrote: > According to Embedded Trace Macrocell Architecture Specification > ETMv4.0 to ETM4.6 [0], TRCSSPCICR<n> is present only if all of > the following are true: > > - TRCIDR4.NUMSSCC > n. > - TRCIDR4.NUMPC > 0b0000. > - TRCSSCSR<n>.PC == 0b1. > > Comment for etm4x_sspcicrn_present() is align with the specification. > However, the check should use drvdata->nr_pe_cmp to check TRCIDR4.NUMPC > not nr_pe. > > Link: https://developer.arm.com/documentation/ihi0064/latest/ [0] > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> Reviewed-by: Leo Yan <leo.yan(a)arm.com>

1 month

Re: [PATCH v4 3/9] coresight: etm4x: fix leaked trace id

by Suzuki K Poulose

On 15/04/2026 09:45, Jie Gan wrote: > > > On 4/15/2026 4:32 PM, Leo Yan wrote: >> On Wed, Apr 15, 2026 at 09:01:09AM +0100, Yeoreum Yun wrote: >> >> [...] >> >>>>> 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. >>>> >>> >>> When I see the CORESIGHT_TRACE_ID_MAX: >>> >>> /* architecturally we have 128 IDs some of which are reserved */ >>> #define CORESIGHT_TRACE_IDS_MAX 128 >>> >>> I think this came from the hardware restriction for number of TRACE_IDs. >>> In this case, clamping the device_id to trace_id seems more complex and >>> reduce some performance perspective. >> >> Sigh, my stupid. Please ignore my previous comment, let us first fix >> ID leak issue. >> >> Given Jie's comment on the use-out issue, it is valid for me especially >> if a system have many dummy tracers. We can defer to refactor it >> later (e.g., use separate ranges for hardware and dummy tracers). >> >> thanks for correction! > > Just share some info: > > With my memory, The ARM AMBA ATB Protocol Specification defined a 7-bit > width field for the trace ID, that's where the 128 comes from. (in each > frame, we also have 7-bit field for containing the trace ID) That is true and some IDs in the range (0-128) are reserved. So we actually have less than 128. We need the dynamic allocation, preferrably isolated to a "pool" for the relevant session to make the full use of the space. Suzuki > > Thanks, > Jie >

1 month

← Newer
1
2
3
4
5
6
7
8
...
274
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight