CoreSight

coresight@lists.linaro.org

6 participants
2752 discussions

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

by Leo Yan

On Tue, May 19, 2026 at 04:48:08PM +0100, Yeoreum Yun wrote: > In the perf enable path, there are missing cases where > cscfg_csdev_disable_active_config() is not called: > > - Branch broadcast is selected but not supported by the hardware > - etm4_enable_hw() fails > > This can lead to a leak of config_desc->active_cnt. > Fix this by properly calling cscfg_csdev_disable_active_config() > in these error paths. > > Fixes: 810ac401db1f ("coresight: etm4x: Add complex configuration handlers to etmv4") > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> > --- > .../hwtracing/coresight/coresight-etm4x-core.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index f0a9f2ef4b27..0889937811cb 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -899,6 +899,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); > ret = -EINVAL; > goto out; > } else { > @@ -915,6 +917,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, > struct coresight_path *path) > { > struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > + struct perf_event_attr *attr = &event->attr; > int ret; > > if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) > @@ -926,7 +929,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, > /* Configure the tracer based on the session's specifics */ > ret = etm4_parse_event_config(csdev, event); > if (ret) > - goto out; > + goto err; > > drvdata->trcid = path->trace_id; > > @@ -935,16 +938,19 @@ static int etm4_enable_perf(struct coresight_device *csdev, > > /* And enable it */ > ret = etm4_enable_hw(drvdata); > - > -out: > - /* Failed to start tracer; roll back to DISABLED mode */ > if (ret) { > - coresight_set_mode(csdev, CS_MODE_DISABLED); > - return ret; > + if (ATTR_CFG_GET_FLD(attr, configid)) > + cscfg_csdev_disable_active_config(csdev); > + goto err; > } > > csdev->path = path; > return 0; > + > +err: > + /* Failed to start tracer; roll back to DISABLED mode */ > + coresight_set_mode(csdev, CS_MODE_DISABLED); > + return ret; > } Seems to me, it is not readable if spread error handling into both child and parent functions. I would like we can stick to use the same function for resource allocation and rollback for failures. How about below change (I did not verify it yet): diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 0889937811cb..8347efb2069b 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -794,8 +794,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, .ATTR_CFG_FLD_timestamp_CFG = U64_MAX, }; struct perf_event_attr *attr = &event->attr; - unsigned long cfg_hash; - int preset, cc_threshold; + int cc_threshold; u8 ts_level; /* Clear configuration from previous run */ @@ -882,16 +881,6 @@ static int etm4_parse_event_config(struct coresight_device *csdev, /* bit[12], Return stack enable bit */ config->cfg |= TRCCONFIGR_RS; - /* - * Set any selected configuration and preset. A zero configid means no - * configuration active, preset = 0 means no preset selected. - */ - cfg_hash = ATTR_CFG_GET_FLD(attr, configid); - if (cfg_hash) { - preset = ATTR_CFG_GET_FLD(attr, preset); - ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); - } - /* branch broadcast - enable if selected and supported */ if (ATTR_CFG_GET_FLD(attr, branch_broadcast)) { if (!caps->trcbb) { @@ -899,8 +888,6 @@ 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); ret = -EINVAL; goto out; } else { @@ -918,7 +905,8 @@ static int etm4_enable_perf(struct coresight_device *csdev, { struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct perf_event_attr *attr = &event->attr; - int ret; + unsigned long cfg_hash; + int preset, ret; if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) return -EINVAL; @@ -931,6 +919,18 @@ static int etm4_enable_perf(struct coresight_device *csdev, if (ret) goto err; + /* + * Set any selected configuration and preset. A zero configid means no + * configuration active, preset = 0 means no preset selected. + */ + cfg_hash = ATTR_CFG_GET_FLD(attr, configid); + if (cfg_hash) { + preset = ATTR_CFG_GET_FLD(attr, preset); + ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); + if (ret) + goto err; + } + drvdata->trcid = path->trace_id; /* Populate pause state */ @@ -938,15 +938,15 @@ static int etm4_enable_perf(struct coresight_device *csdev, /* And enable it */ ret = etm4_enable_hw(drvdata); - if (ret) { - if (ATTR_CFG_GET_FLD(attr, configid)) - cscfg_csdev_disable_active_config(csdev); - goto err; - } + if (ret) + goto err_enable_hw; csdev->path = path; return 0; +err_enable_hw: + if (cfg_hash) + cscfg_csdev_disable_active_config(csdev); err: /* Failed to start tracer; roll back to DISABLED mode */ coresight_set_mode(csdev, CS_MODE_DISABLED);

1 day, 4 hours

Re: [PATCH v7 08/13] coresight: etm4x: fix inconsistencies with sysfs configuration

by Leo Yan

On Thu, May 28, 2026 at 03:26:57PM +0100, Yeoreum Yun wrote: [...] > > > @@ -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; > > > > Wouldn't it make more sense to keep using drvdata->config for cfgfs? > > This would avoid cfgfs updating the active configuration while a session > > is enabled. > > > > My understanding is after refactoring cfgfs, we will never expose > > active_config or config anymore so this should not an issue. Before > > that, maybe we just keep to use config so can avoid mess. > > I don't think so. since the cfgfs's config is updated right before > enable and in this patchset, It's applied after "take the mode". > > If we do it into drvdta->config, then contention would be increased > with the access of sysfs and that nullifies almost this patch purpose > and because of we use "one config" this makes a corruption with perf > and sysfs. Here have two different contention: the contention between sysfs and cfgfs, and the contention between cfgfs and a runtime config (active config). My suggestion is to avoid the later contention, which is the main idea in this series that avoid the runtime config is modified in the middle of a session. That said, I have no strong opinion. > > > static void etm4_enable_sysfs_smp_call(void *info) > > > { > > > struct etm4_enable_arg *arg = info; > > > + struct etmv4_drvdata *drvdata; > > > struct coresight_device *csdev; > > > + unsigned long cfg_hash; > > > + int preset; > > > > > > if (WARN_ON(!arg)) > > > return; > > > > > > - csdev = arg->drvdata->csdev; > > > + drvdata = arg->drvdata; > > > + csdev = drvdata->csdev; > > > if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) { > > > /* Someone is already using the tracer */ > > > arg->rc = -EBUSY; > > > return; > > > } > > > > > > - arg->rc = etm4_enable_hw(arg->drvdata); > > > + /* enable any config activated by configfs */ > > > + cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset); > > > > > > - /* The tracer didn't start */ > > > + drvdata->active_config = arg->config; > > > > Can move this down to just before drvdata->trcid assignment so cscfg > > operations can be put together? > > No. Copy the arg->config must be before cscfg_csdev_enable_active_config(). > If that's after, it overrides the "preset" and this is different from > the former behavior. Please copy config first, then do cscfg stuffs. > > > + arg->rc = etm4_enable_hw(drvdata); > > > if (arg->rc) { > > > - coresight_set_mode(csdev, CS_MODE_DISABLED); > > > - return; > > > + cscfg_csdev_disable_active_config(csdev); > > > + goto err; > > > > Add a new goto tag (like err_enable_hw) for the disable_active_config > > rollback? > > This is only place where cscfg_csdev_disable_active_config(). > Why do we need to goto for cleanup where there is no duplication? It is about to use a central place for handling errors. > > > @@ -1449,7 +1458,7 @@ static void etm4_init_arch_data(void *info) > > > > > > /* EXLEVEL_S, bits[19:16] Secure state instruction tracing */ > > > caps->s_ex_level = FIELD_GET(TRCIDR3_EXLEVEL_S_MASK, etmidr3); > > > - drvdata->config.s_ex_level = caps->s_ex_level; > > > + config->s_ex_level = caps->s_ex_level; > > > > config->s_ex_level is redundant and not really a configuration item. > > We should use caps->s_ex_level instead of config->s_ex_level wherever > > it is used. > > Agree, but this includes it should change the omse function > declations to pass "caps" argument: > - etm4_set_comparator_filter() > - etm4_set_start_stop_filter() > > If that's okay, I'll respin with it. This is fine for me. Thanks, Leo

1 day, 5 hours

Re: [PATCH v7 08/13] coresight: etm4x: fix inconsistencies with sysfs configuration

by Leo Yan

On Tue, May 19, 2026 at 04:48:07PM +0100, Yeoreum Yun wrote: [...] > --- 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; Wouldn't it make more sense to keep using drvdata->config for cfgfs? This would avoid cfgfs updating the active configuration while a session is enabled. My understanding is after refactoring cfgfs, we will never expose active_config or config anymore so this should not an issue. Before that, maybe we just keep to use config so can avoid mess. > 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 9b477b36b432..f0a9f2ef4b27 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -243,6 +243,7 @@ void etm4_release_trace_id(struct etmv4_drvdata *drvdata) > struct etm4_enable_arg { > struct etmv4_drvdata *drvdata; > struct coresight_path *path; > + struct etmv4_config config; > int rc; > }; > > @@ -268,10 +269,11 @@ static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata) > static u64 etm4x_get_kern_user_filter(struct etmv4_drvdata *drvdata) > { > u64 trfcr = drvdata->trfcr; > + struct etmv4_config *config = &drvdata->active_config; > > - if (drvdata->config.mode & ETM_MODE_EXCL_KERN) > + if (config->mode & ETM_MODE_EXCL_KERN) > trfcr &= ~TRFCR_EL1_ExTRE; > - if (drvdata->config.mode & ETM_MODE_EXCL_USER) > + if (config->mode & ETM_MODE_EXCL_USER) > trfcr &= ~TRFCR_EL1_E0TRE; > > return trfcr; > @@ -279,7 +281,7 @@ static u64 etm4x_get_kern_user_filter(struct etmv4_drvdata *drvdata) > > /* > * etm4x_allow_trace - Allow CPU tracing in the respective ELs, > - * as configured by the drvdata->config.mode for the current > + * as configured by the drvdata->active_config.mode for the current > * session. Even though we have TRCVICTLR bits to filter the > * trace in the ELs, it doesn't prevent the ETM from generating > * a packet (e.g, TraceInfo) that might contain the addresses from > @@ -290,12 +292,13 @@ static u64 etm4x_get_kern_user_filter(struct etmv4_drvdata *drvdata) > static void etm4x_allow_trace(struct etmv4_drvdata *drvdata) > { > u64 trfcr, guest_trfcr; > + struct etmv4_config *config = &drvdata->active_config; > > /* If the CPU doesn't support FEAT_TRF, nothing to do */ > if (!drvdata->trfcr) > return; > > - if (drvdata->config.mode & ETM_MODE_EXCL_HOST) > + if (config->mode & ETM_MODE_EXCL_HOST) > trfcr = drvdata->trfcr & ~(TRFCR_EL1_ExTRE | TRFCR_EL1_E0TRE); > else > trfcr = etm4x_get_kern_user_filter(drvdata); > @@ -303,7 +306,7 @@ static void etm4x_allow_trace(struct etmv4_drvdata *drvdata) > write_trfcr(trfcr); > > /* Set filters for guests and pass to KVM */ > - if (drvdata->config.mode & ETM_MODE_EXCL_GUEST) > + if (config->mode & ETM_MODE_EXCL_GUEST) > guest_trfcr = drvdata->trfcr & ~(TRFCR_EL1_ExTRE | TRFCR_EL1_E0TRE); > else > guest_trfcr = etm4x_get_kern_user_filter(drvdata); > @@ -497,7 +500,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) > { > int i, rc; > 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 device *etm_dev = &csdev->dev; > struct csdev_access *csa = &csdev->access; > @@ -618,27 +621,53 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) > static void etm4_enable_sysfs_smp_call(void *info) > { > struct etm4_enable_arg *arg = info; > + struct etmv4_drvdata *drvdata; > struct coresight_device *csdev; > + unsigned long cfg_hash; > + int preset; > > if (WARN_ON(!arg)) > return; > > - csdev = arg->drvdata->csdev; > + drvdata = arg->drvdata; > + csdev = drvdata->csdev; > if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) { > /* Someone is already using the tracer */ > arg->rc = -EBUSY; > return; > } > > - arg->rc = etm4_enable_hw(arg->drvdata); > + /* enable any config activated by configfs */ > + cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset); > > - /* The tracer didn't start */ > + drvdata->active_config = arg->config; Can move this down to just before drvdata->trcid assignment so cscfg operations can be put together? > + > + if (cfg_hash) { > + arg->rc = cscfg_csdev_enable_active_config(csdev, > + cfg_hash, > + preset); > + if (arg->rc) > + goto err; > + } > + > + drvdata->trcid = arg->path->trace_id; > + > + /* Tracer will never be paused in sysfs mode */ > + drvdata->paused = false; > + > + arg->rc = etm4_enable_hw(drvdata); > if (arg->rc) { > - coresight_set_mode(csdev, CS_MODE_DISABLED); > - return; > + cscfg_csdev_disable_active_config(csdev); > + goto err; Add a new goto tag (like err_enable_hw) for the disable_active_config rollback? > } > > + drvdata->sticky_enable = true; > csdev->path = arg->path; > + > + return; > +err: > + /* The tracer didn't start */ > + coresight_set_mode(csdev, CS_MODE_DISABLED); > } > > /* > @@ -676,7 +705,7 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata, > int ctridx; > int rselector; > const struct etmv4_caps *caps = &drvdata->caps; > - struct etmv4_config *config = &drvdata->config; > + struct etmv4_config *config = &drvdata->active_config; > > /* No point in trying if we don't have at least one counter */ > if (!caps->nr_cntr) > @@ -760,7 +789,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > int ret = 0; > struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > const struct etmv4_caps *caps = &drvdata->caps; > - struct etmv4_config *config = &drvdata->config; > + struct etmv4_config *config = &drvdata->active_config; > struct perf_event_attr max_timestamp = { > .ATTR_CFG_FLD_timestamp_CFG = U64_MAX, > }; > @@ -922,25 +951,7 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa > { > struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > struct etm4_enable_arg arg = { }; > - unsigned long cfg_hash; > - int ret, preset; > - > - /* 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; > + int ret; > > /* > * Executing etm4_enable_hw on the cpu whose ETM is being enabled > @@ -948,20 +959,20 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa > */ > arg.drvdata = drvdata; > arg.path = path; > + > + raw_spin_lock(&drvdata->spinlock); > + arg.config = drvdata->config; > + raw_spin_unlock(&drvdata->spinlock); > + > 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; > } > > @@ -1048,7 +1059,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; > @@ -1091,6 +1102,8 @@ static void etm4_disable_sysfs_smp_call(void *info) > etm4_disable_hw(drvdata); > > drvdata->csdev->path = NULL; > + cscfg_csdev_disable_active_config(drvdata->csdev); Move cscfg_csdev_disable_active_config() just after etm4_disable_hw() so the sequence is consistent with perf mode. [...] > @@ -1449,7 +1458,7 @@ static void etm4_init_arch_data(void *info) > > /* EXLEVEL_S, bits[19:16] Secure state instruction tracing */ > caps->s_ex_level = FIELD_GET(TRCIDR3_EXLEVEL_S_MASK, etmidr3); > - drvdata->config.s_ex_level = caps->s_ex_level; > + config->s_ex_level = caps->s_ex_level; config->s_ex_level is redundant and not really a configuration item. We should use caps->s_ex_level instead of config->s_ex_level wherever it is used. Thanks, Leo

