[PATCH v2 0/6] ARM: at91: fix irq_pm_install_action WARNING

[PATCH v2 0/6] ARM: at91: fix irq_pm_install_action WARNING

[Boris Brezillon]

My apologies to those of you who already received this series, but I
didn't increment the patch version and forgot some subsystem maintainers
and MLs.

Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
test which triggers a WARNING backtrace on at91 platforms.
While this WARN_ON is absolutely necessary to warn users that they should
not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
there is no easy way to solve this issue on at91 platforms.

The main reason is that the init timer is often using a shared irq line
and thus request this irq with IRQF_NO_SUSPEND flag set, while other
peripherals request the same irq line without this flag.

This problem has recently been addressed by this patch [2] which adds
a new IRQF_COND_SUSPEND flag, that authorize mixing IRQF_NO_SUSPEND and
!IRQF_NO_SUSPEND as long as irq handlers setting IRQF_COND_SUSPEND
can safely be called in suspended state.

Doing this also implies taking care of system wakeup in devices handlers
if they tag the IRQ line as a wakeup source.
The first patch of this series exports the pm_system_wakeup symbol so
that drivers can call pm_system_wakeup from their interrupt handler.

This series then patches all at91 drivers that can have devices sharing
their IRQ line with a timer.

This series depends on [2].

Best Regards,

Boris

[1]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/irq/pm.c?id=cab303be91dc47942bc25de33dc1140123540800
[2]https://lkml.org/lkml/2015/2/26/675

Changes since v1:
 - replaced the IRQF_SUSPEND_NOACTION flag by IRQF_COND_SUSPEND
 - properly addressed wakeup handling in drivers

Boris Brezillon (6):
  PM / wakeup: export pm_system_wakeup symbol
  rtc: at91sam9: rework wakeup and interrupt handling
  rtc: at91rm9200: rework wakeup and interrupt handling
  clk: at91: implement suspend/resume for the PMC irqchip
  watchdog: at91sam9: request the irq with IRQF_NO_SUSPEND
  tty: serial: atmel: rework interrupt and wakeup handling

 drivers/base/power/wakeup.c       |  1 +
 drivers/clk/at91/pmc.c            | 20 ++++++++++-
 drivers/clk/at91/pmc.h            |  1 +
 drivers/rtc/rtc-at91rm9200.c      | 62 +++++++++++++++++++++++++--------
 drivers/rtc/rtc-at91sam9.c        | 73 ++++++++++++++++++++++++++++++++-------
 drivers/tty/serial/atmel_serial.c | 49 +++++++++++++++++++++++---
 drivers/watchdog/at91sam9_wdt.c   |  3 +-
 7 files changed, 177 insertions(+), 32 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/6] PM / wakeup: export pm_system_wakeup symbol

[Boris Brezillon]

Export pm_system_wakeup function to allow irq handlers to deal with system
wakeup.
This is needed for shared IRQ lines where one of the handler is registered
with IRQF_NO_SUSPEND, while the other ones want to configure it as a wakeup
source.
In this specific case, irq core does not handle the wakeup process and
leave the decision to each irq handler.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/base/power/wakeup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index c2744b3..aab7158 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -730,6 +730,7 @@ void pm_system_wakeup(void)
 	pm_abort_suspend = true;
 	freeze_wake();
 }
