Re: [PATCH v5 3/6] genirq/PM: Introduce IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag

[Doug Anderson]

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