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

[Leo Yan <leo.yan@arm.com>]

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