+EXPORT_SYMBOL_GPL(pm_system_wakeup);
 
 void pm_wakeup_clear(void)
 {
-- 
1.9.1

[PATCH v2 3/6] rtc: at91rm9200: rework wakeup and interrupt handling

[Boris Brezillon]

The IRQ line used by the RTC device is usually shared with the system
timer (PIT) on at91 platforms.
Since timers are registering their handlers with IRQF_NO_SUSPEND, we
should expect being called in suspended state, and properly wake the
system up when this is the case.

Set IRQF_COND_SUSPEND flag when registering the IRQ handler to inform
irq core that it can safely be called while the system is suspended.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/rtc/rtc-at91rm9200.c | 62 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 70a5d94..b4f7744 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -31,6 +31,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/suspend.h>
 #include <linux/uaccess.h>
 
 #include "rtc-at91rm9200.h"
@@ -54,6 +55,10 @@ static void __iomem *at91_rtc_regs;
 static int irq;
 static DEFINE_SPINLOCK(at91_rtc_lock);
 static u32 at91_rtc_shadow_imr;
+static bool suspended;
+static DEFINE_SPINLOCK(suspended_lock);
+static unsigned long cached_events;
+static u32 at91_rtc_imr;
 
 static void at91_rtc_write_ier(u32 mask)
 {
@@ -290,7 +295,9 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
 	struct rtc_device *rtc = platform_get_drvdata(pdev);
 	unsigned int rtsr;
 	unsigned long events = 0;
+	int ret = IRQ_NONE;
 
+	spin_lock(&suspended_lock);
 	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_read_imr();
 	if (rtsr) {		/* this interrupt is shared!  Is it ours? */
 		if (rtsr & AT91_RTC_ALARM)
@@ -304,14 +311,22 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
 
 		at91_rtc_write(AT91_RTC_SCCR, rtsr);	/* clear status reg */
 
-		rtc_update_irq(rtc, 1, events);
+		if (!suspended) {
+			rtc_update_irq(rtc, 1, events);
 
-		dev_dbg(&pdev->dev, "%s(): num=%ld, events=0x%02lx\n", __func__,
-			events >> 8, events & 0x000000FF);
+			dev_dbg(&pdev->dev, "%s(): num=%ld, events=0x%02lx\n",
+				__func__, events >> 8, events & 0x000000FF);
+		} else {
+			cached_events |= events;
+			at91_rtc_write_idr(at91_rtc_imr);
+			pm_system_wakeup();
+		}
 
-		return IRQ_HANDLED;
+		ret = IRQ_HANDLED;
 	}
-	return IRQ_NONE;		/* not handled */
+	spin_lock(&suspended_lock);
+
+	return ret;
 }
 
 static const struct at91_rtc_config at91rm9200_config = {
@@ -401,8 +416,8 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 					AT91_RTC_CALEV);
 
 	ret = devm_request_irq(&pdev->dev, irq, at91_rtc_interrupt,
-				IRQF_SHARED,
-				"at91_rtc", pdev);
+			       IRQF_SHARED | IRQF_COND_SUSPEND,
+			       "at91_rtc", pdev);
 	if (ret) {
 		dev_err(&pdev->dev, "IRQ %d already in use.\n", irq);
 		return ret;
@@ -454,8 +469,6 @@ static void at91_rtc_shutdown(struct platform_device *pdev)
 
 /* AT91RM9200 RTC Power management control */
 
-static u32 at91_rtc_imr;
-
 static int at91_rtc_suspend(struct device *dev)
 {
 	/* this IRQ is shared with DBGU and other hardware which isn't
@@ -464,21 +477,42 @@ static int at91_rtc_suspend(struct device *dev)
 	at91_rtc_imr = at91_rtc_read_imr()
 			& (AT91_RTC_ALARM|AT91_RTC_SECEV);
 	if (at91_rtc_imr) {
-		if (device_may_wakeup(dev))
+		if (device_may_wakeup(dev)) {
+			unsigned long flags;
+
 			enable_irq_wake(irq);
-		else
+
+			spin_lock_irqsave(&suspended_lock, flags);
+			suspended = true;
+			spin_unlock_irqrestore(&suspended_lock, flags);
+		} else {
 			at91_rtc_write_idr(at91_rtc_imr);
+		}
 	}
 	return 0;
 }
 
 static int at91_rtc_resume(struct device *dev)
 {
+	struct rtc_device *rtc = dev_get_drvdata(dev);
+
 	if (at91_rtc_imr) {
-		if (device_may_wakeup(dev))
+		if (device_may_wakeup(dev)) {
+			unsigned long flags;
+
+			spin_lock_irqsave(&suspended_lock, flags);
+
+			if (cached_events) {
+				rtc_update_irq(rtc, 1, cached_events);
+				cached_events = 0;
+			}
+
+			suspended = false;
+			spin_unlock_irqrestore(&suspended_lock, flags);
+
 			disable_irq_wake(irq);
-		else
-			at91_rtc_write_ier(at91_rtc_imr);
+		}
+		at91_rtc_write_ier(at91_rtc_imr);
 	}
 	return 0;
 }
-- 
1.9.1

[PATCH v2 4/6] clk: at91: implement suspend/resume for the PMC irqchip

[Boris Brezillon]

The irq line used by the PMC block is shared with several peripherals
including the init timer which is registering its handler with
IRQF_NO_SUSPEND.
Implement the appropriate suspend/resume callback for the PMC irqchip,
and inform irq core that PMC irq handler can be safely called while
the system is suspended by setting IRQF_COND_SUSPEND.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/clk/at91/pmc.c | 20 +++++++++++++++++++-
 drivers/clk/at91/pmc.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index f07c815..3f27d21 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -89,12 +89,29 @@ static int pmc_irq_set_type(struct irq_data *d, unsigned type)
 	return 0;
 }
 
+static void pmc_irq_suspend(struct irq_data *d)
+{
+	struct at91_pmc *pmc = irq_data_get_irq_chip_data(d);
+
+	pmc->imr = pmc_read(pmc, AT91_PMC_IMR);
+	pmc_write(pmc, AT91_PMC_IDR, pmc->imr);
+}
+
+static void pmc_irq_resume(struct irq_data *d)
+{
+	struct at91_pmc *pmc = irq_data_get_irq_chip_data(d);
+
+	pmc_write(pmc, AT91_PMC_IER, pmc->imr);
+}
+
 static struct irq_chip pmc_irq = {
 	.name = "PMC",
 	.irq_disable = pmc_irq_mask,
 	.irq_mask = pmc_irq_mask,
 	.irq_unmask = pmc_irq_unmask,
 	.irq_set_type = pmc_irq_set_type,
+	.irq_suspend = pmc_irq_suspend,
+	.irq_resume = pmc_irq_resume,
 };
 
 static struct lock_class_key pmc_lock_class;
@@ -224,7 +241,8 @@ static struct at91_pmc *__init at91_pmc_init(struct device_node *np,
 		goto out_free_pmc;
 
 	pmc_write(pmc, AT91_PMC_IDR, 0xffffffff);
-	if (request_irq(pmc->virq, pmc_irq_handler, IRQF_SHARED, "pmc", pmc))
+	if (request_irq(pmc->virq, pmc_irq_handler,
+			IRQF_SHARED | IRQF_COND_SUSPEND, "pmc", pmc))
 		goto out_remove_irqdomain;
 
 	return pmc;
diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
index 52d2041..69abb08 100644
--- a/drivers/clk/at91/pmc.h
+++ b/drivers/clk/at91/pmc.h
@@ -33,6 +33,7 @@ struct at91_pmc {
 	spinlock_t lock;
 	const struct at91_pmc_caps *caps;
 	struct irq_domain *irqdomain;
+	u32 imr;
 };
 
 static inline void pmc_lock(struct at91_pmc *pmc)
-- 
1.9.1

Re: [PATCH v2 4/6] clk: at91: implement suspend/resume for the PMC irqchip

[Mike Turquette]

Quoting Boris Brezillon (2015-03-02 01:18:16)
> The irq line used by the PMC block is shared with several peripherals
> including the init timer which is registering its handler with
> IRQF_NO_SUSPEND.
> Implement the appropriate suspend/resume callback for the PMC irqchip,
> and inform irq core that PMC irq handler can be safely called while
> the system is suspended by setting IRQF_COND_SUSPEND.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Looks OK to me. I'm not picking it due to the dependency you listed in
the cover letter.

Regards,
Mike

> ---
>  drivers/clk/at91/pmc.c | 20 +++++++++++++++++++-
>  drivers/clk/at91/pmc.h |  1 +
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index f07c815..3f27d21 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -89,12 +89,29 @@ static int pmc_irq_set_type(struct irq_data *d, unsigned type)
>         return 0;
>  }
>  
> +static void pmc_irq_suspend(struct irq_data *d)
> +{
> +       struct at91_pmc *pmc = irq_data_get_irq_chip_data(d);
> +
> +       pmc->imr = pmc_read(pmc, AT91_PMC_IMR);
> +       pmc_write(pmc, AT91_PMC_IDR, pmc->imr);
> +}
> +
> +static void pmc_irq_resume(struct irq_data *d)
> +{
> +       struct at91_pmc *pmc = irq_data_get_irq_chip_data(d);
> +
> +       pmc_write(pmc, AT91_PMC_IER, pmc->imr);
> +}
> +
>  static struct irq_chip pmc_irq = {
>         .name = "PMC",
>         .irq_disable = pmc_irq_mask,
>         .irq_mask = pmc_irq_mask,
>         .irq_unmask = pmc_irq_unmask,
>         .irq_set_type = pmc_irq_set_type,
> +       .irq_suspend = pmc_irq_suspend,
> +       .irq_resume = pmc_irq_resume,
>  };
>  
>  static struct lock_class_key pmc_lock_class;
> @@ -224,7 +241,8 @@ static struct at91_pmc *__init at91_pmc_init(struct device_node *np,
>                 goto out_free_pmc;
>  
>         pmc_write(pmc, AT91_PMC_IDR, 0xffffffff);
> -       if (request_irq(pmc->virq, pmc_irq_handler, IRQF_SHARED, "pmc", pmc))
> +       if (request_irq(pmc->virq, pmc_irq_handler,
> +                       IRQF_SHARED | IRQF_COND_SUSPEND, "pmc", pmc))
>                 goto out_remove_irqdomain;
>  
>         return pmc;
> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
> index 52d2041..69abb08 100644
> --- a/drivers/clk/at91/pmc.h
> +++ b/drivers/clk/at91/pmc.h
> @@ -33,6 +33,7 @@ struct at91_pmc {
>         spinlock_t lock;
>         const struct at91_pmc_caps *caps;
>         struct irq_domain *irqdomain;
> +       u32 imr;
>  };
>  
>  static inline void pmc_lock(struct at91_pmc *pmc)
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/6] tty: serial: atmel: rework interrupt and wakeup handling

[Boris Brezillon]

The IRQ line connected to the DBGU UART is often shared with a timer device
which request the IRQ with IRQF_NO_SUSPEND.
Since the UART driver is correctly disabling IRQs when entering suspend
we can safely request the IRQ with IRQF_COND_SUSPEND so that irq core
will not complain about mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND.

Rework the interrupt handler to wake the system up when an interrupt
happens on the DEBUG_UART while the system is suspended.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/tty/serial/atmel_serial.c | 49 +++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 846552b..4e959c4 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -47,6 +47,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/err.h>
 #include <linux/irq.h>
+#include <linux/suspend.h>
 
 #include <asm/io.h>
 #include <asm/ioctls.h>
@@ -173,6 +174,12 @@ struct atmel_uart_port {
 	bool			ms_irq_enabled;
 	bool			is_usart;	/* usart or uart */
 	struct timer_list	uart_timer;	/* uart timer */
+
+	bool			suspended;
+	unsigned int		pending;
+	unsigned int		pending_status;
+	spinlock_t		lock_suspended;
+
 	int (*prepare_rx)(struct uart_port *port);
 	int (*prepare_tx)(struct uart_port *port);
 	void (*schedule_rx)(struct uart_port *port);
@@ -1179,12 +1186,15 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
 {
 	struct uart_port *port = dev_id;
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
-	unsigned int status, pending, pass_counter = 0;
+	unsigned int status, pending, mask, pass_counter = 0;
 	bool gpio_handled = false;
 
+	spin_lock(&atmel_port->lock_suspended);
+
 	do {
 		status = atmel_get_lines_status(port);
-		pending = status & UART_GET_IMR(port);
+		mask = UART_GET_IMR(port);
+		pending = status & mask;
 		if (!gpio_handled) {
 			/*
 			 * Dealing with GPIO interrupt
@@ -1206,11 +1216,21 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
 		if (!pending)
 			break;
 
+		if (atmel_port->suspended) {
+			atmel_port->pending |= pending;
+			atmel_port->pending_status = status;
+			UART_PUT_IDR(port, mask);
+			pm_system_wakeup();
+			break;
+		}
+
 		atmel_handle_receive(port, pending);
 		atmel_handle_status(port, pending, status);
 		atmel_handle_transmit(port, pending);
 	} while (pass_counter++ < ATMEL_ISR_PASS_LIMIT);
 
+	spin_unlock(&atmel_port->lock_suspended);
+
 	return pass_counter ? IRQ_HANDLED : IRQ_NONE;
 }
 
@@ -1742,7 +1762,8 @@ static int atmel_startup(struct uart_port *port)
 	/*
 	 * Allocate the IRQ
 	 */
-	retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
+	retval = request_irq(port->irq, atmel_interrupt,
+			IRQF_SHARED | IRQF_COND_SUSPEND,
 			tty ? tty->name : "atmel_serial", port);
 	if (retval) {
 		dev_err(port->dev, "atmel_startup - Can't get irq\n");
@@ -2513,8 +2534,14 @@ static int atmel_serial_suspend(struct platform_device *pdev,
 
 	/* we can not wake up if we're running on slow clock */
 	atmel_port->may_wakeup = device_may_wakeup(&pdev->dev);
-	if (atmel_serial_clk_will_stop())
+	if (atmel_serial_clk_will_stop()) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&atmel_port->lock_suspended, flags);
+		atmel_port->suspended = true;
+		spin_unlock_irqrestore(&atmel_port->lock_suspended, flags);
 		device_set_wakeup_enable(&pdev->dev, 0);
+	}
 
 	uart_suspend_port(&atmel_uart, port);
 
@@ -2525,6 +2552,18 @@ static int atmel_serial_resume(struct platform_device *pdev)
 {
 	struct uart_port *port = platform_get_drvdata(pdev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&atmel_port->lock_suspended, flags);
+	if (atmel_port->pending) {
+		atmel_handle_receive(port, atmel_port->pending);
+		atmel_handle_status(port, atmel_port->pending,
+				    atmel_port->pending_status);
+		atmel_handle_transmit(port, atmel_port->pending);
+		atmel_port->pending = 0;
+	}
+	atmel_port->suspended = false;
+	spin_unlock_irqrestore(&atmel_port->lock_suspended, flags);
 
 	uart_resume_port(&atmel_uart, port);
 	device_set_wakeup_enable(&pdev->dev, atmel_port->may_wakeup);
@@ -2593,6 +2632,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
 	port->backup_imr = 0;
 	port->uart.line = ret;
 
+	spin_lock_init(&port->lock_suspended);
+
 	ret = atmel_init_gpios(port, &pdev->dev);
 	if (ret < 0)
 		dev_err(&pdev->dev, "%s",
-- 
1.9.1

[PATCH v2 2/6] rtc: at91sam9: rework wakeup and interrupt handling

[Boris Brezillon]

The IRQ line used by the RTC device is usually shared with the system timer
(PIT) on at91 platforms.
Since timers are registering their handlers with IRQF_NO_SUSPEND, we should
expect being called in suspended state, and properly wake the system up
when this is the case.

Set IRQF_COND_SUSPEND flag when registering the IRQ handler to inform
irq core that it can safely be called while the system is suspended.

Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/rtc/rtc-at91sam9.c | 73 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 61 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 2183fd2..5ccaee3 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/suspend.h>
 #include <linux/clk.h>
 
 /*
@@ -77,6 +78,9 @@ struct sam9_rtc {
 	unsigned int		gpbr_offset;
 	int 			irq;
 	struct clk		*sclk;
+	bool			suspended;
+	unsigned long		events;
+	spinlock_t		lock;
 };
 
 #define rtt_readl(rtc, field) \
@@ -271,14 +275,9 @@ static int at91_rtc_proc(struct device *dev, struct seq_file *seq)
 	return 0;
 }
 
-/*
- * IRQ handler for the RTC
- */
-static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc)
+static irqreturn_t at91_rtc_cache_events(struct sam9_rtc *rtc)
 {
-	struct sam9_rtc *rtc = _rtc;
 	u32 sr, mr;
-	unsigned long events = 0;
 
 	/* Shared interrupt may be for another device.  Note: reading
 	 * SR clears it, so we must only read it in this irq handler!
@@ -290,18 +289,54 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc)
 
 	/* alarm status */
 	if (sr & AT91_RTT_ALMS)
-		events |= (RTC_AF | RTC_IRQF);
+		rtc->events |= (RTC_AF | RTC_IRQF);
 
 	/* timer update/increment */
 	if (sr & AT91_RTT_RTTINC)
-		events |= (RTC_UF | RTC_IRQF);
+		rtc->events |= (RTC_UF | RTC_IRQF);
+
+	return IRQ_HANDLED;
+}
+
+static void at91_rtc_flush_events(struct sam9_rtc *rtc)
+{
+	if (!rtc->events)
+		return;
 
-	rtc_update_irq(rtc->rtcdev, 1, events);
+	rtc_update_irq(rtc->rtcdev, 1, rtc->events);
+	rtc->events = 0;
 
 	pr_debug("%s: num=%ld, events=0x%02lx\n", __func__,
-		events >> 8, events & 0x000000FF);
+		rtc->events >> 8, rtc->events & 0x000000FF);
+}
 
-	return IRQ_HANDLED;
+/*
+ * IRQ handler for the RTC
+ */
+static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc)
+{
+	struct sam9_rtc *rtc = _rtc;
+	int ret;
+
+	spin_lock(&rtc->lock);
+
+	ret = at91_rtc_cache_events(rtc);
+
+	/* We're called in suspended state */
+	if (rtc->suspended) {
+		/* Mask irqs coming from this peripheral */
+		rtt_writel(rtc, MR,
+			   rtt_readl(rtc, MR) &
+			   ~(AT91_RTT_ALMIEN | AT91_RTT_RTTINCIEN));
+		/* Trigger a system wakeup */
+		pm_system_wakeup();
+	} else {
+		at91_rtc_flush_events(rtc);
+	}
+
+	spin_unlock(&rtc->lock);
+
+	return ret;
 }
 
 static const struct rtc_class_ops at91_rtc_ops = {
@@ -421,7 +456,8 @@ static int at91_rtc_probe(struct platform_device *pdev)
 
 	/* register irq handler after we know what name we'll use */
 	ret = devm_request_irq(&pdev->dev, rtc->irq, at91_rtc_interrupt,
-				IRQF_SHARED, dev_name(&rtc->rtcdev->dev), rtc);
+			       IRQF_SHARED | IRQF_COND_SUSPEND,
+			       dev_name(&rtc->rtcdev->dev), rtc);
 	if (ret) {
 		dev_dbg(&pdev->dev, "can't share IRQ %d?\n", rtc->irq);
 		return ret;
@@ -482,7 +518,12 @@ static int at91_rtc_suspend(struct device *dev)
 	rtc->imr = mr & (AT91_RTT_ALMIEN | AT91_RTT_RTTINCIEN);
 	if (rtc->imr) {
 		if (device_may_wakeup(dev) && (mr & AT91_RTT_ALMIEN)) {
+			unsigned long flags;
+
 			enable_irq_wake(rtc->irq);
+			spin_lock_irqsave(&rtc->lock, flags);
+			rtc->suspended = true;
+			spin_unlock_irqrestore(&rtc->lock, flags);
 			/* don't let RTTINC cause wakeups */
 			if (mr & AT91_RTT_RTTINCIEN)
 				rtt_writel(rtc, MR, mr & ~AT91_RTT_RTTINCIEN);
@@ -499,10 +540,18 @@ static int at91_rtc_resume(struct device *dev)
 	u32		mr;
 
 	if (rtc->imr) {
+		unsigned long flags;
+
 		if (device_may_wakeup(dev))
 			disable_irq_wake(rtc->irq);
 		mr = rtt_readl(rtc, MR);
 		rtt_writel(rtc, MR, mr | rtc->imr);
+
+		spin_lock_irqsave(&rtc->lock, flags);
+		rtc->suspended = false;
+		at91_rtc_cache_events(rtc);
+		at91_rtc_flush_events(rtc);
+		spin_unlock_irqrestore(&rtc->lock, flags);
 	}
 
 	return 0;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/6] rtc: at91sam9: rework wakeup and interrupt handling

[Mark Rutland]

Hi Boris,

On Mon, Mar 02, 2015 at 09:18:14AM +0000, Boris Brezillon wrote:
> The IRQ line used by the RTC device is usually shared with the system timer
> (PIT) on at91 platforms.
> Since timers are registering their handlers with IRQF_NO_SUSPEND, we should
> expect being called in suspended state, and properly wake the system up
> when this is the case.
> 
> Set IRQF_COND_SUSPEND flag when registering the IRQ handler to inform
> irq core that it can safely be called while the system is suspended.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/rtc/rtc-at91sam9.c | 73 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 61 insertions(+), 12 deletions(-)

[...]

> +/*
> + * IRQ handler for the RTC
> + */
> +static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc)
> +{
> +	struct sam9_rtc *rtc = _rtc;
> +	int ret;
> +
> +	spin_lock(&rtc->lock);
> +
> +	ret = at91_rtc_cache_events(rtc);
> +
> +	/* We're called in suspended state */
> +	if (rtc->suspended) {
> +		/* Mask irqs coming from this peripheral */
> +		rtt_writel(rtc, MR,
> +			   rtt_readl(rtc, MR) &
> +			   ~(AT91_RTT_ALMIEN | AT91_RTT_RTTINCIEN));
> +		/* Trigger a system wakeup */
> +		pm_system_wakeup();
> +	} else {
> +		at91_rtc_flush_events(rtc);
> +	}
> +
> +	spin_unlock(&rtc->lock);
> +
> +	return ret;

[...]

> @@ -421,7 +456,8 @@ static int at91_rtc_probe(struct platform_device *pdev)
>  
>  	/* register irq handler after we know what name we'll use */
>  	ret = devm_request_irq(&pdev->dev, rtc->irq, at91_rtc_interrupt,
> -				IRQF_SHARED, dev_name(&rtc->rtcdev->dev), rtc);
> +			       IRQF_SHARED | IRQF_COND_SUSPEND,
> +			       dev_name(&rtc->rtcdev->dev), rtc);

To try to avoid thie getting cargo-culted, it's probably worth expanding
the comment to cover the IRQF_COND_SUSPEND usage. Something like:

	/* 
	 * Register irq handler after we know what name we'll use.
	 *
	 * The interrupt line may be shared with a timer (the PIT), so
	 * we must use IRQF_COND_SUSPEND and call pm_system_wakeup()
	 * when a genuine wakeup event occurs.
	 */

Otherwise this looks good to me.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Boris Brezillon]

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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 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;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

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;
>

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

[Mark Rutland]

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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Mark Rutland]

[...]

> > >  		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.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

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

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

[Mark Rutland]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

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

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.

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

[Rafael J. Wysocki]

On Fri, Mar 6, 2015 at 12:06 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

> >> > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Peter Zijlstra]

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.

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

[Peter Zijlstra]

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.

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

[Boris Brezillon]

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 ?


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Mark Rutland]

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.

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

[Boris Brezillon]

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


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris Brezillon]

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



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [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.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Peter Zijlstra]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sylvain Rochet]

