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

[Suzuki K Poulose]

On 06/07/2026 18:39, Leo Yan wrote:
> 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

Could we stop modifying the drvdata->ss_status in enable_hw() ? We moved
it outside the config claiming that the status is not used(or useful). #
Then why not remove the updating from enable_hw()

Suzuki


>   | 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?
>
> Thanks,
> Leo