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:
> 
> [...]
> 
> > @@ -619,27 +622,52 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> >  static void etm4_enable_sysfs_smp_call(void *info)
> >  {
> >  	struct etm4_enable_arg *arg = info;
> > +	struct etmv4_drvdata *drvdata;
> >  	struct coresight_device *csdev;
> > +	unsigned long cfg_hash;
> > +	int preset;
> >  
> >  	if (WARN_ON(!arg))
> >  		return;
> >  
> > -	csdev = arg->drvdata->csdev;
> > +	drvdata = arg->drvdata;
> > +	csdev = drvdata->csdev;
> >  	if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) {
> >  		/* Someone is already using the tracer */
> >  		arg->rc = -EBUSY;
> >  		return;
> >  	}
> >  
> > -	arg->rc = etm4_enable_hw(arg->drvdata);
> > +	drvdata->active_config = arg->config;
> >  
> > -	/* The tracer didn't start */
> > +	/* enable any config activated by configfs */
> > +	cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset);
> > +	if (cfg_hash) {
> > +		arg->rc = cscfg_csdev_enable_active_config(csdev,
> > +							   cfg_hash,
> > +							   preset);
> > +		if (arg->rc)
> > +			goto err;
> > +	}
> 
> 
> I try best to not paste any non-sense AI reviews, record one from
> Sashiko [1]:
> 
>  | If a user writes to a sysfs attribute like pe_sel_store(), it acquires
>  | drvdata->spinlock without disabling interrupts. If an IPI is then handled
>  | on the same CPU, etm4_enable_sysfs_smp_call() will call
>  | cscfg_csdev_enable_active_config() which attempts to acquire the exact
>  | same spinlock, hanging the CPU."
> 
> The flow for acquiring drvdata->spinlock in SMP call is:
> 
>   etm4_enable_sysfs_smp_call()
>     `> cscfg_csdev_enable_active_config()
>          `> cscfg_csdev_enable_config()
>               `> cscfg_prog_config()
>                    `> cscfg_set_on_enable()
>                         `> raw_spin_lock_irqsave(feat_csdev->drv_spinlock, flags);
> 
> We might expect the complaint from LOCKDEP with this patch.

Hmm. I miss the this point and there is two solution:

  1. Remove "feat_csdev->drv_spinlock"

    Before this patch, the "feat_csdev->drv_spinlock" is for
    synchronizing with the "drvdata->config". However,
    After this patch, the the configfs only show the "active" config
    and the active config can be used only after taking the "mode".

    Furthermore, the sysfs only uses the "config" not "active config",
    there is no race between the sysfs and the configfs.

    When I check the, "feat_csdev->drv_spinlock" is for the
    active/deactive the configfs config, so I seems safe to remove.

  2. sysfs with _irqsave?

    However, this make a latency for the sysfs.

I think the (1) seems good.

Any comments? @Suzuki and @Leo?

-- 
Sincerely,
Yeoreum Yun