[-- Attachment #1: Type: text/plain, Size: 2058 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Sylvain Rochet]

[-- Attachment #1.1: Type: text/plain, Size: 2526 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sylvain Rochet]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Rafael J. Wysocki]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Rafael J. Wysocki]

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?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Alexandre Belloni]

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.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

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 speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Alexandre Belloni]

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

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

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 speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Boris Brezillon]

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Rafael J. Wysocki]

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.



-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Boris Brezillon]

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


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Nicolas Ferre]

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,
-- 
Nicolas Ferre

Re: [PATCH v2 0/6] ARM: at91: fix irq_pm_install_action WARNING

[Alexandre Belloni]

On 02/03/2015 at 10:18:12 +0100, Boris Brezillon wrote :
> My apologies to those of you who already received this series, but I
> didn't increment the patch version and forgot some subsystem maintainers
> and MLs.
> 
> Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
> test which triggers a WARNING backtrace on at91 platforms.
> While this WARN_ON is absolutely necessary to warn users that they should
> not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
> there is no easy way to solve this issue on at91 platforms.
> 
> The main reason is that the init timer is often using a shared irq line
> and thus request this irq with IRQF_NO_SUSPEND flag set, while other
> peripherals request the same irq line without this flag.
> 
> This problem has recently been addressed by this patch [2] which adds
> a new IRQF_COND_SUSPEND flag, that authorize mixing IRQF_NO_SUSPEND and
> !IRQF_NO_SUSPEND as long as irq handlers setting IRQF_COND_SUSPEND
> can safely be called in suspended state.
> 
> Doing this also implies taking care of system wakeup in devices handlers
> if they tag the IRQ line as a wakeup source.
> The first patch of this series exports the pm_system_wakeup symbol so
> that drivers can call pm_system_wakeup from their interrupt handler.
> 
> This series then patches all at91 drivers that can have devices sharing
> their IRQ line with a timer.
> 
> This series depends on [2].
> 

