Re: [PATCH] irqchip: stm32: add a second level init to request hwspinlock

[Marc Zyngier]

On Fri, 08 Mar 2019 14:03:55 +0000,
Fabien DESSENNE <fabien.dessenne@xxxxxx> wrote:

Fabien,

> 
> 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.

Let me get this straight:

- Either you don't have dependencies on anything, and you need to
  enable your irqchip early -> you use IRQCHIP_DECLARE.

- Or you have dependencies on other subsystems -> You *do not* use
  IRQCHIP_DECLARE, and use the expected dependency system (deferred
  probing)

There is no intermediate state. The other irqchip controllers that use
IRQCHIP_DECLARE *do NOT* have dependencies on external subsystems.

> 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.

Yes, you missed the fact that each time someone tries to add some
driver probing via an initcall, we push back. This is an internal
kernel mechanism that is not to be used by random, non architectural
drivers such as this interrupt controller.

Furthermore, you're playing with stuff that is outside of the exported
API of the DT framework. Clearing node flags is not something I really
want to see, as you're messing with a state machine that isn't under
your control.

> 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.

There is no such requirements that holds for secondary interrupt
controllers.

> - During the second init, the dependency with the hwspinlock driver is 
> implemented correctly : it makes use of defered probe when needed.

Then do the right thing all the way: move your favourite toy irqchip
to being a proper device driver, use the right abstraction, and stop
piling ugly hacks on top of each other. Other irqchip drivers do that
just fine (all GPIO irqchips, for example), and most drivers are
already able to defer their probe routine.

> I understand that this patch is 'surprising' but I hope that my
> explanations justify its implementation.

Surprising is not the word I'd have used. The explanations do not
justify anything, as all you're saying is "it is the way it is because
it is the way it is". So please fix your irqchip driver properly, in a
way that is maintainable in the long term, using the abstractions that
are available. If such abstractions are not good enough, please
explain what you need and we'll work something out.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.