linux-kernel - Re: [PATCH v3 2/2] i2c: Add GPIO-based hotplug gate

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]

Message-ID: <c577a92a-d1e4-df7e-250c-33d47b4d5bcb@linaro.org>
Date:   Tue, 15 Aug 2023 07:20:35 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:     Svyatoslav Ryhel <clamor95@...il.com>,
        Andi Shyti <andi.shyti@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Wolfram Sang <wsa@...nel.org>, linux-i2c@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] i2c: Add GPIO-based hotplug gate

On 10/08/2023 23:52, Michał Mirosław wrote:
>>>>>>>> Shared IRQ with devm is a recipe for disaster. Are you sure this is a
>>>>>>>> shared one? You have a remove() function which also points that it is
>>>>>>>> not safe. You can:
>>>>>>>> 1. investigate to be sure it is 100% safe (please document why do you
>>>>>>>> think it is safe)
> [...]
>>>> True, therefore non-devm interrupts are recommended also in such case.
>>>> Maybe one of my solutions is actually not recommended.
>>>>
>>>> However if done right, driver with non-shared interrupts, is expected to
>>>> disable interrupts in remove(), thus there is no risk. We have big
>>>> discussions in the past about it, so feel free to dig through LKML to
>>>> read more about. Anyway shared and devm is a clear no go.
>>>
>>> Can you share pointers to some of those discussions? Quick search
>>> about devm_request_irq() and friends found only a thread from 2013
>>
>> Just look at CONFIG_DEBUG_SHIRQ. Some things lore points:
>> https://lore.kernel.org/all/1592130544-19759-2-git-send-email-krzk@kernel.org/
>> https://lore.kernel.org/all/20200616103956.GL4447@sirena.org.uk/
>>
>> I think pretty clear:
>> https://lore.kernel.org/all/87mu52ca4b.fsf@nanos.tec.linutronix.de/
>> https://lore.kernel.org/all/CA+h21hrxQ1fRahyQGFS42Xuop_Q2petE=No1dft4nVb-ijUu2g@mail.gmail.com/
>>
>> Also:
>> https://lore.kernel.org/all/651c9a33-71e6-c042-58e2-6ad501e984cd@pengutronix.de/
>> https://lore.kernel.org/all/36AC4067-78C6-4986-8B97-591F93E266D8@gmail.com/
> [...]
> 
> Thanks! It all looks like a proof by example [1]: a broken driver [2]
> was converted to devres [3] and allowed a shared interrupt [4] and now is
> used to back an argument that devres and/or shared IRQs are bad. I have
> a hard time accepting this line of reasoning.
> 
> So: sure, if you disable device's clock, you should first disable the
> interrupt handler one way or another, and if you request a shared interrupt
> then you have to write the handler expecting spurious invocations anytime
> between entry to register_irq() and return from free_irq() (BTW, DEBUG_SHIRQ
> is here to help test exactly this). And, when used correctly, devres can
> release you from having to write remove() and error paths (but I guess it
> might be a challenge to find a single driver that is a complete, good and
> complex-enough example).
> 
> Coming back from the digression: I gathered following items from the
> review of the i2c-hotplug-gpio driver:
> 
>   1. TODO: register i2c_hotplug_deactivate(priv) using
>      devm_add_action_or_reset() before registering the IRQ handler
>      and remove remove();
> 
>   2. shared IRQ: it is expected to be an edge-triggered, rarely
>      signalled interrupt and the handler will work fine if called
>      spuriously; it is not required to be shared for my Transformer,
>      but I can't say much about other hardware. Would a comment help?

We have way too lengthy discussion and now we are circling back. Can you
refer to the first email I wrote?

"You can:
1. investigate to be sure it is 100% safe (please document why do you
think it is safe)
2. drop devm
3. drop shared flag."


Best regards,
Krzysztof