1 day, 6 hours

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

by Leo Yan

On Tue, May 19, 2026 at 04:48:04PM +0100, Yeoreum Yun wrote: [...] > @@ -571,11 +571,11 @@ 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 */ > + /* always clear status and pending bits on restart if using single-shot */ > if (config->ss_ctrl[i] || config->ss_pe_cmp[i]) > - config->ss_status[i] &= ~TRCSSCSRn_STATUS; > + drvdata->ss_status[i] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING); > etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i)); > - etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i)); > + etm4x_relaxed_write32(csa, drvdata->ss_status[i], TRCSSCSRn(i)); > if (etm4x_sspcicrn_present(drvdata, i)) > etm4x_relaxed_write32(csa, config->ss_pe_cmp[i], TRCSSPCICRn(i)); > } > @@ -772,6 +772,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > /* Clear configuration from previous run */ > memset(config, 0, sizeof(struct etmv4_config)); > > + Unexpected new line? > @@ -1497,8 +1498,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); Since etm4_enable_hw() clears the TRCSSCSRn_STATUS and TRCSSCSRn_PENDING bits every time, here is no need to clear the status bits during probe. In the future, we may want to preserve the status within a session and clear it only when starting a new session. Clearing the status bits here still cannot handle stale status across multiple sessions, so we can defer this improvement for later. Thanks, Leo

