Hi, On Fri, Sep 4, 2020 at 2:54 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote: > > Doug, > > On Thu, Sep 03 2020 at 16:19, Doug Anderson wrote: > > On Thu, Sep 3, 2020 at 5:57 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote: > >> That pending interrupt will not prevent the machine from going into > >> suspend and if it's an edge interrupt then an unmask in > >> suspend_device_irq() won't help. Edge interrupts are not resent in > >> hardware. They are fire and forget from the POV of the device > >> hardware. > > > > Ah, interesting. I didn't think about this case exactly. I might > > have a fix for it anyway. At some point in time I was thinking that > > the world could be solved by relying on lazily-disabled interrupts and > > I wrote up a patch to make sure that they woke things up. If you're > > willing to check out our gerrit you can look at: > > > > https://crrev.com/c/2314693 > > > > ...if not I can post it as a RFC for you. > > I actually tried despite my usual aversion against web > interfaces. Aversion confirmed :) > > You could have included the 5 lines of patch into your reply to spare me > the experience. :) Sorry! :( Inline patches are a bit of a pain for me since I'm certifiably insane and use the gmail web interface for kernel mailing lists. Everyone has their pet aversions, I guess. ;-) > > I'm sure I've solved the problem in a completely incorrect and broken > > way, but hopefully the idea makes sense. In discussion we decided not > > to go this way because it looked like IRQ clients could request an IRQ > > with IRQ_DISABLE_UNLAZY and then that'd break us. :( ...but even so I > > think the patch is roughly right and would address your point #1. > > Kinda :) But that's still incomplete because it does not handle the case > where the interrupt arrives between disable_irq() and enable_irq_wake(). > See below. Huh, I thought I'd handled this with the code in irq_set_irq_wake() which checked if it was pending and did a wakeup. In any case, I trust your understanding of this code far better than I trust mine. How should we proceed then? Do you want to post up an official patch? At the moment I don't have any test cases that need your patch since the interrupts I'm dealing with are not lazily disabled. However, I still do agree that it's the right thing to do. > >> 2) irq chip has a irq_disable() callback or has IRQ_DISABLE_UNLAZY set > >> > >> In that case disable_irq() will mask it at the hardware level and it > >> stays that way until enable_irq() is invoked. > >> > >> #1 kinda works and the gap is reasonably trivial to fix in > >> suspend_device_irq() by checking the pending state and telling the PM > >> core that there is a wakeup pending. > >> > >> #2 Needs an indication from the chip flags that an interrupt which is > >> masked has to be unmasked when it is a enabled wakeup source. > >> > >> I assume your problem is #2, right? If it's #1 then UNMASK_IF_WAKEUP is > >> the wrong answer. > > > > Right, the problem is #2. We're not in the lazy mode. > > Right and that's where we want the new chip flag with the unmask if > armed. OK, so we're back in Maulik's court to spin, right? I think the last word before our tangent was at: http://lore.kernel.org/r/87y2m1vhkm.fsf@xxxxxxxxxxxxxxxxxxxxxxx There you were leaning towards #2 ("a new function disable_wakeup_irq_for_suspend()"). Presumably you'd now be suggesting #1 ("Do the symmetric thing") since I've pointed out the bunch of drivers that would need to change. -Doug
