Hi, On Wed, Aug 26, 2020 at 3:15 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote: > > On Wed, Aug 26 2020 at 15:22, Maulik Shah wrote: > > On 8/26/2020 3:08 AM, Thomas Gleixner wrote: > >>> Where is the corresponding change to resume_irq()? Don't we need to > >>> disable an irq if it was disabled on suspend and forcibly enabled here? > >>> > > I should have added comment explaining why i did not added. > > I thought of having corresponding change to resume_irq() but i did not > > kept intentionally since i didn't > > observe any issue in my testing. > > That makes it correct in which way? Did not explode in my face is hardly > proof of anything. > > > Actually the drivers which called (disable_irq() + enable_irq_wake()), > > are invoking enable_irq() > > in the resume path everytime. With the driver's call to enable_irq() > > things are restoring back already. > > No, that's just wrong because you again create inconsistent state. > > > If above is not true in some corner case, then the IRQ handler of > > driver won't get invoked, in such case, why even to wake up with such > > IRQs in the first place, right? > > I don't care about the corner case. If the driver misses to do it is > buggy in the first place. Silently papering over it is just mindless > hackery. > > There are two reasonable choices here: > > 1) Do the symmetric thing > > 2) Let the drivers call a new function disable_wakeup_irq_for_suspend() > which marks the interrupt to be enabled from the core on suspend and > remove the enable call on the resume callback of the driver. > > Then you don't need the resume part in the core and state still is > consistent. > > I'm leaning towards #2 because that makes a lot of sense. IIUC, #2 requires that we change existing drivers that are currently using disable_irq() + enable_irq_wake(), right? Presumably, if we're going to do #2, we should declare that what drivers used to do is now considered illegal, right? Perhaps we could detect that and throw a warning so that they know that they need to change to use the new disable_wakeup_irq_for_suspend() API. Otherwise these drivers will work fine on some systems (like they always have) but will fail in weird corner cases for systems that are relying on drivers to call disable_wakeup_irq_for_suspend(). That doesn't sound super great to me... ...or, if doing the symmetric thing isn't too bad, we could do that? -Doug
