Re: [PATCH v9 04/13] coresight: etm4x: fix inconsistencies with sysfs configuration

[Leo Yan <leo.yan@arm.com>]

On Sat, Jul 25, 2026 at 12:36:36PM +0100, Yeoreum Yun wrote:

[...]

> As the active_config is used for cfg-configfs, etmv4 doesn't need to set
> its lock for the cfg-configfs since the active_config is proceted by cs_mode
> otherwise it would make a possible cpu-stall when it get interrupt while
> setting the sysfs configuration.
> 
> Therefore, set the drv_spinlock for cfg-configfs as NULL and
> let the cfg-configfs disable irq without grap drv_spinlock when it is NULL.

I understand that this patch tries to put all the changes (active_config
and drv_spinlock) into a single patch so that it can be backported to
stable kernels.

However, the change is now quite large, and I suspect it will also be
difficult for maintainers to backport it to stable kernels. The fixes
tag 54ff892b76c6 is quite old, while this patch also touches cfgfs
which was introduced much later.

Can we treat this as a refactoring instead and split it into at least
two patches? This would make it easier to review now and easier to
understand later if someone will read the changes.

 - Lock refactoring
 - SMP call refactoring
 - active_config refactoring

>  static int cscfg_set_on_enable(struct cscfg_feature_csdev *feat_csdev)
>  {
> -	unsigned long flags;
>  	int i;
>  
> -	raw_spin_lock_irqsave(feat_csdev->drv_spinlock, flags);
> -	for (i = 0; i < feat_csdev->nr_regs; i++)
> -		cscfg_set_reg(&feat_csdev->regs_csdev[i]);
> -	raw_spin_unlock_irqrestore(feat_csdev->drv_spinlock, flags);
> +	scoped_guard (feat_csdev_lock, feat_csdev) {

scoped_guard(feat_csdev_lock, feat_csdev) {

> +		for (i = 0; i < feat_csdev->nr_regs; i++)
> +			cscfg_set_reg(&feat_csdev->regs_csdev[i]);
> +	}
>  	dev_dbg(&feat_csdev->csdev->dev, "Feature %s: %s",
>  		feat_csdev->feat_desc->name, "set on enable");
>  	return 0;
> @@ -88,13 +87,12 @@ static int cscfg_set_on_enable(struct cscfg_feature_csdev *feat_csdev)
>  /* copy back values from the driver locations referenced in cscfg_reg_csdev */
>  static void cscfg_save_on_disable(struct cscfg_feature_csdev *feat_csdev)
>  {
> -	unsigned long flags;
>  	int i;
>  
> -	raw_spin_lock_irqsave(feat_csdev->drv_spinlock, flags);
> -	for (i = 0; i < feat_csdev->nr_regs; i++)
> -		cscfg_save_reg(&feat_csdev->regs_csdev[i]);
> -	raw_spin_unlock_irqrestore(feat_csdev->drv_spinlock, flags);
> +	scoped_guard (feat_csdev_lock, feat_csdev) {

scoped_guard(feat_csdev_lock, feat_csdev) {

> +		for (i = 0; i < feat_csdev->nr_regs; i++)
> +			cscfg_save_reg(&feat_csdev->regs_csdev[i]);
> +	}
>  	dev_dbg(&feat_csdev->csdev->dev, "Feature %s: %s",
>  		feat_csdev->feat_desc->name, "save on disable");
>  }
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index 90fd937d3bd8..0782db3b1b74 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -8,6 +8,7 @@
>  #define _CORESIGHT_CORESIGHT_CONFIG_H
>  
>  #include <linux/coresight.h>
> +#include <linux/cleanup.h>

Alphabet order. Move cleanup.h above coresight.h.

>  #include <linux/types.h>
>  
>  /* CoreSight Configuration Management - component and system wide configuration */
> @@ -259,4 +260,29 @@ 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)				\

Could use inline here?

static inline void feat_csdev_lock_irqsave(..., unsigned long *flags)
{
    ...
}

> +	do {								\
> +		raw_spinlock_t *__lock = feat_csdev->drv_spinlock;	\
> +		typecheck(unsigned long, flags);			\

After using inline, no need typecheck.

> +		if (__lock)						\
> +			raw_spin_lock_irqsave(__lock, flags);		\
> +		else							\
> +			local_irq_save(flags);				\

If __lock is NULL, do we still need local_irq_save()? Seems to me, if
lock is NULL pointer, it means no race condition.

> +	} 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);	\
> +		else							\
> +			local_irq_restore(flags);			\
> +	} while (0)

Use inline for feat_csdev_unlock() and rename it to
feat_csdev_unlock_irqrestore().

Otherwise, LGTM.

Thanks,
Leo