| Message ID | 20201210194042.860029489@linutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | genirq: Treewide hunt for irq descriptor abuse and assorted fixes | expand |
On Thu, Dec 10, 2020 at 10:14 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > irq_set_lockdep_class() is used from modules and requires irq_to_desc() to > be exported. Move it into the core code which lifts another requirement for > the export. ... > + if (IS_ENABLED(CONFIG_LOCKDEP)) > + __irq_set_lockdep_class(irq, lock_class, request_class); Maybe I missed something, but even if the compiler does not warn the use of if IS_ENABLED() with complimentary #ifdef seems inconsistent. > +#ifdef CONFIG_LOCKDEP ... > +EXPORT_SYMBOL_GPL(irq_set_lockdep_class); > +#endif
On Fri, Dec 11 2020 at 22:08, Thomas Gleixner wrote: > On Fri, Dec 11 2020 at 19:53, Andy Shevchenko wrote: > >> On Thu, Dec 10, 2020 at 10:14 PM Thomas Gleixner <tglx@linutronix.de> wrote: >>> >>> irq_set_lockdep_class() is used from modules and requires irq_to_desc() to >>> be exported. Move it into the core code which lifts another requirement for >>> the export. >> >> ... >> >>> + if (IS_ENABLED(CONFIG_LOCKDEP)) >>> + __irq_set_lockdep_class(irq, lock_class, request_class); > > You are right. Let me fix that. No. I have to correct myself. You're wrong. The inline is evaluated in the compilation units which include that header and because the function declaration is unconditional it is happy. Now the optimizer stage makes the whole thing a NOOP if CONFIG_LOCKDEP=n and thereby drops the reference to the function which makes it not required for linking. So in the file where the function is implemented: #ifdef CONFIG_LOCKDEP void __irq_set_lockdep_class(....) { } #endif The whole block is either discarded because CONFIG_LOCKDEP is not defined or compile if it is defined which makes it available for the linker. And in the latter case the optimizer keeps the call in the inline (it optimizes the condition away because it's always true). So in both cases the compiler and the linker are happy and everything works as expected. It would fail if the header file had the following: #ifdef CONFIG_LOCKDEP void __irq_set_lockdep_class(....); #endif Because then it would complain about the missing function prototype when it evaluates the inline. Thanks, tglx
On Sat, Dec 12, 2020 at 12:07 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Fri, Dec 11 2020 at 22:08, Thomas Gleixner wrote: > > > On Fri, Dec 11 2020 at 19:53, Andy Shevchenko wrote: > > > >> On Thu, Dec 10, 2020 at 10:14 PM Thomas Gleixner <tglx@linutronix.de> wrote: > >>> > >>> irq_set_lockdep_class() is used from modules and requires irq_to_desc() to > >>> be exported. Move it into the core code which lifts another requirement for > >>> the export. > >> > >> ... > >> > >>> + if (IS_ENABLED(CONFIG_LOCKDEP)) > >>> + __irq_set_lockdep_class(irq, lock_class, request_class); > > > > You are right. Let me fix that. > > No. I have to correct myself. You're wrong. > > The inline is evaluated in the compilation units which include that > header and because the function declaration is unconditional it is > happy. > > Now the optimizer stage makes the whole thing a NOOP if CONFIG_LOCKDEP=n > and thereby drops the reference to the function which makes it not > required for linking. > > So in the file where the function is implemented: > > #ifdef CONFIG_LOCKDEP > void __irq_set_lockdep_class(....) > { > } > #endif > > The whole block is either discarded because CONFIG_LOCKDEP is not > defined or compile if it is defined which makes it available for the > linker. > > And in the latter case the optimizer keeps the call in the inline (it > optimizes the condition away because it's always true). > > So in both cases the compiler and the linker are happy and everything > works as expected. > > It would fail if the header file had the following: > > #ifdef CONFIG_LOCKDEP > void __irq_set_lockdep_class(....); > #endif > > Because then it would complain about the missing function prototype when > it evaluates the inline. I understand that (that's why I put "if even no warning") and what I'm talking about is the purpose of IS_ENABLED(). It's usually good for compile testing !CONFIG_FOO cases. But here it seems inconsistent. The pattern I usually see in the cases like this is #ifdef CONFIG_LOCKDEP void __irq_set_lockdep_class(....); #else static inline void ... {} #endif and call it directly in the caller. It's not a big deal, so up to you.
--- a/include/linux/irqdesc.h +++ b/include/linux/irqdesc.h @@ -240,16 +240,14 @@ static inline bool irq_is_percpu_devid(u return irq_check_status_bit(irq, IRQ_PER_CPU_DEVID); } +void __irq_set_lockdep_class(unsigned int irq, struct lock_class_key *lock_class, + struct lock_class_key *request_class); static inline void irq_set_lockdep_class(unsigned int irq, struct lock_class_key *lock_class, struct lock_class_key *request_class) { - struct irq_desc *desc = irq_to_desc(irq); - - if (desc) { - lockdep_set_class(&desc->lock, lock_class); - lockdep_set_class(&desc->request_mutex, request_class); - } + if (IS_ENABLED(CONFIG_LOCKDEP)) + __irq_set_lockdep_class(irq, lock_class, request_class); } #endif --- a/kernel/irq/irqdesc.c +++ b/kernel/irq/irqdesc.c @@ -968,3 +968,17 @@ unsigned int kstat_irqs_usr(unsigned int rcu_read_unlock(); return sum; } + +#ifdef CONFIG_LOCKDEP +void __irq_set_lockdep_class(unsigned int irq, struct lock_class_key *lock_class, + struct lock_class_key *request_class) +{ + struct irq_desc *desc = irq_to_desc(irq); + + if (desc) { + lockdep_set_class(&desc->lock, lock_class); + lockdep_set_class(&desc->request_mutex, request_class); + } +} +EXPORT_SYMBOL_GPL(irq_set_lockdep_class); +#endif
irq_set_lockdep_class() is used from modules and requires irq_to_desc() to be exported. Move it into the core code which lifts another requirement for the export. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- include/linux/irqdesc.h | 10 ++++------ kernel/irq/irqdesc.c | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 6 deletions(-)