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

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Leo,

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

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.

Therefore, It might be right to remain this as it is.
and let's remove it at the time of refactorying.

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

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.

> 
> > +
> > +	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?

This is only place where cscfg_csdev_disable_active_config().
Why do we need to goto for cleanup where there is no duplication?

> 
> >  	}
> >  
> > +	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.

Fair enough. I'll do that.

> 
> [...]
> 
> > @@ -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.

> 
> Thanks,
> Leo

-- 
Sincerely,
Yeoreum Yun