Re: [PATCH v8 08/13] coresight: etm4x: fix inconsistencies with sysfs configuration

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

Hi Leo,

> On Mon, Jun 29, 2026 at 10:00:01AM +0100, Yeoreum Yun wrote:
> 
> [...]
> 
> > @@ -922,25 +950,7 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
> >  {
> >  	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> >  	struct etm4_enable_arg arg = { };
> > -	unsigned long cfg_hash;
> > -	int ret, preset;
> > -
> > -	/* 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;
> > +	int ret;
> >  
> >  	/*
> >  	 * Executing etm4_enable_hw on the cpu whose ETM is being enabled
> > @@ -948,20 +958,20 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
> >  	 */
> >  	arg.drvdata = drvdata;
> >  	arg.path = path;
> > +
> > +	raw_spin_lock(&drvdata->spinlock);
> > +	arg.config = drvdata->config;
> > +	raw_spin_unlock(&drvdata->spinlock);
> > +
> >  	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;
> >  }
> 
> This is most valuable change for me, as now we will have much clear
> scope for what is protected ("drvdata->config").
> 
> However, a corner case was mentioned by Sashiko:
> 
>  | It appears etm4_enable_hw() modifies drvdata->ss_status while executing
>  | via IPI, but sshot_ctrl_store() can modify the same array concurrently
>  | from process context since the lock is no longer held across the
>  | smp_call_function_single() call.
> 
> "drvdata->ss_status" is a race condition between SMP call and sysfs
> knobs. Should we change to spinlock_irqsave/irqrestore when access
> drvdata->ss_status?

I know this, but  This is currently ignorable since
whatever perf or sysfs session, the ss_status is always clear
the PENDING and STATUS bits (and we make a compromise for this right now).

And the sshot_ctrl_store()'s action is always clear above bits.

So, This is ignorable comment from Sashiko.

Thanks!

-- 
Sincerely,
Yeoreum Yun