1 day, 7 hours

Re: [PATCH v7 03/13] coresight: etm4x: introduce ETM_MAX_SEQ_TRANSITIONS

by Leo Yan

On Tue, May 19, 2026 at 04:48:02PM +0100, Yeoreum Yun wrote: > According to IHI006H Embedded Trace Macrocell Architecture > Specification [0], n could be 0-2 for TCRSEQEVR<n> when > TCRIDR5.NUMSEQSTATE is 0b100. > > Therefore, introduce ETM_MAX_SEQ_TRANSITIONS macro and apply this > in TCRSEQEVR<n> relevant fields. > > Link: https://developer.arm.com/documentation/ihi0064/latest/ [0] > Suggestedby: Leo Yan <leo.yan(a)arm.com> s/Suggestedby/Suggested-by > Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com> The change looks good to me: Reviewed-by: Leo Yan <leo.yan(a)arm.com>

1 day, 7 hours

Re: [PATCH] perf cs-etm: stamp pid/tid/EL on each buffered packet to fix cross-pid attribution

by James Clark

On 15/05/2026 3:11 am, Amir Ayupov wrote: > In a system-wide `perf record -e cs_etm/.../u` capture on aarch64, > synthesized samples emitted by `perf script --itrace=il64` are > sometimes attributed to the WRONG sample.pid/tid (and to the wrong > EL/cpumode) for the chunk of branches that straddle a context-switch > boundary on a CPU. A branch actually retired by process A is emitted > with sample.pid set to the thread that next ran on the same CPU. > > Mechanism: > 1. ETM emits CONTEXTIDR/EL packets in-stream when the kernel updates > CONTEXTIDR_EL1 on context switch / EL change. OpenCSD turns these > into OCSD_GEN_TRC_ELEM_PE_CONTEXT elements interleaved with > OCSD_GEN_TRC_ELEM_INSTR_RANGE elements for retired branch ranges. > 2. cs_etm_decoder__buffer_range() queues each INSTR_RANGE into > packet_queue->packet_buffer[]; packets carry start/end addrs, > instr_count, last-instruction info, etc., but NO owner identity. > 3. PE_CONTEXT goes through cs_etm_decoder__set_tid() -> > cs_etm__set_thread(), which immediately mutates tidq->thread and > tidq->el. Queued packets are not drained first; reset_timestamp() > is called so the next TIMESTAMP triggers OCSD_RESP_WAIT and a > drain. > 4. By drain time in cs_etm__process_traceid_queue() -> > cs_etm__sample(), sample.pid/tid is read from the now-mutated > tidq->thread and sample.cpumode from the now-mutated tidq->el. > Pre-context INSTR_RANGEs get the post-context owner. > > The same race affects branch samples via tidq->prev_packet_thread / > tidq->prev_packet_el, captured at packet-swap time from > tidq->thread / tidq->el (which may already have flipped). > > This is independent of PERF_RECORD_SWITCH_CPU_WIDE, which is > deliberately not used to assign sample identity in this path. The > bug applies to any cs_etm capture with in-stream CONTEXTIDR > (PIDFMT_CTXTID or PIDFMT_CTXTID2). > > Effect on downstream tools: branches that should belong to the > previous thread on the CPU get attributed to the next thread. When > the two threads share a binary, leaked branches' VAs land in the > wrong thread's mappings; samples whose IPs land in r-x mappings > silently pollute that binary's profile, while samples landing in > R-only/RW mappings show up as out-of-range / non-text samples. > Either way, AutoFDO/BOLT profiles built from `perf script --itrace` > output of system-wide cs_etm captures contain misattributed samples. > > Concrete example from `perf script --itrace=il64` of the same > captured branch (same timestamp, same IP, same from/to addrs) before > and after this fix: > > before: launcher_multia 2638146/2638146 705897.219172: \ > fffcda6b124c 0xfffcda641958/0xfffcda6b123c > after: ws-tcf-sr-io13 2736581/2741587 705897.219172: \ > fffcda6b124c 0xfffcda641958/0xfffcda6b123c > > The branch was retired by ws-tcf-sr-io13 (tid 2741587) but, before > the fix, was attributed to launcher_multia (the next thread to run on > that CPU after the context switch). After the fix, it is correctly > attributed to ws-tcf-sr-io13. > > Why not "drain on PE_CONTEXT then switch" (deferred-set_thread): > tidq->thread has two consumers \u2014 sample emission needs the OUTGOING > identity for queued packets, but cs_etm__mem_access() needs the > CURRENT thread's maps to fetch instruction bytes for OpenCSD. The > two needs are temporally inverted; a single tidq->thread cannot > serve both. Keeping tidq->thread current and stamping owner identity > per packet is the only design that decouples them cleanly. > > Fix: capture the owning pid/tid/EL on each buffered packet at > cs_etm_decoder__buffer_packet() time (before any subsequent > PE_CONTEXT can mutate tidq->thread / tidq->el), and read them at > sample emission time. > > - struct cs_etm_packet gains pid_t pid, pid_t tid, int el (storing > an ocsd_ex_level value; typed as int so the struct does not > depend on OpenCSD headers, which are only included inside > HAVE_CSTRACE_SUPPORT). > - cs_etm__etmq_get_pid_tid_el() (formerly cs_etm__etmq_get_pid_tid) > returns all three. > - cs_etm__synth_instruction_sample() reads sample.pid / sample.tid > from tidq->packet->{pid,tid} and derives sample.cpumode from > tidq->packet->el. > - cs_etm__synth_branch_sample() reads sample.pid / sample.tid / > cpumode from tidq->prev_packet->{pid,tid,el}. > - The separate prev_packet_thread / prev_packet_el bookkeeping in > cs_etm__packet_swap() / cs_etm__init_traceid_queue() / > cs_etm__free_traceid_queues() is removed; the per-packet stamp > on prev_packet now carries that information. > > Cost: 12 bytes added to struct cs_etm_packet (~12-16 KB per > packet_queue with CS_ETM_PACKET_MAX_BUFFER=1024), 16 bytes saved per > cs_etm_traceid_queue (one struct thread * + one ocsd_ex_level). > > A residual gap: cs_etm__copy_insn() reads sample.insn bytes via > cs_etm__mem_access(), which still uses tidq->thread (the current > thread), so the inline insn bytes for an outgoing-thread sample may > be looked up against the wrong address space. Fixing this requires > threading the packet's owner pid through cs_etm__mem_access and is > left for a follow-up. sample.ip / sample.pid attribution \u2014 what > AutoFDO/BOLT consume \u2014 is correct. > Hi Amir, Can you test the patch here to see if it fixes your issue [1]? We thought it didn't make sense to store the thread on every packet when there is only one active thread for the decoder and one for sample generation. We also fixed the other issue mentioned above about cs_etm__copy_insn() not working. Thanks James [1]: https://lore.kernel.org/linux-perf-users/20260526-james-cs-context-tracking…

