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

[Mark Rutland]

[...]

> > 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
> suspend_enter().

[...]

> > 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()
section.

> But then it might as well be deferred until after
> resume_device_irqs().

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
configuration.

I think it's possible (assuming the caveats on [b] above) to provide
[a,b,c] for this case.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/