Messages in this thread | | | From | Fabien DESSENNE <> | Subject | Re: [PATCH] irqchip: stm32: add a second level init to request hwspinlock | Date | Fri, 8 Mar 2019 14:03:55 +0000 |
| |
Hi Marc,
Thank you for your feedback. Let me try to explain this patch, and the reason of its unusual implementation choices.
Regarding the driver init mode: As an important requirement, I want to keep this irq driver declared with IRQCHIP_DECLARE(), so it is initialized early from start_kernel()/init_IRQ(). Most of the other irq drivers are implemented this way and I imagine that this ensures the availability of the irq drivers, before the other platform drivers get probed.
Regarding the second init: With the usage of the hwspinlock framework (used to protect against coprocessor concurrent access to registers) we have a problem as the hwspinlock driver is not available when the irq driver is being initialized. In order to solve this, I added a second initialization where we get a reference to hwspinlock. You pointed that we are not supposed to use of_node_clear_flag (which allows to get a second init call) : I spent some time to find any information about it, but could not find any reason to not use it. Please, let me know if I missed something here.
Regarding the inits sequence and dependencies: - The second init is guaranteed to be called after the first one, since start_kernel()->init_IRQ() is called before platform drivers init. - During the second init, the dependency with the hwspinlock driver is implemented correctly : it makes use of defered probe when needed.
I understand that this patch is 'surprising' but I hope that my explanations justify its implementation. Waiting for your feedback
BR
Fabien
On 07/03/2019 5:44 PM, Marc Zyngier wrote: > On 07/03/2019 16:23, Fabien Dessenne wrote: >> Requesting hwspinlock, at the first time it is used, is not correct: >> indeed, at that moment we are under raw_spin_lock_irqsave() context and >> hwspin_lock_request_specific() may sleep ("BUG: sleeping function called >> from invalid context"). >> Requesting hwspinlock during the init (stm32*_exti_of_init()) is also >> not possible (the hwspinlock framework is not ready at that stage of the >> kernel init). >> As a consequence, add a second level init (probed with arch_initcall) >> where we can safely request hwspinlock. > No, this is fairly broken. You're playing with stuff you're not supposed > to (OF_POPULATE? really?), and adding initcalls is completely unreliable > (things depend on the link order and will randomly break). > > If you need dependencies, implement them correctly. Turn this driver > into a real device driver (in the platform device sense), and return > PROBE_DEFER when you can't find your dependency. > > Thanks, > > M. | |