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

[Yeoreum Yun]

Hi Suzuki,

> > 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()
> 
> This is because of the consistency with the former.
> If we remove those parts in the etm4x_enbale_hw(), the PENDING and STATUS
> bits will be never cleared according to the case.
> 
> This will be handled in the future for the PENDING and STATUS bit
> hanlding in the perf session and sysfs accordingly
> before that, to keep the old behavior, remain as-is.

If you don't mind, could I keep it as-is and post next version among
this week?

Thanks!

-- 
Sincerely,
Yeoreum Yun