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

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

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

-- 
Sincerely,
Yeoreum Yun