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

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

On Tue, Aug 11, 2026 at 04:45:21PM +0100, Leo Yan wrote:
> 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

It couldn't since separation of Lock and SMP can introduce the bug for
that patch. and the Lock and SMP call refactoring isn't meaningful
without active_config.

With this perspective, I think it would be better as-is.

> >  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) {

Thanks. I'll change it.

> > +		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.

okay.

> 
> >  #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)
> {
>     ...
> }

I think this is much annyoing. since the deference might add more
instruction to save the flags. Otherwise the typecheck is for
compilet-time check and not for runtime.

So, it would be better to remain as-is.

> 
> > +	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.

Yeap. When I check again it doesn't need to disable the irq.
I'll remove it.

> > +	} 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().

See above please.

> 
> Otherwise, LGTM.

Thanks!

-- 
Sincerely,
Yeoreum Yun