For the whole series:
Reviewed-by: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/6] ARM: at91: fix irq_pm_install_action WARNING

[Nicolas Ferre]

Le 02/03/2015 10:18, Boris Brezillon a écrit :
> My apologies to those of you who already received this series, but I
> didn't increment the patch version and forgot some subsystem maintainers
> and MLs.
> 
> Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
> test which triggers a WARNING backtrace on at91 platforms.
> While this WARN_ON is absolutely necessary to warn users that they should
> not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
> there is no easy way to solve this issue on at91 platforms.
> 
> The main reason is that the init timer is often using a shared irq line
> and thus request this irq with IRQF_NO_SUSPEND flag set, while other
> peripherals request the same irq line without this flag.
> 
> This problem has recently been addressed by this patch [2] which adds
> a new IRQF_COND_SUSPEND flag, that authorize mixing IRQF_NO_SUSPEND and
> !IRQF_NO_SUSPEND as long as irq handlers setting IRQF_COND_SUSPEND
> can safely be called in suspended state.
> 
> Doing this also implies taking care of system wakeup in devices handlers
> if they tag the IRQ line as a wakeup source.
> The first patch of this series exports the pm_system_wakeup symbol so
> that drivers can call pm_system_wakeup from their interrupt handler.
> 
> This series then patches all at91 drivers that can have devices sharing
> their IRQ line with a timer.
> 
> This series depends on [2].

