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

[Mark Rutland]

Hi Boris,

TL;DR - I guess using IRQF_NO_SUSPEND for now is OK a a best-effort
approach for now, so don't let my comments block this patch.

However, there are still some potential issues in what would already be
a failure case: your usual wakeup mechanism not waking the system up in
time to poke the watchdog, and then the watchdog raising an IRQ that
never gets taken because the system is in a suspended state.

> > Is the timer we use to ping the watchdog guaranted to result in a wakeup
> > before an interrupt will be triggered? If so, then I think we're ok.
> 
> It should be (I don't recall exactly what the logic is, but it's at
> least half the watchdog time limit).

Ok. If that's the case then my main fear is gone.

[...]

> If we want the watchdog to be inactive when entering suspend, then we
> shouldn't reboot the machine when receiving a watchdog irq while the
> system is suspended.

For this I would expect IRQF_COND_SUSPEND, because we don't care about
the suspended case. We just don't want to negatively impact the timers.

> ITOH, with the hardware mode (reset handled by the watchdog IP) you
> can't disable the watchdog when entering suspend, so I would expect the
> same behavior for the SW mode.

For this I would expect IRQF_COND_SUSPEND and enable_irq_wake().

If we really want a wakeup IRQ guaranteed to be called immediately
(without bothering to do most of the resume work first), none of the
current semantics align.

> > Regardless, if the only reason we care about taking the interrupt during
> > the suspend/resume phases is due to the timer sharing the IRQ, then
> > shouldn't we be using IRQF_COND_SUSPEND?
> 
> I'm not sure, but IMO this interrupt should be flagged as NO_SUSPEND,
> because it's here to reset the system (even if it is suspended).
> If you flag the irq line as COND_SUSPEND, and atmel decide to give this
> peripheral its own IRQ line (on new SoCs), then your watchdog will not
> reboot the system when it is suspended.
> Another solution would be to support wakeup for this peripheral and
> delay the system reboot until it has resumed.

>From the above it sounds like we'd need wakeup and guaranteed immediate
handler calling. That either needs rethink of IRQF_NO_SUSPEND +
enable_irq_wake(), or something like a new IRQF_SW_WATCHDOG +
enable_irq_wake().

> Anyway, if we decide to go for the wakeup approach, I'd prefer to post
> another patch on top of this one.

If everyone else is happy with this using IRQF_NO_SUSPEND for now then
don't let my comments above block this patch.

Mark.