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

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

Hi Leo,

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

Yes. but I think we should include the separation of ss_status in
this patchset.

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

Thanks. I'll take with this as reference.

>
> [...]
>
> > @@ -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.

Good catch. I think we should access "active_config" after taking a
mode. That means cscfg_csdev_enable/disable_active_config() should
be called after/before taking mode like PERF session otherwise
active_config could be a race if perf and sysfs session are enabled parallely.

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

Right. I've only thought about perf case not a sysfs knobs for this.

Thanks.

--
Sincerely,
Yeoreum Yun