Re: [PATCH v2 5/6] watchdog: at91sam9: request the irq with IRQF_NO_SUSPEND

[Mark Rutland <mark.rutland@arm.com>]

Hi Rafael,

> enable_irq_wake() has no effect on IRQF_NO_SUSPEND interrupts, so if the
> driver uses IRQF_NO_SUSPEND, it does not need to use enable_irq_wake()
> in addition to that.

That's not generally true -- certainly not for irq_chips without the
IRQCHIP_SKIP_SET_WAKE flag.

Consider systems where the suspended state results in power to the CPU
being cut, and we rely on an external piece of logic attached to the
irq_chip to detect wakeup IRQs and restore power.

In those cases irq_chip::irq_set_wake() must be called to ensure that
the wakeup logic is configured. If the wakeup logic is not configured to
look out for an IRQ, then when the IRQ is asserted by a device the
wakeup logic may not trigger. Thus the CPU power never gets restored, so
the CPU cannot handle the interrupt.

This is handled in enable_irq_wake() -- either the chip has the
IRQCHIP_SKIP_SET_WAKE flag set or chip->irq_set_wake() must succeed. If
neither is true enable_irq_wake() will return an error code to indicate
we can't use the IRQ for wakeup.

The request_irq path never results in a call to chip->irq_set_wake(),
even with the IRQF_NO_SUSPEND flag. So requesting an irq with
IRQF_NO_SUSPEND does not guarantee wakeup; it only guarantees that the
CPU can take the interrupt _around_ the suspended state, not necessarily
while _in_ the suspended state.

> Drivers using IRQF_COND_SUSPEND generally should use enable_irq_wake() too
> in case they end up in a situation without sharing a NO_SUSPEND interrupt, in
> which case their interrupt handlers won't be called after suspend_device_irqs(),
> so they need to rely on the core to do the wakeup.
> 
> > I agree that if problematic, it's an existing bug. Given Boris's
> > comments in the other thread this may just a minor semantic issue w.r.t.
> > IRQF_NO_SUSPEND vs IRQF_COND_SUSPEND.
> 
> It depends on whether or not the watchdog's interrupt handler has to be
> called immediately after receiving an interrupt (IRQF_NO_SUSPEND is
> better then) or it can be deferred till the resume_device_irqs() time.

We seem to be conflating some related properties:

[a] The IRQ will be left unmasked.
[b] The IRQ will be handled immediately when taken.
[c] The IRQ will wake the system from suspend.

Requesting an IRQ with IRQF_NO_SUSPEND guarantees [a,b], but does not
guarantee [c].

A successful enable_irq_wake() on an IRQ guarantees [a,c], but usually
does not give [b] unless the IRQ was requested with IRQF_COND_SUSPEND
and happens to be shared with an IRQF_NO_SUSPEND user.

It sounds like for this kind of watchdog device we want [a,b,c], even if
the IRQ is not shared with an IRQF_NO_SUSPEND user.

Thanks,
Mark.