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

[Leo Yan <leo.yan@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.

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

Thanks,
Leo