Re: [PATCH v6 07/13] coresight: etm4x: fix inconsistencies with sysfs configuration

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

Hi Leo,

> On Wed, Apr 22, 2026 at 02:21:57PM +0100, Yeoreum Yun wrote:
> 
> [...]
> 
> >   - Since active_config and related fields are accessed only by the local CPU
> >     in etm4_enable/disable_sysfs_smp_call() (similar to perf enable/disable),
> >     remove the lock/unlock from the sysfs enable/disable path and
> >     startup/dying_cpu except when to access config fields.
> 
> Thanks for writing this up, which is helpful for understanding.
> 
> [...]
> 
> > @@ -918,40 +948,29 @@ 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);
> 
> With the patch [1], we can move cscfg_config_sysfs_get_active_cfg() to
> smp call. As a result, all things for enabling cscfg can be in the
> same place.
> 
> [1] https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_management_improvement-v12-14-1c9dcb1de8c9@arm.com/
> 
> > -	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;
> >  
> >  	/*
> >  	 * Executing etm4_enable_hw on the cpu whose ETM is being enabled
> >  	 * ensures that register writes occur when cpu is powered.
> >  	 */
> >  	arg.drvdata = drvdata;
> > +	arg.path = path;
> > +	arg.cfg_hash = cfg_hash;
> > +	arg.preset = preset;
> 
> Connect with the comment above , don't need to pass cfg_hash/preset anymore.

This sounds like we're going to merge your patch first and then
apply this patch series. Isn't it compromised with Suzuki?
If so, I'll send this patch based on your own but if not,
We still need this code and it's the same comment for your own
in other patch.

> 
> > +	raw_spin_lock(&drvdata->spinlock);
> > +	arg.config = drvdata->config;
> > +	raw_spin_unlock(&drvdata->spinlock);
> 
> Seems to me, this is right way for locking - here we simply use
> spinlock for exclusive access config from sysfs knobs.
> 
> However, we avoid the config copy and directly access in SMP call?
> we still can use the raw spinlock in SMP call.

No we couldn't. for example, suppose cpu0 modifies
its own drvdata->data and holding lock, but other cpu try to enable
cpu0's etm4x if SMP call is entered before release lock,
It becomes deadlock situation. IOW, to grap the drvdata->spinlock in
the SMP call, all sysfs raw_spin_lock() should be changed into
raw_spin_lock_irqsave(). However this would make an unexpected latency
for modifying drvdata via sysfs, It should grab lock in here.

> 
> My suggestion is:
> 
> - First use a patch to move the drvdata assignment to SMP call and
>   remove spinlock;

> - Then, rebase this patch for moving cscfg into SMP call.
> 
> If so, we only need add a new field "arg->path", right?

Nope base on above.

> 
> 
> > @@ -1857,13 +1875,11 @@ static int etm4_starting_cpu(unsigned int cpu)
> >  	if (!etmdrvdata[cpu])
> >  		return 0;
> >  
> > -	raw_spin_lock(&etmdrvdata[cpu]->spinlock);
> 
> With the change [2], the starting and dying functions have been
> removed.
> 
> If you rebase patches on the top PM series, then you will not bother
> this. Anyway, this is right to remove spinlock for hotplug notifiers.
> 
> [2] https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_management_improvement-v12-27-1c9dcb1de8c9@arm.com/

Agree but let me confirm the plan for merge.

-- 
Sincerely,
Yeoreum Yun