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