I'm okay with all the patches:
Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

As it seems easier to keep the whole series together, I'll let Rafael
take it.

Thanks a lot for having taking care of this.

Best regards,


> [1]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/irq/pm.c?id=cab303be91dc47942bc25de33dc1140123540800
> [2]https://lkml.org/lkml/2015/2/26/675
> 
> Changes since v1:
>  - replaced the IRQF_SUSPEND_NOACTION flag by IRQF_COND_SUSPEND
>  - properly addressed wakeup handling in drivers
> 
> Boris Brezillon (6):
>   PM / wakeup: export pm_system_wakeup symbol
>   rtc: at91sam9: rework wakeup and interrupt handling
>   rtc: at91rm9200: rework wakeup and interrupt handling
>   clk: at91: implement suspend/resume for the PMC irqchip
>   watchdog: at91sam9: request the irq with IRQF_NO_SUSPEND
>   tty: serial: atmel: rework interrupt and wakeup handling
> 
>  drivers/base/power/wakeup.c       |  1 +
>  drivers/clk/at91/pmc.c            | 20 ++++++++++-
>  drivers/clk/at91/pmc.h            |  1 +
>  drivers/rtc/rtc-at91rm9200.c      | 62 +++++++++++++++++++++++++--------
>  drivers/rtc/rtc-at91sam9.c        | 73 ++++++++++++++++++++++++++++++++-------
>  drivers/tty/serial/atmel_serial.c | 49 +++++++++++++++++++++++---
>  drivers/watchdog/at91sam9_wdt.c   |  3 +-
>  7 files changed, 177 insertions(+), 32 deletions(-)
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/6] ARM: at91: fix irq_pm_install_action WARNING

