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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Mar 2019 14:03:55 +0000
From:   Fabien DESSENNE <fabien.dessenne@...com>
To:     Marc Zyngier <marc.zyngier@....com>,
        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>
CC:     Benjamin GAIGNARD <benjamin.gaignard@...com>
Subject: Re: [PATCH] irqchip: stm32: add a second level init to request
 hwspinlock

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.