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

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

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.

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

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

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

> +	struct etmv4_config config;

We don't need this. We can defer to get drvdata->config in SMP call.

>  	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
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);
  }


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

Thanks,
Leo