[Rafael J. Wysocki]

On Tuesday, March 03, 2015 04:35:40 PM Nicolas Ferre wrote:
> Le 02/03/2015 10:18, Boris Brezillon a écrit :
> > My apologies to those of you who already received this series, but I
> > didn't increment the patch version and forgot some subsystem maintainers
> > and MLs.
> > 
> > Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
> > test which triggers a WARNING backtrace on at91 platforms.
> > While this WARN_ON is absolutely necessary to warn users that they should
> > not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
> > there is no easy way to solve this issue on at91 platforms.
> > 
> > The main reason is that the init timer is often using a shared irq line
> > and thus request this irq with IRQF_NO_SUSPEND flag set, while other
> > peripherals request the same irq line without this flag.
> > 
> > This problem has recently been addressed by this patch [2] which adds
> > a new IRQF_COND_SUSPEND flag, that authorize mixing IRQF_NO_SUSPEND and
> > !IRQF_NO_SUSPEND as long as irq handlers setting IRQF_COND_SUSPEND
> > can safely be called in suspended state.
> > 
> > Doing this also implies taking care of system wakeup in devices handlers
> > if they tag the IRQ line as a wakeup source.
> > The first patch of this series exports the pm_system_wakeup symbol so
> > that drivers can call pm_system_wakeup from their interrupt handler.
> > 
> > This series then patches all at91 drivers that can have devices sharing
> > their IRQ line with a timer.
> > 
> > This series depends on [2].
> 
> I'm okay with all the patches:
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> As it seems easier to keep the whole series together, I'll let Rafael
> take it.

