Re: [PATCH v11 4/9] coresight: etm3x: fix inconsistencies with sysfs configuration

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

> Hi,
> 
> On 9/15/26 12:34, Yeoreum Yun wrote:
> > The current ETM3x 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.
> > 
> > To resolve these issues, separate the configuration into:
> > 
> >    - active_config: the configuration applied to the current session
> >    - config: the configuration set via sysfs
> > 
> 
> Same comment as for the etm4 config naming

Acked.

> 
> 
> > Additionally:
> > 
> >    - Since active_config and related fields are accessed only by the local CPU
> >      in etm_enable/disable_sysfs_smp_call() (similar to perf enable/disable),
> >      remove the lock/unlock from the sysfs enable/disable path and
> >      starting/dying_cpu path except when to access config fields only.
> > 
> >    - Some of sysfs interface read etm register directly. To reduce lock
> >      scope while the etm_enable_hw()/etm_disable_hw(), handle it via
> >      IPI so that registers could be read from on proper CPU.
> > 
> > Fixes: 1925a470ce69 ("coresight: etm3x: splitting struct etm_drvdata")
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > ---
> >   drivers/hwtracing/coresight/coresight-etm.h        |  4 +-
> >   drivers/hwtracing/coresight/coresight-etm3x-core.c | 68 ++++++++++--------
> >   .../hwtracing/coresight/coresight-etm3x-sysfs.c    | 80 +++++++++++++++-------
> >   3 files changed, 100 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h
> > index 1d753cca29439..f3796162168d4 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm.h
> > +++ b/drivers/hwtracing/coresight/coresight-etm.h
> > @@ -226,7 +226,8 @@ struct etm_config {
> >    * @etmccr:	value of register ETMCCR.
> >    * @etmccer:	value of register ETMCCER.
> >    * @traceid:	value of the current ID for this component.
> > - * @config:	structure holding configuration parameters.
> > + * @active_config:	structure holding current running configuration.
> > + * @config:	structure holding sysfs mode configuration.
> >    */
> >   struct etm_drvdata {
> >   	struct csdev_access		csa;
> > @@ -248,6 +249,7 @@ struct etm_drvdata {
> >   	u32				etmccr;
> >   	u32				etmccer;
> >   	u32				traceid;
> > +	struct etm_config		active_config;
> >   	struct etm_config		config;
> >   };
> > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > index 862ad0786699c..fd76a57e5f861 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> > @@ -308,7 +308,7 @@ void etm_config_trace_mode(struct etm_config *config)
> >   static int etm_parse_event_config(struct etm_drvdata *drvdata,
> >   				  struct perf_event *event)
> >   {
> > -	struct etm_config *config = &drvdata->config;
> > +	struct etm_config *config = &drvdata->active_config;
> >   	struct perf_event_attr *attr = &event->attr;
> >   	u8 ts_level;
> > @@ -367,7 +367,7 @@ static int etm_enable_hw(struct etm_drvdata *drvdata)
> >   {
> >   	int i, rc;
> >   	u32 etmcr;
> > -	struct etm_config *config = &drvdata->config;
> > +	struct etm_config *config = &drvdata->active_config;
> >   	struct coresight_device *csdev = drvdata->csdev;
> >   	CS_UNLOCK(drvdata->csa.base);
> > @@ -442,32 +442,30 @@ static int etm_enable_hw(struct etm_drvdata *drvdata)
> >   struct etm_enable_arg {
> >   	struct etm_drvdata *drvdata;
> >   	struct coresight_path *path;
> > +	struct etm_config config;
> >   	int rc;
> >   };
> >   static void etm_enable_sysfs_smp_call(void *info)
> >   {
> >   	struct etm_enable_arg *arg = info;
> > +	struct etm_drvdata *drvdata;
> >   	struct coresight_device *csdev;
> >   	if (WARN_ON(!arg))
> >   		return;
> > -	csdev = arg->drvdata->csdev;
> > -	if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) {
> > -		/* Someone is already using the tracer */
> > -		arg->rc = -EBUSY;
> > -		return;
> > -	}
> > +	drvdata = arg->drvdata;
> > +	csdev = drvdata->csdev;
> > -	arg->rc = etm_enable_hw(arg->drvdata);
> > +	drvdata->active_config = arg->config;
> > +	drvdata->traceid = arg->path->trace_id;
> > -	/* The tracer didn't start */
> > -	if (arg->rc) {
> > -		coresight_set_mode(csdev, CS_MODE_DISABLED);
> > +	arg->rc = etm_enable_hw(arg->drvdata);
> > +	if (arg->rc)
> >   		return;
> > -	}
> > +	drvdata->sticky_enable = true;
> >   	csdev->path = arg->path;
> >   }
> > @@ -512,9 +510,10 @@ static int etm_enable_sysfs(struct coresight_device *csdev, struct coresight_pat
> >   	struct etm_enable_arg arg = { };
> >   	int ret;
> > -	spin_lock(&drvdata->spinlock);
> > -
> > -	drvdata->traceid = path->trace_id;
> > +	if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) {
> > +		/* Someone is already using the tracer */
> > +		return -EBUSY;
> > +	}
> >   	/*
> >   	 * Configure the ETM only if the CPU is online.  If it isn't online
> > @@ -523,23 +522,27 @@ static int etm_enable_sysfs(struct coresight_device *csdev, struct coresight_pat
> >   	if (cpu_online(drvdata->cpu)) {
> >   		arg.drvdata = drvdata;
> >   		arg.path = path;
> > +
> > +		scoped_guard(spinlock, &drvdata->spinlock) {
> > +			arg.config = drvdata->config;
> > +		}
> > +
> 
> Again I think the arg.config is unnecessary

Agree. I'll change as etm4's comment!

Thanks!

[...]


-- 
Sincerely,
Yeoreum Yun