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

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

On Tue, Aug 11, 2026 at 07:01:54PM +0100, Yeoreum Yun wrote:
> > On Tue, Aug 11, 2026 at 05:56:19PM +0100, Yeoreum Yun wrote:
> > 
> > [...]
> > 
> > > > 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.
> > 
> > Each time I read through this patch, I find it a bit difficult to follow
> > the overall logic, as it combines several changes together.
> > 
> > I have no strong opinion for this though. Perhaps we could split out the
> > support for a NULL feat_csdev->drv_spinlock into a separate change
> > first, as that seems independent and should not introduce regression.
> 
> If the NULL lock is separated, since it before the SMP refactoring
> there will be a *race* for it.
> OTOH, if SMP first, absent of NULL lock would make a deadelock.
> 
> So If we really want to seperate, we should the SMP and Lock
> refactorying must be one group.
> 
> However, seperating the active_config from there, I'm not sure whether
> This would really make a difficulty of backport.
> We might separate the active_config as a cleanup but, this would require
> also for backporting but active_config one wouldn't have a fix tag.
> 
> So, I think it would be better to keep as-is.
> 
> > 
> > > > > +#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.
> > 
> > An inline function provides stronger type checking at the API boundary.
> > It is readable and easier to maintain. Dereferencing *flags would be
> > fine, as this is not a hot path.
> > 
> > I was inspired by the implementation in include/linux/serial_core.h
> > (see uart_port_lock_irqsave() and uart_port_unlock_irqrestore()).
> 
> I think there’s always been quite a bit of debate around this,
> particularly regarding the maintainability of inline functions versus
> function-like macros. Personally, I don’t find this particular function
> any harder to read or maintain as a macro.
> On the contrary, even though this isn’t a hot path, adding an extra
> instruction still feels like a less preferable trade-off to me.
> 
> @Suzuki, What do you think? inline or macro for feat_csdev_lock_irqsave()?

Gentile ping in case of forgotten.

> 
> -- 
> Sincerely,
> Yeoreum Yun

-- 
Sincerely,
Yeoreum Yun