OK, I'll queue up the patches for the next PM pull request.

Thanks!

Re: [PATCH v2 0/6] ARM: at91: fix irq_pm_install_action WARNING

[Mark Rutland]

Hi Boris,

Other than the watchdog patch, which I'm a little confused by, this all
looks fine to me. So for the other patches:

Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>

Sorry I haven't replied to anything for the last few days, and thanks
for digging into this.

Thanks,
Mark.

On Mon, Mar 02, 2015 at 09:18:12AM +0000, Boris Brezillon wrote:
> My apologies to those of you who already received this series, but I
> didn't increment the patch version and forgot some subsystem maintainers
> and MLs.
> 
> Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
> test which triggers a WARNING backtrace on at91 platforms.
> While this WARN_ON is absolutely necessary to warn users that they should
> not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
> there is no easy way to solve this issue on at91 platforms.
> 
> The main reason is that the init timer is often using a shared irq line
> and thus request this irq with IRQF_NO_SUSPEND flag set, while other
> peripherals request the same irq line without this flag.
> 
> This problem has recently been addressed by this patch [2] which adds
> a new IRQF_COND_SUSPEND flag, that authorize mixing IRQF_NO_SUSPEND and
> !IRQF_NO_SUSPEND as long as irq handlers setting IRQF_COND_SUSPEND
> can safely be called in suspended state.
> 
> Doing this also implies taking care of system wakeup in devices handlers
> if they tag the IRQ line as a wakeup source.
> The first patch of this series exports the pm_system_wakeup symbol so
> that drivers can call pm_system_wakeup from their interrupt handler.
> 
> This series then patches all at91 drivers that can have devices sharing
> their IRQ line with a timer.
> 
> This series depends on [2].
> 
> Best Regards,
> 
> Boris
> 
> [1]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/irq/pm.c?id=cab303be91dc47942bc25de33dc1140123540800
> [2]https://lkml.org/lkml/2015/2/26/675
> 
> Changes since v1:
>  - replaced the IRQF_SUSPEND_NOACTION flag by IRQF_COND_SUSPEND
>  - properly addressed wakeup handling in drivers
> 
> Boris Brezillon (6):
>   PM / wakeup: export pm_system_wakeup symbol
>   rtc: at91sam9: rework wakeup and interrupt handling
>   rtc: at91rm9200: rework wakeup and interrupt handling
>   clk: at91: implement suspend/resume for the PMC irqchip
>   watchdog: at91sam9: request the irq with IRQF_NO_SUSPEND
>   tty: serial: atmel: rework interrupt and wakeup handling
> 
>  drivers/base/power/wakeup.c       |  1 +
>  drivers/clk/at91/pmc.c            | 20 ++++++++++-
>  drivers/clk/at91/pmc.h            |  1 +
>  drivers/rtc/rtc-at91rm9200.c      | 62 +++++++++++++++++++++++++--------
>  drivers/rtc/rtc-at91sam9.c        | 73 ++++++++++++++++++++++++++++++++-------
>  drivers/tty/serial/atmel_serial.c | 49 +++++++++++++++++++++++---
>  drivers/watchdog/at91sam9_wdt.c   |  3 +-
>  7 files changed, 177 insertions(+), 32 deletions(-)
> 
> -- 
> 1.9.1
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html