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

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

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