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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <18de3399-363f-e180-d078-1dfd3bb31c34@st.com>
Date:   Fri, 8 Mar 2019 16:01:17 +0000
From:   Fabien DESSENNE <fabien.dessenne@...com>
To:     Marc Zyngier <marc.zyngier@....com>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre TORGUE <alexandre.torgue@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Benjamin GAIGNARD <benjamin.gaignard@...com>
Subject: Re: [PATCH] irqchip: stm32: add a second level init to request
 hwspinlock

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