3 days, 9 hours

Re: [PATCH] coresight: fix resource leaks on path build failure

by James Clark

On 20/05/2026 11:13 am, Jie Gan wrote: > > > On 5/20/2026 5:27 PM, Mike Leach wrote: >> >> >>> -----Original Message----- >>> From: James Clark <james.clark(a)linaro.org> >>> Sent: Wednesday, May 20, 2026 9:38 AM >>> To: Jie Gan <jie.gan(a)oss.qualcomm.com> >>> Cc: coresight(a)lists.linaro.org; linux-arm-kernel(a)lists.infradead.org; >>> linux- >>> kernel(a)vger.kernel.org; Suzuki Poulose <Suzuki.Poulose(a)arm.com>; Mike >>> Leach <Mike.Leach(a)arm.com>; Leo Yan <Leo.Yan(a)arm.com>; Alexander >>> Shishkin <alexander.shishkin(a)linux.intel.com>; Mathieu Poirier >>> <mathieu.poirier(a)linaro.org>; Tingwei Zhang >>> <tingwei.zhang(a)oss.qualcomm.com>; Greg Kroah-Hartman >>> <gregkh(a)linuxfoundation.org> >>> Subject: Re: [PATCH] coresight: fix resource leaks on path build failure >>> >>> >>> >>> On 20/05/2026 2:55 am, Jie Gan wrote: >>>> >>>> >>>> On 5/19/2026 9:57 PM, James Clark wrote: >>>>> >>>>> >>>>> On 13/05/2026 2:32 am, Jie Gan wrote: >>>>>> Two related leaks when _coresight_build_path() encounters an error >>>>>> after >>>>>> coresight_grab_device() has already incremented the pm_runtime, >>> module, >>>>>> and device references for a node: >>>>>> >>>>>> 1. In _coresight_build_path(), if kzalloc_obj() for the path node >>>>>> fails >>>>>> after coresight_grab_device() succeeds, >>>>>> coresight_drop_device() was >>>>>> never called, permanently leaking all three references. >>>>>> >>>>>> 2. In coresight_build_path(), on failure the partial path was >>>>>> freed with >>>>>> kfree(path) instead of coresight_release_path(path). kfree() >>>>>> only >>>>>> frees the coresight_path struct itself; it does not iterate >>>>>> path_list >>>>>> to call coresight_drop_device() and kfree() for each >>>>>> coresight_node >>>>>> already added by deeper recursive calls, leaking both the >>>>>> pm_runtime, >>>>>> module, and device references and the node memory for every >>>>>> element >>>>>> on the partial path. >>>>>> >>>>>> Fix both by adding coresight_drop_device() in the OOM unwind of >>>>>> _coresight_build_path(), and replacing kfree(path) with >>>>>> coresight_release_path(path) in coresight_build_path(). >>>>>> >>>>>> Fixes: 32b0707a4182 ("coresight: Add try_get_module() in >>>>>> coresight_grab_device()") >>>>>> Fixes: b3e94405941e ("coresight: associating path with session rather >>>>>> than tracer") >>>>>> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> >>>>>> --- >>>>>> drivers/hwtracing/coresight/coresight-core.c | 6 ++++-- >>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ >>>>>> hwtracing/coresight/coresight-core.c >>>>>> index 46f247f73cf6..c1354ea8e11d 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-core.c >>>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c >>>>>> @@ -825,8 +825,10 @@ static int _coresight_build_path(struct >>>>>> coresight_device *csdev, >>>>>> return ret; >>>>>> node = kzalloc_obj(struct coresight_node); >>>>>> - if (!node) >>>>>> + if (!node) { >>>>>> + coresight_drop_device(csdev); >>>>>> return -ENOMEM; >>>>>> + } >>>>>> node->csdev = csdev; >>>>>> list_add(&node->link, &path->path_list); >>>>>> @@ -851,7 +853,7 @@ struct coresight_path >>>>>> *coresight_build_path(struct coresight_device *source, >>>>>> rc = _coresight_build_path(source, source, sink, path); >>>>>> if (rc) { >>>>>> - kfree(path); >>>>>> + coresight_release_path(path); >>>>>> return ERR_PTR(rc); >>>>>> } >>>>>> >>>>>> --- >>>>>> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83 >>>>>> change-id: 20260513-fix-memory-leak-issue-034b4a45265e >>>>>> >>>>>> Best regards, >>>>> >>>>> Looks good to me, but sashiko is complaining: https://sashiko.dev/#/ >>>>> patchset/20260513-fix-memory-leak-issue- >>>>> v1-1-49822d7bc7d4%40oss.qualcomm.com >>>>> >>>>> I'm trying to understand why it's saying that, but I think the >>>>> scenario is that if there are multiple correct paths to a sink, when >>>>> one path partially fails and a second path succeeds you could get a >>>>> path_list with some garbage entries in it. >>>> >>>> I think the coresight_release_path is added to address this situation. >>>> We suffered the path partially failure, and we need release all nodes >>>> already added to the path. >>>> >>> >>> It wouldn't call coresight_release_path() in this case though. If one >>> path ends up building to success but a parallel path partially failed >>> then _coresight_build_path() still returns success. During the search it >>> would have still added the nodes from the partially failed path to the >>> path_list. This is only an issue if there are multiple correct paths. >>> > > The point here is there are multiple routes from the same source device > to the same sink device, am right? > > I have no experience on this scenario. So with the scenario, the > build_path may succeeded in one route and failed in another route, but > finally, the _coresight_build_path still returns success, is that correct? > >>>>> >>>>> That's kind of a different and existing issue to the one you've fixed, >>>>> and assumes that multiple paths to one sink are possible, which I'm >>>>> not sure is supported? >>>> >>>> Each path is unique. We only deal with the issue path for balancing the >>>> reference count. >>>> >>>> Thanks, >>>> Jie >>>> >>> >>> I'm not exactly sure what you mean by unique, but the same source and >>> sink could potentially be connected through two different sets of links. >>> >> >> Multiple paths between a source and sink are not permitted under the >> CoreSight spec. >> > > As Mike mentioned, my understanding is that a source device is only > allowed to be added to one valid path—this is what I mean by “unique.” > > Thanks, > Jie > That's ok then we can ignore this for this patch. But it would be good to enforce that in _coresight_build_path() with some kind of assert. Or at least add a comment to appease the AI reviewers. >> If such a system was to be built - then a fix would need to be in the >> declaration of connections - e.g. miss one path out in the device tree >> for example. Not up to the Coresight drivers to handle out of >> specification hardware. >> >> Mike >> >> >>>>> >>>>> It might be as easy as breaking the loop early for any return value >>>>> other than -ENODEV, but I'll leave it to you to decide whether to do >>>>> that here or not. >>>>> >>>>> Reviewed-by: James Clark <james.clark(a)linaro.org> >>>>> >>>> >> >

1 week, 2 days

Re: [PATCH] coresight: fix resource leaks on path build failure

by James Clark

On 20/05/2026 2:55 am, Jie Gan wrote: > > > On 5/19/2026 9:57 PM, James Clark wrote: >> >> >> On 13/05/2026 2:32 am, Jie Gan wrote: >>> Two related leaks when _coresight_build_path() encounters an error after >>> coresight_grab_device() has already incremented the pm_runtime, module, >>> and device references for a node: >>> >>> 1. In _coresight_build_path(), if kzalloc_obj() for the path node fails >>> after coresight_grab_device() succeeds, coresight_drop_device() was >>> never called, permanently leaking all three references. >>> >>> 2. In coresight_build_path(), on failure the partial path was freed with >>> kfree(path) instead of coresight_release_path(path). kfree() only >>> frees the coresight_path struct itself; it does not iterate >>> path_list >>> to call coresight_drop_device() and kfree() for each coresight_node >>> already added by deeper recursive calls, leaking both the >>> pm_runtime, >>> module, and device references and the node memory for every element >>> on the partial path. >>> >>> Fix both by adding coresight_drop_device() in the OOM unwind of >>> _coresight_build_path(), and replacing kfree(path) with >>> coresight_release_path(path) in coresight_build_path(). >>> >>> Fixes: 32b0707a4182 ("coresight: Add try_get_module() in >>> coresight_grab_device()") >>> Fixes: b3e94405941e ("coresight: associating path with session rather >>> than tracer") >>> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> >>> --- >>> drivers/hwtracing/coresight/coresight-core.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ >>> hwtracing/coresight/coresight-core.c >>> index 46f247f73cf6..c1354ea8e11d 100644 >>> --- a/drivers/hwtracing/coresight/coresight-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-core.c >>> @@ -825,8 +825,10 @@ static int _coresight_build_path(struct >>> coresight_device *csdev, >>> return ret; >>> node = kzalloc_obj(struct coresight_node); >>> - if (!node) >>> + if (!node) { >>> + coresight_drop_device(csdev); >>> return -ENOMEM; >>> + } >>> node->csdev = csdev; >>> list_add(&node->link, &path->path_list); >>> @@ -851,7 +853,7 @@ struct coresight_path >>> *coresight_build_path(struct coresight_device *source, >>> rc = _coresight_build_path(source, source, sink, path); >>> if (rc) { >>> - kfree(path); >>> + coresight_release_path(path); >>> return ERR_PTR(rc); >>> } >>> >>> --- >>> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83 >>> change-id: 20260513-fix-memory-leak-issue-034b4a45265e >>> >>> Best regards, >> >> Looks good to me, but sashiko is complaining: https://sashiko.dev/#/ >> patchset/20260513-fix-memory-leak-issue- >> v1-1-49822d7bc7d4%40oss.qualcomm.com >> >> I'm trying to understand why it's saying that, but I think the >> scenario is that if there are multiple correct paths to a sink, when >> one path partially fails and a second path succeeds you could get a >> path_list with some garbage entries in it. > > I think the coresight_release_path is added to address this situation. > We suffered the path partially failure, and we need release all nodes > already added to the path. > It wouldn't call coresight_release_path() in this case though. If one path ends up building to success but a parallel path partially failed then _coresight_build_path() still returns success. During the search it would have still added the nodes from the partially failed path to the path_list. This is only an issue if there are multiple correct paths. >> >> That's kind of a different and existing issue to the one you've fixed, >> and assumes that multiple paths to one sink are possible, which I'm >> not sure is supported? > > Each path is unique. We only deal with the issue path for balancing the > reference count. > > Thanks, > Jie > I'm not exactly sure what you mean by unique, but the same source and sink could potentially be connected through two different sets of links. >> >> It might be as easy as breaking the loop early for any return value >> other than -ENODEV, but I'll leave it to you to decide whether to do >> that here or not. >> >> Reviewed-by: James Clark <james.clark(a)linaro.org> >> >

1 week, 2 days

Re: [PATCH] coresight: fix resource leaks on path build failure

by James Clark

On 13/05/2026 2:32 am, Jie Gan wrote: > Two related leaks when _coresight_build_path() encounters an error after > coresight_grab_device() has already incremented the pm_runtime, module, > and device references for a node: > > 1. In _coresight_build_path(), if kzalloc_obj() for the path node fails > after coresight_grab_device() succeeds, coresight_drop_device() was > never called, permanently leaking all three references. > > 2. In coresight_build_path(), on failure the partial path was freed with > kfree(path) instead of coresight_release_path(path). kfree() only > frees the coresight_path struct itself; it does not iterate path_list > to call coresight_drop_device() and kfree() for each coresight_node > already added by deeper recursive calls, leaking both the pm_runtime, > module, and device references and the node memory for every element > on the partial path. > > Fix both by adding coresight_drop_device() in the OOM unwind of > _coresight_build_path(), and replacing kfree(path) with > coresight_release_path(path) in coresight_build_path(). > > Fixes: 32b0707a4182 ("coresight: Add try_get_module() in coresight_grab_device()") > Fixes: b3e94405941e ("coresight: associating path with session rather than tracer") > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 46f247f73cf6..c1354ea8e11d 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -825,8 +825,10 @@ static int _coresight_build_path(struct coresight_device *csdev, > return ret; > > node = kzalloc_obj(struct coresight_node); > - if (!node) > + if (!node) { > + coresight_drop_device(csdev); > return -ENOMEM; > + } > > node->csdev = csdev; > list_add(&node->link, &path->path_list); > @@ -851,7 +853,7 @@ struct coresight_path *coresight_build_path(struct coresight_device *source, > > rc = _coresight_build_path(source, source, sink, path); > if (rc) { > - kfree(path); > + coresight_release_path(path); > return ERR_PTR(rc); > } > > > --- > base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83 > change-id: 20260513-fix-memory-leak-issue-034b4a45265e > > Best regards, Looks good to me, but sashiko is complaining: https://sashiko.dev/#/patchset/20260513-fix-memory-leak-issue-v1-1-49822d7b… I'm trying to understand why it's saying that, but I think the scenario is that if there are multiple correct paths to a sink, when one path partially fails and a second path succeeds you could get a path_list with some garbage entries in it. That's kind of a different and existing issue to the one you've fixed, and assumes that multiple paths to one sink are possible, which I'm not sure is supported? It might be as easy as breaking the loop early for any return value other than -ENODEV, but I'll leave it to you to decide whether to do that here or not. Reviewed-by: James Clark <james.clark(a)linaro.org>

1 week, 3 days

Re: [PATCH] coresight: platform: defer connection counter increment until alloc succeeds

by James Clark

On 11/05/2026 5:19 am, Jie Gan wrote: > coresight_add_out_conn() increments nr_outconns before calling > devm_krealloc_array() and again before devm_kmalloc(). If either > allocation fails, the counter is already bumped while the corresponding > array entry is NULL or uninitialized garbage. > > coresight_add_in_conn() has the same problem with nr_inconns and > devm_krealloc_array(). > > In both cases the probe returns -ENOMEM, which causes > coresight_get_platform_data() to call coresight_release_platform_data() > for cleanup. That function iterates up to nr_outconns (or nr_inconns) > entries and dereferences each pointer unconditionally, hitting the NULL > or garbage entry and panicking instead of failing gracefully. > > Fix by moving the counter increments to after all allocations succeed, > so the struct is always consistent on any error path. > > Fixes: 3d4ff657e454 ("coresight: Dynamically add connections") > Fixes: e3f4e68797a9 ("coresight: Store in-connections as well as out-connections") > Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> Reviewed-by: James Clark <james.clark(a)linaro.org> > --- > drivers/hwtracing/coresight/coresight-platform.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c > index e337b6e2bf32..93c2d075cad6 100644 > --- a/drivers/hwtracing/coresight/coresight-platform.c > +++ b/drivers/hwtracing/coresight/coresight-platform.c > @@ -45,9 +45,8 @@ coresight_add_out_conn(struct device *dev, > } > } > > - pdata->nr_outconns++; > pdata->out_conns = > - devm_krealloc_array(dev, pdata->out_conns, pdata->nr_outconns, > + devm_krealloc_array(dev, pdata->out_conns, pdata->nr_outconns + 1, > sizeof(*pdata->out_conns), GFP_KERNEL); > if (!pdata->out_conns) > return ERR_PTR(-ENOMEM); > @@ -63,7 +62,8 @@ coresight_add_out_conn(struct device *dev, > * used right away. > */ > *conn = *new_conn; > - pdata->out_conns[pdata->nr_outconns - 1] = conn; > + pdata->out_conns[pdata->nr_outconns] = conn; > + pdata->nr_outconns++; > return conn; > } > EXPORT_SYMBOL_GPL(coresight_add_out_conn); > @@ -86,13 +86,13 @@ int coresight_add_in_conn(struct coresight_connection *out_conn) > return 0; > } > > - pdata->nr_inconns++; > pdata->in_conns = > - devm_krealloc_array(dev, pdata->in_conns, pdata->nr_inconns, > + devm_krealloc_array(dev, pdata->in_conns, pdata->nr_inconns + 1, > sizeof(*pdata->in_conns), GFP_KERNEL); > if (!pdata->in_conns) > return -ENOMEM; > - pdata->in_conns[pdata->nr_inconns - 1] = out_conn; > + pdata->in_conns[pdata->nr_inconns] = out_conn; > + pdata->nr_inconns++; > return 0; > } > EXPORT_SYMBOL_GPL(coresight_add_in_conn); > > --- > base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83 > change-id: 20260511-fix-ref-count-issue-7c44ce39700f > > Best regards,

1 week, 3 days

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight