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