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