| Message ID | 1425287898-15093-6-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Accepted |
| Headers | show |
On 03/02/2015 01:18 AM, Boris Brezillon wrote: > The watchdog interrupt (only used when activating software watchdog) > shouldn't be suspended when entering suspend mode, because it is shared > with a timer device (which request the line with IRQF_NO_SUSPEND) and once > the watchdog "Mode Register" has been written, it cannot be changed (which > means we cannot disable the watchdog interrupt when entering suspend). > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> Acked-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/at91sam9_wdt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c > index 6df9405..1443b3c 100644 > --- a/drivers/watchdog/at91sam9_wdt.c > +++ b/drivers/watchdog/at91sam9_wdt.c > @@ -208,7 +208,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt) > > if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) { > err = request_irq(wdt->irq, wdt_interrupt, > - IRQF_SHARED | IRQF_IRQPOLL, > + IRQF_SHARED | IRQF_IRQPOLL | > + IRQF_NO_SUSPEND, > pdev->name, wdt); > if (err) > return err; >
Hi Boris, On Mon, Mar 02, 2015 at 09:18:17AM +0000, Boris Brezillon wrote: > The watchdog interrupt (only used when activating software watchdog) > shouldn't be suspended when entering suspend mode, because it is shared > with a timer device (which request the line with IRQF_NO_SUSPEND) and once > the watchdog "Mode Register" has been written, it cannot be changed (which > means we cannot disable the watchdog interrupt when entering suspend). > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/watchdog/at91sam9_wdt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c > index 6df9405..1443b3c 100644 > --- a/drivers/watchdog/at91sam9_wdt.c > +++ b/drivers/watchdog/at91sam9_wdt.c > @@ -208,7 +208,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt) > > if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) { > err = request_irq(wdt->irq, wdt_interrupt, > - IRQF_SHARED | IRQF_IRQPOLL, > + IRQF_SHARED | IRQF_IRQPOLL | > + IRQF_NO_SUSPEND, I'm a little confused by this. What happens if the watchdog fires when we're actually in the suspended state (when IRQF_NO_SUSPEND interrupts aren't guaranteed to be delivered). Does this rely on the watchdog IRQ being taken while in the actual suspended state (but not waking up the system while handling it)? Thanks, Mark.
On Wednesday, March 04, 2015 06:38:09 PM Mark Rutland wrote: > Hi Boris, > > On Mon, Mar 02, 2015 at 09:18:17AM +0000, Boris Brezillon wrote: > > The watchdog interrupt (only used when activating software watchdog) > > shouldn't be suspended when entering suspend mode, because it is shared > > with a timer device (which request the line with IRQF_NO_SUSPEND) and once > > the watchdog "Mode Register" has been written, it cannot be changed (which > > means we cannot disable the watchdog interrupt when entering suspend). > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/watchdog/at91sam9_wdt.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c > > index 6df9405..1443b3c 100644 > > --- a/drivers/watchdog/at91sam9_wdt.c > > +++ b/drivers/watchdog/at91sam9_wdt.c > > @@ -208,7 +208,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt) > > > > if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) { > > err = request_irq(wdt->irq, wdt_interrupt, > > - IRQF_SHARED | IRQF_IRQPOLL, > > + IRQF_SHARED | IRQF_IRQPOLL | > > + IRQF_NO_SUSPEND, > > I'm a little confused by this. What happens if the watchdog fires when > we're actually in the suspended state (when IRQF_NO_SUSPEND interrupts > aren't guaranteed to be delivered). Why wouldn't they be delivered? If that's suspend-to-idle, we'll handle them normally. If that's full suspend, they may not be handled at the last stage (when we run on one CPU with interrupts off), but that was the case before the wakeup interrupts rework already and I'd expect it to be taken into account somehow in the existing code (or if it isn't taken into account, we have a bug, but it is not related to this series). > Does this rely on the watchdog IRQ being taken while in the actual > suspended state (but not waking up the system while handling it)? It is going to work this way at least AFAICT.
Hi Mark, On Wed, 4 Mar 2015 18:38:09 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > Hi Boris, > > On Mon, Mar 02, 2015 at 09:18:17AM +0000, Boris Brezillon wrote: > > The watchdog interrupt (only used when activating software watchdog) > > shouldn't be suspended when entering suspend mode, because it is shared > > with a timer device (which request the line with IRQF_NO_SUSPEND) and once > > the watchdog "Mode Register" has been written, it cannot be changed (which > > means we cannot disable the watchdog interrupt when entering suspend). > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/watchdog/at91sam9_wdt.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c > > index 6df9405..1443b3c 100644 > > --- a/drivers/watchdog/at91sam9_wdt.c > > +++ b/drivers/watchdog/at91sam9_wdt.c > > @@ -208,7 +208,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt) > > > > if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) { > > err = request_irq(wdt->irq, wdt_interrupt, > > - IRQF_SHARED | IRQF_IRQPOLL, > > + IRQF_SHARED | IRQF_IRQPOLL | > > + IRQF_NO_SUSPEND, > > I'm a little confused by this. What happens if the watchdog fires when > we're actually in the suspended state (when IRQF_NO_SUSPEND interrupts > aren't guaranteed to be delivered). It reboot the system. > > Does this rely on the watchdog IRQ being taken while in the actual > suspended state (but not waking up the system while handling it)? Actually this interrupt handler is just here to reboot the system if the user didn't refresh the watchdog. Do we really have to wake up the system to then call emergency_restart ?
Hi Boris, I'd missed the fact that this was for SW watchdog as opposed to HW watchdog, which may explain my confusion. [...] > > > err = request_irq(wdt->irq, wdt_interrupt, > > > - IRQF_SHARED | IRQF_IRQPOLL, > > > + IRQF_SHARED | IRQF_IRQPOLL | > > > + IRQF_NO_SUSPEND, > > > > I'm a little confused by this. What happens if the watchdog fires when > > we're actually in the suspended state (when IRQF_NO_SUSPEND interrupts > > aren't guaranteed to be delivered). > > It reboot the system. 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. If not, then don't we need to clear a potentially pending watchdog irq at resume time so at to not immediately reboot the machine? I couldn't see any logic to that effect in the driver. 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? Thanks, Mark.
[...] > > > err = request_irq(wdt->irq, wdt_interrupt, > > > - IRQF_SHARED | IRQF_IRQPOLL, > > > + IRQF_SHARED | IRQF_IRQPOLL | > > > + IRQF_NO_SUSPEND, > > > > I'm a little confused by this. What happens if the watchdog fires when > > we're actually in the suspended state (when IRQF_NO_SUSPEND interrupts > > aren't guaranteed to be delivered). > > Why wouldn't they be delivered? > > If that's suspend-to-idle, we'll handle them normally. If that's full suspend, > they may not be handled at the last stage (when we run on one CPU with interrupts > off), but that was the case before the wakeup interrupts rework already and I'd > expect it to be taken into account somehow in the existing code (or if it isn't > taken into account, we have a bug, but it is not related to this series). There's no enable_irq_wake(wdt->irq), and I was under the impression this is for full suspend. 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. Thanks, Mark.
Hi Boris, On Thu, 5 Mar 2015 10:53:08 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > Hi Boris, > > I'd missed the fact that this was for SW watchdog as opposed to HW > watchdog, which may explain my confusion. > > [...] > > > > > err = request_irq(wdt->irq, wdt_interrupt, > > > > - IRQF_SHARED | IRQF_IRQPOLL, > > > > + IRQF_SHARED | IRQF_IRQPOLL | > > > > + IRQF_NO_SUSPEND, > > > > > > I'm a little confused by this. What happens if the watchdog fires when > > > we're actually in the suspended state (when IRQF_NO_SUSPEND interrupts > > > aren't guaranteed to be delivered). > > > > It reboot the system. > > 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). > > If not, then don't we need to clear a potentially pending watchdog irq > at resume time so at to not immediately reboot the machine? I couldn't > see any logic to that effect in the driver. That depends on what we want. 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. 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. > > 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. Anyway, if we decide to go for the wakeup approach, I'd prefer to post another patch on top of this one. Best Regards, Boris
On Thu, 5 Mar 2015 12:17:23 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Boris, ^ Mark, I'm suffering from a dual personality disorder :-) > > On Thu, 5 Mar 2015 10:53:08 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > > Hi Boris, > > > > I'd missed the fact that this was for SW watchdog as opposed to HW > > watchdog, which may explain my confusion. > > > > [...] > > > > > > > err = request_irq(wdt->irq, wdt_interrupt, > > > > > - IRQF_SHARED | IRQF_IRQPOLL, > > > > > + IRQF_SHARED | IRQF_IRQPOLL | > > > > > + IRQF_NO_SUSPEND, > > > > > > > > I'm a little confused by this. What happens if the watchdog fires when > > > > we're actually in the suspended state (when IRQF_NO_SUSPEND interrupts > > > > aren't guaranteed to be delivered). > > > > > > It reboot the system. > > > > 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). > > > > > If not, then don't we need to clear a potentially pending watchdog irq > > at resume time so at to not immediately reboot the machine? I couldn't > > see any logic to that effect in the driver. > > That depends on what we want. > 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. > 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. > > > > > 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. > > Anyway, if we decide to go for the wakeup approach, I'd prefer to post > another patch on top of this one. > > Best Regards, > > Boris > >
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.
On Thu, Mar 5, 2015 at 11:57 AM, Mark Rutland <mark.rutland@arm.com> wrote: > [...] > >> > > err = request_irq(wdt->irq, wdt_interrupt, >> > > - IRQF_SHARED | IRQF_IRQPOLL, >> > > + IRQF_SHARED | IRQF_IRQPOLL | >> > > + IRQF_NO_SUSPEND, >> > >> > I'm a little confused by this. What happens if the watchdog fires when >> > we're actually in the suspended state (when IRQF_NO_SUSPEND interrupts >> > aren't guaranteed to be delivered). >> >> Why wouldn't they be delivered? >> >> If that's suspend-to-idle, we'll handle them normally. If that's full suspend, >> they may not be handled at the last stage (when we run on one CPU with interrupts >> off), but that was the case before the wakeup interrupts rework already and I'd >> expect it to be taken into account somehow in the existing code (or if it isn't >> taken into account, we have a bug, but it is not related to this series). > > There's no enable_irq_wake(wdt->irq), and I was under the impression this > is for full suspend. 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. 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. Rafael
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.
On Thursday, March 05, 2015 04:32:27 PM Mark Rutland wrote: > 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. Right. I forgot about that part. > 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? > > 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]. 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(). > 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. That's correct too. > 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(). But then it might as well be deferred until after resume_device_irqs(). Rafael
[...] > > 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.
On Fri, Mar 6, 2015 at 12:06 PM, Mark Rutland <mark.rutland@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 >> 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. OK 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. Rafael
> >> > 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. [...] > > 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. > > OK > > 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. Agreed. I'd expect an IRQF_SW_WATCHDOG or something to that effect should also be required for that case. Thanks, Mark.
On Thu, Mar 05, 2015 at 04:10:16PM +0100, Rafael J. Wysocki wrote: > 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. I still feel we should BUG when someone is calling enable_irq_wake() on an irq with only one desc which has NO_SUSPEND set.
On Fri, Mar 06, 2015 at 11:06:18AM +0000, Mark Rutland 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. I don't think we should want to make such distinction; we should treat all suspend states the same. Drivers should not want to rely on the fact that one state (suspend-to-idle) might maybe deal with interrupts while other states do not. > > > 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. Note that COND_SUSPEND must have SHARED set. > 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. While I appreciate the use-case; we should be careful not to make of mess of things either.
On Thu, Mar 05, 2015 at 11:53:08AM +0000, Mark Rutland wrote: > If everyone else is happy with this using IRQF_NO_SUSPEND for now then > don't let my comments above block this patch. Yeah, I'm really not happy with NO_SUSPEND + enable_irq_wake(). I really want that combo to BUG/WARN -- esp. since there's so much cargo culted crap out there. We should make robust interfaces, not randomly toggle flags until it mostly works by accident rather than by design -- which is what this feels like. And while I appreciate the watchdog use-case; I think the easiest solution for now is to simply disable the wathdog over suspend until we've come up with something that makes sense. As it is, you need to 'suspend' the watchdog at some point anyhow; you don't want that thing to wake you from whatever suspend state you're in.
Hello, On Sat, Mar 07, 2015 at 10:18:46AM +0100, Peter Zijlstra wrote: > On Thu, Mar 05, 2015 at 11:53:08AM +0000, Mark Rutland wrote: > > If everyone else is happy with this using IRQF_NO_SUSPEND for now then > > don't let my comments above block this patch. > > Yeah, I'm really not happy with NO_SUSPEND + enable_irq_wake(). > > I really want that combo to BUG/WARN -- esp. since there's so much cargo > culted crap out there. > > We should make robust interfaces, not randomly toggle flags until it > mostly works by accident rather than by design -- which is what this > feels like. > > And while I appreciate the watchdog use-case; I think the easiest > solution for now is to simply disable the wathdog over suspend until > we've come up with something that makes sense. > > As it is, you need to 'suspend' the watchdog at some point anyhow; you > don't want that thing to wake you from whatever suspend state you're in. The Atmel watchdog can't be stopped once it's started. This is actually very useful so we can reset if suspend or resume failed, the only drawback is that you have to wake up from time to time (e.g. by using the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. I am working on safety products and keeping the watchdog running whatever the state is almost mandatory, one of the test on those products is about unsoldering all the crystals live from your product and the product should detect the general fault and power on a buzzer and a yellow fault LED. This is usually done with a watchdog clocked from an internal RC of the µC/SoC (so it can't be unsoldered ;-) and a GPIO with a reset state in input/pull-up, the device clamps the GPIO to ground, if the watchdog resets the system the GPIO is going to switch back to input therefore changing its state. This can of course be done with an external watchdog circuitry, but it costs more and will consume much more than using à 1 µA RC oscillator/watchdog already present in the µC/SoC. Sylvain
On Sat 2015-03-07 11:20:56, Sylvain Rochet wrote: > Hello, > > On Sat, Mar 07, 2015 at 10:18:46AM +0100, Peter Zijlstra wrote: > > On Thu, Mar 05, 2015 at 11:53:08AM +0000, Mark Rutland wrote: > > > If everyone else is happy with this using IRQF_NO_SUSPEND for now then > > > don't let my comments above block this patch. > > > > Yeah, I'm really not happy with NO_SUSPEND + enable_irq_wake(). > > > > I really want that combo to BUG/WARN -- esp. since there's so much cargo > > culted crap out there. > > > > We should make robust interfaces, not randomly toggle flags until it > > mostly works by accident rather than by design -- which is what this > > feels like. > > > > And while I appreciate the watchdog use-case; I think the easiest > > solution for now is to simply disable the wathdog over suspend until > > we've come up with something that makes sense. > > > > As it is, you need to 'suspend' the watchdog at some point anyhow; you > > don't want that thing to wake you from whatever suspend state you're in. > > The Atmel watchdog can't be stopped once it's started. This is actually > very useful so we can reset if suspend or resume failed, the only > drawback is that you have to wake up from time to time (e.g. by using > the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes after watchdog kills the system. But you did not ask for dead system, you asked for suspend. And while that behaviour is useful for you, I don't think it is exactly useful behaviour, nor it is the behaviour user would expect. Pavel
Hello, On Sat, Mar 07, 2015 at 11:39:39AM +0100, Pavel Machek wrote: > On Sat 2015-03-07 11:20:56, Sylvain Rochet wrote: > > Hello, > > > > On Sat, Mar 07, 2015 at 10:18:46AM +0100, Peter Zijlstra wrote: > > > On Thu, Mar 05, 2015 at 11:53:08AM +0000, Mark Rutland wrote: > > > > If everyone else is happy with this using IRQF_NO_SUSPEND for now then > > > > don't let my comments above block this patch. > > > > > > Yeah, I'm really not happy with NO_SUSPEND + enable_irq_wake(). > > > > > > I really want that combo to BUG/WARN -- esp. since there's so much cargo > > > culted crap out there. > > > > > > We should make robust interfaces, not randomly toggle flags until it > > > mostly works by accident rather than by design -- which is what this > > > feels like. > > > > > > And while I appreciate the watchdog use-case; I think the easiest > > > solution for now is to simply disable the wathdog over suspend until > > > we've come up with something that makes sense. > > > > > > As it is, you need to 'suspend' the watchdog at some point anyhow; you > > > don't want that thing to wake you from whatever suspend state you're in. > > > > The Atmel watchdog can't be stopped once it's started. This is actually > > very useful so we can reset if suspend or resume failed, the only > > drawback is that you have to wake up from time to time (e.g. by using > > the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. > > Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes > after watchdog kills the system. But you did not ask for dead system, > you asked for suspend. Yeah, that's why I'm setting the RTC/RTT in the pm_enter() callback on our products. On wake-up I'm checking if we woke up using the RTC/RTT in the pm_suspend_again() callback, if true we clear the watchdog and we go back to sleep. This can't easily be mainlined because it adds RTC/RTT/WDT calls from PM code, which will not be accepted anyway. > And while that behaviour is useful for you, I don't think it is > exactly useful behaviour, nor it is the behaviour user would expect. I agree, but it still can't be stopped, IMHO users wanting to suspend without being protected by the watchdog during suspend and resume should just don't use the hardware watchdog at all, they are already missing the watchdog in a tricky area where the kernel can fail more than anywhere else, the software watchdog should be fine as well for them. Sylvain
On 07/03/2015 at 11:39:39 +0100, Pavel Machek wrote : > > The Atmel watchdog can't be stopped once it's started. This is actually > > very useful so we can reset if suspend or resume failed, the only > > drawback is that you have to wake up from time to time (e.g. by using > > the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. > > Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes > after watchdog kills the system. But you did not ask for dead system, > you asked for suspend. > > And while that behaviour is useful for you, I don't think it is > exactly useful behaviour, nor it is the behaviour user would expect. > I think you misunderstood, that is exactly the expected behaviour. This is hardware defined. Once the watchdog is started, nobody can stop it. Trying to change the mode register will result in a reset of the SoC. It is documented in the datasheet and any user wanting another behaviour is out of luck. So basically, when using a watchdog, you have to wake up every 15-16s to restart it.
On Sat 2015-03-07 12:06:45, Alexandre Belloni wrote: > On 07/03/2015 at 11:39:39 +0100, Pavel Machek wrote : > > > The Atmel watchdog can't be stopped once it's started. This is actually > > > very useful so we can reset if suspend or resume failed, the only > > > drawback is that you have to wake up from time to time (e.g. by using > > > the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. > > > > Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes > > after watchdog kills the system. But you did not ask for dead system, > > you asked for suspend. > > > > And while that behaviour is useful for you, I don't think it is > > exactly useful behaviour, nor it is the behaviour user would expect. > > > > I think you misunderstood, that is exactly the expected behaviour. This > is hardware defined. Once the watchdog is started, nobody can stop it. > Trying to change the mode register will result in a reset of the > SoC. Well, it boils down to "what is stronger". Desire to suspend the system, or desire to reboot the system. It is "echo mem > state", not "echo reboot > state". > It is documented in the datasheet and any user wanting another behaviour > is out of luck. Actaully, your platform should just refuse to enter suspend-to-RAM when hw watchdog is enabled. Pavel
Hello, On Sat, Mar 07, 2015 at 12:29:33PM +0100, Pavel Machek wrote: > On Sat 2015-03-07 12:06:45, Alexandre Belloni wrote: > > On 07/03/2015 at 11:39:39 +0100, Pavel Machek wrote : > > > > The Atmel watchdog can't be stopped once it's started. This is actually > > > > very useful so we can reset if suspend or resume failed, the only > > > > drawback is that you have to wake up from time to time (e.g. by using > > > > the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. > > > > > > Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes > > > after watchdog kills the system. But you did not ask for dead system, > > > you asked for suspend. > > > > > > And while that behaviour is useful for you, I don't think it is > > > exactly useful behaviour, nor it is the behaviour user would expect. > > > > > > > I think you misunderstood, that is exactly the expected behaviour. This > > is hardware defined. Once the watchdog is started, nobody can stop it. > > Trying to change the mode register will result in a reset of the > > SoC. > > Well, it boils down to "what is stronger". Desire to suspend the > system, or desire to reboot the system. > > It is "echo mem > state", not "echo reboot > state". Maybe we should warn the watchdog is enabled and the system is going to reboot if nothing woke-up the system before the watchdog expire, but the maximum watchdog is 16s so it can't get unnoticed during development, I am confident embedded engineers are smart enough to understand what is happening without a displayed warning :-) > > It is documented in the datasheet and any user wanting another behaviour > > is out of luck. > > Actaully, your platform should just refuse to enter suspend-to-RAM > when hw watchdog is enabled. Yeah that's what I said, hardware watchdog or suspend: chose one or use the software watchdog instead or "hack" around the way I am doing ;-) Sylvain
On Saturday, March 07, 2015 12:06:45 PM Alexandre Belloni wrote: > On 07/03/2015 at 11:39:39 +0100, Pavel Machek wrote : > > > The Atmel watchdog can't be stopped once it's started. This is actually > > > very useful so we can reset if suspend or resume failed, the only > > > drawback is that you have to wake up from time to time (e.g. by using > > > the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. > > > > Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes > > after watchdog kills the system. But you did not ask for dead system, > > you asked for suspend. > > > > And while that behaviour is useful for you, I don't think it is > > exactly useful behaviour, nor it is the behaviour user would expect. > > > > I think you misunderstood, that is exactly the expected behaviour. This > is hardware defined. Once the watchdog is started, nobody can stop it. > Trying to change the mode register will result in a reset of the SoC. > > It is documented in the datasheet and any user wanting another behaviour > is out of luck. > > So basically, when using a watchdog, you have to wake up every 15-16s to > restart it. So question is if we need a separate interrupt handler for that, expecially since it is shared with the PIT timer anyway. Seems to me that the simplest way out of this conundrum would be to simply make the timer's interrupt handler kick the watchdog every once a while and get rid of the separate watchdog interrupt handler entirely. While at it, can anyone explain to me please how the suspend state (full suspend) looks like on that platform? What's different from the working state in particular.
On Saturday, March 07, 2015 12:29:33 PM Pavel Machek wrote: > On Sat 2015-03-07 12:06:45, Alexandre Belloni wrote: > > On 07/03/2015 at 11:39:39 +0100, Pavel Machek wrote : > > > > The Atmel watchdog can't be stopped once it's started. This is actually > > > > very useful so we can reset if suspend or resume failed, the only > > > > drawback is that you have to wake up from time to time (e.g. by using > > > > the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. > > > > > > Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes > > > after watchdog kills the system. But you did not ask for dead system, > > > you asked for suspend. > > > > > > And while that behaviour is useful for you, I don't think it is > > > exactly useful behaviour, nor it is the behaviour user would expect. > > > > > > > I think you misunderstood, that is exactly the expected behaviour. This > > is hardware defined. Once the watchdog is started, nobody can stop it. > > Trying to change the mode register will result in a reset of the > > SoC. > > Well, it boils down to "what is stronger". Desire to suspend the > system, or desire to reboot the system. > > It is "echo mem > state", not "echo reboot > state". > > > It is documented in the datasheet and any user wanting another behaviour > > is out of luck. > > Actaully, your platform should just refuse to enter suspend-to-RAM > when hw watchdog is enabled. Quite likely, depending on how exactly the suspend is implemented.
Hi, On 08/03/2015 at 02:12:53 +0100, Rafael J. Wysocki wrote : > > > I think you misunderstood, that is exactly the expected behaviour. This > > > is hardware defined. Once the watchdog is started, nobody can stop it. > > > Trying to change the mode register will result in a reset of the > > > SoC. > > > > Well, it boils down to "what is stronger". Desire to suspend the > > system, or desire to reboot the system. > > > > It is "echo mem > state", not "echo reboot > state". > > > > > It is documented in the datasheet and any user wanting another behaviour > > > is out of luck. > > > > Actaully, your platform should just refuse to enter suspend-to-RAM > > when hw watchdog is enabled. > > Quite likely, depending on how exactly the suspend is implemented. > We've had absolutely zero complain on that. It is quite clear in the datasheet that failing to refresh the watchdog once started will lead to a reset and that it is impossible to stop. It is actually quite convenient to also ensure that you can actually wake up from suspend because that can obviously go wrong.
On Monday, March 09, 2015 08:55:46 AM Alexandre Belloni wrote: > Hi, > > On 08/03/2015 at 02:12:53 +0100, Rafael J. Wysocki wrote : > > > > I think you misunderstood, that is exactly the expected behaviour. This > > > > is hardware defined. Once the watchdog is started, nobody can stop it. > > > > Trying to change the mode register will result in a reset of the > > > > SoC. > > > > > > Well, it boils down to "what is stronger". Desire to suspend the > > > system, or desire to reboot the system. > > > > > > It is "echo mem > state", not "echo reboot > state". > > > > > > > It is documented in the datasheet and any user wanting another behaviour > > > > is out of luck. > > > > > > Actaully, your platform should just refuse to enter suspend-to-RAM > > > when hw watchdog is enabled. > > > > Quite likely, depending on how exactly the suspend is implemented. > > > > We've had absolutely zero complain on that. It is quite clear in the > datasheet that failing to refresh the watchdog once started will lead to > a reset and that it is impossible to stop. > It is actually quite convenient to also ensure that you can actually > wake up from suspend because that can obviously go wrong. I gather then that the suspend implementation is such that touching the watchdog periodically while suspended is not a problem. Again, can you please tell me how suspend is implemented on at91?
Hi, On 09/03/2015 at 15:30:01 +0100, Rafael J. Wysocki wrote : > > > > Actaully, your platform should just refuse to enter suspend-to-RAM > > > > when hw watchdog is enabled. > > > > > > Quite likely, depending on how exactly the suspend is implemented. > > > > > > > We've had absolutely zero complain on that. It is quite clear in the > > datasheet that failing to refresh the watchdog once started will lead to > > a reset and that it is impossible to stop. > > It is actually quite convenient to also ensure that you can actually > > wake up from suspend because that can obviously go wrong. > > I gather then that the suspend implementation is such that touching the > watchdog periodically while suspended is not a problem. > > Again, can you please tell me how suspend is implemented on at91? > It actually depends on the architecture (at91rm9200, at91sam9 or sama5) but basically, the clocks are switched off in almost all the peripheral drivers then the ram self refresh activated, the master clock is switched off using code running from SRAM and the core is then waiting for interrupt.
On Tuesday, March 10, 2015 10:33:17 PM Alexandre Belloni wrote: > Hi, > > On 09/03/2015 at 15:30:01 +0100, Rafael J. Wysocki wrote : > > > > > Actaully, your platform should just refuse to enter suspend-to-RAM > > > > > when hw watchdog is enabled. > > > > > > > > Quite likely, depending on how exactly the suspend is implemented. > > > > > > > > > > We've had absolutely zero complain on that. It is quite clear in the > > > datasheet that failing to refresh the watchdog once started will lead to > > > a reset and that it is impossible to stop. > > > It is actually quite convenient to also ensure that you can actually > > > wake up from suspend because that can obviously go wrong. > > > > I gather then that the suspend implementation is such that touching the > > watchdog periodically while suspended is not a problem. > > > > Again, can you please tell me how suspend is implemented on at91? > > > > It actually depends on the architecture (at91rm9200, at91sam9 or sama5) > but basically, the clocks are switched off in almost all the peripheral > drivers then the ram self refresh activated, the master clock is > switched off using code running from SRAM and the core is then waiting > for interrupt. OK, so it looks like enable_irq_wake() doesn't actually affect the hardware on those platforms, is that correct?
On 10/03/2015 at 23:31:52 +0100, Rafael J. Wysocki wrote : > On Tuesday, March 10, 2015 10:33:17 PM Alexandre Belloni wrote: > > Hi, > > > > On 09/03/2015 at 15:30:01 +0100, Rafael J. Wysocki wrote : > > > > > > Actaully, your platform should just refuse to enter suspend-to-RAM > > > > > > when hw watchdog is enabled. > > > > > > > > > > Quite likely, depending on how exactly the suspend is implemented. > > > > > > > > > > > > > We've had absolutely zero complain on that. It is quite clear in the > > > > datasheet that failing to refresh the watchdog once started will lead to > > > > a reset and that it is impossible to stop. > > > > It is actually quite convenient to also ensure that you can actually > > > > wake up from suspend because that can obviously go wrong. > > > > > > I gather then that the suspend implementation is such that touching the > > > watchdog periodically while suspended is not a problem. > > > > > > Again, can you please tell me how suspend is implemented on at91? > > > > > > > It actually depends on the architecture (at91rm9200, at91sam9 or sama5) > > but basically, the clocks are switched off in almost all the peripheral > > drivers then the ram self refresh activated, the master clock is > > switched off using code running from SRAM and the core is then waiting > > for interrupt. > > OK, so it looks like enable_irq_wake() doesn't actually affect the hardware > on those platforms, is that correct? > I didn't exactly look in details but apart from the wakeup from gpio handling (keeping the pio controller clocked in the case one of its gpio has wakeup enabled), I don't think it does much more. It uses irq_gc_set_wake().
On Tuesday, March 10, 2015 11:33:05 PM Alexandre Belloni wrote: > On 10/03/2015 at 23:31:52 +0100, Rafael J. Wysocki wrote : > > On Tuesday, March 10, 2015 10:33:17 PM Alexandre Belloni wrote: > > > Hi, > > > > > > On 09/03/2015 at 15:30:01 +0100, Rafael J. Wysocki wrote : > > > > > > > Actaully, your platform should just refuse to enter suspend-to-RAM > > > > > > > when hw watchdog is enabled. > > > > > > > > > > > > Quite likely, depending on how exactly the suspend is implemented. > > > > > > > > > > > > > > > > We've had absolutely zero complain on that. It is quite clear in the > > > > > datasheet that failing to refresh the watchdog once started will lead to > > > > > a reset and that it is impossible to stop. > > > > > It is actually quite convenient to also ensure that you can actually > > > > > wake up from suspend because that can obviously go wrong. > > > > > > > > I gather then that the suspend implementation is such that touching the > > > > watchdog periodically while suspended is not a problem. > > > > > > > > Again, can you please tell me how suspend is implemented on at91? > > > > > > > > > > It actually depends on the architecture (at91rm9200, at91sam9 or sama5) > > > but basically, the clocks are switched off in almost all the peripheral > > > drivers then the ram self refresh activated, the master clock is > > > switched off using code running from SRAM and the core is then waiting > > > for interrupt. > > > > OK, so it looks like enable_irq_wake() doesn't actually affect the hardware > > on those platforms, is that correct? > > > > I didn't exactly look in details but apart from the wakeup from gpio > handling (keeping the pio controller clocked in the case one of its gpio > has wakeup enabled), I don't think it does much more. It uses > irq_gc_set_wake(). Well, that only modifies gc->wake_active, so no hardware interactions.
Rafael, Alexandre, On Wed, 11 Mar 2015 02:03:08 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Tuesday, March 10, 2015 11:33:05 PM Alexandre Belloni wrote: > > On 10/03/2015 at 23:31:52 +0100, Rafael J. Wysocki wrote : > > > On Tuesday, March 10, 2015 10:33:17 PM Alexandre Belloni wrote: > > > > Hi, > > > > > > > > On 09/03/2015 at 15:30:01 +0100, Rafael J. Wysocki wrote : > > > > > > > > Actaully, your platform should just refuse to enter suspend-to-RAM > > > > > > > > when hw watchdog is enabled. > > > > > > > > > > > > > > Quite likely, depending on how exactly the suspend is implemented. > > > > > > > > > > > > > > > > > > > We've had absolutely zero complain on that. It is quite clear in the > > > > > > datasheet that failing to refresh the watchdog once started will lead to > > > > > > a reset and that it is impossible to stop. > > > > > > It is actually quite convenient to also ensure that you can actually > > > > > > wake up from suspend because that can obviously go wrong. > > > > > > > > > > I gather then that the suspend implementation is such that touching the > > > > > watchdog periodically while suspended is not a problem. > > > > > > > > > > Again, can you please tell me how suspend is implemented on at91? > > > > > > > > > > > > > It actually depends on the architecture (at91rm9200, at91sam9 or sama5) > > > > but basically, the clocks are switched off in almost all the peripheral > > > > drivers then the ram self refresh activated, the master clock is > > > > switched off using code running from SRAM and the core is then waiting > > > > for interrupt. > > > > > > OK, so it looks like enable_irq_wake() doesn't actually affect the hardware > > > on those platforms, is that correct? > > > > > > > I didn't exactly look in details but apart from the wakeup from gpio > > handling (keeping the pio controller clocked in the case one of its gpio > > has wakeup enabled), I don't think it does much more. It uses > > irq_gc_set_wake(). > > Well, that only modifies gc->wake_active, so no hardware interactions. I'm not sure I understand the whole discussion, but calling enable_irq_wake() does affect suspend behavior on at91 platforms. Take a look at the suspend() implementation [1], it's making use of the wake_active field (modified by irq_gc_set_wake) when entering suspend in order to keep wakeup IRQ sources enabled. Best Regards, Boris [1]http://lxr.free-electrons.com/source/drivers/irqchip/irq-atmel-aic.c#L106
On Sun, 08 Mar 2015 02:11:45 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Saturday, March 07, 2015 12:06:45 PM Alexandre Belloni wrote: > > On 07/03/2015 at 11:39:39 +0100, Pavel Machek wrote : > > > > The Atmel watchdog can't be stopped once it's started. This is actually > > > > very useful so we can reset if suspend or resume failed, the only > > > > drawback is that you have to wake up from time to time (e.g. by using > > > > the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. > > > > > > Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes > > > after watchdog kills the system. But you did not ask for dead system, > > > you asked for suspend. > > > > > > And while that behaviour is useful for you, I don't think it is > > > exactly useful behaviour, nor it is the behaviour user would expect. > > > > > > > I think you misunderstood, that is exactly the expected behaviour. This > > is hardware defined. Once the watchdog is started, nobody can stop it. > > Trying to change the mode register will result in a reset of the SoC. > > > > It is documented in the datasheet and any user wanting another behaviour > > is out of luck. > > > > So basically, when using a watchdog, you have to wake up every 15-16s to > > restart it. > > So question is if we need a separate interrupt handler for that, expecially > since it is shared with the PIT timer anyway. > > Seems to me that the simplest way out of this conundrum would be to simply > make the timer's interrupt handler kick the watchdog every once a while and > get rid of the separate watchdog interrupt handler entirely. The watchdog interrupt handler is not here to ping the watchdog, it's here to reset the platform if the watchdog hasn't been refreshed appropriately. IOW, it's a software watchdog using at91 WDT capabilities to determine when it should reboot the system. IIRC, we need this on some at91 platforms to fix a HW bug (maybe Nicolas can confirm this).
Le 11/03/2015 09:38, Boris Brezillon a écrit : > On Sun, 08 Mar 2015 02:11:45 +0100 > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > >> On Saturday, March 07, 2015 12:06:45 PM Alexandre Belloni wrote: >>> On 07/03/2015 at 11:39:39 +0100, Pavel Machek wrote : >>>>> The Atmel watchdog can't be stopped once it's started. This is actually >>>>> very useful so we can reset if suspend or resume failed, the only >>>>> drawback is that you have to wake up from time to time (e.g. by using >>>>> the RTC/RTT) to clear the watchdog and then go back to sleep ASAP. >>>> >>>> Yeah. So you do "echo mem > /sys/power/state", and few seconds/minutes >>>> after watchdog kills the system. But you did not ask for dead system, >>>> you asked for suspend. >>>> >>>> And while that behaviour is useful for you, I don't think it is >>>> exactly useful behaviour, nor it is the behaviour user would expect. >>>> >>> >>> I think you misunderstood, that is exactly the expected behaviour. This >>> is hardware defined. Once the watchdog is started, nobody can stop it. >>> Trying to change the mode register will result in a reset of the SoC. >>> >>> It is documented in the datasheet and any user wanting another behaviour >>> is out of luck. >>> >>> So basically, when using a watchdog, you have to wake up every 15-16s to >>> restart it. >> >> So question is if we need a separate interrupt handler for that, expecially >> since it is shared with the PIT timer anyway. >> >> Seems to me that the simplest way out of this conundrum would be to simply >> make the timer's interrupt handler kick the watchdog every once a while and >> get rid of the separate watchdog interrupt handler entirely. > > The watchdog interrupt handler is not here to ping the watchdog, it's > here to reset the platform if the watchdog hasn't been refreshed > appropriately. > > IOW, it's a software watchdog using at91 WDT capabilities to determine > when it should reboot the system. > IIRC, we need this on some at91 platforms to fix a HW bug (maybe > Nicolas can confirm this). Yes, the HW bug that we address in these functions: at91sam9260_restart() and at91sam9g45_restart(). We have this issue because of NAND flash lines shared with DDR that are driven during product reboot on old products (Cf. these functions comments). This bug would kick-in when doing "software reset"/"watchdog reset"/"push button reset". Only the "software reset" is handled by the functions above. So, yes, this "software watchdog" is there for this purpose IIRC. Bye,
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c index 6df9405..1443b3c 100644 --- a/drivers/watchdog/at91sam9_wdt.c +++ b/drivers/watchdog/at91sam9_wdt.c @@ -208,7 +208,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt) if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) { err = request_irq(wdt->irq, wdt_interrupt, - IRQF_SHARED | IRQF_IRQPOLL, + IRQF_SHARED | IRQF_IRQPOLL | + IRQF_NO_SUSPEND, pdev->name, wdt); if (err) return err;
The watchdog interrupt (only used when activating software watchdog) shouldn't be suspended when entering suspend mode, because it is shared with a timer device (which request the line with IRQF_NO_SUSPEND) and once the watchdog "Mode Register" has been written, it cannot be changed (which means we cannot disable the watchdog interrupt when entering suspend). Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/watchdog/at91sam9_wdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)