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

From: Fabien DESSENNE <fabien.dessenne@st.com>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Jason Cooper <jason@lakedaemon.net>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre TORGUE <alexandre.torgue@st.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-stm32@st-md-mailman.stormreply.com" 
	<linux-stm32@st-md-mailman.stormreply.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Benjamin GAIGNARD <benjamin.gaignard@st.com>
Subject: Re: [PATCH] irqchip: stm32: add a second level init to request hwspinlock
Date: Fri, 8 Mar 2019 16:01:17 +0000	[thread overview]
Message-ID: <18de3399-363f-e180-d078-1dfd3bb31c34@st.com> (raw)
In-Reply-To: <864l8d1iz6.wl-marc.zyngier@arm.com>

Hi Marc,

Thanks for your detailed answer.

I will study how to move this driver to a device one.

BR

Fabien

On 08/03/2019 4:30 PM, Marc Zyngier wrote:
> On Fri, 08 Mar 2019 14:03:55 +0000,
> Fabien DESSENNE <fabien.dessenne@st.com> 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.
>