CoreSight April 2026

coresight@lists.linaro.org

5 participants
34 discussions

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

by Mike Leach

Hi, On 4/21/26 11:30, Yeoreum Yun wrote: >> Hi Mike, >> >>> Hi, >>> >>> This register [bit 31] indicates if a single shot comparator has matched. So >>> read-back provides information to the user post run to determine which if >>> any of the comparators set in this way has actually matched. >> >> Okay. so after disable sysfs session, to check former session >> check whether comprator has matched. >> >>> >>> Moreover, the specification states "Software must reset this bit to 0 to >>> re-enable single-shot control" and "Reset state is unknown. STATUS must be >>> written to set an initial state...." >>> >>> Therefore this register must be written as part of any configuration so >>> should be available in the drvdata->config for both read and write, >> >> But I don't think this is the reason for locate ss_status into "config" >> since its write purpose is not to configure but the "clear" former bit. >> That's why I think it's enough to clear when the new sysfs session starts. >> > > IOW, I think it's better to remove ss_status from configfs item > and > - add field ss_cmp in etm4_cpas > - add another field ss_status under "etm4_drvdata" to show "PENDING > and STATUS" bits to sysfs after finishing session. > > Is is valid for you? > No. Why two different locations for a single register read? If I have the ETMv4 hardware manual I am going to look for a something that is recognizable as being related to the single shot comparator status register(s). So in sysfs I would expect to see all the bits from the register, displayed, without masking off the STATUS and PENDING bits as happens now. In the code I would expect to see a single location with a sensible name - ss_cmp doesn't really correlate terribly well with TRCSSCSR. If you do not like the original ss_status, then ss_cmp_status may actually be better, ss_cmp could be either the ss comparator status or control register. Regards Mike >> -- >> Sincerely, >> Yeoreum Yun >> > > -- > Sincerely, > Yeoreum Yun

14 hours, 4 minutes

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

by Leo Yan

On Tue, Apr 21, 2026 at 12:14:20PM +0100, Yeoreum Yun wrote: [...] > > > - 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(). > > This is not true. > at the time of etm4_disable_sysfs() "mode" is already taken > (whether sysfs or perf). In this situation, it's enough to > call cscfg_csdev_disable_active_config() before chaning > mode to DISABLED. To be clear, I am trying to understand issue _before_ your patch. Without this patch, cscfg_csdev_disable_active_config() is not protected by the mode. > > > struct etm4_enable_arg { > > > struct etmv4_drvdata *drvdata; > > > + struct etmv4_config config; > > > > We don't need this. We can defer to get drvdata->config in SMP call. > > This is not true ane make a situation more complex. > If we get config in SMP call, that would be a problem when some user is > trying to modify config. > > IOW, while user modifying config via sysfs, and SMP call happens, > it makes a dead lock. so for this if we try to grab the lock in SMP call, > we should change every sysfs raw_spin_lock() into raw_spin_lock_irqsave(). > > Instead of this it would be much clear and simpler to get config in here > rather than to add some latencies via sysfs. Thanks for info. If so, it is fine for me to add "config". > > > @@ -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? > > No. at least when the initialise, I think we should fill the its > contesnt with the "etm4_set_default()". > > That's why the consequence call etm4_set_default() called with > active_config and config is coped with the default configutation. I'm concerned that some config fields may be reused across sessions. For example, a sysfs session copies drvdata->config into drvdata->active_config, and later a perf session may reuse stale values. The same issue can happen in the reverse direction. A clean approach would be to treat drvdata->active_config as per-session state: 1) clear drvdata->active_config at session start 2) apply the session-specific config 2.1) sysfs: use drvdata->config 2.2) perf: use event config 3) then apply configfs configuration So we should clear drvdata->active_config at the start of each session and rebuild it with the correct configuration. Thanks, Leo

14 hours, 52 minutes

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

17 hours, 35 minutes

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

18 hours, 22 minutes

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>

19 hours, 30 minutes

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); > } >

19 hours, 33 minutes

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>

21 hours, 39 minutes

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

4 days, 19 hours

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

4 days, 20 hours

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

5 days, 11 hours

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight April 2026