Re: [PATCH 1/2] coresight: etm4x: fix inconsistencies with sysfs configration

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

On Tue, Mar 17, 2026 at 06:17:04PM +0000, Yeoreum Yun wrote:
> The current ETM4x configuration via sysfs can lead to the following
> inconsistencies:
> 
>   - If a configuration is modified via sysfs while a perf session is
>     active, the running configuration may differ between before
>     a sched-out and after a subsequent sched-in.
> 
>   - Once a perf session is enabled, some read-only register fields
>     (e.g., TRCSSCSR<n>) may not be reported correctly,
>     because drvdata->config is cleared while enabling with perf mode,
>     even though the information was previously read via etm4_init_arch_data().
> 
> To resolve these inconsistencies, the configuration should be separated into:
> 
>   - active_config, which represents the currently applied configuration
>   - config, which stores the settings configured via sysfs.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
>  .../hwtracing/coresight/coresight-etm4x-cfg.c |  2 +-
>  .../coresight/coresight-etm4x-core.c          | 45 +++++++++++--------
>  drivers/hwtracing/coresight/coresight-etm4x.h |  2 +
>  3 files changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> index c302072b293a..84213d40d1ae 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;

I'd suggest we leave out complex cfg things, we can refactor it
later.

In this series, let us first separate active_config and config, and
keep using drvdata->config to save complex cfg ?

>  	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 d565a73f0042..c552129c4a0c 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -88,9 +88,11 @@ static int etm4_probe_cpu(unsigned int cpu);
>   */
>  static bool etm4x_sspcicrn_present(struct etmv4_drvdata *drvdata, int n)
>  {
> +	struct etmv4_config *config = &drvdata->active_config;
> +
>  	return (n < drvdata->nr_ss_cmp) &&
>  	       drvdata->nr_pe &&
> -	       (drvdata->config.ss_status[n] & TRCSSCSRn_PC);
> +	       (config->ss_status[n] & TRCSSCSRn_PC);

As Suzuki suggested in another reply, we need to extract capabilities
into a separate structure.  I'd also extract status related registers
into a new structure:

  struct etm4_cap {
      int nr_ss_cmp;
      bool pe_comparator;    // TRCSSCSRn.PC
      bool dv_comparator;    // TRCSSCSRn.DV
      bool da_comparator;    // TRCSSCSRn.DA
      bool inst_comparator;  // TRCSSCSRn.INST

      int ns_ex_level;
      int nr_pe;
      int nr_pe_cmp;
      int nr_resource;
      ...
  }

  struct etm4_status_reg {
      u32 ss_status[ETM_MAX_SS_CMP];
      u32 cntr_val[ETMv4_MAX_CNTR];
  }

[...]

> @@ -911,14 +915,17 @@ 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);
> +
> +	raw_spin_lock(&drvdata->spinlock);
> +
> +	drvdata->active_config = drvdata->config;

This is not an issue introduced by this patch, but we might need to
consider to copy active config until it has acquired SYSFS mode.
Otherwise, it might update config here but will disturb a perf session
has been running.

> @@ -2246,7 +2254,8 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
>  	if (!desc.name)
>  		return -ENOMEM;
>  
> -	etm4_set_default(&drvdata->config);
> +	etm4_set_default(&drvdata->active_config);

Should we set default values to drvdata->config ?

My understanding is drvdata->active_config would be always set at the
runtime, but "drvdata->config" should be initialized properly so it
can be consumed by sysfs knobs.

Thanks,
Leo