On Sat, Aug 05, 2023 at 09:17:50PM +0200, Krzysztof Kozlowski wrote: > On 01/08/2023 00:50, Michał Mirosław wrote: > > On Mon, Jul 31, 2023 at 02:59:41PM +0200, Krzysztof Kozlowski wrote: > >> On 31/07/2023 10:49, Michał Mirosław wrote: > >>> On Mon, Jul 31, 2023 at 08:58:14AM +0200, Krzysztof Kozlowski wrote: > >>>> On 30/07/2023 23:55, Michał Mirosław wrote: > >>>>> On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote: > >>>>>> On 29/07/2023 18:08, Svyatoslav Ryhel wrote: > >>>>>>> From: Michał Mirosław <mirq-linux@xxxxxxxxxxxx> > >>>>>>> > >>>>>>> Implement driver for hot-plugged I2C busses, where some devices on > >>>>>>> a bus are hot-pluggable and their presence is indicated by GPIO line. > >>>>> [...] > >>>>>>> + priv->irq = platform_get_irq(pdev, 0); > >>>>>>> + if (priv->irq < 0) > >>>>>>> + return dev_err_probe(&pdev->dev, priv->irq, > >>>>>>> + "failed to get IRQ %d\n", priv->irq); > >>>>>>> + > >>>>>>> + ret = devm_request_threaded_irq(&pdev->dev, priv->irq, NULL, > >>>>>>> + i2c_hotplug_interrupt, > >>>>>>> + IRQF_ONESHOT | IRQF_SHARED, > >>>>>> > >>>>>> 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@xxxxxxxxxx/ > https://lore.kernel.org/all/20200616103956.GL4447@xxxxxxxxxxxxx/ > > I think pretty clear: > https://lore.kernel.org/all/87mu52ca4b.fsf@xxxxxxxxxxxxxxxxxxxxxxx/ > https://lore.kernel.org/all/CA+h21hrxQ1fRahyQGFS42Xuop_Q2petE=No1dft4nVb-ijUu2g@xxxxxxxxxxxxxx/ > > Also: > https://lore.kernel.org/all/651c9a33-71e6-c042-58e2-6ad501e984cd@xxxxxxxxxxxxxx/ > https://lore.kernel.org/all/36AC4067-78C6-4986-8B97-591F93E266D8@xxxxxxxxx/ [...] 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? 3. TODO: DT-binding needs an expanded example and fixing some schema issues; 4. question from Andi in another thread: I'll answer shortly. Please correct me if I missed something. Best Regards Michał Mirosław [1] https://en.wikipedia.org/wiki/Proof_by_example [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aa11e38ce6fe8846fec046a95cecd5d4690c48cd [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f8a3e7fd5bd08e3fd9847c04a5a445e2994f6b3 [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df0a2fdab0068f7452bf0a97ea9ba0ad69d49a1f
