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

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