Re: [PATCH v11 3/9] coresight: etm4x: fix inconsistencies with sysfs configuration

[Yeoreum Yun]

On Tue, Sep 15, 2026 at 11:53:33AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] coresight: data race when updating cntr_val in etm4_disable_sysfs()
> - [Medium] coresight: feat_csdev_lock macro fails to disable IRQs when __lock is NULL
> --
> 
> commit 5571e5128409a42536b24f594b727e037107f3f4
> Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> 
> coresight: etm4x: fix inconsistencies with sysfs configuration
> 
> This commit separates the active configuration applied to the current session
> from the sysfs configuration to resolve inconsistencies. It applies configfs
> settings after taking the appropriate mode and removes the lock/unlock around
> sysfs enable/disable paths except when accessing config fields.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index 90fd937d3bd83..ae6295e308131 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> 
> [ ... ]
> 
> > @@ -259,4 +260,25 @@ void cscfg_csdev_disable_config(struct cscfg_config_csdev *config_csdev);
> >  /* reset a feature to default values */
> >  void cscfg_reset_feat(struct cscfg_feature_csdev *feat_csdev);
> >  
> > +#define feat_csdev_lock(feat_csdev, flags)				\
> > +	do {								\
> > +		raw_spinlock_t *__lock = feat_csdev->drv_spinlock;	\
> > +		typecheck(unsigned long, flags);			\
> > +		if (__lock)						\
> > +			raw_spin_lock_irqsave(__lock, flags);		\
> > +	} while (0)
> > +
> > +#define feat_csdev_unlock(feat_csdev, flags)				\
> > +	do {								\
> > +		raw_spinlock_t *__lock = feat_csdev->drv_spinlock;	\
> > +		typecheck(unsigned long, flags);			\
> > +		if (__lock)						\
> > +			raw_spin_unlock_irqrestore(__lock, flags);	\
> > +	} while (0)
> 
> [Severity: Medium]
> Will these macros fail to disable IRQs when __lock is NULL?
> 
> The intent is to let cfg-configfs operate without grabbing the drv_spinlock
> when it is NULL. However, raw_spin_lock_irqsave() also saves and disables
> IRQs. If the lock is bypassed completely here, IRQs are left enabled.
> 
> Could this lead to reentrancy bugs or deadlocks if generic coresight configfs
> routines like cscfg_csdev_enable_active_config() are called from contexts
> where IRQs are enabled? Should there be a fallback like local_irq_save() when
> __lock is NULL to ensure the critical section remains protected from
> interrupts?

It was discussed with Leo but we couldn't find any issue for this.

> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index 2247ad55d4442..2df7f592bb5cf 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> 
> [ ... ]
> 
> > @@ -1140,9 +1152,17 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> >  	smp_call_function_single(drvdata->cpu, etm4_disable_sysfs_smp_call,
> >  				 drvdata, 1);
> >  
> > -	raw_spin_unlock(&drvdata->spinlock);
> > +	/*
> > +	 * Userspace may read ss_status and cntr_val through sysfs after
> > +	 * the sysfs-session has been disabled.
> > +	 */
> > +	for (i = 0; i < drvdata->nr_ss_cmp; i++)
> > +		config->ss_status[i] = active_config->ss_status[i];
> >  
> > -	cscfg_csdev_disable_active_config(csdev);
> > +	for (i = 0; i < drvdata->nr_cntr; i++)
> > +		config->cntr_val[i] = active_config->cntr_val[i];
> 
> [Severity: Medium]
> Does this create a data race with sysfs operations?
> 
> The etm4_disable_sysfs() function removes drvdata->spinlock but continues to
> copy active_config values into config->cntr_val locklessly.
> 
> Meanwhile, concurrent sysfs show and store functions access these fields under
> the spinlock:
> 
> cntr_val_store() {
>         ...
>         raw_spin_lock(&drvdata->spinlock);
>         idx = config->cntr_idx;
>         config->cntr_val[idx] = val;
>         raw_spin_unlock(&drvdata->spinlock);
>         ...
> }
> 
> If etm4_disable_sysfs() runs while another thread concurrently reads or
> writes cntr_val through sysfs, can it corrupt the configuration state?

The store is prohibited at when sysfs sessino is enabled.
So This is not true.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260915-separate_etm_cfg_v2-v11-0-d2b258d51747@xxxxxxx?part=3

-- 
Sincerely,
Yeoreum Yun