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

[sashiko-bot@kernel.org]

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@arm.com>

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?

> 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?

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