[PATCH v2 5/6] watchdog: at91sam9: request the irq with IRQF_NO_SUSPEND
Rafael J. Wysocki
rafael at kernel.org
Fri Mar 6 04:39:14 PST 2015
On Fri, Mar 6, 2015 at 12:06 PM, Mark Rutland <mark.rutland at arm.com> wrote:
>> > 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.
>> Right. "Suspended state" meaning full suspend here I suppose?
> Yes; any state deeper than suspend-to-idle.
>> > 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].
>> That's correct. IRQF_NO_SUSPEND does not guarantee that interrupts from
>> that IRQ will have any effect after arch_suspend_disable_irqs() in
>> > 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.
>> We can't guarantee that, though. arch_suspend_disable_irqs() disables
>> interrupts on the last working CPU and it won't get any. It may be
>> brought out of a low-power state by a pending interrupt, but it won't act
>> upon that interrupt immediately anyway, only after the arch_suspend_enable_irqs()
>> in suspend_enter().
> Ok, so [b] needs the caveat that it's only handled "immediately" outside
> of the arch_suspend_disable_irqs() ... arch_suspend_enable_irqs()
>> But then it might as well be deferred until after
> That was my original line of thinking, in which case the watchdog driver
> should use IRQF_COND_SUSPEND rather than IRQF_NO_SUSPEND, with
> enable_irq_wake() if we care about the watchdog during suspend. I'm
> happy with this.
> Considering that the use-case of a watchdog is to alert us to something
> going hideously wrong in the kernel, we want to handle the IRQ after
> executing the smallest amount of kernel code possible. For that, they
> need to have their handlers to be called "immediately" outside of the
> arch_suspend_disable_irqs() ... arch_suspend_enable_irqs() window, and
> need to be enabled during suspend to attempt to catch bad wakeup device
> I think it's possible (assuming the caveats on [b] above) to provide
> [a,b,c] for this case.
But in this case the request_irq() passing IRQF_NO_SUSPEND *and* requiring
enable_irq_wake() in addition to that needs a big fat comment explaining the
whole thing or we'll forget about the gory details at one point and no one will
know what's going on in there.
More information about the linux-arm-kernel