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

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

On Tue, Apr 21, 2026 at 11:46:01AM +0100, Leo Yan wrote:
> On Wed, Apr 15, 2026 at 05:55:23PM +0100, Yeoreum Yun wrote:
> > The current ETM4x configuration via sysfs can lead to
> > several inconsistencies:
> >
> >   - If the configuration is modified via sysfs while a perf session is
> >     active, the running configuration may differ before a sched-out and
> >     after a subsequent sched-in.
> >
> >   - If a perf session and a sysfs session enable tracing concurrently,
> >     the configuration from configfs may become corrupted.
>
> I think this happens because the sysfs path calls
> cscfg_csdev_enable_active_config() without first acquiring the mode,
> allowing a perf session to acquire the mode and call the same function
> concurrently.

Yes. That's why this patch changes to call
cscfg_csdev_enable_active_config() after taking a mode.

>
> >   - 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 resolve these issues, separate the configuration into:
> >
> >   - active_config: the configuration applied to the current session
> >   - config: the configuration set via sysfs
> >
> > Additionally:
> >
> >   - Apply the configuration from configfs after taking the appropriate mode.
> >
> >   - Since active_config and related fields are accessed only by the local CPU
> >     in etm4_enable/disable_sysfs_smp_call() (similar to perf enable/disable),
> >     remove the lock/unlock from the sysfs enable/disable path and
> >     startup/dying_cpu except when to access config fields.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > ---
> >  .../hwtracing/coresight/coresight-etm4x-cfg.c |   2 +-
> >  .../coresight/coresight-etm4x-core.c          | 107 ++++++++++--------
> >  drivers/hwtracing/coresight/coresight-etm4x.h |   2 +
> >  3 files changed, 63 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> > index d14d7c8a23e5..0553771d04e7 100644
> > --- 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;
> >  	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 b199aebbdb60..15aaf4a898e1 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -245,6 +245,10 @@ void etm4_release_trace_id(struct etmv4_drvdata *drvdata)
> >
> >  struct etm4_enable_arg {
> >  	struct etmv4_drvdata *drvdata;
> > +	unsigned long cfg_hash;
> > +	int preset;
>
> Since smp call cannot directly call cscfg_config_sysfs_get_active_cfg()
> due to it runs in atomic context but ...active_cfg() tries to acquire
> mutex. So we firstly retrieve pass 'cfg_hash' and 'preset' in sleepable
> context and deliver them to the SMP call.
>
> After coresight cfg refactoring, we should remove cfg_hash/preset from
> ETM driver, the ETM driver only needs to retrieve register list and can
> do this in smp call.
>
> Before we finish cfg refactoring, it is fine for me to add these two
> parameters into etm4_enable_arg.

Sound good and interting to refactorying. but as you said
this is fine for fixing a bug purpose.
>
> > +	u8 trace_id;
>
> Can we add 'path' instead ? The SMP call can retrieve path->trace_id.
> This can benefit for future clean up (e.g., we can store config into
> path so we can retrieve config from path pointer), and this allows us
> for further refactoring to unify etm4_enable_sysfs_smp_call() and
> etm4_enable_perf().

Okay. I'll change it with the path.

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

>
> >  	int rc;
> >  };
>
> [...]
>
> > @@ -918,40 +946,29 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
> >
> >  	/* 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;
> >
> >  	/*
> >  	 * Executing etm4_enable_hw on the cpu whose ETM is being enabled
> >  	 * ensures that register writes occur when cpu is powered.
> >  	 */
> >  	arg.drvdata = drvdata;
> > +	arg.cfg_hash = cfg_hash;
> > +	arg.preset = preset;
> > +	arg.trace_id = path->trace_id;
> > +
> > +	raw_spin_lock(&drvdata->spinlock);
> > +	arg.config = drvdata->config;
> > +	raw_spin_unlock(&drvdata->spinlock);
>
> Can we defer this in smp call ?  And we can consolidate a bit

Nope. Please see the above answer.

> configurations, we can consider to use a separate patch for this.
>
>   etm4x_apply_config(drvdata, event, hash, preset)
>   {
>       /* perf mode */
>       if (event) {
>           etm4_parse_event_config(drvdata->csdev, event);
>       } else if (mode == CS_MODE_PERF) {
>           scoped_guard(raw_spinlock, &drvdata->spinlock)
>               &drvdata->active_config = drvdata->config;
>       }
>
>       /* At the end, we always apply the config */
>       cscfg_csdev_enable_active_config(drvdata->csdev, hash, preset);
>   }

etm4_parse_event_config() already calls cscfg_csdev_enable_active_config()
But this is refactorying perspective. I think we can postpone this
via another patchset.

>
> > +
> >  	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;
> >  }
> >
> > @@ -1038,7 +1055,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;
> > @@ -1074,6 +1091,8 @@ static void etm4_disable_sysfs_smp_call(void *info)
> >
> >  	etm4_disable_hw(drvdata);
> >
> > +	cscfg_csdev_disable_active_config(drvdata->csdev);
> > +
> >  	coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
> >  }
> >
> > @@ -1124,7 +1143,6 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> >  	 * DYING hotplug callback is serviced by the ETM driver.
> >  	 */
> >  	cpus_read_lock();
> > -	raw_spin_lock(&drvdata->spinlock);
> >
> >  	/*
> >  	 * Executing etm4_disable_hw on the cpu whose ETM is being disabled
> > @@ -1133,10 +1151,6 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> >  	smp_call_function_single(drvdata->cpu, etm4_disable_sysfs_smp_call,
> >  				 drvdata, 1);
> >
> > -	raw_spin_unlock(&drvdata->spinlock);
> > -
> > -	cscfg_csdev_disable_active_config(csdev);
> > -
> >  	cpus_read_unlock();
> >
> >  	/*
> > @@ -1379,6 +1393,7 @@ static void etm4_init_arch_data(void *info)
> >  	struct etm4_init_arg *init_arg = info;
> >  	struct etmv4_drvdata *drvdata;
> >  	struct etmv4_caps *caps;
> > +	struct etmv4_config *config;
> >  	struct csdev_access *csa;
> >  	struct device *dev = init_arg->dev;
> >  	int i;
> > @@ -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.

Thanks.

--
Sincerely,
Yeoreum Yun