[PATCH v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[PATCH v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[Boris BREZILLON]

Hello,

This patch series is a porposal to enhance the sam9 watchdog timer support.

The at91sam9 watchdog timer can only be configured once, and the current
implementation tries to configure it in a static way:
- 2 seconds timeout
- wdt restart every 500ms

If the timer has already been configured with different values, it returns an
error and do not create any watchdog device.

This is not critical if the watchdog is disabled, but if it has been enabled
with different timeout values it will lead to a SoC reset.

This patch series tries to address this issue by adapting the heartbeat value
according the WDT timer config:
- it first tries to configure the timer as requested.
- if it fails it fallbacks to the current config, adapting its heartbeat timer
to the needs

This patch series also move to a dynamically allocated at91wdt device instead
of the static instance. I'm not sure this is the best solution, so please tell
me if you prefer to keep static instance of watchdog.

It adds a new at91 wdt type: software. This new type make use of the at91 wdt
interrupt to trigger a software reboot.

Finally it adds several properties to the device tree bindings.

Best Regards,
Boris

Changes since v4:
 - fix coding style issues
 - remove unneeded watchdog_active test

Changes since v3:
 - fix a bug in heartbeat time computation
 - fix a bug in at91_wdt_set_timeout when new timeout is bigger than the old
   one
 - rename at91_wdt_ping into at91_wdt_start
 - remove unneeded ping callback assignment

Changes since v2:
 - fix documentation
 - rework the heartbeat computation to get a more flexible behaviour
 - fix xx_to_yy macros
 - modify warning and error messages
 - remove unneeded parenthesis in arithmetic operations
 - use devm functions to map io memory
 - remove unneeded devm_kfree calls

Change since v1:
 - fix typo in documentaion
 - fix irq dt definition for sama5d3 SoC


Boris BREZILLON (4):
  watchdog: at91sam9_wdt: better watchdog support
  watchdog: at91sam9_wdt: update device tree doc
  ARM: at91/dt: add sam9 watchdog default options to SoCs
  ARM: at91/dt: add watchdog properties to kizbox board

 .../devicetree/bindings/watchdog/atmel-wdt.txt     |   30 +-
 arch/arm/boot/dts/at91sam9260.dtsi                 |    5 +
 arch/arm/boot/dts/at91sam9263.dtsi                 |    5 +
 arch/arm/boot/dts/at91sam9g45.dtsi                 |    5 +
 arch/arm/boot/dts/at91sam9n12.dtsi                 |    5 +
 arch/arm/boot/dts/at91sam9x5.dtsi                  |    5 +
 arch/arm/boot/dts/kizbox.dts                       |    6 +
 arch/arm/boot/dts/sama5d3.dtsi                     |    5 +
 drivers/watchdog/at91sam9_wdt.c                    |  309 ++++++++++++++------
 9 files changed, 287 insertions(+), 88 deletions(-)

-- 
1.7.9.5

[PATCH v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[Boris BREZILLON]

Hello,

This patch series is a porposal to enhance the sam9 watchdog timer support.

The at91sam9 watchdog timer can only be configured once, and the current
implementation tries to configure it in a static way:
- 2 seconds timeout
- wdt restart every 500ms

If the timer has already been configured with different values, it returns an
error and do not create any watchdog device.

This is not critical if the watchdog is disabled, but if it has been enabled
with different timeout values it will lead to a SoC reset.

This patch series tries to address this issue by adapting the heartbeat value
according the WDT timer config:
- it first tries to configure the timer as requested.
- if it fails it fallbacks to the current config, adapting its heartbeat timer
to the needs

This patch series also move to a dynamically allocated at91wdt device instead
of the static instance. I'm not sure this is the best solution, so please tell
me if you prefer to keep static instance of watchdog.

It adds a new at91 wdt type: software. This new type make use of the at91 wdt
interrupt to trigger a software reboot.

Finally it adds several properties to the device tree bindings.

Best Regards,
Boris

Changes since v4:
 - fix coding style issues
 - remove unneeded watchdog_active test

Changes since v3:
 - fix a bug in heartbeat time computation
 - fix a bug in at91_wdt_set_timeout when new timeout is bigger than the old
   one
 - rename at91_wdt_ping into at91_wdt_start
 - remove unneeded ping callback assignment

Changes since v2:
 - fix documentation
 - rework the heartbeat computation to get a more flexible behaviour
 - fix xx_to_yy macros
 - modify warning and error messages
 - remove unneeded parenthesis in arithmetic operations
 - use devm functions to map io memory
 - remove unneeded devm_kfree calls

Change since v1:
 - fix typo in documentaion
 - fix irq dt definition for sama5d3 SoC


Boris BREZILLON (4):
  watchdog: at91sam9_wdt: better watchdog support
  watchdog: at91sam9_wdt: update device tree doc
  ARM: at91/dt: add sam9 watchdog default options to SoCs
  ARM: at91/dt: add watchdog properties to kizbox board

 .../devicetree/bindings/watchdog/atmel-wdt.txt     |   30 +-
 arch/arm/boot/dts/at91sam9260.dtsi                 |    5 +
 arch/arm/boot/dts/at91sam9263.dtsi                 |    5 +
 arch/arm/boot/dts/at91sam9g45.dtsi                 |    5 +
 arch/arm/boot/dts/at91sam9n12.dtsi                 |    5 +
 arch/arm/boot/dts/at91sam9x5.dtsi                  |    5 +
 arch/arm/boot/dts/kizbox.dts                       |    6 +
 arch/arm/boot/dts/sama5d3.dtsi                     |    5 +
 drivers/watchdog/at91sam9_wdt.c                    |  309 ++++++++++++++------
 9 files changed, 287 insertions(+), 88 deletions(-)

-- 
1.7.9.5

[PATCH v5 1/4] watchdog: at91sam9_wdt: better watchdog support

[Boris BREZILLON]

The at91sam9 watchdog timer can only be configured once, and the current
implementation tries to configure it in a static way:
- 2 seconds timeout
- wdt restart every 500ms

If the timer has already been configured with different values, it returns an
error and do not create any watchdog device.

This is not critical if the watchdog is disabled, but if it has been enabled with
different timeout values it will lead to a SoC reset.

This patch series tries to address this issue by adapting the heartbeat value
according the WDT timer config:
- it first tries to configure the timer as requested.
- if it fails it fallbacks to the current config, adapting its heartbeat timer
to the needs

This patch series also move to a dynamically allocated at91wdt device instead
of the static instance.

It adds a new at91 wdt type: software. This new type make use of the at91 wdt
interrupt to trigger a software reboot.

Finally it adds several properties to the device tree bindings.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/watchdog/at91sam9_wdt.c |  309 ++++++++++++++++++++++++++++-----------
 1 file changed, 223 insertions(+), 86 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index be37dde..9bd089e 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -19,11 +19,13 @@
 
 #include <linux/errno.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <linux/jiffies.h>
@@ -31,22 +33,33 @@
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 
 #include "at91sam9_wdt.h"
 
 #define DRV_NAME "AT91SAM9 Watchdog"
 
-#define wdt_read(field) \
-	__raw_readl(at91wdt_private.base + field)
-#define wdt_write(field, val) \
-	__raw_writel((val), at91wdt_private.base + field)
+#define wdt_read(wdt, field) \
+	__raw_readl((wdt)->base + (field))
+#define wdt_write(wtd, field, val) \
+	__raw_writel((val), (wdt)->base + (field))
 
 /* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
  * use this to convert a watchdog
  * value from/to milliseconds.
  */
-#define ms_to_ticks(t)	(((t << 8) / 1000) - 1)
-#define ticks_to_ms(t)	(((t + 1) * 1000) >> 8)
+#define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
+#define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
+#define ticks_to_secs(t)		(((t) + 1) >> 8)
+#define secs_to_ticks(s)		(((s) << 8) - 1)
+
+#define WDT_MR_RESET	0x3FFF2FFF
+
+/* Watchdog max counter value in ticks */
+#define WDT_COUNTER_MAX_TICKS	0xFFF
+
+/* Watchdog max delta/value in secs */
+#define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
 
 /* Hardware timeout in seconds */
 #define WDT_HW_TIMEOUT 2
@@ -66,23 +79,40 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static struct watchdog_device at91_wdt_dev;
-static void at91_ping(unsigned long data);
-
-static struct {
+#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
+struct at91wdt {
+	struct watchdog_device wdd;
 	void __iomem *base;
 	unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
 	struct timer_list timer;	/* The timer that pings the watchdog */
-} at91wdt_private;
+	u32 mr;
+	u32 mr_mask;
+	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
+	bool nowayout;
+	unsigned int irq;
+};
 
 /* ......................................................................... */
 
+static irqreturn_t wdt_interrupt(int irq, void *dev_id)
+{
+	struct at91wdt *wdt = (struct at91wdt *)dev_id;
+
+	if (wdt_read(wdt, AT91_WDT_SR)) {
+		pr_crit("at91sam9 WDT software reset\n");
+		emergency_restart();
+		pr_crit("Reboot didn't ?????\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
 /*
  * Reload the watchdog timer.  (ie, pat the watchdog)
  */
-static inline void at91_wdt_reset(void)
+static inline void at91_wdt_reset(struct at91wdt *wdt)
 {
-	wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 }
 
 /*
@@ -90,26 +120,21 @@ static inline void at91_wdt_reset(void)
  */
 static void at91_ping(unsigned long data)
 {
-	if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
-	    (!watchdog_active(&at91_wdt_dev))) {
-		at91_wdt_reset();
-		mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
-	} else
+	struct at91wdt *wdt = (struct at91wdt *)data;
+	if (time_before(jiffies, wdt->next_heartbeat) ||
+	    !watchdog_active(&wdt->wdd)) {
+		at91_wdt_reset(wdt);
+		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+	} else {
 		pr_crit("I will reset your machine !\n");
-}
-
-static int at91_wdt_ping(struct watchdog_device *wdd)
-{
-	/* calculate when the next userspace timeout will be */
-	at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
-	return 0;
+	}
 }
 
 static int at91_wdt_start(struct watchdog_device *wdd)
 {
-	/* calculate the next userspace timeout and modify the timer */
-	at91_wdt_ping(wdd);
-	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	struct at91wdt *wdt = to_wdt(wdd);
+	/* calculate when the next userspace timeout will be */
+	wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
 	return 0;
 }
 
@@ -122,39 +147,89 @@ static int at91_wdt_stop(struct watchdog_device *wdd)
 static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
 {
 	wdd->timeout = new_timeout;
-	return 0;
+	return at91_wdt_start(wdd);
 }
 
-/*
- * Set the watchdog time interval in 1/256Hz (write-once)
- * Counter is 12 bit.
- */
-static int at91_wdt_settimeout(unsigned int timeout)
+static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 {
-	unsigned int reg;
-	unsigned int mr;
-
-	/* Check if disabled */
-	mr = wdt_read(AT91_WDT_MR);
-	if (mr & AT91_WDT_WDDIS) {
-		pr_err("sorry, watchdog is disabled\n");
-		return -EIO;
+	u32 tmp;
+	u32 delta;
+	u32 value;
+	int err;
+	u32 mask = wdt->mr_mask;
+	unsigned long min_heartbeat = 1;
+	struct device *dev = &pdev->dev;
+
+	tmp = wdt_read(wdt, AT91_WDT_MR);
+	if ((tmp & mask) != (wdt->mr & mask)) {
+		if (tmp == WDT_MR_RESET) {
+			wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+			tmp = wdt_read(wdt, AT91_WDT_MR);
+		}
+	}
+
+	if (tmp & AT91_WDT_WDDIS) {
+		if (wdt->mr & AT91_WDT_WDDIS)
+			return 0;
+		dev_err(dev, "watchdog is disabled\n");
+		return -EINVAL;
+	}
+
+	value = tmp & AT91_WDT_WDV;
+	delta = (tmp & AT91_WDT_WDD) >> 16;
+
+	if (delta < value)
+		min_heartbeat = ticks_to_hz_roundup(value - delta);
+
+	wdt->heartbeat = ticks_to_hz_rounddown(value);
+	if (!wdt->heartbeat) {
+		dev_err(dev,
+			"heartbeat is too small for the system to handle it correctly\n");
+		return -EINVAL;
+	}
+
+	if (wdt->heartbeat < min_heartbeat + 4) {
+		wdt->heartbeat = min_heartbeat;
+		dev_warn(dev,
+			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
+		if (wdt->heartbeat < 4)
+			dev_warn(dev,
+				 "heartbeat might be too small for the system to handle it correctly\n");
+	} else {
+		wdt->heartbeat -= 4;
 	}
 
-	/*
-	 * All counting occurs at SLOW_CLOCK / 128 = 256 Hz
-	 *
-	 * Since WDV is a 12-bit counter, the maximum period is
-	 * 4096 / 256 = 16 seconds.
-	 */
-	reg = AT91_WDT_WDRSTEN	/* causes watchdog reset */
-		/* | AT91_WDT_WDRPROC	causes processor reset only */
-		| AT91_WDT_WDDBGHLT	/* disabled in debug mode */
-		| AT91_WDT_WDD		/* restart at any time */
-		| (timeout & AT91_WDT_WDV);  /* timer value */
-	wdt_write(AT91_WDT_MR, reg);
+	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
+		err = request_irq(wdt->irq, wdt_interrupt,
+				  IRQF_SHARED | IRQF_IRQPOLL,
+				  pdev->name, wdt);
+		if (err)
+			return err;
+	}
+
+	if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
+		dev_warn(dev,
+			 "watchdog already configured differently (mr = %x expecting %x)\n",
+			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
+
+	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
+	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+
+	/* Try to set timeout from device tree first */
+	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
+		watchdog_init_timeout(&wdt->wdd, heartbeat, dev);
+	watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
+	err = watchdog_register_device(&wdt->wdd);
+	if (err)
+		goto out_stop_timer;
+
+	wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
 
 	return 0;
+
+out_stop_timer:
+	del_timer(&wdt->timer);
+	return err;
 }
 
 /* ......................................................................... */
@@ -169,61 +244,123 @@ static const struct watchdog_ops at91_wdt_ops = {
 	.owner =	THIS_MODULE,
 	.start =	at91_wdt_start,
 	.stop =		at91_wdt_stop,
-	.ping =		at91_wdt_ping,
 	.set_timeout =	at91_wdt_set_timeout,
 };
 
-static struct watchdog_device at91_wdt_dev = {
-	.info =		&at91_wdt_info,
-	.ops =		&at91_wdt_ops,
-	.timeout =	WDT_HEARTBEAT,
-	.min_timeout =	1,
-	.max_timeout =	0xFFFF,
-};
+#if defined(CONFIG_OF)
+static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
+{
+	u32 min = 0;
+	u32 max = WDT_COUNTER_MAX_SECS;
+	const char *tmp;
+
+	/* Get the interrupts property */
+	wdt->irq = irq_of_parse_and_map(np, 0);
+	if (!wdt->irq)
+		dev_warn(wdt->wdd.parent, "failed to get IRQ from DT\n");
+
+	if (!of_property_read_u32_index(np, "atmel,max-heartbeat-sec", 0,
+					&max)) {
+		if (!max || max > WDT_COUNTER_MAX_SECS)
+			max = WDT_COUNTER_MAX_SECS;
+
+		if (!of_property_read_u32_index(np, "atmel,min-heartbeat-sec",
+						0, &min)) {
+			if (min >= max)
+				min = max - 1;
+		}
+	}
+
+	min = secs_to_ticks(min);
+	max = secs_to_ticks(max);
+
+	wdt->mr_mask = 0x3FFFFFFF;
+	wdt->mr = 0;
+	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
+	    !strcmp(tmp, "software")) {
+		wdt->mr |= AT91_WDT_WDFIEN;
+		wdt->mr_mask &= ~AT91_WDT_WDRPROC;
+	} else {
+		wdt->mr |= AT91_WDT_WDRSTEN;
+	}
+
+	if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
+	    !strcmp(tmp, "proc"))
+		wdt->mr |= AT91_WDT_WDRPROC;
+
+	if (of_property_read_bool(np, "atmel,disable")) {
+		wdt->mr |= AT91_WDT_WDDIS;
+		wdt->mr_mask &= AT91_WDT_WDDIS;
+	}
+
+	if (of_property_read_bool(np, "atmel,idle-halt"))
+		wdt->mr |= AT91_WDT_WDIDLEHLT;
+
+	if (of_property_read_bool(np, "atmel,dbg-halt"))
+		wdt->mr |= AT91_WDT_WDDBGHLT;
+
+	wdt->mr |= max | ((max - min) << 16);
+
+	return 0;
+}
+#else
+static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
+{
+	return 0;
+}
+#endif
 
 static int __init at91wdt_probe(struct platform_device *pdev)
 {
 	struct resource	*r;
-	int res;
+	int err;
+	struct at91wdt *wdt;
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!r)
-		return -ENODEV;
-	at91wdt_private.base = ioremap(r->start, resource_size(r));
-	if (!at91wdt_private.base) {
-		dev_err(&pdev->dev, "failed to map registers, aborting.\n");
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
 		return -ENOMEM;
-	}
 
-	at91_wdt_dev.parent = &pdev->dev;
-	watchdog_init_timeout(&at91_wdt_dev, heartbeat, &pdev->dev);
-	watchdog_set_nowayout(&at91_wdt_dev, nowayout);
+	wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
+		  AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
+	wdt->mr_mask = 0x3FFFFFFF;
+	wdt->nowayout = nowayout;
+	wdt->wdd.parent = &pdev->dev;
+	wdt->wdd.info = &at91_wdt_info;
+	wdt->wdd.ops = &at91_wdt_ops;
+	wdt->wdd.timeout = WDT_HEARTBEAT;
+	wdt->wdd.min_timeout = 1;
+	wdt->wdd.max_timeout = 0xFFFF;
 
-	/* Set watchdog */
-	res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
-	if (res)
-		return res;
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(wdt->base))
+		return PTR_ERR(wdt->base);
+
+	if (pdev->dev.of_node) {
+		err = of_at91wdt_init(pdev->dev.of_node, wdt);
+		if (err)
+			return err;
+	}
 
-	res = watchdog_register_device(&at91_wdt_dev);
-	if (res)
-		return res;
+	err = at91_wdt_init(pdev, wdt);
+	if (err)
+		return err;
 
-	at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;
-	setup_timer(&at91wdt_private.timer, at91_ping, 0);
-	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	platform_set_drvdata(pdev, wdt);
 
 	pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
-		at91_wdt_dev.timeout, nowayout);
+		wdt->wdd.timeout, wdt->nowayout);
 
 	return 0;
 }
 
 static int __exit at91wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&at91_wdt_dev);
+	struct at91wdt *wdt = platform_get_drvdata(pdev);
+	watchdog_unregister_device(&wdt->wdd);
 
 	pr_warn("I quit now, hardware will probably reboot!\n");
-	del_timer(&at91wdt_private.timer);
+	del_timer(&wdt->timer);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH v5 1/4] watchdog: at91sam9_wdt: better watchdog support

[Boris BREZILLON]

The at91sam9 watchdog timer can only be configured once, and the current
implementation tries to configure it in a static way:
- 2 seconds timeout
- wdt restart every 500ms

If the timer has already been configured with different values, it returns an
error and do not create any watchdog device.

This is not critical if the watchdog is disabled, but if it has been enabled with
different timeout values it will lead to a SoC reset.

This patch series tries to address this issue by adapting the heartbeat value
according the WDT timer config:
- it first tries to configure the timer as requested.
- if it fails it fallbacks to the current config, adapting its heartbeat timer
to the needs

This patch series also move to a dynamically allocated at91wdt device instead
of the static instance.

It adds a new at91 wdt type: software. This new type make use of the at91 wdt
interrupt to trigger a software reboot.

Finally it adds several properties to the device tree bindings.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/watchdog/at91sam9_wdt.c |  309 ++++++++++++++++++++++++++++-----------
 1 file changed, 223 insertions(+), 86 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index be37dde..9bd089e 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -19,11 +19,13 @@
 
 #include <linux/errno.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <linux/jiffies.h>
@@ -31,22 +33,33 @@
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 
 #include "at91sam9_wdt.h"
 
 #define DRV_NAME "AT91SAM9 Watchdog"
 
-#define wdt_read(field) \
-	__raw_readl(at91wdt_private.base + field)
-#define wdt_write(field, val) \
-	__raw_writel((val), at91wdt_private.base + field)
+#define wdt_read(wdt, field) \
+	__raw_readl((wdt)->base + (field))
+#define wdt_write(wtd, field, val) \
+	__raw_writel((val), (wdt)->base + (field))
 
 /* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
  * use this to convert a watchdog
  * value from/to milliseconds.
  */
-#define ms_to_ticks(t)	(((t << 8) / 1000) - 1)
-#define ticks_to_ms(t)	(((t + 1) * 1000) >> 8)
+#define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
+#define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
+#define ticks_to_secs(t)		(((t) + 1) >> 8)
+#define secs_to_ticks(s)		(((s) << 8) - 1)
+
+#define WDT_MR_RESET	0x3FFF2FFF
+
+/* Watchdog max counter value in ticks */
+#define WDT_COUNTER_MAX_TICKS	0xFFF
+
+/* Watchdog max delta/value in secs */
+#define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
 
 /* Hardware timeout in seconds */
 #define WDT_HW_TIMEOUT 2
@@ -66,23 +79,40 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static struct watchdog_device at91_wdt_dev;
-static void at91_ping(unsigned long data);
-
-static struct {
+#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
+struct at91wdt {
+	struct watchdog_device wdd;
 	void __iomem *base;
 	unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
 	struct timer_list timer;	/* The timer that pings the watchdog */
-} at91wdt_private;
+	u32 mr;
+	u32 mr_mask;
+	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
+	bool nowayout;
+	unsigned int irq;
+};
 
 /* ......................................................................... */
 
+static irqreturn_t wdt_interrupt(int irq, void *dev_id)
+{
+	struct at91wdt *wdt = (struct at91wdt *)dev_id;
+
+	if (wdt_read(wdt, AT91_WDT_SR)) {
+		pr_crit("at91sam9 WDT software reset\n");
+		emergency_restart();
+		pr_crit("Reboot didn't ?????\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
 /*
  * Reload the watchdog timer.  (ie, pat the watchdog)
  */
-static inline void at91_wdt_reset(void)
+static inline void at91_wdt_reset(struct at91wdt *wdt)
 {
-	wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 }
 
 /*
@@ -90,26 +120,21 @@ static inline void at91_wdt_reset(void)
  */
 static void at91_ping(unsigned long data)
 {
-	if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
-	    (!watchdog_active(&at91_wdt_dev))) {
-		at91_wdt_reset();
-		mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
-	} else
+	struct at91wdt *wdt = (struct at91wdt *)data;
+	if (time_before(jiffies, wdt->next_heartbeat) ||
+	    !watchdog_active(&wdt->wdd)) {
+		at91_wdt_reset(wdt);
+		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+	} else {
 		pr_crit("I will reset your machine !\n");
-}
-
-static int at91_wdt_ping(struct watchdog_device *wdd)
-{
-	/* calculate when the next userspace timeout will be */
-	at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
-	return 0;
+	}
 }
 
 static int at91_wdt_start(struct watchdog_device *wdd)
 {
-	/* calculate the next userspace timeout and modify the timer */
-	at91_wdt_ping(wdd);
-	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	struct at91wdt *wdt = to_wdt(wdd);
+	/* calculate when the next userspace timeout will be */
+	wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
 	return 0;
 }
 
@@ -122,39 +147,89 @@ static int at91_wdt_stop(struct watchdog_device *wdd)
 static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
 {
 	wdd->timeout = new_timeout;
-	return 0;
+	return at91_wdt_start(wdd);
 }
 
-/*
- * Set the watchdog time interval in 1/256Hz (write-once)
- * Counter is 12 bit.
- */
-static int at91_wdt_settimeout(unsigned int timeout)
+static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 {
-	unsigned int reg;
-	unsigned int mr;
-
-	/* Check if disabled */
-	mr = wdt_read(AT91_WDT_MR);
-	if (mr & AT91_WDT_WDDIS) {
-		pr_err("sorry, watchdog is disabled\n");
-		return -EIO;
+	u32 tmp;
+	u32 delta;
+	u32 value;
+	int err;
+	u32 mask = wdt->mr_mask;
+	unsigned long min_heartbeat = 1;
+	struct device *dev = &pdev->dev;
+
+	tmp = wdt_read(wdt, AT91_WDT_MR);
+	if ((tmp & mask) != (wdt->mr & mask)) {
+		if (tmp == WDT_MR_RESET) {
+			wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+			tmp = wdt_read(wdt, AT91_WDT_MR);
+		}
+	}
+
+	if (tmp & AT91_WDT_WDDIS) {
+		if (wdt->mr & AT91_WDT_WDDIS)
+			return 0;
+		dev_err(dev, "watchdog is disabled\n");
+		return -EINVAL;
+	}
+
+	value = tmp & AT91_WDT_WDV;
+	delta = (tmp & AT91_WDT_WDD) >> 16;
+
+	if (delta < value)
+		min_heartbeat = ticks_to_hz_roundup(value - delta);
+
+	wdt->heartbeat = ticks_to_hz_rounddown(value);
+	if (!wdt->heartbeat) {
+		dev_err(dev,
+			"heartbeat is too small for the system to handle it correctly\n");
+		return -EINVAL;
+	}
+
+	if (wdt->heartbeat < min_heartbeat + 4) {
+		wdt->heartbeat = min_heartbeat;
+		dev_warn(dev,
+			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
+		if (wdt->heartbeat < 4)
+			dev_warn(dev,
+				 "heartbeat might be too small for the system to handle it correctly\n");
+	} else {
+		wdt->heartbeat -= 4;
 	}
 
-	/*
-	 * All counting occurs at SLOW_CLOCK / 128 = 256 Hz
-	 *
-	 * Since WDV is a 12-bit counter, the maximum period is
-	 * 4096 / 256 = 16 seconds.
-	 */
-	reg = AT91_WDT_WDRSTEN	/* causes watchdog reset */
-		/* | AT91_WDT_WDRPROC	causes processor reset only */
-		| AT91_WDT_WDDBGHLT	/* disabled in debug mode */
-		| AT91_WDT_WDD		/* restart at any time */
-		| (timeout & AT91_WDT_WDV);  /* timer value */
-	wdt_write(AT91_WDT_MR, reg);
+	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
+		err = request_irq(wdt->irq, wdt_interrupt,
+				  IRQF_SHARED | IRQF_IRQPOLL,
+				  pdev->name, wdt);
+		if (err)
+			return err;
+	}
+
+	if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
+		dev_warn(dev,
+			 "watchdog already configured differently (mr = %x expecting %x)\n",
+			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
+
+	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
+	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+
+	/* Try to set timeout from device tree first */
+	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
+		watchdog_init_timeout(&wdt->wdd, heartbeat, dev);
+	watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
+	err = watchdog_register_device(&wdt->wdd);
+	if (err)
+		goto out_stop_timer;
+
+	wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
 
 	return 0;
+
+out_stop_timer:
+	del_timer(&wdt->timer);
+	return err;
 }
 
 /* ......................................................................... */
@@ -169,61 +244,123 @@ static const struct watchdog_ops at91_wdt_ops = {
 	.owner =	THIS_MODULE,
 	.start =	at91_wdt_start,
 	.stop =		at91_wdt_stop,
-	.ping =		at91_wdt_ping,
 	.set_timeout =	at91_wdt_set_timeout,
 };
 
-static struct watchdog_device at91_wdt_dev = {
-	.info =		&at91_wdt_info,
-	.ops =		&at91_wdt_ops,
-	.timeout =	WDT_HEARTBEAT,
-	.min_timeout =	1,
-	.max_timeout =	0xFFFF,
-};
+#if defined(CONFIG_OF)
+static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
+{
+	u32 min = 0;
+	u32 max = WDT_COUNTER_MAX_SECS;
+	const char *tmp;
+
+	/* Get the interrupts property */
+	wdt->irq = irq_of_parse_and_map(np, 0);
+	if (!wdt->irq)
+		dev_warn(wdt->wdd.parent, "failed to get IRQ from DT\n");
+
+	if (!of_property_read_u32_index(np, "atmel,max-heartbeat-sec", 0,
+					&max)) {
+		if (!max || max > WDT_COUNTER_MAX_SECS)
+			max = WDT_COUNTER_MAX_SECS;
+
+		if (!of_property_read_u32_index(np, "atmel,min-heartbeat-sec",
+						0, &min)) {
+			if (min >= max)
+				min = max - 1;
+		}
+	}
+
+	min = secs_to_ticks(min);
+	max = secs_to_ticks(max);
+
+	wdt->mr_mask = 0x3FFFFFFF;
+	wdt->mr = 0;
+	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
+	    !strcmp(tmp, "software")) {
+		wdt->mr |= AT91_WDT_WDFIEN;
+		wdt->mr_mask &= ~AT91_WDT_WDRPROC;
+	} else {
+		wdt->mr |= AT91_WDT_WDRSTEN;
+	}
+
+	if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
+	    !strcmp(tmp, "proc"))
+		wdt->mr |= AT91_WDT_WDRPROC;
+
+	if (of_property_read_bool(np, "atmel,disable")) {
+		wdt->mr |= AT91_WDT_WDDIS;
+		wdt->mr_mask &= AT91_WDT_WDDIS;
+	}
+
+	if (of_property_read_bool(np, "atmel,idle-halt"))
+		wdt->mr |= AT91_WDT_WDIDLEHLT;
+
+	if (of_property_read_bool(np, "atmel,dbg-halt"))
+		wdt->mr |= AT91_WDT_WDDBGHLT;
+
+	wdt->mr |= max | ((max - min) << 16);
+
+	return 0;
+}
+#else
+static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
+{
+	return 0;
+}
+#endif
 
 static int __init at91wdt_probe(struct platform_device *pdev)
 {
 	struct resource	*r;
-	int res;
+	int err;
+	struct at91wdt *wdt;
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!r)
-		return -ENODEV;
-	at91wdt_private.base = ioremap(r->start, resource_size(r));
-	if (!at91wdt_private.base) {
-		dev_err(&pdev->dev, "failed to map registers, aborting.\n");
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
 		return -ENOMEM;
-	}
 
-	at91_wdt_dev.parent = &pdev->dev;
-	watchdog_init_timeout(&at91_wdt_dev, heartbeat, &pdev->dev);
-	watchdog_set_nowayout(&at91_wdt_dev, nowayout);
+	wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
+		  AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
+	wdt->mr_mask = 0x3FFFFFFF;
+	wdt->nowayout = nowayout;
+	wdt->wdd.parent = &pdev->dev;
+	wdt->wdd.info = &at91_wdt_info;
+	wdt->wdd.ops = &at91_wdt_ops;
+	wdt->wdd.timeout = WDT_HEARTBEAT;
+	wdt->wdd.min_timeout = 1;
+	wdt->wdd.max_timeout = 0xFFFF;
 
-	/* Set watchdog */
-	res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
-	if (res)
-		return res;
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(wdt->base))
+		return PTR_ERR(wdt->base);
+
+	if (pdev->dev.of_node) {
+		err = of_at91wdt_init(pdev->dev.of_node, wdt);
+		if (err)
+			return err;
+	}
 
-	res = watchdog_register_device(&at91_wdt_dev);
-	if (res)
-		return res;
+	err = at91_wdt_init(pdev, wdt);
+	if (err)
+		return err;
 
-	at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;
-	setup_timer(&at91wdt_private.timer, at91_ping, 0);
-	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	platform_set_drvdata(pdev, wdt);
 
 	pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
-		at91_wdt_dev.timeout, nowayout);
+		wdt->wdd.timeout, wdt->nowayout);
 
 	return 0;
 }
 
 static int __exit at91wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&at91_wdt_dev);
+	struct at91wdt *wdt = platform_get_drvdata(pdev);
+	watchdog_unregister_device(&wdt->wdd);
 
 	pr_warn("I quit now, hardware will probably reboot!\n");
-	del_timer(&at91wdt_private.timer);
+	del_timer(&wdt->timer);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH v5 1/4] watchdog: at91sam9_wdt: better watchdog support

[Boris BREZILLON]

The at91sam9 watchdog timer can only be configured once, and the current
implementation tries to configure it in a static way:
- 2 seconds timeout
- wdt restart every 500ms

If the timer has already been configured with different values, it returns an
error and do not create any watchdog device.

This is not critical if the watchdog is disabled, but if it has been enabled with
different timeout values it will lead to a SoC reset.

This patch series tries to address this issue by adapting the heartbeat value
according the WDT timer config:
- it first tries to configure the timer as requested.
- if it fails it fallbacks to the current config, adapting its heartbeat timer
to the needs

This patch series also move to a dynamically allocated at91wdt device instead
of the static instance.

It adds a new at91 wdt type: software. This new type make use of the at91 wdt
interrupt to trigger a software reboot.

Finally it adds several properties to the device tree bindings.

Signed-off-by: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/watchdog/at91sam9_wdt.c |  309 ++++++++++++++++++++++++++++-----------
 1 file changed, 223 insertions(+), 86 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index be37dde..9bd089e 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -19,11 +19,13 @@
 
 #include <linux/errno.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <linux/jiffies.h>
@@ -31,22 +33,33 @@
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 
 #include "at91sam9_wdt.h"
 
 #define DRV_NAME "AT91SAM9 Watchdog"
 
-#define wdt_read(field) \
-	__raw_readl(at91wdt_private.base + field)
-#define wdt_write(field, val) \
-	__raw_writel((val), at91wdt_private.base + field)
+#define wdt_read(wdt, field) \
+	__raw_readl((wdt)->base + (field))
+#define wdt_write(wtd, field, val) \
+	__raw_writel((val), (wdt)->base + (field))
 
 /* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
  * use this to convert a watchdog
  * value from/to milliseconds.
  */
-#define ms_to_ticks(t)	(((t << 8) / 1000) - 1)
-#define ticks_to_ms(t)	(((t + 1) * 1000) >> 8)
+#define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
+#define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
+#define ticks_to_secs(t)		(((t) + 1) >> 8)
+#define secs_to_ticks(s)		(((s) << 8) - 1)
+
+#define WDT_MR_RESET	0x3FFF2FFF
+
+/* Watchdog max counter value in ticks */
+#define WDT_COUNTER_MAX_TICKS	0xFFF
+
+/* Watchdog max delta/value in secs */
+#define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
 
 /* Hardware timeout in seconds */
 #define WDT_HW_TIMEOUT 2
@@ -66,23 +79,40 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static struct watchdog_device at91_wdt_dev;
-static void at91_ping(unsigned long data);
-
-static struct {
+#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
+struct at91wdt {
+	struct watchdog_device wdd;
 	void __iomem *base;
 	unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
 	struct timer_list timer;	/* The timer that pings the watchdog */
-} at91wdt_private;
+	u32 mr;
+	u32 mr_mask;
+	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
+	bool nowayout;
+	unsigned int irq;
+};
 
 /* ......................................................................... */
 
+static irqreturn_t wdt_interrupt(int irq, void *dev_id)
+{
+	struct at91wdt *wdt = (struct at91wdt *)dev_id;
+
+	if (wdt_read(wdt, AT91_WDT_SR)) {
+		pr_crit("at91sam9 WDT software reset\n");
+		emergency_restart();
+		pr_crit("Reboot didn't ?????\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
 /*
  * Reload the watchdog timer.  (ie, pat the watchdog)
  */
-static inline void at91_wdt_reset(void)
+static inline void at91_wdt_reset(struct at91wdt *wdt)
 {
-	wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 }
 
 /*
@@ -90,26 +120,21 @@ static inline void at91_wdt_reset(void)
  */
 static void at91_ping(unsigned long data)
 {
-	if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
-	    (!watchdog_active(&at91_wdt_dev))) {
-		at91_wdt_reset();
-		mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
-	} else
+	struct at91wdt *wdt = (struct at91wdt *)data;
+	if (time_before(jiffies, wdt->next_heartbeat) ||
+	    !watchdog_active(&wdt->wdd)) {
+		at91_wdt_reset(wdt);
+		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+	} else {
 		pr_crit("I will reset your machine !\n");
-}
-
-static int at91_wdt_ping(struct watchdog_device *wdd)
-{
-	/* calculate when the next userspace timeout will be */
-	at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
-	return 0;
+	}
 }
 
 static int at91_wdt_start(struct watchdog_device *wdd)
 {
-	/* calculate the next userspace timeout and modify the timer */
-	at91_wdt_ping(wdd);
-	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	struct at91wdt *wdt = to_wdt(wdd);
+	/* calculate when the next userspace timeout will be */
+	wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
 	return 0;
 }
 
@@ -122,39 +147,89 @@ static int at91_wdt_stop(struct watchdog_device *wdd)
 static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
 {
 	wdd->timeout = new_timeout;
-	return 0;
+	return at91_wdt_start(wdd);
 }
 
-/*
- * Set the watchdog time interval in 1/256Hz (write-once)
- * Counter is 12 bit.
- */
-static int at91_wdt_settimeout(unsigned int timeout)
+static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 {
-	unsigned int reg;
-	unsigned int mr;
-
-	/* Check if disabled */
-	mr = wdt_read(AT91_WDT_MR);
-	if (mr & AT91_WDT_WDDIS) {
-		pr_err("sorry, watchdog is disabled\n");
-		return -EIO;
+	u32 tmp;
+	u32 delta;
+	u32 value;
+	int err;
+	u32 mask = wdt->mr_mask;
+	unsigned long min_heartbeat = 1;
+	struct device *dev = &pdev->dev;
+
+	tmp = wdt_read(wdt, AT91_WDT_MR);
+	if ((tmp & mask) != (wdt->mr & mask)) {
+		if (tmp == WDT_MR_RESET) {
+			wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+			tmp = wdt_read(wdt, AT91_WDT_MR);
+		}
+	}
+
+	if (tmp & AT91_WDT_WDDIS) {
+		if (wdt->mr & AT91_WDT_WDDIS)
+			return 0;
+		dev_err(dev, "watchdog is disabled\n");
+		return -EINVAL;
+	}
+
+	value = tmp & AT91_WDT_WDV;
+	delta = (tmp & AT91_WDT_WDD) >> 16;
+
+	if (delta < value)
+		min_heartbeat = ticks_to_hz_roundup(value - delta);
+
+	wdt->heartbeat = ticks_to_hz_rounddown(value);
+	if (!wdt->heartbeat) {
+		dev_err(dev,
+			"heartbeat is too small for the system to handle it correctly\n");
+		return -EINVAL;
+	}
+
+	if (wdt->heartbeat < min_heartbeat + 4) {
+		wdt->heartbeat = min_heartbeat;
+		dev_warn(dev,
+			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
+		if (wdt->heartbeat < 4)
+			dev_warn(dev,
+				 "heartbeat might be too small for the system to handle it correctly\n");
+	} else {
+		wdt->heartbeat -= 4;
 	}
 
-	/*
-	 * All counting occurs at SLOW_CLOCK / 128 = 256 Hz
-	 *
-	 * Since WDV is a 12-bit counter, the maximum period is
-	 * 4096 / 256 = 16 seconds.
-	 */
-	reg = AT91_WDT_WDRSTEN	/* causes watchdog reset */
-		/* | AT91_WDT_WDRPROC	causes processor reset only */
-		| AT91_WDT_WDDBGHLT	/* disabled in debug mode */
-		| AT91_WDT_WDD		/* restart at any time */
-		| (timeout & AT91_WDT_WDV);  /* timer value */
-	wdt_write(AT91_WDT_MR, reg);
+	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
+		err = request_irq(wdt->irq, wdt_interrupt,
+				  IRQF_SHARED | IRQF_IRQPOLL,
+				  pdev->name, wdt);
+		if (err)
+			return err;
+	}
+
+	if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
+		dev_warn(dev,
+			 "watchdog already configured differently (mr = %x expecting %x)\n",
+			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
+
+	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
+	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+
+	/* Try to set timeout from device tree first */
+	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
+		watchdog_init_timeout(&wdt->wdd, heartbeat, dev);
+	watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
+	err = watchdog_register_device(&wdt->wdd);
+	if (err)
+		goto out_stop_timer;
+
+	wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
 
 	return 0;
+
+out_stop_timer:
+	del_timer(&wdt->timer);
+	return err;
 }
 
 /* ......................................................................... */
@@ -169,61 +244,123 @@ static const struct watchdog_ops at91_wdt_ops = {
 	.owner =	THIS_MODULE,
 	.start =	at91_wdt_start,
 	.stop =		at91_wdt_stop,
-	.ping =		at91_wdt_ping,
 	.set_timeout =	at91_wdt_set_timeout,
 };
 
-static struct watchdog_device at91_wdt_dev = {
-	.info =		&at91_wdt_info,
-	.ops =		&at91_wdt_ops,
-	.timeout =	WDT_HEARTBEAT,
-	.min_timeout =	1,
-	.max_timeout =	0xFFFF,
-};
+#if defined(CONFIG_OF)
+static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
+{
+	u32 min = 0;
+	u32 max = WDT_COUNTER_MAX_SECS;
+	const char *tmp;
+
+	/* Get the interrupts property */
+	wdt->irq = irq_of_parse_and_map(np, 0);
+	if (!wdt->irq)
+		dev_warn(wdt->wdd.parent, "failed to get IRQ from DT\n");
+
+	if (!of_property_read_u32_index(np, "atmel,max-heartbeat-sec", 0,
+					&max)) {
+		if (!max || max > WDT_COUNTER_MAX_SECS)
+			max = WDT_COUNTER_MAX_SECS;
+
+		if (!of_property_read_u32_index(np, "atmel,min-heartbeat-sec",
+						0, &min)) {
+			if (min >= max)
+				min = max - 1;
+		}
+	}
+
+	min = secs_to_ticks(min);
+	max = secs_to_ticks(max);
+
+	wdt->mr_mask = 0x3FFFFFFF;
+	wdt->mr = 0;
+	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
+	    !strcmp(tmp, "software")) {
+		wdt->mr |= AT91_WDT_WDFIEN;
+		wdt->mr_mask &= ~AT91_WDT_WDRPROC;
+	} else {
+		wdt->mr |= AT91_WDT_WDRSTEN;
+	}
+
+	if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
+	    !strcmp(tmp, "proc"))
+		wdt->mr |= AT91_WDT_WDRPROC;
+
+	if (of_property_read_bool(np, "atmel,disable")) {
+		wdt->mr |= AT91_WDT_WDDIS;
+		wdt->mr_mask &= AT91_WDT_WDDIS;
+	}
+
+	if (of_property_read_bool(np, "atmel,idle-halt"))
+		wdt->mr |= AT91_WDT_WDIDLEHLT;
+
+	if (of_property_read_bool(np, "atmel,dbg-halt"))
+		wdt->mr |= AT91_WDT_WDDBGHLT;
+
+	wdt->mr |= max | ((max - min) << 16);
+
+	return 0;
+}
+#else
+static inline int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
+{
+	return 0;
+}
+#endif
 
 static int __init at91wdt_probe(struct platform_device *pdev)
 {
 	struct resource	*r;
-	int res;
+	int err;
+	struct at91wdt *wdt;
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!r)
-		return -ENODEV;
-	at91wdt_private.base = ioremap(r->start, resource_size(r));
-	if (!at91wdt_private.base) {
-		dev_err(&pdev->dev, "failed to map registers, aborting.\n");
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
 		return -ENOMEM;
-	}
 
-	at91_wdt_dev.parent = &pdev->dev;
-	watchdog_init_timeout(&at91_wdt_dev, heartbeat, &pdev->dev);
-	watchdog_set_nowayout(&at91_wdt_dev, nowayout);
+	wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
+		  AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
+	wdt->mr_mask = 0x3FFFFFFF;
+	wdt->nowayout = nowayout;
+	wdt->wdd.parent = &pdev->dev;
+	wdt->wdd.info = &at91_wdt_info;
+	wdt->wdd.ops = &at91_wdt_ops;
+	wdt->wdd.timeout = WDT_HEARTBEAT;
+	wdt->wdd.min_timeout = 1;
+	wdt->wdd.max_timeout = 0xFFFF;
 
-	/* Set watchdog */
-	res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
-	if (res)
-		return res;
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(wdt->base))
+		return PTR_ERR(wdt->base);
+
+	if (pdev->dev.of_node) {
+		err = of_at91wdt_init(pdev->dev.of_node, wdt);
+		if (err)
+			return err;
+	}
 
-	res = watchdog_register_device(&at91_wdt_dev);
-	if (res)
-		return res;
+	err = at91_wdt_init(pdev, wdt);
+	if (err)
+		return err;
 
-	at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;
-	setup_timer(&at91wdt_private.timer, at91_ping, 0);
-	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	platform_set_drvdata(pdev, wdt);
 
 	pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
-		at91_wdt_dev.timeout, nowayout);
+		wdt->wdd.timeout, wdt->nowayout);
 
 	return 0;
 }
 
 static int __exit at91wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&at91_wdt_dev);
+	struct at91wdt *wdt = platform_get_drvdata(pdev);
+	watchdog_unregister_device(&wdt->wdd);
 
 	pr_warn("I quit now, hardware will probably reboot!\n");
-	del_timer(&at91wdt_private.timer);
+	del_timer(&wdt->timer);
 
 	return 0;
 }
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 v5 1/4] watchdog: at91sam9_wdt: better watchdog support

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:12AM +0200, Boris BREZILLON wrote:
> The at91sam9 watchdog timer can only be configured once, and the current
> implementation tries to configure it in a static way:
> - 2 seconds timeout
> - wdt restart every 500ms
> 
> If the timer has already been configured with different values, it returns an
> error and do not create any watchdog device.
> 
> This is not critical if the watchdog is disabled, but if it has been enabled with
> different timeout values it will lead to a SoC reset.
> 
> This patch series tries to address this issue by adapting the heartbeat value
> according the WDT timer config:
> - it first tries to configure the timer as requested.
> - if it fails it fallbacks to the current config, adapting its heartbeat timer
> to the needs
> 
> This patch series also move to a dynamically allocated at91wdt device instead
> of the static instance.
> 
> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
> interrupt to trigger a software reboot.
> 
> Finally it adds several properties to the device tree bindings.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v5 1/4] watchdog: at91sam9_wdt: better watchdog support

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:12AM +0200, Boris BREZILLON wrote:
> The at91sam9 watchdog timer can only be configured once, and the current
> implementation tries to configure it in a static way:
> - 2 seconds timeout
> - wdt restart every 500ms
> 
> If the timer has already been configured with different values, it returns an
> error and do not create any watchdog device.
> 
> This is not critical if the watchdog is disabled, but if it has been enabled with
> different timeout values it will lead to a SoC reset.
> 
> This patch series tries to address this issue by adapting the heartbeat value
> according the WDT timer config:
> - it first tries to configure the timer as requested.
> - if it fails it fallbacks to the current config, adapting its heartbeat timer
> to the needs
> 
> This patch series also move to a dynamically allocated at91wdt device instead
> of the static instance.
> 
> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
> interrupt to trigger a software reboot.
> 
> Finally it adds several properties to the device tree bindings.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH v5 1/4] watchdog: at91sam9_wdt: better watchdog support

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:12AM +0200, Boris BREZILLON wrote:
> The at91sam9 watchdog timer can only be configured once, and the current
> implementation tries to configure it in a static way:
> - 2 seconds timeout
> - wdt restart every 500ms
> 
> If the timer has already been configured with different values, it returns an
> error and do not create any watchdog device.
> 
> This is not critical if the watchdog is disabled, but if it has been enabled with
> different timeout values it will lead to a SoC reset.
> 
> This patch series tries to address this issue by adapting the heartbeat value
> according the WDT timer config:
> - it first tries to configure the timer as requested.
> - if it fails it fallbacks to the current config, adapting its heartbeat timer
> to the needs
> 
> This patch series also move to a dynamically allocated at91wdt device instead
> of the static instance.
> 
> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
> interrupt to trigger a software reboot.
> 
> Finally it adds several properties to the device tree bindings.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/4] watchdog: at91sam9_wdt: update device tree doc

[Boris BREZILLON]

Add new at91sam9 watchdog properties to the documentation.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 .../devicetree/bindings/watchdog/atmel-wdt.txt     |   30 ++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
index fcdd48f..f90e294 100644
--- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
@@ -9,11 +9,37 @@ Required properties:
 
 Optional properties:
 - timeout-sec: contains the watchdog timeout in seconds.
+- interrupts : Should contain WDT interrupt.
+- atmel,max-heartbeat-sec : Should contain the maximum heartbeat value in
+	seconds. This value should be less or equal to 16. It is used to
+	compute the WDV field.
+- atmel,min-heartbeat-sec : Should contain the minimum heartbeat value in
+	seconds. This value must be smaller than the max-heartbeat-sec value.
+	It is used to compute the WDD field.
+- atmel,watchdog-type : Should be "hardware" or "software". Hardware watchdog
+	use the at91 watchdog reset. Software watchdog use the watchdog
+	interrupt to trigger a software reset.
+- atmel,reset-type : Should be "proc" or "all".
+	"all" : assert peripherals and processor reset signals
+	"proc" : assert the processor reset signal
+	This is valid only when using "hardware" watchdog.
+- atmel,disable : Should be present if you want to disable the watchdog.
+- atmel,idle-halt : Should be present if you want to stop the watchdog when
+	entering idle state.
+- atmel,dbg-halt : Should be present if you want to stop the watchdog when
+	entering debug state.
 
 Example:
-
 	watchdog@fffffd40 {
 		compatible = "atmel,at91sam9260-wdt";
 		reg = <0xfffffd40 0x10>;
-		timeout-sec = <10>;
+		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+		timeout-sec = <15>;
+		atmel,watchdog-type = "hardware";
+		atmel,reset-type = "all";
+		atmel,dbg-halt;
+		atmel,idle-halt;
+		atmel,max-heartbeat-sec = <16>;
+		atmel,min-heartbeat-sec = <0>;
+		status = "okay";
 	};
-- 
1.7.9.5

[PATCH v5 2/4] watchdog: at91sam9_wdt: update device tree doc

[Boris BREZILLON]

Add new at91sam9 watchdog properties to the documentation.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 .../devicetree/bindings/watchdog/atmel-wdt.txt     |   30 ++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
index fcdd48f..f90e294 100644
--- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
@@ -9,11 +9,37 @@ Required properties:
 
 Optional properties:
 - timeout-sec: contains the watchdog timeout in seconds.
+- interrupts : Should contain WDT interrupt.
+- atmel,max-heartbeat-sec : Should contain the maximum heartbeat value in
+	seconds. This value should be less or equal to 16. It is used to
+	compute the WDV field.
+- atmel,min-heartbeat-sec : Should contain the minimum heartbeat value in
+	seconds. This value must be smaller than the max-heartbeat-sec value.
+	It is used to compute the WDD field.
+- atmel,watchdog-type : Should be "hardware" or "software". Hardware watchdog
+	use the at91 watchdog reset. Software watchdog use the watchdog
+	interrupt to trigger a software reset.
+- atmel,reset-type : Should be "proc" or "all".
+	"all" : assert peripherals and processor reset signals
+	"proc" : assert the processor reset signal
+	This is valid only when using "hardware" watchdog.
+- atmel,disable : Should be present if you want to disable the watchdog.
+- atmel,idle-halt : Should be present if you want to stop the watchdog when
+	entering idle state.
+- atmel,dbg-halt : Should be present if you want to stop the watchdog when
+	entering debug state.
 
 Example:
-
 	watchdog at fffffd40 {
 		compatible = "atmel,at91sam9260-wdt";
 		reg = <0xfffffd40 0x10>;
-		timeout-sec = <10>;
+		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+		timeout-sec = <15>;
+		atmel,watchdog-type = "hardware";
+		atmel,reset-type = "all";
+		atmel,dbg-halt;
+		atmel,idle-halt;
+		atmel,max-heartbeat-sec = <16>;
+		atmel,min-heartbeat-sec = <0>;
+		status = "okay";
 	};
-- 
1.7.9.5

Re: [PATCH v5 2/4] watchdog: at91sam9_wdt: update device tree doc

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:13AM +0200, Boris BREZILLON wrote:
> Add new at91sam9 watchdog properties to the documentation.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v5 2/4] watchdog: at91sam9_wdt: update device tree doc

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:13AM +0200, Boris BREZILLON wrote:
> Add new at91sam9 watchdog properties to the documentation.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v5 3/4] ARM: at91/dt: add sam9 watchdog default options to SoCs

[Boris BREZILLON]

Set default watchdog options in every SoC compatible with the sam9 watchdog.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 arch/arm/boot/dts/at91sam9260.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9263.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9g45.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9n12.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9x5.dtsi  |    5 +++++
 arch/arm/boot/dts/sama5d3.dtsi     |    5 +++++
 6 files changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi
index 56ee828..997901f 100644
--- a/arch/arm/boot/dts/at91sam9260.dtsi
+++ b/arch/arm/boot/dts/at91sam9260.dtsi
@@ -648,6 +648,11 @@
 			watchdog@fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 		};
diff --git a/arch/arm/boot/dts/at91sam9263.dtsi b/arch/arm/boot/dts/at91sam9263.dtsi
index d5bd65f..45fb0a4 100644
--- a/arch/arm/boot/dts/at91sam9263.dtsi
+++ b/arch/arm/boot/dts/at91sam9263.dtsi
@@ -523,6 +523,11 @@
 			watchdog@fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index c3e5148..16534c7 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -639,6 +639,11 @@
 			watchdog@fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/at91sam9n12.dtsi b/arch/arm/boot/dts/at91sam9n12.dtsi
index 9fb7ffd..eaef94b 100644
--- a/arch/arm/boot/dts/at91sam9n12.dtsi
+++ b/arch/arm/boot/dts/at91sam9n12.dtsi
@@ -537,6 +537,11 @@
 			watchdog@fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 		};
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index e74dc15..6d31fd7 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -820,6 +820,11 @@
 			watchdog@fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index b7f4961..3a17a3e 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -891,6 +891,11 @@
 			watchdog@fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <4 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
-- 
1.7.9.5

[PATCH v5 3/4] ARM: at91/dt: add sam9 watchdog default options to SoCs

[Boris BREZILLON]

Set default watchdog options in every SoC compatible with the sam9 watchdog.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 arch/arm/boot/dts/at91sam9260.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9263.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9g45.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9n12.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9x5.dtsi  |    5 +++++
 arch/arm/boot/dts/sama5d3.dtsi     |    5 +++++
 6 files changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi
index 56ee828..997901f 100644
--- a/arch/arm/boot/dts/at91sam9260.dtsi
+++ b/arch/arm/boot/dts/at91sam9260.dtsi
@@ -648,6 +648,11 @@
 			watchdog at fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 		};
diff --git a/arch/arm/boot/dts/at91sam9263.dtsi b/arch/arm/boot/dts/at91sam9263.dtsi
index d5bd65f..45fb0a4 100644
--- a/arch/arm/boot/dts/at91sam9263.dtsi
+++ b/arch/arm/boot/dts/at91sam9263.dtsi
@@ -523,6 +523,11 @@
 			watchdog at fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index c3e5148..16534c7 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -639,6 +639,11 @@
 			watchdog at fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/at91sam9n12.dtsi b/arch/arm/boot/dts/at91sam9n12.dtsi
index 9fb7ffd..eaef94b 100644
--- a/arch/arm/boot/dts/at91sam9n12.dtsi
+++ b/arch/arm/boot/dts/at91sam9n12.dtsi
@@ -537,6 +537,11 @@
 			watchdog at fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 		};
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index e74dc15..6d31fd7 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -820,6 +820,11 @@
 			watchdog at fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index b7f4961..3a17a3e 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -891,6 +891,11 @@
 			watchdog at fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <4 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
-- 
1.7.9.5

[PATCH v5 3/4] ARM: at91/dt: add sam9 watchdog default options to SoCs

[Boris BREZILLON]

Set default watchdog options in every SoC compatible with the sam9 watchdog.

Signed-off-by: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/at91sam9260.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9263.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9g45.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9n12.dtsi |    5 +++++
 arch/arm/boot/dts/at91sam9x5.dtsi  |    5 +++++
 arch/arm/boot/dts/sama5d3.dtsi     |    5 +++++
 6 files changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi
index 56ee828..997901f 100644
--- a/arch/arm/boot/dts/at91sam9260.dtsi
+++ b/arch/arm/boot/dts/at91sam9260.dtsi
@@ -648,6 +648,11 @@
 			watchdog@fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 		};
diff --git a/arch/arm/boot/dts/at91sam9263.dtsi b/arch/arm/boot/dts/at91sam9263.dtsi
index d5bd65f..45fb0a4 100644
--- a/arch/arm/boot/dts/at91sam9263.dtsi
+++ b/arch/arm/boot/dts/at91sam9263.dtsi
@@ -523,6 +523,11 @@
 			watchdog@fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index c3e5148..16534c7 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -639,6 +639,11 @@
 			watchdog@fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/at91sam9n12.dtsi b/arch/arm/boot/dts/at91sam9n12.dtsi
index 9fb7ffd..eaef94b 100644
--- a/arch/arm/boot/dts/at91sam9n12.dtsi
+++ b/arch/arm/boot/dts/at91sam9n12.dtsi
@@ -537,6 +537,11 @@
 			watchdog@fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 		};
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index e74dc15..6d31fd7 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -820,6 +820,11 @@
 			watchdog@fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index b7f4961..3a17a3e 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -891,6 +891,11 @@
 			watchdog@fffffe40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffe40 0x10>;
+				interrupts = <4 IRQ_TYPE_LEVEL_HIGH 7>;
+				atmel,watchdog-type = "hardware";
+				atmel,reset-type = "all";
+				atmel,dbg-halt;
+				atmel,idle-halt;
 				status = "disabled";
 			};
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 v5 3/4] ARM: at91/dt: add sam9 watchdog default options to SoCs

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:14AM +0200, Boris BREZILLON wrote:
> Set default watchdog options in every SoC compatible with the sam9 watchdog.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v5 3/4] ARM: at91/dt: add sam9 watchdog default options to SoCs

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:14AM +0200, Boris BREZILLON wrote:
> Set default watchdog options in every SoC compatible with the sam9 watchdog.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v5 4/4] ARM: at91/dt: add watchdog properties to kizbox board

[Boris BREZILLON]

Add watchdog specific config for kizbox board.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 arch/arm/boot/dts/kizbox.dts |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/kizbox.dts b/arch/arm/boot/dts/kizbox.dts
index 02df191..928f6ee 100644
--- a/arch/arm/boot/dts/kizbox.dts
+++ b/arch/arm/boot/dts/kizbox.dts
@@ -53,6 +53,12 @@
 				status = "okay";
 			};
 
+			watchdog@fffffd40 {
+				timeout-sec = <15>;
+				atmel,max-heartbeat-sec = <16>;
+				atmel,min-heartbeat-sec = <0>;
+				status = "okay";
+			};
 		};
 
 		nand0: nand@40000000 {
-- 
1.7.9.5

[PATCH v5 4/4] ARM: at91/dt: add watchdog properties to kizbox board

[Boris BREZILLON]

Add watchdog specific config for kizbox board.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 arch/arm/boot/dts/kizbox.dts |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/kizbox.dts b/arch/arm/boot/dts/kizbox.dts
index 02df191..928f6ee 100644
--- a/arch/arm/boot/dts/kizbox.dts
+++ b/arch/arm/boot/dts/kizbox.dts
@@ -53,6 +53,12 @@
 				status = "okay";
 			};
 
+			watchdog at fffffd40 {
+				timeout-sec = <15>;
+				atmel,max-heartbeat-sec = <16>;
+				atmel,min-heartbeat-sec = <0>;
+				status = "okay";
+			};
 		};
 
 		nand0: nand at 40000000 {
-- 
1.7.9.5

[PATCH v5 4/4] ARM: at91/dt: add watchdog properties to kizbox board

[Boris BREZILLON]

Add watchdog specific config for kizbox board.

Signed-off-by: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/kizbox.dts |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/kizbox.dts b/arch/arm/boot/dts/kizbox.dts
index 02df191..928f6ee 100644
--- a/arch/arm/boot/dts/kizbox.dts
+++ b/arch/arm/boot/dts/kizbox.dts
@@ -53,6 +53,12 @@
 				status = "okay";
 			};
 
+			watchdog@fffffd40 {
+				timeout-sec = <15>;
+				atmel,max-heartbeat-sec = <16>;
+				atmel,min-heartbeat-sec = <0>;
+				status = "okay";
+			};
 		};
 
 		nand0: nand@40000000 {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 v5 4/4] ARM: at91/dt: add watchdog properties to kizbox board

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:15AM +0200, Boris BREZILLON wrote:
> Add watchdog specific config for kizbox board.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

[PATCH v5 4/4] ARM: at91/dt: add watchdog properties to kizbox board

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:15AM +0200, Boris BREZILLON wrote:
> Add watchdog specific config for kizbox board.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH v5 4/4] ARM: at91/dt: add watchdog properties to kizbox board

[Guenter Roeck]

On Fri, Oct 04, 2013 at 09:24:15AM +0200, Boris BREZILLON wrote:
> Add watchdog specific config for kizbox board.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>

Acked-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[Wim Van Sebroeck]

Hi Boris,

> Hello,
> 
> This patch series is a porposal to enhance the sam9 watchdog timer support.
> 
> The at91sam9 watchdog timer can only be configured once, and the current
> implementation tries to configure it in a static way:
> - 2 seconds timeout
> - wdt restart every 500ms
> 
> If the timer has already been configured with different values, it returns an
> error and do not create any watchdog device.
> 
> This is not critical if the watchdog is disabled, but if it has been enabled
> with different timeout values it will lead to a SoC reset.
> 
> This patch series tries to address this issue by adapting the heartbeat value
> according the WDT timer config:
> - it first tries to configure the timer as requested.
> - if it fails it fallbacks to the current config, adapting its heartbeat timer
> to the needs
> 
> This patch series also move to a dynamically allocated at91wdt device instead
> of the static instance. I'm not sure this is the best solution, so please tell
> me if you prefer to keep static instance of watchdog.
> 
> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
> interrupt to trigger a software reboot.
> 
> Finally it adds several properties to the device tree bindings.
> 
> Best Regards,
> Boris
> 
> Changes since v4:
>  - fix coding style issues
>  - remove unneeded watchdog_active test
> 
> Changes since v3:
>  - fix a bug in heartbeat time computation
>  - fix a bug in at91_wdt_set_timeout when new timeout is bigger than the old
>    one
>  - rename at91_wdt_ping into at91_wdt_start
>  - remove unneeded ping callback assignment
> 
> Changes since v2:
>  - fix documentation
>  - rework the heartbeat computation to get a more flexible behaviour
>  - fix xx_to_yy macros
>  - modify warning and error messages
>  - remove unneeded parenthesis in arithmetic operations
>  - use devm functions to map io memory
>  - remove unneeded devm_kfree calls
> 
> Change since v1:
>  - fix typo in documentaion
>  - fix irq dt definition for sama5d3 SoC
> 
> 
> Boris BREZILLON (4):
>   watchdog: at91sam9_wdt: better watchdog support
>   watchdog: at91sam9_wdt: update device tree doc
>   ARM: at91/dt: add sam9 watchdog default options to SoCs
>   ARM: at91/dt: add watchdog properties to kizbox board
> 
>  .../devicetree/bindings/watchdog/atmel-wdt.txt     |   30 +-
>  arch/arm/boot/dts/at91sam9260.dtsi                 |    5 +
>  arch/arm/boot/dts/at91sam9263.dtsi                 |    5 +
>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    5 +
>  arch/arm/boot/dts/at91sam9n12.dtsi                 |    5 +
>  arch/arm/boot/dts/at91sam9x5.dtsi                  |    5 +
>  arch/arm/boot/dts/kizbox.dts                       |    6 +
>  arch/arm/boot/dts/sama5d3.dtsi                     |    5 +
>  drivers/watchdog/at91sam9_wdt.c                    |  309 ++++++++++++++------
>  9 files changed, 287 insertions(+), 88 deletions(-)

These 4 patches have been added to linux-watchdog-next.

Kind regards,
Wim.

Re: [PATCH v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[boris brezillon]

Hi Wim,

I'm sorry for the inconvenience, but I found some bugs in my patch series:

1) the secs_to_ticks returns an erronous value when 0 is passed as an 
argument
2) the calculated heartbeat is too small for some use cases
     (i.e. kexecing a new kernel might trigger a watchdog reset before 
the new kernel
      is able to load the watchdog driver)
3) when initializing the watchdog driver, the timer should be configured 
with the min_heartbeat value
     instead of the standard heartbeat value, because we don't for how 
long the timer has been running.

I'll send a new patch fixing those issues.
I hope it won't bother you :-(.

Best Regards,

Boris

On 29/10/2013 08:50, Wim Van Sebroeck wrote:
> Hi Boris,
>
>> Hello,
>>
>> This patch series is a porposal to enhance the sam9 watchdog timer support.
>>
>> The at91sam9 watchdog timer can only be configured once, and the current
>> implementation tries to configure it in a static way:
>> - 2 seconds timeout
>> - wdt restart every 500ms
>>
>> If the timer has already been configured with different values, it returns an
>> error and do not create any watchdog device.
>>
>> This is not critical if the watchdog is disabled, but if it has been enabled
>> with different timeout values it will lead to a SoC reset.
>>
>> This patch series tries to address this issue by adapting the heartbeat value
>> according the WDT timer config:
>> - it first tries to configure the timer as requested.
>> - if it fails it fallbacks to the current config, adapting its heartbeat timer
>> to the needs
>>
>> This patch series also move to a dynamically allocated at91wdt device instead
>> of the static instance. I'm not sure this is the best solution, so please tell
>> me if you prefer to keep static instance of watchdog.
>>
>> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
>> interrupt to trigger a software reboot.
>>
>> Finally it adds several properties to the device tree bindings.
>>
>> Best Regards,
>> Boris
>>
>> Changes since v4:
>>   - fix coding style issues
>>   - remove unneeded watchdog_active test
>>
>> Changes since v3:
>>   - fix a bug in heartbeat time computation
>>   - fix a bug in at91_wdt_set_timeout when new timeout is bigger than the old
>>     one
>>   - rename at91_wdt_ping into at91_wdt_start
>>   - remove unneeded ping callback assignment
>>
>> Changes since v2:
>>   - fix documentation
>>   - rework the heartbeat computation to get a more flexible behaviour
>>   - fix xx_to_yy macros
>>   - modify warning and error messages
>>   - remove unneeded parenthesis in arithmetic operations
>>   - use devm functions to map io memory
>>   - remove unneeded devm_kfree calls
>>
>> Change since v1:
>>   - fix typo in documentaion
>>   - fix irq dt definition for sama5d3 SoC
>>
>>
>> Boris BREZILLON (4):
>>    watchdog: at91sam9_wdt: better watchdog support
>>    watchdog: at91sam9_wdt: update device tree doc
>>    ARM: at91/dt: add sam9 watchdog default options to SoCs
>>    ARM: at91/dt: add watchdog properties to kizbox board
>>
>>   .../devicetree/bindings/watchdog/atmel-wdt.txt     |   30 +-
>>   arch/arm/boot/dts/at91sam9260.dtsi                 |    5 +
>>   arch/arm/boot/dts/at91sam9263.dtsi                 |    5 +
>>   arch/arm/boot/dts/at91sam9g45.dtsi                 |    5 +
>>   arch/arm/boot/dts/at91sam9n12.dtsi                 |    5 +
>>   arch/arm/boot/dts/at91sam9x5.dtsi                  |    5 +
>>   arch/arm/boot/dts/kizbox.dts                       |    6 +
>>   arch/arm/boot/dts/sama5d3.dtsi                     |    5 +
>>   drivers/watchdog/at91sam9_wdt.c                    |  309 ++++++++++++++------
>>   9 files changed, 287 insertions(+), 88 deletions(-)
> These 4 patches have been added to linux-watchdog-next.
>
> Kind regards,
> Wim.
>

[PATCH v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[boris brezillon]

Hi Wim,

I'm sorry for the inconvenience, but I found some bugs in my patch series:

1) the secs_to_ticks returns an erronous value when 0 is passed as an 
argument
2) the calculated heartbeat is too small for some use cases
     (i.e. kexecing a new kernel might trigger a watchdog reset before 
the new kernel
      is able to load the watchdog driver)
3) when initializing the watchdog driver, the timer should be configured 
with the min_heartbeat value
     instead of the standard heartbeat value, because we don't for how 
long the timer has been running.

I'll send a new patch fixing those issues.
I hope it won't bother you :-(.

Best Regards,

Boris

On 29/10/2013 08:50, Wim Van Sebroeck wrote:
> Hi Boris,
>
>> Hello,
>>
>> This patch series is a porposal to enhance the sam9 watchdog timer support.
>>
>> The at91sam9 watchdog timer can only be configured once, and the current
>> implementation tries to configure it in a static way:
>> - 2 seconds timeout
>> - wdt restart every 500ms
>>
>> If the timer has already been configured with different values, it returns an
>> error and do not create any watchdog device.
>>
>> This is not critical if the watchdog is disabled, but if it has been enabled
>> with different timeout values it will lead to a SoC reset.
>>
>> This patch series tries to address this issue by adapting the heartbeat value
>> according the WDT timer config:
>> - it first tries to configure the timer as requested.
>> - if it fails it fallbacks to the current config, adapting its heartbeat timer
>> to the needs
>>
>> This patch series also move to a dynamically allocated at91wdt device instead
>> of the static instance. I'm not sure this is the best solution, so please tell
>> me if you prefer to keep static instance of watchdog.
>>
>> It adds a new at91 wdt type: software. This new type make use of the at91 wdt
>> interrupt to trigger a software reboot.
>>
>> Finally it adds several properties to the device tree bindings.
>>
>> Best Regards,
>> Boris
>>
>> Changes since v4:
>>   - fix coding style issues
>>   - remove unneeded watchdog_active test
>>
>> Changes since v3:
>>   - fix a bug in heartbeat time computation
>>   - fix a bug in at91_wdt_set_timeout when new timeout is bigger than the old
>>     one
>>   - rename at91_wdt_ping into at91_wdt_start
>>   - remove unneeded ping callback assignment
>>
>> Changes since v2:
>>   - fix documentation
>>   - rework the heartbeat computation to get a more flexible behaviour
>>   - fix xx_to_yy macros
>>   - modify warning and error messages
>>   - remove unneeded parenthesis in arithmetic operations
>>   - use devm functions to map io memory
>>   - remove unneeded devm_kfree calls
>>
>> Change since v1:
>>   - fix typo in documentaion
>>   - fix irq dt definition for sama5d3 SoC
>>
>>
>> Boris BREZILLON (4):
>>    watchdog: at91sam9_wdt: better watchdog support
>>    watchdog: at91sam9_wdt: update device tree doc
>>    ARM: at91/dt: add sam9 watchdog default options to SoCs
>>    ARM: at91/dt: add watchdog properties to kizbox board
>>
>>   .../devicetree/bindings/watchdog/atmel-wdt.txt     |   30 +-
>>   arch/arm/boot/dts/at91sam9260.dtsi                 |    5 +
>>   arch/arm/boot/dts/at91sam9263.dtsi                 |    5 +
>>   arch/arm/boot/dts/at91sam9g45.dtsi                 |    5 +
>>   arch/arm/boot/dts/at91sam9n12.dtsi                 |    5 +
>>   arch/arm/boot/dts/at91sam9x5.dtsi                  |    5 +
>>   arch/arm/boot/dts/kizbox.dts                       |    6 +
>>   arch/arm/boot/dts/sama5d3.dtsi                     |    5 +
>>   drivers/watchdog/at91sam9_wdt.c                    |  309 ++++++++++++++------
>>   9 files changed, 287 insertions(+), 88 deletions(-)
> These 4 patches have been added to linux-watchdog-next.
>
> Kind regards,
> Wim.
>

Re: [PATCH v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[Wim Van Sebroeck]

Hi Boris,

> I'm sorry for the inconvenience, but I found some bugs in my patch series:
> 
> 1) the secs_to_ticks returns an erronous value when 0 is passed as an 
> argument
> 2) the calculated heartbeat is too small for some use cases
>     (i.e. kexecing a new kernel might trigger a watchdog reset before 
> the new kernel
>      is able to load the watchdog driver)
> 3) when initializing the watchdog driver, the timer should be configured 
> with the min_heartbeat value
>     instead of the standard heartbeat value, because we don't for how 
> long the timer has been running.
> 
> I'll send a new patch fixing those issues.
> I hope it won't bother you :-(.

No problem :-). Just sent me the fixes when they are ready.

Kind regards,
Wim.

Re: [PATCH v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[boris brezillon]

On 29/10/2013 13:58, Wim Van Sebroeck wrote:
> Hi Boris,
>
>> I'm sorry for the inconvenience, but I found some bugs in my patch series:
>>
>> 1) the secs_to_ticks returns an erronous value when 0 is passed as an
>> argument
>> 2) the calculated heartbeat is too small for some use cases
>>      (i.e. kexecing a new kernel might trigger a watchdog reset before
>> the new kernel
>>       is able to load the watchdog driver)
>> 3) when initializing the watchdog driver, the timer should be configured
>> with the min_heartbeat value
>>      instead of the standard heartbeat value, because we don't for how
>> long the timer has been running.
>>
>> I'll send a new patch fixing those issues.
>> I hope it won't bother you :-(.
> No problem :-). Just sent me the fixes when they are ready.

Already done : https://lkml.org/lkml/2013/10/29/207.

Thanks.

Best Regards,

Boris

>
> Kind regards,
> Wim.
>

[PATCH v5 0/4] watchdog: at91sam9_wdt: handle already configured wdt

[boris brezillon]

On 29/10/2013 13:58, Wim Van Sebroeck wrote:
> Hi Boris,
>
>> I'm sorry for the inconvenience, but I found some bugs in my patch series:
>>
>> 1) the secs_to_ticks returns an erronous value when 0 is passed as an
>> argument
>> 2) the calculated heartbeat is too small for some use cases
>>      (i.e. kexecing a new kernel might trigger a watchdog reset before
>> the new kernel
>>       is able to load the watchdog driver)
>> 3) when initializing the watchdog driver, the timer should be configured
>> with the min_heartbeat value
>>      instead of the standard heartbeat value, because we don't for how
>> long the timer has been running.
>>
>> I'll send a new patch fixing those issues.
>> I hope it won't bother you :-(.
> No problem :-). Just sent me the fixes when they are ready.

Already done : https://lkml.org/lkml/2013/10/29/207.

Thanks.

Best Regards,

Boris

>
> Kind regards,
> Wim.
>

[PATCH] watchdog: at91sam9_wdt: various fixes

[Boris BREZILLON]

Fix the secs_to_ticks macro in case 0 is passed as an argument.

Rework the heartbeat calculation to increase the security margin of the
watchdog reset timer.

Use the min_heartbeat value instead of the calculated heartbeat value for
the first watchdog reset.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 9bd089e..f1b59f1 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -51,7 +51,7 @@
 #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
 #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
 #define ticks_to_secs(t)		(((t) + 1) >> 8)
-#define secs_to_ticks(s)		(((s) << 8) - 1)
+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
 
 #define WDT_MR_RESET	0x3FFF2FFF
 
@@ -61,6 +61,11 @@
 /* Watchdog max delta/value in secs */
 #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
 
+/* Watchdog heartbeat shift used for security margin:
+ * we'll try to rshift the heartbeat value with this value to secure
+ * the watchdog reset. */
+#define WDT_HEARTBEAT_SHIFT	2
+
 /* Hardware timeout in seconds */
 #define WDT_HW_TIMEOUT 2
 
@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	int err;
 	u32 mask = wdt->mr_mask;
 	unsigned long min_heartbeat = 1;
+	unsigned long max_heartbeat;
 	struct device *dev = &pdev->dev;
+	int shift;
 
 	tmp = wdt_read(wdt, AT91_WDT_MR);
 	if ((tmp & mask) != (wdt->mr & mask)) {
@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	if (delta < value)
 		min_heartbeat = ticks_to_hz_roundup(value - delta);
 
-	wdt->heartbeat = ticks_to_hz_rounddown(value);
-	if (!wdt->heartbeat) {
+	max_heartbeat = ticks_to_hz_rounddown(value);
+	if (!max_heartbeat) {
 		dev_err(dev,
 			"heartbeat is too small for the system to handle it correctly\n");
 		return -EINVAL;
 	}
 
-	if (wdt->heartbeat < min_heartbeat + 4) {
+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
+		if ((max_heartbeat >> shift) < min_heartbeat)
+			continue;
+
+		wdt->heartbeat = max_heartbeat >> shift;
+		break;
+	}
+
+	if (!shift)
 		wdt->heartbeat = min_heartbeat;
+
+	if (max_heartbeat < min_heartbeat + 4)
 		dev_warn(dev,
 			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
-		if (wdt->heartbeat < 4)
-			dev_warn(dev,
-				 "heartbeat might be too small for the system to handle it correctly\n");
-	} else {
-		wdt->heartbeat -= 4;
-	}
 
 	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
 		err = request_irq(wdt->irq, wdt_interrupt,
@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
 
 	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+	/* Use min_heartbeat the first time because the watchdog timer might
+	 * be running for a long time when we reach this init function. */
+	mod_timer(&wdt->timer, jiffies + min_heartbeat);
 
 	/* Try to set timeout from device tree first */
 	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
-- 
1.7.9.5

[PATCH] watchdog: at91sam9_wdt: various fixes

[Boris BREZILLON]

Fix the secs_to_ticks macro in case 0 is passed as an argument.

Rework the heartbeat calculation to increase the security margin of the
watchdog reset timer.

Use the min_heartbeat value instead of the calculated heartbeat value for
the first watchdog reset.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 9bd089e..f1b59f1 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -51,7 +51,7 @@
 #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
 #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
 #define ticks_to_secs(t)		(((t) + 1) >> 8)
-#define secs_to_ticks(s)		(((s) << 8) - 1)
+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
 
 #define WDT_MR_RESET	0x3FFF2FFF
 
@@ -61,6 +61,11 @@
 /* Watchdog max delta/value in secs */
 #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
 
+/* Watchdog heartbeat shift used for security margin:
+ * we'll try to rshift the heartbeat value with this value to secure
+ * the watchdog reset. */
+#define WDT_HEARTBEAT_SHIFT	2
+
 /* Hardware timeout in seconds */
 #define WDT_HW_TIMEOUT 2
 
@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	int err;
 	u32 mask = wdt->mr_mask;
 	unsigned long min_heartbeat = 1;
+	unsigned long max_heartbeat;
 	struct device *dev = &pdev->dev;
+	int shift;
 
 	tmp = wdt_read(wdt, AT91_WDT_MR);
 	if ((tmp & mask) != (wdt->mr & mask)) {
@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	if (delta < value)
 		min_heartbeat = ticks_to_hz_roundup(value - delta);
 
-	wdt->heartbeat = ticks_to_hz_rounddown(value);
-	if (!wdt->heartbeat) {
+	max_heartbeat = ticks_to_hz_rounddown(value);
+	if (!max_heartbeat) {
 		dev_err(dev,
 			"heartbeat is too small for the system to handle it correctly\n");
 		return -EINVAL;
 	}
 
-	if (wdt->heartbeat < min_heartbeat + 4) {
+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
+		if ((max_heartbeat >> shift) < min_heartbeat)
+			continue;
+
+		wdt->heartbeat = max_heartbeat >> shift;
+		break;
+	}
+
+	if (!shift)
 		wdt->heartbeat = min_heartbeat;
+
+	if (max_heartbeat < min_heartbeat + 4)
 		dev_warn(dev,
 			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
-		if (wdt->heartbeat < 4)
-			dev_warn(dev,
-				 "heartbeat might be too small for the system to handle it correctly\n");
-	} else {
-		wdt->heartbeat -= 4;
-	}
 
 	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
 		err = request_irq(wdt->irq, wdt_interrupt,
@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
 
 	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
+	/* Use min_heartbeat the first time because the watchdog timer might
+	 * be running for a long time when we reach this init function. */
+	mod_timer(&wdt->timer, jiffies + min_heartbeat);
 
 	/* Try to set timeout from device tree first */
 	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
-- 
1.7.9.5

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[Guenter Roeck]

On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
> Fix the secs_to_ticks macro in case 0 is passed as an argument.
> 
> Rework the heartbeat calculation to increase the security margin of the
> watchdog reset timer.
> 
> Use the min_heartbeat value instead of the calculated heartbeat value for
> the first watchdog reset.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Hi Boris,

can you possibly split the three changes into separate patches ?

The first is a no-brainer. Gives my opinion of my code review capabilities
a slight damper ;-).

For the other two problems, it might make sense to describe
the problems you are trying to solve.

Couple of comments inline.

Thanks,
Guenter


> ---
>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> index 9bd089e..f1b59f1 100644
> --- a/drivers/watchdog/at91sam9_wdt.c
> +++ b/drivers/watchdog/at91sam9_wdt.c
> @@ -51,7 +51,7 @@
>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
> -#define secs_to_ticks(s)		(((s) << 8) - 1)
> +#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
>  
	(s)

>  #define WDT_MR_RESET	0x3FFF2FFF
>  
> @@ -61,6 +61,11 @@
>  /* Watchdog max delta/value in secs */
>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>  
> +/* Watchdog heartbeat shift used for security margin:
> + * we'll try to rshift the heartbeat value with this value to secure
> + * the watchdog reset. */
> +#define WDT_HEARTBEAT_SHIFT	2
> +
>  /* Hardware timeout in seconds */
>  #define WDT_HW_TIMEOUT 2
>  
> @@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>  	int err;
>  	u32 mask = wdt->mr_mask;
>  	unsigned long min_heartbeat = 1;
> +	unsigned long max_heartbeat;
>  	struct device *dev = &pdev->dev;
> +	int shift;
>  
>  	tmp = wdt_read(wdt, AT91_WDT_MR);
>  	if ((tmp & mask) != (wdt->mr & mask)) {
> @@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>  	if (delta < value)
>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
>  
> -	wdt->heartbeat = ticks_to_hz_rounddown(value);
> -	if (!wdt->heartbeat) {
> +	max_heartbeat = ticks_to_hz_rounddown(value);
> +	if (!max_heartbeat) {
>  		dev_err(dev,
>  			"heartbeat is too small for the system to handle it correctly\n");
>  		return -EINVAL;
>  	}
>  
> -	if (wdt->heartbeat < min_heartbeat + 4) {
> +	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
> +		if ((max_heartbeat >> shift) < min_heartbeat)
> +			continue;
> +
> +		wdt->heartbeat = max_heartbeat >> shift;
> +		break;
> +	}
> +
> +	if (!shift)
>  		wdt->heartbeat = min_heartbeat;

Correct me if I am wrong, but it seems to me that

	if ((max_heartbeat >> 2) >= min_heartbeat)
		 wdt->heartbeat = max_heartbeat >> 2;
	else if ((max_heartbeat >> 1) >= min_heartbeat)
		wdt->heartbeat = max_heartbeat >> 1;
	else
		wdt->heartbeat = min_heartbeat;

would accomplish the same and be easier to understand.

However, given that, I wonder if it is really necessary to bail out above if
max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
in this case.

> +
> +	if (max_heartbeat < min_heartbeat + 4)
>  		dev_warn(dev,
>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> -		if (wdt->heartbeat < 4)
> -			dev_warn(dev,
> -				 "heartbeat might be too small for the system to handle it correctly\n");
> -	} else {
> -		wdt->heartbeat -= 4;
> -	}
>  
>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>  		err = request_irq(wdt->irq, wdt_interrupt,
> @@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>  
>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> -	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> +	/* Use min_heartbeat the first time because the watchdog timer might
> +	 * be running for a long time when we reach this init function. */

	/*
	 * Multi-line comment style
	 *
	 * Not really sure I understand what this accomplishes. What problem
	 * are you trying to solve here ?
	 */

> +	mod_timer(&wdt->timer, jiffies + min_heartbeat);
>  
>  	/* Try to set timeout from device tree first */
>  	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] watchdog: at91sam9_wdt: various fixes

[Guenter Roeck]

On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
> Fix the secs_to_ticks macro in case 0 is passed as an argument.
> 
> Rework the heartbeat calculation to increase the security margin of the
> watchdog reset timer.
> 
> Use the min_heartbeat value instead of the calculated heartbeat value for
> the first watchdog reset.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>

Hi Boris,

can you possibly split the three changes into separate patches ?

The first is a no-brainer. Gives my opinion of my code review capabilities
a slight damper ;-).

For the other two problems, it might make sense to describe
the problems you are trying to solve.

Couple of comments inline.

Thanks,
Guenter


> ---
>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> index 9bd089e..f1b59f1 100644
> --- a/drivers/watchdog/at91sam9_wdt.c
> +++ b/drivers/watchdog/at91sam9_wdt.c
> @@ -51,7 +51,7 @@
>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
> -#define secs_to_ticks(s)		(((s) << 8) - 1)
> +#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
>  
	(s)

>  #define WDT_MR_RESET	0x3FFF2FFF
>  
> @@ -61,6 +61,11 @@
>  /* Watchdog max delta/value in secs */
>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>  
> +/* Watchdog heartbeat shift used for security margin:
> + * we'll try to rshift the heartbeat value with this value to secure
> + * the watchdog reset. */
> +#define WDT_HEARTBEAT_SHIFT	2
> +
>  /* Hardware timeout in seconds */
>  #define WDT_HW_TIMEOUT 2
>  
> @@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>  	int err;
>  	u32 mask = wdt->mr_mask;
>  	unsigned long min_heartbeat = 1;
> +	unsigned long max_heartbeat;
>  	struct device *dev = &pdev->dev;
> +	int shift;
>  
>  	tmp = wdt_read(wdt, AT91_WDT_MR);
>  	if ((tmp & mask) != (wdt->mr & mask)) {
> @@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>  	if (delta < value)
>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
>  
> -	wdt->heartbeat = ticks_to_hz_rounddown(value);
> -	if (!wdt->heartbeat) {
> +	max_heartbeat = ticks_to_hz_rounddown(value);
> +	if (!max_heartbeat) {
>  		dev_err(dev,
>  			"heartbeat is too small for the system to handle it correctly\n");
>  		return -EINVAL;
>  	}
>  
> -	if (wdt->heartbeat < min_heartbeat + 4) {
> +	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
> +		if ((max_heartbeat >> shift) < min_heartbeat)
> +			continue;
> +
> +		wdt->heartbeat = max_heartbeat >> shift;
> +		break;
> +	}
> +
> +	if (!shift)
>  		wdt->heartbeat = min_heartbeat;

Correct me if I am wrong, but it seems to me that

	if ((max_heartbeat >> 2) >= min_heartbeat)
		 wdt->heartbeat = max_heartbeat >> 2;
	else if ((max_heartbeat >> 1) >= min_heartbeat)
		wdt->heartbeat = max_heartbeat >> 1;
	else
		wdt->heartbeat = min_heartbeat;

would accomplish the same and be easier to understand.

However, given that, I wonder if it is really necessary to bail out above if
max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
in this case.

> +
> +	if (max_heartbeat < min_heartbeat + 4)
>  		dev_warn(dev,
>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> -		if (wdt->heartbeat < 4)
> -			dev_warn(dev,
> -				 "heartbeat might be too small for the system to handle it correctly\n");
> -	} else {
> -		wdt->heartbeat -= 4;
> -	}
>  
>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>  		err = request_irq(wdt->irq, wdt_interrupt,
> @@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>  
>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> -	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> +	/* Use min_heartbeat the first time because the watchdog timer might
> +	 * be running for a long time when we reach this init function. */

	/*
	 * Multi-line comment style
	 *
	 * Not really sure I understand what this accomplishes. What problem
	 * are you trying to solve here ?
	 */

> +	mod_timer(&wdt->timer, jiffies + min_heartbeat);
>  
>  	/* Try to set timeout from device tree first */
>  	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[boris brezillon]

On 29/10/2013 16:45, Guenter Roeck wrote:
> On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
>> Fix the secs_to_ticks macro in case 0 is passed as an argument.
>>
>> Rework the heartbeat calculation to increase the security margin of the
>> watchdog reset timer.
>>
>> Use the min_heartbeat value instead of the calculated heartbeat value for
>> the first watchdog reset.
>>
>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> Hi Boris,
>
> can you possibly split the three changes into separate patches ?

Sure. My first idea was to split this in 3 patches, but, as the buggy 
at91 watchdog series was
already applied to linux-watchdog-next, I thought it would be faster to 
only provide one
patch to fix all the issues at once.

>
> The first is a no-brainer. Gives my opinion of my code review capabilities
> a slight damper ;-).
>
> For the other two problems, it might make sense to describe
> the problems you are trying to solve.
>
> Couple of comments inline.
>
> Thanks,
> Guenter
>
>
>> ---
>>   drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
>> index 9bd089e..f1b59f1 100644
>> --- a/drivers/watchdog/at91sam9_wdt.c
>> +++ b/drivers/watchdog/at91sam9_wdt.c
>> @@ -51,7 +51,7 @@
>>   #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
>>   #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
>>   #define ticks_to_secs(t)		(((t) + 1) >> 8)
>> -#define secs_to_ticks(s)		(((s) << 8) - 1)
>> +#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
>>   
> 	(s)
>
>>   #define WDT_MR_RESET	0x3FFF2FFF
>>   
>> @@ -61,6 +61,11 @@
>>   /* Watchdog max delta/value in secs */
>>   #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>>   
>> +/* Watchdog heartbeat shift used for security margin:
>> + * we'll try to rshift the heartbeat value with this value to secure
>> + * the watchdog reset. */
>> +#define WDT_HEARTBEAT_SHIFT	2
>> +
>>   /* Hardware timeout in seconds */
>>   #define WDT_HW_TIMEOUT 2
>>   
>> @@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>   	int err;
>>   	u32 mask = wdt->mr_mask;
>>   	unsigned long min_heartbeat = 1;
>> +	unsigned long max_heartbeat;
>>   	struct device *dev = &pdev->dev;
>> +	int shift;
>>   
>>   	tmp = wdt_read(wdt, AT91_WDT_MR);
>>   	if ((tmp & mask) != (wdt->mr & mask)) {
>> @@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>   	if (delta < value)
>>   		min_heartbeat = ticks_to_hz_roundup(value - delta);
>>   
>> -	wdt->heartbeat = ticks_to_hz_rounddown(value);
>> -	if (!wdt->heartbeat) {
>> +	max_heartbeat = ticks_to_hz_rounddown(value);
>> +	if (!max_heartbeat) {
>>   		dev_err(dev,
>>   			"heartbeat is too small for the system to handle it correctly\n");
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (wdt->heartbeat < min_heartbeat + 4) {
>> +	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
>> +		if ((max_heartbeat >> shift) < min_heartbeat)
>> +			continue;
>> +
>> +		wdt->heartbeat = max_heartbeat >> shift;
>> +		break;
>> +	}
>> +
>> +	if (!shift)
>>   		wdt->heartbeat = min_heartbeat;
> Correct me if I am wrong, but it seems to me that
>
> 	if ((max_heartbeat >> 2) >= min_heartbeat)
> 		 wdt->heartbeat = max_heartbeat >> 2;
> 	else if ((max_heartbeat >> 1) >= min_heartbeat)
> 		wdt->heartbeat = max_heartbeat >> 1;
> 	else
> 		wdt->heartbeat = min_heartbeat;
>
> would accomplish the same and be easier to understand.

This is exactly what I'm trying to accomplish.
I used the for loop in case we ever want to change the 
WDT_HEARTBEAT_SHIFT value
(which is unlikely to happen).

I'll move to your proposition.

>
> However, given that, I wonder if it is really necessary to bail out above if
> max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
> in this case.

Yes it is necessary. The max_heartbeat is a configuration that cannot be 
changed once configured.
We have to inform the user that his max_heartbeat value is too small to 
be handled correctly by the Linux kernel.

If we simply use the min_heartbeat value as heartbeat and ignore the 
wrong max_heartbeat value,
the watchdog will reset the SoC before we can ever reset the watchdog 
counter.

>
>> +
>> +	if (max_heartbeat < min_heartbeat + 4)
>>   		dev_warn(dev,
>>   			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
>> -		if (wdt->heartbeat < 4)
>> -			dev_warn(dev,
>> -				 "heartbeat might be too small for the system to handle it correctly\n");
>> -	} else {
>> -		wdt->heartbeat -= 4;
>> -	}
>>   
>>   	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>>   		err = request_irq(wdt->irq, wdt_interrupt,
>> @@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>   			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>>   
>>   	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>> -	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>> +	/* Use min_heartbeat the first time because the watchdog timer might
>> +	 * be running for a long time when we reach this init function. */
> 	/*
> 	 * Multi-line comment style
> 	 *
> 	 * Not really sure I understand what this accomplishes. What problem
> 	 * are you trying to solve here ?
> 	 */

Sure, I'll change the comment style.

What, I'm trying to explain, is that the watchdog might (or should) have 
been resetted
before loading the linux kernel. But loading the kernel takes some time 
(uncompressing,
low level init, ...), and if we take the heartbeat (or max_heartbeat / 4 
in the common case) value as
the next trigger to reset the watchdog counter, the watchdog timer might 
have already expired.

Here is an example:
  - max_heartbeat configured to 8 seconds
  - min_heartbeat configured to 1 second
  => heartbeat = 2s
  - kernel boot time (before at91 watchdog is loaded) = 6 secs

If I use the heartbeat value when configuring the first expiration of 
the timer,
it might be too late to reset the watchdog counter.

I'll try to find a proper to explain this use case :-).

>> +	mod_timer(&wdt->timer, jiffies + min_heartbeat);
>>   
>>   	/* Try to set timeout from device tree first */
>>   	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

[PATCH] watchdog: at91sam9_wdt: various fixes

[boris brezillon]

On 29/10/2013 16:45, Guenter Roeck wrote:
> On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
>> Fix the secs_to_ticks macro in case 0 is passed as an argument.
>>
>> Rework the heartbeat calculation to increase the security margin of the
>> watchdog reset timer.
>>
>> Use the min_heartbeat value instead of the calculated heartbeat value for
>> the first watchdog reset.
>>
>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> Hi Boris,
>
> can you possibly split the three changes into separate patches ?

Sure. My first idea was to split this in 3 patches, but, as the buggy 
at91 watchdog series was
already applied to linux-watchdog-next, I thought it would be faster to 
only provide one
patch to fix all the issues at once.

>
> The first is a no-brainer. Gives my opinion of my code review capabilities
> a slight damper ;-).
>
> For the other two problems, it might make sense to describe
> the problems you are trying to solve.
>
> Couple of comments inline.
>
> Thanks,
> Guenter
>
>
>> ---
>>   drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
>> index 9bd089e..f1b59f1 100644
>> --- a/drivers/watchdog/at91sam9_wdt.c
>> +++ b/drivers/watchdog/at91sam9_wdt.c
>> @@ -51,7 +51,7 @@
>>   #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
>>   #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
>>   #define ticks_to_secs(t)		(((t) + 1) >> 8)
>> -#define secs_to_ticks(s)		(((s) << 8) - 1)
>> +#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
>>   
> 	(s)
>
>>   #define WDT_MR_RESET	0x3FFF2FFF
>>   
>> @@ -61,6 +61,11 @@
>>   /* Watchdog max delta/value in secs */
>>   #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>>   
>> +/* Watchdog heartbeat shift used for security margin:
>> + * we'll try to rshift the heartbeat value with this value to secure
>> + * the watchdog reset. */
>> +#define WDT_HEARTBEAT_SHIFT	2
>> +
>>   /* Hardware timeout in seconds */
>>   #define WDT_HW_TIMEOUT 2
>>   
>> @@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>   	int err;
>>   	u32 mask = wdt->mr_mask;
>>   	unsigned long min_heartbeat = 1;
>> +	unsigned long max_heartbeat;
>>   	struct device *dev = &pdev->dev;
>> +	int shift;
>>   
>>   	tmp = wdt_read(wdt, AT91_WDT_MR);
>>   	if ((tmp & mask) != (wdt->mr & mask)) {
>> @@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>   	if (delta < value)
>>   		min_heartbeat = ticks_to_hz_roundup(value - delta);
>>   
>> -	wdt->heartbeat = ticks_to_hz_rounddown(value);
>> -	if (!wdt->heartbeat) {
>> +	max_heartbeat = ticks_to_hz_rounddown(value);
>> +	if (!max_heartbeat) {
>>   		dev_err(dev,
>>   			"heartbeat is too small for the system to handle it correctly\n");
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (wdt->heartbeat < min_heartbeat + 4) {
>> +	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
>> +		if ((max_heartbeat >> shift) < min_heartbeat)
>> +			continue;
>> +
>> +		wdt->heartbeat = max_heartbeat >> shift;
>> +		break;
>> +	}
>> +
>> +	if (!shift)
>>   		wdt->heartbeat = min_heartbeat;
> Correct me if I am wrong, but it seems to me that
>
> 	if ((max_heartbeat >> 2) >= min_heartbeat)
> 		 wdt->heartbeat = max_heartbeat >> 2;
> 	else if ((max_heartbeat >> 1) >= min_heartbeat)
> 		wdt->heartbeat = max_heartbeat >> 1;
> 	else
> 		wdt->heartbeat = min_heartbeat;
>
> would accomplish the same and be easier to understand.

This is exactly what I'm trying to accomplish.
I used the for loop in case we ever want to change the 
WDT_HEARTBEAT_SHIFT value
(which is unlikely to happen).

I'll move to your proposition.

>
> However, given that, I wonder if it is really necessary to bail out above if
> max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
> in this case.

Yes it is necessary. The max_heartbeat is a configuration that cannot be 
changed once configured.
We have to inform the user that his max_heartbeat value is too small to 
be handled correctly by the Linux kernel.

If we simply use the min_heartbeat value as heartbeat and ignore the 
wrong max_heartbeat value,
the watchdog will reset the SoC before we can ever reset the watchdog 
counter.

>
>> +
>> +	if (max_heartbeat < min_heartbeat + 4)
>>   		dev_warn(dev,
>>   			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
>> -		if (wdt->heartbeat < 4)
>> -			dev_warn(dev,
>> -				 "heartbeat might be too small for the system to handle it correctly\n");
>> -	} else {
>> -		wdt->heartbeat -= 4;
>> -	}
>>   
>>   	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>>   		err = request_irq(wdt->irq, wdt_interrupt,
>> @@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>   			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>>   
>>   	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>> -	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>> +	/* Use min_heartbeat the first time because the watchdog timer might
>> +	 * be running for a long time when we reach this init function. */
> 	/*
> 	 * Multi-line comment style
> 	 *
> 	 * Not really sure I understand what this accomplishes. What problem
> 	 * are you trying to solve here ?
> 	 */

Sure, I'll change the comment style.

What, I'm trying to explain, is that the watchdog might (or should) have 
been resetted
before loading the linux kernel. But loading the kernel takes some time 
(uncompressing,
low level init, ...), and if we take the heartbeat (or max_heartbeat / 4 
in the common case) value as
the next trigger to reset the watchdog counter, the watchdog timer might 
have already expired.

Here is an example:
  - max_heartbeat configured to 8 seconds
  - min_heartbeat configured to 1 second
  => heartbeat = 2s
  - kernel boot time (before at91 watchdog is loaded) = 6 secs

If I use the heartbeat value when configuring the first expiration of 
the timer,
it might be too late to reset the watchdog counter.

I'll try to find a proper to explain this use case :-).

>> +	mod_timer(&wdt->timer, jiffies + min_heartbeat);
>>   
>>   	/* Try to set timeout from device tree first */
>>   	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[Guenter Roeck]

On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
> On 29/10/2013 16:45, Guenter Roeck wrote:
> >On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
> >>Fix the secs_to_ticks macro in case 0 is passed as an argument.
> >>
> >>Rework the heartbeat calculation to increase the security margin of the
> >>watchdog reset timer.
> >>
> >>Use the min_heartbeat value instead of the calculated heartbeat value for
> >>the first watchdog reset.
> >>
> >>Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> >Hi Boris,
> >
> >can you possibly split the three changes into separate patches ?
> 
> Sure. My first idea was to split this in 3 patches, but, as the
> buggy at91 watchdog series was
> already applied to linux-watchdog-next, I thought it would be faster
> to only provide one
> patch to fix all the issues at once.
> 
> >
> >The first is a no-brainer. Gives my opinion of my code review capabilities
> >a slight damper ;-).
> >
> >For the other two problems, it might make sense to describe
> >the problems you are trying to solve.
> >
> >Couple of comments inline.
> >
> >Thanks,
> >Guenter
> >
> >
> >>---
> >>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
> >>  1 file changed, 24 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> >>index 9bd089e..f1b59f1 100644
> >>--- a/drivers/watchdog/at91sam9_wdt.c
> >>+++ b/drivers/watchdog/at91sam9_wdt.c
> >>@@ -51,7 +51,7 @@
> >>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
> >>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
> >>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
> >>-#define secs_to_ticks(s)		(((s) << 8) - 1)
> >>+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
> >	(s)
> >
> >>  #define WDT_MR_RESET	0x3FFF2FFF
> >>@@ -61,6 +61,11 @@
> >>  /* Watchdog max delta/value in secs */
> >>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
> >>+/* Watchdog heartbeat shift used for security margin:
> >>+ * we'll try to rshift the heartbeat value with this value to secure
> >>+ * the watchdog reset. */
> >>+#define WDT_HEARTBEAT_SHIFT	2
> >>+
> >>  /* Hardware timeout in seconds */
> >>  #define WDT_HW_TIMEOUT 2
> >>@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>  	int err;
> >>  	u32 mask = wdt->mr_mask;
> >>  	unsigned long min_heartbeat = 1;
> >>+	unsigned long max_heartbeat;
> >>  	struct device *dev = &pdev->dev;
> >>+	int shift;
> >>  	tmp = wdt_read(wdt, AT91_WDT_MR);
> >>  	if ((tmp & mask) != (wdt->mr & mask)) {
> >>@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>  	if (delta < value)
> >>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
> >>-	wdt->heartbeat = ticks_to_hz_rounddown(value);
> >>-	if (!wdt->heartbeat) {
> >>+	max_heartbeat = ticks_to_hz_rounddown(value);
> >>+	if (!max_heartbeat) {
> >>  		dev_err(dev,
> >>  			"heartbeat is too small for the system to handle it correctly\n");
> >>  		return -EINVAL;
> >>  	}
> >>-	if (wdt->heartbeat < min_heartbeat + 4) {
> >>+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
> >>+		if ((max_heartbeat >> shift) < min_heartbeat)
> >>+			continue;
> >>+
> >>+		wdt->heartbeat = max_heartbeat >> shift;
> >>+		break;
> >>+	}
> >>+
> >>+	if (!shift)
> >>  		wdt->heartbeat = min_heartbeat;
> >Correct me if I am wrong, but it seems to me that
> >
> >	if ((max_heartbeat >> 2) >= min_heartbeat)
> >		 wdt->heartbeat = max_heartbeat >> 2;
> >	else if ((max_heartbeat >> 1) >= min_heartbeat)
> >		wdt->heartbeat = max_heartbeat >> 1;
> >	else
> >		wdt->heartbeat = min_heartbeat;
> >
> >would accomplish the same and be easier to understand.
> 
> This is exactly what I'm trying to accomplish.
> I used the for loop in case we ever want to change the
> WDT_HEARTBEAT_SHIFT value
> (which is unlikely to happen).
> 
> I'll move to your proposition.
> 
> >
> >However, given that, I wonder if it is really necessary to bail out above if
> >max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
> >in this case.
> 
> Yes it is necessary. The max_heartbeat is a configuration that
> cannot be changed once configured.
> We have to inform the user that his max_heartbeat value is too small
> to be handled correctly by the Linux kernel.
> 
> If we simply use the min_heartbeat value as heartbeat and ignore the
> wrong max_heartbeat value,
> the watchdog will reset the SoC before we can ever reset the
> watchdog counter.
> 
> >
> >>+
> >>+	if (max_heartbeat < min_heartbeat + 4)
> >>  		dev_warn(dev,
> >>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> >>-		if (wdt->heartbeat < 4)
> >>-			dev_warn(dev,
> >>-				 "heartbeat might be too small for the system to handle it correctly\n");
> >>-	} else {
> >>-		wdt->heartbeat -= 4;
> >>-	}
> >>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> >>  		err = request_irq(wdt->irq, wdt_interrupt,
> >>@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> >>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> >>-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> >>+	/* Use min_heartbeat the first time because the watchdog timer might
> >>+	 * be running for a long time when we reach this init function. */
> >	/*
> >	 * Multi-line comment style
> >	 *
> >	 * Not really sure I understand what this accomplishes. What problem
> >	 * are you trying to solve here ?
> >	 */
> 
> Sure, I'll change the comment style.
> 
> What, I'm trying to explain, is that the watchdog might (or should)
> have been resetted
> before loading the linux kernel. But loading the kernel takes some
> time (uncompressing,
> low level init, ...), and if we take the heartbeat (or max_heartbeat
> / 4 in the common case) value as
> the next trigger to reset the watchdog counter, the watchdog timer
> might have already expired.
> 
But increasing anything in the watchdog driver itself won't help you there.
You can not execute any kernel code before that kernel code is running.

> Here is an example:
>  - max_heartbeat configured to 8 seconds
>  - min_heartbeat configured to 1 second
>  => heartbeat = 2s
>  - kernel boot time (before at91 watchdog is loaded) = 6 secs
> 
Guess that is where I got lost. Do you mean the time from loading the driver 
to starting the watchdog application ? Because the init function is only
executed after the driver is loaded, so nothing will help you if it takes
too long for the driver to load.

You really have two times to deal with:
- Time from ROMMON handoff to loading the driver
  Nothing you can do there. If the watchdog fires before the driver is loaded,
  you are lost. Only way t handle this situation is to increase the timeout
  in the ROMMON.
- Time from loading driver to watchdog device open. This is really the time
  you are increasing with your change.

Thanks,
Guenter

> If I use the heartbeat value when configuring the first expiration
> of the timer,
> it might be too late to reset the watchdog counter.
> 
> I'll try to find a proper to explain this use case :-).
> 
> >>+	mod_timer(&wdt->timer, jiffies + min_heartbeat);
> >>  	/* Try to set timeout from device tree first */
> >>  	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
> >>-- 
> >>1.7.9.5
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] watchdog: at91sam9_wdt: various fixes

[Guenter Roeck]

On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
> On 29/10/2013 16:45, Guenter Roeck wrote:
> >On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
> >>Fix the secs_to_ticks macro in case 0 is passed as an argument.
> >>
> >>Rework the heartbeat calculation to increase the security margin of the
> >>watchdog reset timer.
> >>
> >>Use the min_heartbeat value instead of the calculated heartbeat value for
> >>the first watchdog reset.
> >>
> >>Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> >Hi Boris,
> >
> >can you possibly split the three changes into separate patches ?
> 
> Sure. My first idea was to split this in 3 patches, but, as the
> buggy at91 watchdog series was
> already applied to linux-watchdog-next, I thought it would be faster
> to only provide one
> patch to fix all the issues at once.
> 
> >
> >The first is a no-brainer. Gives my opinion of my code review capabilities
> >a slight damper ;-).
> >
> >For the other two problems, it might make sense to describe
> >the problems you are trying to solve.
> >
> >Couple of comments inline.
> >
> >Thanks,
> >Guenter
> >
> >
> >>---
> >>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
> >>  1 file changed, 24 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> >>index 9bd089e..f1b59f1 100644
> >>--- a/drivers/watchdog/at91sam9_wdt.c
> >>+++ b/drivers/watchdog/at91sam9_wdt.c
> >>@@ -51,7 +51,7 @@
> >>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
> >>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
> >>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
> >>-#define secs_to_ticks(s)		(((s) << 8) - 1)
> >>+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
> >	(s)
> >
> >>  #define WDT_MR_RESET	0x3FFF2FFF
> >>@@ -61,6 +61,11 @@
> >>  /* Watchdog max delta/value in secs */
> >>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
> >>+/* Watchdog heartbeat shift used for security margin:
> >>+ * we'll try to rshift the heartbeat value with this value to secure
> >>+ * the watchdog reset. */
> >>+#define WDT_HEARTBEAT_SHIFT	2
> >>+
> >>  /* Hardware timeout in seconds */
> >>  #define WDT_HW_TIMEOUT 2
> >>@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>  	int err;
> >>  	u32 mask = wdt->mr_mask;
> >>  	unsigned long min_heartbeat = 1;
> >>+	unsigned long max_heartbeat;
> >>  	struct device *dev = &pdev->dev;
> >>+	int shift;
> >>  	tmp = wdt_read(wdt, AT91_WDT_MR);
> >>  	if ((tmp & mask) != (wdt->mr & mask)) {
> >>@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>  	if (delta < value)
> >>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
> >>-	wdt->heartbeat = ticks_to_hz_rounddown(value);
> >>-	if (!wdt->heartbeat) {
> >>+	max_heartbeat = ticks_to_hz_rounddown(value);
> >>+	if (!max_heartbeat) {
> >>  		dev_err(dev,
> >>  			"heartbeat is too small for the system to handle it correctly\n");
> >>  		return -EINVAL;
> >>  	}
> >>-	if (wdt->heartbeat < min_heartbeat + 4) {
> >>+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
> >>+		if ((max_heartbeat >> shift) < min_heartbeat)
> >>+			continue;
> >>+
> >>+		wdt->heartbeat = max_heartbeat >> shift;
> >>+		break;
> >>+	}
> >>+
> >>+	if (!shift)
> >>  		wdt->heartbeat = min_heartbeat;
> >Correct me if I am wrong, but it seems to me that
> >
> >	if ((max_heartbeat >> 2) >= min_heartbeat)
> >		 wdt->heartbeat = max_heartbeat >> 2;
> >	else if ((max_heartbeat >> 1) >= min_heartbeat)
> >		wdt->heartbeat = max_heartbeat >> 1;
> >	else
> >		wdt->heartbeat = min_heartbeat;
> >
> >would accomplish the same and be easier to understand.
> 
> This is exactly what I'm trying to accomplish.
> I used the for loop in case we ever want to change the
> WDT_HEARTBEAT_SHIFT value
> (which is unlikely to happen).
> 
> I'll move to your proposition.
> 
> >
> >However, given that, I wonder if it is really necessary to bail out above if
> >max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
> >in this case.
> 
> Yes it is necessary. The max_heartbeat is a configuration that
> cannot be changed once configured.
> We have to inform the user that his max_heartbeat value is too small
> to be handled correctly by the Linux kernel.
> 
> If we simply use the min_heartbeat value as heartbeat and ignore the
> wrong max_heartbeat value,
> the watchdog will reset the SoC before we can ever reset the
> watchdog counter.
> 
> >
> >>+
> >>+	if (max_heartbeat < min_heartbeat + 4)
> >>  		dev_warn(dev,
> >>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> >>-		if (wdt->heartbeat < 4)
> >>-			dev_warn(dev,
> >>-				 "heartbeat might be too small for the system to handle it correctly\n");
> >>-	} else {
> >>-		wdt->heartbeat -= 4;
> >>-	}
> >>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> >>  		err = request_irq(wdt->irq, wdt_interrupt,
> >>@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> >>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> >>-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> >>+	/* Use min_heartbeat the first time because the watchdog timer might
> >>+	 * be running for a long time when we reach this init function. */
> >	/*
> >	 * Multi-line comment style
> >	 *
> >	 * Not really sure I understand what this accomplishes. What problem
> >	 * are you trying to solve here ?
> >	 */
> 
> Sure, I'll change the comment style.
> 
> What, I'm trying to explain, is that the watchdog might (or should)
> have been resetted
> before loading the linux kernel. But loading the kernel takes some
> time (uncompressing,
> low level init, ...), and if we take the heartbeat (or max_heartbeat
> / 4 in the common case) value as
> the next trigger to reset the watchdog counter, the watchdog timer
> might have already expired.
> 
But increasing anything in the watchdog driver itself won't help you there.
You can not execute any kernel code before that kernel code is running.

> Here is an example:
>  - max_heartbeat configured to 8 seconds
>  - min_heartbeat configured to 1 second
>  => heartbeat = 2s
>  - kernel boot time (before at91 watchdog is loaded) = 6 secs
> 
Guess that is where I got lost. Do you mean the time from loading the driver 
to starting the watchdog application ? Because the init function is only
executed after the driver is loaded, so nothing will help you if it takes
too long for the driver to load.

You really have two times to deal with:
- Time from ROMMON handoff to loading the driver
  Nothing you can do there. If the watchdog fires before the driver is loaded,
  you are lost. Only way t handle this situation is to increase the timeout
  in the ROMMON.
- Time from loading driver to watchdog device open. This is really the time
  you are increasing with your change.

Thanks,
Guenter

> If I use the heartbeat value when configuring the first expiration
> of the timer,
> it might be too late to reset the watchdog counter.
> 
> I'll try to find a proper to explain this use case :-).
> 
> >>+	mod_timer(&wdt->timer, jiffies + min_heartbeat);
> >>  	/* Try to set timeout from device tree first */
> >>  	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
> >>-- 
> >>1.7.9.5
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> >>the body of a message to majordomo at vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[boris brezillon]

On 29/10/2013 17:43, Guenter Roeck wrote:
> On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
>> On 29/10/2013 16:45, Guenter Roeck wrote:
>>> On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
>>>> Fix the secs_to_ticks macro in case 0 is passed as an argument.
>>>>
>>>> Rework the heartbeat calculation to increase the security margin of the
>>>> watchdog reset timer.
>>>>
>>>> Use the min_heartbeat value instead of the calculated heartbeat value for
>>>> the first watchdog reset.
>>>>
>>>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>>> Hi Boris,
>>>
>>> can you possibly split the three changes into separate patches ?
>> Sure. My first idea was to split this in 3 patches, but, as the
>> buggy at91 watchdog series was
>> already applied to linux-watchdog-next, I thought it would be faster
>> to only provide one
>> patch to fix all the issues at once.
>>
>>> The first is a no-brainer. Gives my opinion of my code review capabilities
>>> a slight damper ;-).
>>>
>>> For the other two problems, it might make sense to describe
>>> the problems you are trying to solve.
>>>
>>> Couple of comments inline.
>>>
>>> Thanks,
>>> Guenter
>>>
>>>
>>>> ---
>>>>   drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
>>>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
>>>> index 9bd089e..f1b59f1 100644
>>>> --- a/drivers/watchdog/at91sam9_wdt.c
>>>> +++ b/drivers/watchdog/at91sam9_wdt.c
>>>> @@ -51,7 +51,7 @@
>>>>   #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
>>>>   #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
>>>>   #define ticks_to_secs(t)		(((t) + 1) >> 8)
>>>> -#define secs_to_ticks(s)		(((s) << 8) - 1)
>>>> +#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
>>> 	(s)
>>>
>>>>   #define WDT_MR_RESET	0x3FFF2FFF
>>>> @@ -61,6 +61,11 @@
>>>>   /* Watchdog max delta/value in secs */
>>>>   #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>>>> +/* Watchdog heartbeat shift used for security margin:
>>>> + * we'll try to rshift the heartbeat value with this value to secure
>>>> + * the watchdog reset. */
>>>> +#define WDT_HEARTBEAT_SHIFT	2
>>>> +
>>>>   /* Hardware timeout in seconds */
>>>>   #define WDT_HW_TIMEOUT 2
>>>> @@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>>>   	int err;
>>>>   	u32 mask = wdt->mr_mask;
>>>>   	unsigned long min_heartbeat = 1;
>>>> +	unsigned long max_heartbeat;
>>>>   	struct device *dev = &pdev->dev;
>>>> +	int shift;
>>>>   	tmp = wdt_read(wdt, AT91_WDT_MR);
>>>>   	if ((tmp & mask) != (wdt->mr & mask)) {
>>>> @@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>>>   	if (delta < value)
>>>>   		min_heartbeat = ticks_to_hz_roundup(value - delta);
>>>> -	wdt->heartbeat = ticks_to_hz_rounddown(value);
>>>> -	if (!wdt->heartbeat) {
>>>> +	max_heartbeat = ticks_to_hz_rounddown(value);
>>>> +	if (!max_heartbeat) {
>>>>   		dev_err(dev,
>>>>   			"heartbeat is too small for the system to handle it correctly\n");
>>>>   		return -EINVAL;
>>>>   	}
>>>> -	if (wdt->heartbeat < min_heartbeat + 4) {
>>>> +	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
>>>> +		if ((max_heartbeat >> shift) < min_heartbeat)
>>>> +			continue;
>>>> +
>>>> +		wdt->heartbeat = max_heartbeat >> shift;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	if (!shift)
>>>>   		wdt->heartbeat = min_heartbeat;
>>> Correct me if I am wrong, but it seems to me that
>>>
>>> 	if ((max_heartbeat >> 2) >= min_heartbeat)
>>> 		 wdt->heartbeat = max_heartbeat >> 2;
>>> 	else if ((max_heartbeat >> 1) >= min_heartbeat)
>>> 		wdt->heartbeat = max_heartbeat >> 1;
>>> 	else
>>> 		wdt->heartbeat = min_heartbeat;
>>>
>>> would accomplish the same and be easier to understand.
>> This is exactly what I'm trying to accomplish.
>> I used the for loop in case we ever want to change the
>> WDT_HEARTBEAT_SHIFT value
>> (which is unlikely to happen).
>>
>> I'll move to your proposition.
>>
>>> However, given that, I wonder if it is really necessary to bail out above if
>>> max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
>>> in this case.
>> Yes it is necessary. The max_heartbeat is a configuration that
>> cannot be changed once configured.
>> We have to inform the user that his max_heartbeat value is too small
>> to be handled correctly by the Linux kernel.
>>
>> If we simply use the min_heartbeat value as heartbeat and ignore the
>> wrong max_heartbeat value,
>> the watchdog will reset the SoC before we can ever reset the
>> watchdog counter.
>>
>>>> +
>>>> +	if (max_heartbeat < min_heartbeat + 4)
>>>>   		dev_warn(dev,
>>>>   			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
>>>> -		if (wdt->heartbeat < 4)
>>>> -			dev_warn(dev,
>>>> -				 "heartbeat might be too small for the system to handle it correctly\n");
>>>> -	} else {
>>>> -		wdt->heartbeat -= 4;
>>>> -	}
>>>>   	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>>>>   		err = request_irq(wdt->irq, wdt_interrupt,
>>>> @@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>>>   			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>>>>   	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>>>> -	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>>>> +	/* Use min_heartbeat the first time because the watchdog timer might
>>>> +	 * be running for a long time when we reach this init function. */
>>> 	/*
>>> 	 * Multi-line comment style
>>> 	 *
>>> 	 * Not really sure I understand what this accomplishes. What problem
>>> 	 * are you trying to solve here ?
>>> 	 */
>> Sure, I'll change the comment style.
>>
>> What, I'm trying to explain, is that the watchdog might (or should)
>> have been resetted
>> before loading the linux kernel. But loading the kernel takes some
>> time (uncompressing,
>> low level init, ...), and if we take the heartbeat (or max_heartbeat
>> / 4 in the common case) value as
>> the next trigger to reset the watchdog counter, the watchdog timer
>> might have already expired.
>>
> But increasing anything in the watchdog driver itself won't help you there.
> You can not execute any kernel code before that kernel code is running.

Of course, but you can at least try to minimize the time between the 
watchdog driver init
and the first wathdog counter reset.

>> Here is an example:
>>   - max_heartbeat configured to 8 seconds
>>   - min_heartbeat configured to 1 second
>>   => heartbeat = 2s
>>   - kernel boot time (before at91 watchdog is loaded) = 6 secs
>>
> Guess that is where I got lost. Do you mean the time from loading the driver
> to starting the watchdog application ? Because the init function is only
> executed after the driver is loaded, so nothing will help you if it takes
> too long for the driver to load.

I think there is another bug here: the driver should not be compiled as 
a module.

Here is why:

At POR the at91 SoC configure its watchdog with these default values:
  - enabled
  - min heartbeat = 0 ticks
  - max heartbeat = 0xfff ticks <=> 16 secs
  - some reset options
After a POR the watchdog can only be reconfigured once (and only once).
This configuration oftenly takes place in the the bootstrap (or bootloader),
but can eventually be done by the Linux kernel.

If the watchdog is kept enabled by the bootstrap (or bootloader), this means
the linux kernel will have to reset the watchdog counter as soon as 
possible to avoid
a possible watchdog reset.

=> If we authorize the at91 sam9 watchdog to be compiled as a module, 
we're not sure
the module will be loaded soon enough to be able to reset the watchdog 
counter.

>
> You really have two times to deal with:
> - Time from ROMMON handoff to loading the driver
>    Nothing you can do there. If the watchdog fires before the driver is loaded,
>    you are lost. Only way t handle this situation is to increase the timeout
>    in the ROMMON.
> - Time from loading driver to watchdog device open. This is really the time
>    you are increasing with your change.

This is where it gets a bit tricky.

The heartbeat I'm talking about is not the "user space" heartbeat
(struct watchdog_device timeout field), it's the "hardware watchdog 
counter reset"
heartbeat (struct at91wdt heartbeat field).

Actually the at91 sam9 wdt driver does not provide a direct access to 
the watchdog reset
counter functionnality.
Instead, it periodically reset the watchdog counter (based on the timing 
config retrieved from
the hw registers), and eventually, if the user open a the watchdog dev 
file and fails to reset
the watchdog (using the user space API), the drivers stops resetting the 
hw counter, which leads
to a watchdog reset.

I hope I'm clear enough, cause it's quite complicated to explain.

Best Regards,

Boris

> Thanks,
> Guenter
>
>> If I use the heartbeat value when configuring the first expiration
>> of the timer,
>> it might be too late to reset the watchdog counter.
>>
>> I'll try to find a proper to explain this use case :-).
>>
>>>> +	mod_timer(&wdt->timer, jiffies + min_heartbeat);
>>>>   	/* Try to set timeout from device tree first */
>>>>   	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
>>>> -- 
>>>> 1.7.9.5
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

[PATCH] watchdog: at91sam9_wdt: various fixes

[boris brezillon]

On 29/10/2013 17:43, Guenter Roeck wrote:
> On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
>> On 29/10/2013 16:45, Guenter Roeck wrote:
>>> On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
>>>> Fix the secs_to_ticks macro in case 0 is passed as an argument.
>>>>
>>>> Rework the heartbeat calculation to increase the security margin of the
>>>> watchdog reset timer.
>>>>
>>>> Use the min_heartbeat value instead of the calculated heartbeat value for
>>>> the first watchdog reset.
>>>>
>>>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>>> Hi Boris,
>>>
>>> can you possibly split the three changes into separate patches ?
>> Sure. My first idea was to split this in 3 patches, but, as the
>> buggy at91 watchdog series was
>> already applied to linux-watchdog-next, I thought it would be faster
>> to only provide one
>> patch to fix all the issues at once.
>>
>>> The first is a no-brainer. Gives my opinion of my code review capabilities
>>> a slight damper ;-).
>>>
>>> For the other two problems, it might make sense to describe
>>> the problems you are trying to solve.
>>>
>>> Couple of comments inline.
>>>
>>> Thanks,
>>> Guenter
>>>
>>>
>>>> ---
>>>>   drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
>>>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
>>>> index 9bd089e..f1b59f1 100644
>>>> --- a/drivers/watchdog/at91sam9_wdt.c
>>>> +++ b/drivers/watchdog/at91sam9_wdt.c
>>>> @@ -51,7 +51,7 @@
>>>>   #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
>>>>   #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
>>>>   #define ticks_to_secs(t)		(((t) + 1) >> 8)
>>>> -#define secs_to_ticks(s)		(((s) << 8) - 1)
>>>> +#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
>>> 	(s)
>>>
>>>>   #define WDT_MR_RESET	0x3FFF2FFF
>>>> @@ -61,6 +61,11 @@
>>>>   /* Watchdog max delta/value in secs */
>>>>   #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>>>> +/* Watchdog heartbeat shift used for security margin:
>>>> + * we'll try to rshift the heartbeat value with this value to secure
>>>> + * the watchdog reset. */
>>>> +#define WDT_HEARTBEAT_SHIFT	2
>>>> +
>>>>   /* Hardware timeout in seconds */
>>>>   #define WDT_HW_TIMEOUT 2
>>>> @@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>>>   	int err;
>>>>   	u32 mask = wdt->mr_mask;
>>>>   	unsigned long min_heartbeat = 1;
>>>> +	unsigned long max_heartbeat;
>>>>   	struct device *dev = &pdev->dev;
>>>> +	int shift;
>>>>   	tmp = wdt_read(wdt, AT91_WDT_MR);
>>>>   	if ((tmp & mask) != (wdt->mr & mask)) {
>>>> @@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>>>   	if (delta < value)
>>>>   		min_heartbeat = ticks_to_hz_roundup(value - delta);
>>>> -	wdt->heartbeat = ticks_to_hz_rounddown(value);
>>>> -	if (!wdt->heartbeat) {
>>>> +	max_heartbeat = ticks_to_hz_rounddown(value);
>>>> +	if (!max_heartbeat) {
>>>>   		dev_err(dev,
>>>>   			"heartbeat is too small for the system to handle it correctly\n");
>>>>   		return -EINVAL;
>>>>   	}
>>>> -	if (wdt->heartbeat < min_heartbeat + 4) {
>>>> +	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
>>>> +		if ((max_heartbeat >> shift) < min_heartbeat)
>>>> +			continue;
>>>> +
>>>> +		wdt->heartbeat = max_heartbeat >> shift;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	if (!shift)
>>>>   		wdt->heartbeat = min_heartbeat;
>>> Correct me if I am wrong, but it seems to me that
>>>
>>> 	if ((max_heartbeat >> 2) >= min_heartbeat)
>>> 		 wdt->heartbeat = max_heartbeat >> 2;
>>> 	else if ((max_heartbeat >> 1) >= min_heartbeat)
>>> 		wdt->heartbeat = max_heartbeat >> 1;
>>> 	else
>>> 		wdt->heartbeat = min_heartbeat;
>>>
>>> would accomplish the same and be easier to understand.
>> This is exactly what I'm trying to accomplish.
>> I used the for loop in case we ever want to change the
>> WDT_HEARTBEAT_SHIFT value
>> (which is unlikely to happen).
>>
>> I'll move to your proposition.
>>
>>> However, given that, I wonder if it is really necessary to bail out above if
>>> max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
>>> in this case.
>> Yes it is necessary. The max_heartbeat is a configuration that
>> cannot be changed once configured.
>> We have to inform the user that his max_heartbeat value is too small
>> to be handled correctly by the Linux kernel.
>>
>> If we simply use the min_heartbeat value as heartbeat and ignore the
>> wrong max_heartbeat value,
>> the watchdog will reset the SoC before we can ever reset the
>> watchdog counter.
>>
>>>> +
>>>> +	if (max_heartbeat < min_heartbeat + 4)
>>>>   		dev_warn(dev,
>>>>   			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
>>>> -		if (wdt->heartbeat < 4)
>>>> -			dev_warn(dev,
>>>> -				 "heartbeat might be too small for the system to handle it correctly\n");
>>>> -	} else {
>>>> -		wdt->heartbeat -= 4;
>>>> -	}
>>>>   	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>>>>   		err = request_irq(wdt->irq, wdt_interrupt,
>>>> @@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>>>>   			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>>>>   	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>>>> -	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>>>> +	/* Use min_heartbeat the first time because the watchdog timer might
>>>> +	 * be running for a long time when we reach this init function. */
>>> 	/*
>>> 	 * Multi-line comment style
>>> 	 *
>>> 	 * Not really sure I understand what this accomplishes. What problem
>>> 	 * are you trying to solve here ?
>>> 	 */
>> Sure, I'll change the comment style.
>>
>> What, I'm trying to explain, is that the watchdog might (or should)
>> have been resetted
>> before loading the linux kernel. But loading the kernel takes some
>> time (uncompressing,
>> low level init, ...), and if we take the heartbeat (or max_heartbeat
>> / 4 in the common case) value as
>> the next trigger to reset the watchdog counter, the watchdog timer
>> might have already expired.
>>
> But increasing anything in the watchdog driver itself won't help you there.
> You can not execute any kernel code before that kernel code is running.

Of course, but you can at least try to minimize the time between the 
watchdog driver init
and the first wathdog counter reset.

>> Here is an example:
>>   - max_heartbeat configured to 8 seconds
>>   - min_heartbeat configured to 1 second
>>   => heartbeat = 2s
>>   - kernel boot time (before at91 watchdog is loaded) = 6 secs
>>
> Guess that is where I got lost. Do you mean the time from loading the driver
> to starting the watchdog application ? Because the init function is only
> executed after the driver is loaded, so nothing will help you if it takes
> too long for the driver to load.

I think there is another bug here: the driver should not be compiled as 
a module.

Here is why:

At POR the at91 SoC configure its watchdog with these default values:
  - enabled
  - min heartbeat = 0 ticks
  - max heartbeat = 0xfff ticks <=> 16 secs
  - some reset options
After a POR the watchdog can only be reconfigured once (and only once).
This configuration oftenly takes place in the the bootstrap (or bootloader),
but can eventually be done by the Linux kernel.

If the watchdog is kept enabled by the bootstrap (or bootloader), this means
the linux kernel will have to reset the watchdog counter as soon as 
possible to avoid
a possible watchdog reset.

=> If we authorize the at91 sam9 watchdog to be compiled as a module, 
we're not sure
the module will be loaded soon enough to be able to reset the watchdog 
counter.

>
> You really have two times to deal with:
> - Time from ROMMON handoff to loading the driver
>    Nothing you can do there. If the watchdog fires before the driver is loaded,
>    you are lost. Only way t handle this situation is to increase the timeout
>    in the ROMMON.
> - Time from loading driver to watchdog device open. This is really the time
>    you are increasing with your change.

This is where it gets a bit tricky.

The heartbeat I'm talking about is not the "user space" heartbeat
(struct watchdog_device timeout field), it's the "hardware watchdog 
counter reset"
heartbeat (struct at91wdt heartbeat field).

Actually the at91 sam9 wdt driver does not provide a direct access to 
the watchdog reset
counter functionnality.
Instead, it periodically reset the watchdog counter (based on the timing 
config retrieved from
the hw registers), and eventually, if the user open a the watchdog dev 
file and fails to reset
the watchdog (using the user space API), the drivers stops resetting the 
hw counter, which leads
to a watchdog reset.

I hope I'm clear enough, cause it's quite complicated to explain.

Best Regards,

Boris

> Thanks,
> Guenter
>
>> If I use the heartbeat value when configuring the first expiration
>> of the timer,
>> it might be too late to reset the watchdog counter.
>>
>> I'll try to find a proper to explain this use case :-).
>>
>>>> +	mod_timer(&wdt->timer, jiffies + min_heartbeat);
>>>>   	/* Try to set timeout from device tree first */
>>>>   	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
>>>> -- 
>>>> 1.7.9.5
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>>>> the body of a message to majordomo at vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[Guenter Roeck]

On Tue, Oct 29, 2013 at 06:22:47PM +0100, boris brezillon wrote:
> On 29/10/2013 17:43, Guenter Roeck wrote:
> >On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
> >>On 29/10/2013 16:45, Guenter Roeck wrote:
> >>>On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
> >>>>Fix the secs_to_ticks macro in case 0 is passed as an argument.
> >>>>
> >>>>Rework the heartbeat calculation to increase the security margin of the
> >>>>watchdog reset timer.
> >>>>
> >>>>Use the min_heartbeat value instead of the calculated heartbeat value for
> >>>>the first watchdog reset.
> >>>>
> >>>>Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> >>>Hi Boris,
> >>>
> >>>can you possibly split the three changes into separate patches ?
> >>Sure. My first idea was to split this in 3 patches, but, as the
> >>buggy at91 watchdog series was
> >>already applied to linux-watchdog-next, I thought it would be faster
> >>to only provide one
> >>patch to fix all the issues at once.
> >>
> >>>The first is a no-brainer. Gives my opinion of my code review capabilities
> >>>a slight damper ;-).
> >>>
> >>>For the other two problems, it might make sense to describe
> >>>the problems you are trying to solve.
> >>>
> >>>Couple of comments inline.
> >>>
> >>>Thanks,
> >>>Guenter
> >>>
> >>>
> >>>>---
> >>>>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
> >>>>  1 file changed, 24 insertions(+), 11 deletions(-)
> >>>>
> >>>>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> >>>>index 9bd089e..f1b59f1 100644
> >>>>--- a/drivers/watchdog/at91sam9_wdt.c
> >>>>+++ b/drivers/watchdog/at91sam9_wdt.c
> >>>>@@ -51,7 +51,7 @@
> >>>>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
> >>>>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
> >>>>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
> >>>>-#define secs_to_ticks(s)		(((s) << 8) - 1)
> >>>>+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
> >>>	(s)
> >>>
> >>>>  #define WDT_MR_RESET	0x3FFF2FFF
> >>>>@@ -61,6 +61,11 @@
> >>>>  /* Watchdog max delta/value in secs */
> >>>>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
> >>>>+/* Watchdog heartbeat shift used for security margin:
> >>>>+ * we'll try to rshift the heartbeat value with this value to secure
> >>>>+ * the watchdog reset. */
> >>>>+#define WDT_HEARTBEAT_SHIFT	2
> >>>>+
> >>>>  /* Hardware timeout in seconds */
> >>>>  #define WDT_HW_TIMEOUT 2
> >>>>@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>>>  	int err;
> >>>>  	u32 mask = wdt->mr_mask;
> >>>>  	unsigned long min_heartbeat = 1;
> >>>>+	unsigned long max_heartbeat;
> >>>>  	struct device *dev = &pdev->dev;
> >>>>+	int shift;
> >>>>  	tmp = wdt_read(wdt, AT91_WDT_MR);
> >>>>  	if ((tmp & mask) != (wdt->mr & mask)) {
> >>>>@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>>>  	if (delta < value)
> >>>>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
> >>>>-	wdt->heartbeat = ticks_to_hz_rounddown(value);
> >>>>-	if (!wdt->heartbeat) {
> >>>>+	max_heartbeat = ticks_to_hz_rounddown(value);
> >>>>+	if (!max_heartbeat) {
> >>>>  		dev_err(dev,
> >>>>  			"heartbeat is too small for the system to handle it correctly\n");
> >>>>  		return -EINVAL;
> >>>>  	}
> >>>>-	if (wdt->heartbeat < min_heartbeat + 4) {
> >>>>+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
> >>>>+		if ((max_heartbeat >> shift) < min_heartbeat)
> >>>>+			continue;
> >>>>+
> >>>>+		wdt->heartbeat = max_heartbeat >> shift;
> >>>>+		break;
> >>>>+	}
> >>>>+
> >>>>+	if (!shift)
> >>>>  		wdt->heartbeat = min_heartbeat;
> >>>Correct me if I am wrong, but it seems to me that
> >>>
> >>>	if ((max_heartbeat >> 2) >= min_heartbeat)
> >>>		 wdt->heartbeat = max_heartbeat >> 2;
> >>>	else if ((max_heartbeat >> 1) >= min_heartbeat)
> >>>		wdt->heartbeat = max_heartbeat >> 1;
> >>>	else
> >>>		wdt->heartbeat = min_heartbeat;
> >>>
> >>>would accomplish the same and be easier to understand.
> >>This is exactly what I'm trying to accomplish.
> >>I used the for loop in case we ever want to change the
> >>WDT_HEARTBEAT_SHIFT value
> >>(which is unlikely to happen).
> >>
> >>I'll move to your proposition.
> >>
> >>>However, given that, I wonder if it is really necessary to bail out above if
> >>>max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
> >>>in this case.
> >>Yes it is necessary. The max_heartbeat is a configuration that
> >>cannot be changed once configured.
> >>We have to inform the user that his max_heartbeat value is too small
> >>to be handled correctly by the Linux kernel.
> >>
> >>If we simply use the min_heartbeat value as heartbeat and ignore the
> >>wrong max_heartbeat value,
> >>the watchdog will reset the SoC before we can ever reset the
> >>watchdog counter.
> >>
> >>>>+
> >>>>+	if (max_heartbeat < min_heartbeat + 4)
> >>>>  		dev_warn(dev,
> >>>>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> >>>>-		if (wdt->heartbeat < 4)
> >>>>-			dev_warn(dev,
> >>>>-				 "heartbeat might be too small for the system to handle it correctly\n");
> >>>>-	} else {
> >>>>-		wdt->heartbeat -= 4;
> >>>>-	}
> >>>>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> >>>>  		err = request_irq(wdt->irq, wdt_interrupt,
> >>>>@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>>>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> >>>>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> >>>>-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> >>>>+	/* Use min_heartbeat the first time because the watchdog timer might
> >>>>+	 * be running for a long time when we reach this init function. */
> >>>	/*
> >>>	 * Multi-line comment style
> >>>	 *
> >>>	 * Not really sure I understand what this accomplishes. What problem
> >>>	 * are you trying to solve here ?
> >>>	 */
> >>Sure, I'll change the comment style.
> >>
> >>What, I'm trying to explain, is that the watchdog might (or should)
> >>have been resetted
> >>before loading the linux kernel. But loading the kernel takes some
> >>time (uncompressing,
> >>low level init, ...), and if we take the heartbeat (or max_heartbeat
> >>/ 4 in the common case) value as
> >>the next trigger to reset the watchdog counter, the watchdog timer
> >>might have already expired.
> >>
> >But increasing anything in the watchdog driver itself won't help you there.
> >You can not execute any kernel code before that kernel code is running.
> 
> Of course, but you can at least try to minimize the time between the
> watchdog driver init
> and the first wathdog counter reset.
> 
Sure.

> >>Here is an example:
> >>  - max_heartbeat configured to 8 seconds
> >>  - min_heartbeat configured to 1 second
> >>  => heartbeat = 2s
> >>  - kernel boot time (before at91 watchdog is loaded) = 6 secs
> >>
> >Guess that is where I got lost. Do you mean the time from loading the driver
> >to starting the watchdog application ? Because the init function is only
> >executed after the driver is loaded, so nothing will help you if it takes
> >too long for the driver to load.
> 
> I think there is another bug here: the driver should not be compiled
> as a module.
> 
> Here is why:
> 
> At POR the at91 SoC configure its watchdog with these default values:
>  - enabled
>  - min heartbeat = 0 ticks
>  - max heartbeat = 0xfff ticks <=> 16 secs
>  - some reset options
> After a POR the watchdog can only be reconfigured once (and only once).
> This configuration oftenly takes place in the the bootstrap (or bootloader),
> but can eventually be done by the Linux kernel.
> 
> If the watchdog is kept enabled by the bootstrap (or bootloader), this means
> the linux kernel will have to reset the watchdog counter as soon as
> possible to avoid
> a possible watchdog reset.
> 
> => If we authorize the at91 sam9 watchdog to be compiled as a
> module, we're not sure
> the module will be loaded soon enough to be able to reset the
> watchdog counter.
> 
Agreed, but that is only an issue _if_ the watchdog is enabled from ROMMON,
which we don't know. This makes it a configuration issue: If the watchdog 
is enabled by ROMMON, the driver should not be built as module. On the other
side, unless it is known for sure (say, from the HW architecture) that it
is always enabled, we should not force everyone to build it into the kernel.

Other drivers deal with that condition by only resetting and re-initializing
the watchdog if it is already running. This driver is a bit of an exception,
as it always enables the watchdog during initialization. Which is actually
another reason to be able to build it as module: If the watchdog was not
enabled by ROMMON, this ensures that it only starts running when the module
is loaded.

Thanks,
Guenter

> >
> >You really have two times to deal with:
> >- Time from ROMMON handoff to loading the driver
> >   Nothing you can do there. If the watchdog fires before the driver is loaded,
> >   you are lost. Only way t handle this situation is to increase the timeout
> >   in the ROMMON.
> >- Time from loading driver to watchdog device open. This is really the time
> >   you are increasing with your change.
> 
> This is where it gets a bit tricky.
> 
> The heartbeat I'm talking about is not the "user space" heartbeat
> (struct watchdog_device timeout field), it's the "hardware watchdog
> counter reset"
> heartbeat (struct at91wdt heartbeat field).
> 
> Actually the at91 sam9 wdt driver does not provide a direct access
> to the watchdog reset
> counter functionnality.
> Instead, it periodically reset the watchdog counter (based on the
> timing config retrieved from
> the hw registers), and eventually, if the user open a the watchdog
> dev file and fails to reset
> the watchdog (using the user space API), the drivers stops resetting
> the hw counter, which leads
> to a watchdog reset.
> 
> I hope I'm clear enough, cause it's quite complicated to explain.
> 
> Best Regards,
> 
> Boris
> 
> >Thanks,
> >Guenter
> >
> >>If I use the heartbeat value when configuring the first expiration
> >>of the timer,
> >>it might be too late to reset the watchdog counter.
> >>
> >>I'll try to find a proper to explain this use case :-).
> >>
> >>>>+	mod_timer(&wdt->timer, jiffies + min_heartbeat);
> >>>>  	/* Try to set timeout from device tree first */
> >>>>  	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
> >>>>-- 
> >>>>1.7.9.5
> >>>>
> >>>>--
> >>>>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> >>>>the body of a message to majordomo@vger.kernel.org
> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> 
> 

[PATCH] watchdog: at91sam9_wdt: various fixes

[Guenter Roeck]

On Tue, Oct 29, 2013 at 06:22:47PM +0100, boris brezillon wrote:
> On 29/10/2013 17:43, Guenter Roeck wrote:
> >On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
> >>On 29/10/2013 16:45, Guenter Roeck wrote:
> >>>On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
> >>>>Fix the secs_to_ticks macro in case 0 is passed as an argument.
> >>>>
> >>>>Rework the heartbeat calculation to increase the security margin of the
> >>>>watchdog reset timer.
> >>>>
> >>>>Use the min_heartbeat value instead of the calculated heartbeat value for
> >>>>the first watchdog reset.
> >>>>
> >>>>Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> >>>Hi Boris,
> >>>
> >>>can you possibly split the three changes into separate patches ?
> >>Sure. My first idea was to split this in 3 patches, but, as the
> >>buggy at91 watchdog series was
> >>already applied to linux-watchdog-next, I thought it would be faster
> >>to only provide one
> >>patch to fix all the issues at once.
> >>
> >>>The first is a no-brainer. Gives my opinion of my code review capabilities
> >>>a slight damper ;-).
> >>>
> >>>For the other two problems, it might make sense to describe
> >>>the problems you are trying to solve.
> >>>
> >>>Couple of comments inline.
> >>>
> >>>Thanks,
> >>>Guenter
> >>>
> >>>
> >>>>---
> >>>>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
> >>>>  1 file changed, 24 insertions(+), 11 deletions(-)
> >>>>
> >>>>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> >>>>index 9bd089e..f1b59f1 100644
> >>>>--- a/drivers/watchdog/at91sam9_wdt.c
> >>>>+++ b/drivers/watchdog/at91sam9_wdt.c
> >>>>@@ -51,7 +51,7 @@
> >>>>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
> >>>>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
> >>>>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
> >>>>-#define secs_to_ticks(s)		(((s) << 8) - 1)
> >>>>+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
> >>>	(s)
> >>>
> >>>>  #define WDT_MR_RESET	0x3FFF2FFF
> >>>>@@ -61,6 +61,11 @@
> >>>>  /* Watchdog max delta/value in secs */
> >>>>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
> >>>>+/* Watchdog heartbeat shift used for security margin:
> >>>>+ * we'll try to rshift the heartbeat value with this value to secure
> >>>>+ * the watchdog reset. */
> >>>>+#define WDT_HEARTBEAT_SHIFT	2
> >>>>+
> >>>>  /* Hardware timeout in seconds */
> >>>>  #define WDT_HW_TIMEOUT 2
> >>>>@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>>>  	int err;
> >>>>  	u32 mask = wdt->mr_mask;
> >>>>  	unsigned long min_heartbeat = 1;
> >>>>+	unsigned long max_heartbeat;
> >>>>  	struct device *dev = &pdev->dev;
> >>>>+	int shift;
> >>>>  	tmp = wdt_read(wdt, AT91_WDT_MR);
> >>>>  	if ((tmp & mask) != (wdt->mr & mask)) {
> >>>>@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>>>  	if (delta < value)
> >>>>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
> >>>>-	wdt->heartbeat = ticks_to_hz_rounddown(value);
> >>>>-	if (!wdt->heartbeat) {
> >>>>+	max_heartbeat = ticks_to_hz_rounddown(value);
> >>>>+	if (!max_heartbeat) {
> >>>>  		dev_err(dev,
> >>>>  			"heartbeat is too small for the system to handle it correctly\n");
> >>>>  		return -EINVAL;
> >>>>  	}
> >>>>-	if (wdt->heartbeat < min_heartbeat + 4) {
> >>>>+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
> >>>>+		if ((max_heartbeat >> shift) < min_heartbeat)
> >>>>+			continue;
> >>>>+
> >>>>+		wdt->heartbeat = max_heartbeat >> shift;
> >>>>+		break;
> >>>>+	}
> >>>>+
> >>>>+	if (!shift)
> >>>>  		wdt->heartbeat = min_heartbeat;
> >>>Correct me if I am wrong, but it seems to me that
> >>>
> >>>	if ((max_heartbeat >> 2) >= min_heartbeat)
> >>>		 wdt->heartbeat = max_heartbeat >> 2;
> >>>	else if ((max_heartbeat >> 1) >= min_heartbeat)
> >>>		wdt->heartbeat = max_heartbeat >> 1;
> >>>	else
> >>>		wdt->heartbeat = min_heartbeat;
> >>>
> >>>would accomplish the same and be easier to understand.
> >>This is exactly what I'm trying to accomplish.
> >>I used the for loop in case we ever want to change the
> >>WDT_HEARTBEAT_SHIFT value
> >>(which is unlikely to happen).
> >>
> >>I'll move to your proposition.
> >>
> >>>However, given that, I wonder if it is really necessary to bail out above if
> >>>max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
> >>>in this case.
> >>Yes it is necessary. The max_heartbeat is a configuration that
> >>cannot be changed once configured.
> >>We have to inform the user that his max_heartbeat value is too small
> >>to be handled correctly by the Linux kernel.
> >>
> >>If we simply use the min_heartbeat value as heartbeat and ignore the
> >>wrong max_heartbeat value,
> >>the watchdog will reset the SoC before we can ever reset the
> >>watchdog counter.
> >>
> >>>>+
> >>>>+	if (max_heartbeat < min_heartbeat + 4)
> >>>>  		dev_warn(dev,
> >>>>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> >>>>-		if (wdt->heartbeat < 4)
> >>>>-			dev_warn(dev,
> >>>>-				 "heartbeat might be too small for the system to handle it correctly\n");
> >>>>-	} else {
> >>>>-		wdt->heartbeat -= 4;
> >>>>-	}
> >>>>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> >>>>  		err = request_irq(wdt->irq, wdt_interrupt,
> >>>>@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >>>>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> >>>>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> >>>>-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> >>>>+	/* Use min_heartbeat the first time because the watchdog timer might
> >>>>+	 * be running for a long time when we reach this init function. */
> >>>	/*
> >>>	 * Multi-line comment style
> >>>	 *
> >>>	 * Not really sure I understand what this accomplishes. What problem
> >>>	 * are you trying to solve here ?
> >>>	 */
> >>Sure, I'll change the comment style.
> >>
> >>What, I'm trying to explain, is that the watchdog might (or should)
> >>have been resetted
> >>before loading the linux kernel. But loading the kernel takes some
> >>time (uncompressing,
> >>low level init, ...), and if we take the heartbeat (or max_heartbeat
> >>/ 4 in the common case) value as
> >>the next trigger to reset the watchdog counter, the watchdog timer
> >>might have already expired.
> >>
> >But increasing anything in the watchdog driver itself won't help you there.
> >You can not execute any kernel code before that kernel code is running.
> 
> Of course, but you can at least try to minimize the time between the
> watchdog driver init
> and the first wathdog counter reset.
> 
Sure.

> >>Here is an example:
> >>  - max_heartbeat configured to 8 seconds
> >>  - min_heartbeat configured to 1 second
> >>  => heartbeat = 2s
> >>  - kernel boot time (before at91 watchdog is loaded) = 6 secs
> >>
> >Guess that is where I got lost. Do you mean the time from loading the driver
> >to starting the watchdog application ? Because the init function is only
> >executed after the driver is loaded, so nothing will help you if it takes
> >too long for the driver to load.
> 
> I think there is another bug here: the driver should not be compiled
> as a module.
> 
> Here is why:
> 
> At POR the at91 SoC configure its watchdog with these default values:
>  - enabled
>  - min heartbeat = 0 ticks
>  - max heartbeat = 0xfff ticks <=> 16 secs
>  - some reset options
> After a POR the watchdog can only be reconfigured once (and only once).
> This configuration oftenly takes place in the the bootstrap (or bootloader),
> but can eventually be done by the Linux kernel.
> 
> If the watchdog is kept enabled by the bootstrap (or bootloader), this means
> the linux kernel will have to reset the watchdog counter as soon as
> possible to avoid
> a possible watchdog reset.
> 
> => If we authorize the at91 sam9 watchdog to be compiled as a
> module, we're not sure
> the module will be loaded soon enough to be able to reset the
> watchdog counter.
> 
Agreed, but that is only an issue _if_ the watchdog is enabled from ROMMON,
which we don't know. This makes it a configuration issue: If the watchdog 
is enabled by ROMMON, the driver should not be built as module. On the other
side, unless it is known for sure (say, from the HW architecture) that it
is always enabled, we should not force everyone to build it into the kernel.

Other drivers deal with that condition by only resetting and re-initializing
the watchdog if it is already running. This driver is a bit of an exception,
as it always enables the watchdog during initialization. Which is actually
another reason to be able to build it as module: If the watchdog was not
enabled by ROMMON, this ensures that it only starts running when the module
is loaded.

Thanks,
Guenter

> >
> >You really have two times to deal with:
> >- Time from ROMMON handoff to loading the driver
> >   Nothing you can do there. If the watchdog fires before the driver is loaded,
> >   you are lost. Only way t handle this situation is to increase the timeout
> >   in the ROMMON.
> >- Time from loading driver to watchdog device open. This is really the time
> >   you are increasing with your change.
> 
> This is where it gets a bit tricky.
> 
> The heartbeat I'm talking about is not the "user space" heartbeat
> (struct watchdog_device timeout field), it's the "hardware watchdog
> counter reset"
> heartbeat (struct at91wdt heartbeat field).
> 
> Actually the at91 sam9 wdt driver does not provide a direct access
> to the watchdog reset
> counter functionnality.
> Instead, it periodically reset the watchdog counter (based on the
> timing config retrieved from
> the hw registers), and eventually, if the user open a the watchdog
> dev file and fails to reset
> the watchdog (using the user space API), the drivers stops resetting
> the hw counter, which leads
> to a watchdog reset.
> 
> I hope I'm clear enough, cause it's quite complicated to explain.
> 
> Best Regards,
> 
> Boris
> 
> >Thanks,
> >Guenter
> >
> >>If I use the heartbeat value when configuring the first expiration
> >>of the timer,
> >>it might be too late to reset the watchdog counter.
> >>
> >>I'll try to find a proper to explain this use case :-).
> >>
> >>>>+	mod_timer(&wdt->timer, jiffies + min_heartbeat);
> >>>>  	/* Try to set timeout from device tree first */
> >>>>  	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
> >>>>-- 
> >>>>1.7.9.5
> >>>>
> >>>>--
> >>>>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> >>>>the body of a message to majordomo at vger.kernel.org
> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> >>the body of a message to majordomo at vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> 
> 

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[b.brezillon]

On Tue, 29 Oct 2013 14:27:38 -0700, Guenter Roeck <linux@roeck-us.net>
wrote:
> On Tue, Oct 29, 2013 at 06:22:47PM +0100, boris brezillon wrote:
>> On 29/10/2013 17:43, Guenter Roeck wrote:
>> >On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
>> >>On 29/10/2013 16:45, Guenter Roeck wrote:
>> >>>On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
>> >>>>Fix the secs_to_ticks macro in case 0 is passed as an argument.
>> >>>>
>> >>>>Rework the heartbeat calculation to increase the security margin of the
>> >>>>watchdog reset timer.
>> >>>>
>> >>>>Use the min_heartbeat value instead of the calculated heartbeat value for
>> >>>>the first watchdog reset.
>> >>>>
>> >>>>Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>> >>>Hi Boris,
>> >>>
>> >>>can you possibly split the three changes into separate patches ?
>> >>Sure. My first idea was to split this in 3 patches, but, as the
>> >>buggy at91 watchdog series was
>> >>already applied to linux-watchdog-next, I thought it would be faster
>> >>to only provide one
>> >>patch to fix all the issues at once.
>> >>
>> >>>The first is a no-brainer. Gives my opinion of my code review capabilities
>> >>>a slight damper ;-).
>> >>>
>> >>>For the other two problems, it might make sense to describe
>> >>>the problems you are trying to solve.
>> >>>
>> >>>Couple of comments inline.
>> >>>
>> >>>Thanks,
>> >>>Guenter
>> >>>
>> >>>
>> >>>>---
>> >>>>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
>> >>>>  1 file changed, 24 insertions(+), 11 deletions(-)
>> >>>>
>> >>>>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
>> >>>>index 9bd089e..f1b59f1 100644
>> >>>>--- a/drivers/watchdog/at91sam9_wdt.c
>> >>>>+++ b/drivers/watchdog/at91sam9_wdt.c
>> >>>>@@ -51,7 +51,7 @@
>> >>>>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
>> >>>>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
>> >>>>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
>> >>>>-#define secs_to_ticks(s)		(((s) << 8) - 1)
>> >>>>+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
>> >>>	(s)
>> >>>
>> >>>>  #define WDT_MR_RESET	0x3FFF2FFF
>> >>>>@@ -61,6 +61,11 @@
>> >>>>  /* Watchdog max delta/value in secs */
>> >>>>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>> >>>>+/* Watchdog heartbeat shift used for security margin:
>> >>>>+ * we'll try to rshift the heartbeat value with this value to secure
>> >>>>+ * the watchdog reset. */
>> >>>>+#define WDT_HEARTBEAT_SHIFT	2
>> >>>>+
>> >>>>  /* Hardware timeout in seconds */
>> >>>>  #define WDT_HW_TIMEOUT 2
>> >>>>@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>> >>>>  	int err;
>> >>>>  	u32 mask = wdt->mr_mask;
>> >>>>  	unsigned long min_heartbeat = 1;
>> >>>>+	unsigned long max_heartbeat;
>> >>>>  	struct device *dev = &pdev->dev;
>> >>>>+	int shift;
>> >>>>  	tmp = wdt_read(wdt, AT91_WDT_MR);
>> >>>>  	if ((tmp & mask) != (wdt->mr & mask)) {
>> >>>>@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>> >>>>  	if (delta < value)
>> >>>>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
>> >>>>-	wdt->heartbeat = ticks_to_hz_rounddown(value);
>> >>>>-	if (!wdt->heartbeat) {
>> >>>>+	max_heartbeat = ticks_to_hz_rounddown(value);
>> >>>>+	if (!max_heartbeat) {
>> >>>>  		dev_err(dev,
>> >>>>  			"heartbeat is too small for the system to handle it correctly\n");
>> >>>>  		return -EINVAL;
>> >>>>  	}
>> >>>>-	if (wdt->heartbeat < min_heartbeat + 4) {
>> >>>>+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
>> >>>>+		if ((max_heartbeat >> shift) < min_heartbeat)
>> >>>>+			continue;
>> >>>>+
>> >>>>+		wdt->heartbeat = max_heartbeat >> shift;
>> >>>>+		break;
>> >>>>+	}
>> >>>>+
>> >>>>+	if (!shift)
>> >>>>  		wdt->heartbeat = min_heartbeat;
>> >>>Correct me if I am wrong, but it seems to me that
>> >>>
>> >>>	if ((max_heartbeat >> 2) >= min_heartbeat)
>> >>>		 wdt->heartbeat = max_heartbeat >> 2;
>> >>>	else if ((max_heartbeat >> 1) >= min_heartbeat)
>> >>>		wdt->heartbeat = max_heartbeat >> 1;
>> >>>	else
>> >>>		wdt->heartbeat = min_heartbeat;
>> >>>
>> >>>would accomplish the same and be easier to understand.
>> >>This is exactly what I'm trying to accomplish.
>> >>I used the for loop in case we ever want to change the
>> >>WDT_HEARTBEAT_SHIFT value
>> >>(which is unlikely to happen).
>> >>
>> >>I'll move to your proposition.
>> >>
>> >>>However, given that, I wonder if it is really necessary to bail out above if
>> >>>max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
>> >>>in this case.
>> >>Yes it is necessary. The max_heartbeat is a configuration that
>> >>cannot be changed once configured.
>> >>We have to inform the user that his max_heartbeat value is too small
>> >>to be handled correctly by the Linux kernel.
>> >>
>> >>If we simply use the min_heartbeat value as heartbeat and ignore the
>> >>wrong max_heartbeat value,
>> >>the watchdog will reset the SoC before we can ever reset the
>> >>watchdog counter.
>> >>
>> >>>>+
>> >>>>+	if (max_heartbeat < min_heartbeat + 4)
>> >>>>  		dev_warn(dev,
>> >>>>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
>> >>>>-		if (wdt->heartbeat < 4)
>> >>>>-			dev_warn(dev,
>> >>>>-				 "heartbeat might be too small for the system to handle it correctly\n");
>> >>>>-	} else {
>> >>>>-		wdt->heartbeat -= 4;
>> >>>>-	}
>> >>>>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>> >>>>  		err = request_irq(wdt->irq, wdt_interrupt,
>> >>>>@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>> >>>>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>> >>>>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>> >>>>-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>> >>>>+	/* Use min_heartbeat the first time because the watchdog timer might
>> >>>>+	 * be running for a long time when we reach this init function. */
>> >>>	/*
>> >>>	 * Multi-line comment style
>> >>>	 *
>> >>>	 * Not really sure I understand what this accomplishes. What problem
>> >>>	 * are you trying to solve here ?
>> >>>	 */
>> >>Sure, I'll change the comment style.
>> >>
>> >>What, I'm trying to explain, is that the watchdog might (or should)
>> >>have been resetted
>> >>before loading the linux kernel. But loading the kernel takes some
>> >>time (uncompressing,
>> >>low level init, ...), and if we take the heartbeat (or max_heartbeat
>> >>/ 4 in the common case) value as
>> >>the next trigger to reset the watchdog counter, the watchdog timer
>> >>might have already expired.
>> >>
>> >But increasing anything in the watchdog driver itself won't help you there.
>> >You can not execute any kernel code before that kernel code is running.
>>
>> Of course, but you can at least try to minimize the time between the
>> watchdog driver init
>> and the first wathdog counter reset.
>>
> Sure.
> 
>> >>Here is an example:
>> >>  - max_heartbeat configured to 8 seconds
>> >>  - min_heartbeat configured to 1 second
>> >>  => heartbeat = 2s
>> >>  - kernel boot time (before at91 watchdog is loaded) = 6 secs
>> >>
>> >Guess that is where I got lost. Do you mean the time from loading the driver
>> >to starting the watchdog application ? Because the init function is only
>> >executed after the driver is loaded, so nothing will help you if it takes
>> >too long for the driver to load.
>>
>> I think there is another bug here: the driver should not be compiled
>> as a module.
>>
>> Here is why:
>>
>> At POR the at91 SoC configure its watchdog with these default values:
>>  - enabled
>>  - min heartbeat = 0 ticks
>>  - max heartbeat = 0xfff ticks <=> 16 secs
>>  - some reset options
>> After a POR the watchdog can only be reconfigured once (and only once).
>> This configuration oftenly takes place in the the bootstrap (or bootloader),
>> but can eventually be done by the Linux kernel.
>>
>> If the watchdog is kept enabled by the bootstrap (or bootloader), this means
>> the linux kernel will have to reset the watchdog counter as soon as
>> possible to avoid
>> a possible watchdog reset.
>>
>> => If we authorize the at91 sam9 watchdog to be compiled as a
>> module, we're not sure
>> the module will be loaded soon enough to be able to reset the
>> watchdog counter.
>>
> Agreed, but that is only an issue _if_ the watchdog is enabled from ROMMON,
> which we don't know. This makes it a configuration issue: If the watchdog 
> is enabled by ROMMON, the driver should not be built as module. On the other
> side, unless it is known for sure (say, from the HW architecture) that it
> is always enabled, we should not force everyone to build it into the kernel.
> 

This is the case: the HW enable the watchdog with default timings
(min_heartbeat = 0 secs max_heartbeat = 16 secs) after a Reset or a
Power On
Reset.

The enable/disable bit is part of the config and thus can only be
configured once.

You then have these 2 use cases:

1) The ROMMON does not reconfigure the watchdog
   => The linux kernel must reset the watchdog counter within 16 secs.
2) The ROMMON reconfigure the watchdog
   a) The ROMMON enable the watchdog with different timings
      => The linux kernel must reset the watchdog counter within X secs
(according
         to the ROMMON config).
   b) The ROMMON disable the watchdog
      => The watchdog is unusable and the linux watchdog driver is
useless


BTW, 16 seconds should be enough to
 - load the kernel
 - mount a rootfs
 - load the at91 sam9 watchdog module



> Other drivers deal with that condition by only resetting and re-initializing
> the watchdog if it is already running. This driver is a bit of an exception,
> as it always enables the watchdog during initialization. Which is actually
> another reason to be able to build it as module: If the watchdog was not
> enabled by ROMMON, this ensures that it only starts running when the module
> is loaded.
> 
> Thanks,
> Guenter
> 
>> >
>> >You really have two times to deal with:
>> >- Time from ROMMON handoff to loading the driver
>> >   Nothing you can do there. If the watchdog fires before the driver is loaded,
>> >   you are lost. Only way t handle this situation is to increase the timeout
>> >   in the ROMMON.
>> >- Time from loading driver to watchdog device open. This is really the time
>> >   you are increasing with your change.
>>
>> This is where it gets a bit tricky.
>>
>> The heartbeat I'm talking about is not the "user space" heartbeat
>> (struct watchdog_device timeout field), it's the "hardware watchdog
>> counter reset"
>> heartbeat (struct at91wdt heartbeat field).
>>
>> Actually the at91 sam9 wdt driver does not provide a direct access
>> to the watchdog reset
>> counter functionnality.
>> Instead, it periodically reset the watchdog counter (based on the
>> timing config retrieved from
>> the hw registers), and eventually, if the user open a the watchdog
>> dev file and fails to reset
>> the watchdog (using the user space API), the drivers stops resetting
>> the hw counter, which leads
>> to a watchdog reset.
>>
>> I hope I'm clear enough, cause it's quite complicated to explain.
>>
>> Best Regards,
>>
>> Boris
>>
>> >Thanks,
>> >Guenter
>> >
>> >>If I use the heartbeat value when configuring the first expiration
>> >>of the timer,
>> >>it might be too late to reset the watchdog counter.
>> >>
>> >>I'll try to find a proper to explain this use case :-).
>> >>
>> >>>>+	mod_timer(&wdt->timer, jiffies + min_heartbeat);
>> >>>>  	/* Try to set timeout from device tree first */
>> >>>>  	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
>> >>>>--
>> >>>>1.7.9.5
>> >>>>
>> >>>>--
>> >>>>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> >>>>the body of a message to majordomo@vger.kernel.org
>> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>>>
>> >>--
>> >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> >>the body of a message to majordomo@vger.kernel.org
>> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>
>>
>>

[PATCH] watchdog: at91sam9_wdt: various fixes

[b.brezillon at overkiz.com]

On Tue, 29 Oct 2013 14:27:38 -0700, Guenter Roeck <linux@roeck-us.net>
wrote:
> On Tue, Oct 29, 2013 at 06:22:47PM +0100, boris brezillon wrote:
>> On 29/10/2013 17:43, Guenter Roeck wrote:
>> >On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
>> >>On 29/10/2013 16:45, Guenter Roeck wrote:
>> >>>On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
>> >>>>Fix the secs_to_ticks macro in case 0 is passed as an argument.
>> >>>>
>> >>>>Rework the heartbeat calculation to increase the security margin of the
>> >>>>watchdog reset timer.
>> >>>>
>> >>>>Use the min_heartbeat value instead of the calculated heartbeat value for
>> >>>>the first watchdog reset.
>> >>>>
>> >>>>Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>> >>>Hi Boris,
>> >>>
>> >>>can you possibly split the three changes into separate patches ?
>> >>Sure. My first idea was to split this in 3 patches, but, as the
>> >>buggy at91 watchdog series was
>> >>already applied to linux-watchdog-next, I thought it would be faster
>> >>to only provide one
>> >>patch to fix all the issues at once.
>> >>
>> >>>The first is a no-brainer. Gives my opinion of my code review capabilities
>> >>>a slight damper ;-).
>> >>>
>> >>>For the other two problems, it might make sense to describe
>> >>>the problems you are trying to solve.
>> >>>
>> >>>Couple of comments inline.
>> >>>
>> >>>Thanks,
>> >>>Guenter
>> >>>
>> >>>
>> >>>>---
>> >>>>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
>> >>>>  1 file changed, 24 insertions(+), 11 deletions(-)
>> >>>>
>> >>>>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
>> >>>>index 9bd089e..f1b59f1 100644
>> >>>>--- a/drivers/watchdog/at91sam9_wdt.c
>> >>>>+++ b/drivers/watchdog/at91sam9_wdt.c
>> >>>>@@ -51,7 +51,7 @@
>> >>>>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
>> >>>>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
>> >>>>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
>> >>>>-#define secs_to_ticks(s)		(((s) << 8) - 1)
>> >>>>+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
>> >>>	(s)
>> >>>
>> >>>>  #define WDT_MR_RESET	0x3FFF2FFF
>> >>>>@@ -61,6 +61,11 @@
>> >>>>  /* Watchdog max delta/value in secs */
>> >>>>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>> >>>>+/* Watchdog heartbeat shift used for security margin:
>> >>>>+ * we'll try to rshift the heartbeat value with this value to secure
>> >>>>+ * the watchdog reset. */
>> >>>>+#define WDT_HEARTBEAT_SHIFT	2
>> >>>>+
>> >>>>  /* Hardware timeout in seconds */
>> >>>>  #define WDT_HW_TIMEOUT 2
>> >>>>@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>> >>>>  	int err;
>> >>>>  	u32 mask = wdt->mr_mask;
>> >>>>  	unsigned long min_heartbeat = 1;
>> >>>>+	unsigned long max_heartbeat;
>> >>>>  	struct device *dev = &pdev->dev;
>> >>>>+	int shift;
>> >>>>  	tmp = wdt_read(wdt, AT91_WDT_MR);
>> >>>>  	if ((tmp & mask) != (wdt->mr & mask)) {
>> >>>>@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>> >>>>  	if (delta < value)
>> >>>>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
>> >>>>-	wdt->heartbeat = ticks_to_hz_rounddown(value);
>> >>>>-	if (!wdt->heartbeat) {
>> >>>>+	max_heartbeat = ticks_to_hz_rounddown(value);
>> >>>>+	if (!max_heartbeat) {
>> >>>>  		dev_err(dev,
>> >>>>  			"heartbeat is too small for the system to handle it correctly\n");
>> >>>>  		return -EINVAL;
>> >>>>  	}
>> >>>>-	if (wdt->heartbeat < min_heartbeat + 4) {
>> >>>>+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
>> >>>>+		if ((max_heartbeat >> shift) < min_heartbeat)
>> >>>>+			continue;
>> >>>>+
>> >>>>+		wdt->heartbeat = max_heartbeat >> shift;
>> >>>>+		break;
>> >>>>+	}
>> >>>>+
>> >>>>+	if (!shift)
>> >>>>  		wdt->heartbeat = min_heartbeat;
>> >>>Correct me if I am wrong, but it seems to me that
>> >>>
>> >>>	if ((max_heartbeat >> 2) >= min_heartbeat)
>> >>>		 wdt->heartbeat = max_heartbeat >> 2;
>> >>>	else if ((max_heartbeat >> 1) >= min_heartbeat)
>> >>>		wdt->heartbeat = max_heartbeat >> 1;
>> >>>	else
>> >>>		wdt->heartbeat = min_heartbeat;
>> >>>
>> >>>would accomplish the same and be easier to understand.
>> >>This is exactly what I'm trying to accomplish.
>> >>I used the for loop in case we ever want to change the
>> >>WDT_HEARTBEAT_SHIFT value
>> >>(which is unlikely to happen).
>> >>
>> >>I'll move to your proposition.
>> >>
>> >>>However, given that, I wonder if it is really necessary to bail out above if
>> >>>max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
>> >>>in this case.
>> >>Yes it is necessary. The max_heartbeat is a configuration that
>> >>cannot be changed once configured.
>> >>We have to inform the user that his max_heartbeat value is too small
>> >>to be handled correctly by the Linux kernel.
>> >>
>> >>If we simply use the min_heartbeat value as heartbeat and ignore the
>> >>wrong max_heartbeat value,
>> >>the watchdog will reset the SoC before we can ever reset the
>> >>watchdog counter.
>> >>
>> >>>>+
>> >>>>+	if (max_heartbeat < min_heartbeat + 4)
>> >>>>  		dev_warn(dev,
>> >>>>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
>> >>>>-		if (wdt->heartbeat < 4)
>> >>>>-			dev_warn(dev,
>> >>>>-				 "heartbeat might be too small for the system to handle it correctly\n");
>> >>>>-	} else {
>> >>>>-		wdt->heartbeat -= 4;
>> >>>>-	}
>> >>>>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>> >>>>  		err = request_irq(wdt->irq, wdt_interrupt,
>> >>>>@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>> >>>>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>> >>>>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>> >>>>-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>> >>>>+	/* Use min_heartbeat the first time because the watchdog timer might
>> >>>>+	 * be running for a long time when we reach this init function. */
>> >>>	/*
>> >>>	 * Multi-line comment style
>> >>>	 *
>> >>>	 * Not really sure I understand what this accomplishes. What problem
>> >>>	 * are you trying to solve here ?
>> >>>	 */
>> >>Sure, I'll change the comment style.
>> >>
>> >>What, I'm trying to explain, is that the watchdog might (or should)
>> >>have been resetted
>> >>before loading the linux kernel. But loading the kernel takes some
>> >>time (uncompressing,
>> >>low level init, ...), and if we take the heartbeat (or max_heartbeat
>> >>/ 4 in the common case) value as
>> >>the next trigger to reset the watchdog counter, the watchdog timer
>> >>might have already expired.
>> >>
>> >But increasing anything in the watchdog driver itself won't help you there.
>> >You can not execute any kernel code before that kernel code is running.
>>
>> Of course, but you can at least try to minimize the time between the
>> watchdog driver init
>> and the first wathdog counter reset.
>>
> Sure.
> 
>> >>Here is an example:
>> >>  - max_heartbeat configured to 8 seconds
>> >>  - min_heartbeat configured to 1 second
>> >>  => heartbeat = 2s
>> >>  - kernel boot time (before at91 watchdog is loaded) = 6 secs
>> >>
>> >Guess that is where I got lost. Do you mean the time from loading the driver
>> >to starting the watchdog application ? Because the init function is only
>> >executed after the driver is loaded, so nothing will help you if it takes
>> >too long for the driver to load.
>>
>> I think there is another bug here: the driver should not be compiled
>> as a module.
>>
>> Here is why:
>>
>> At POR the at91 SoC configure its watchdog with these default values:
>>  - enabled
>>  - min heartbeat = 0 ticks
>>  - max heartbeat = 0xfff ticks <=> 16 secs
>>  - some reset options
>> After a POR the watchdog can only be reconfigured once (and only once).
>> This configuration oftenly takes place in the the bootstrap (or bootloader),
>> but can eventually be done by the Linux kernel.
>>
>> If the watchdog is kept enabled by the bootstrap (or bootloader), this means
>> the linux kernel will have to reset the watchdog counter as soon as
>> possible to avoid
>> a possible watchdog reset.
>>
>> => If we authorize the at91 sam9 watchdog to be compiled as a
>> module, we're not sure
>> the module will be loaded soon enough to be able to reset the
>> watchdog counter.
>>
> Agreed, but that is only an issue _if_ the watchdog is enabled from ROMMON,
> which we don't know. This makes it a configuration issue: If the watchdog 
> is enabled by ROMMON, the driver should not be built as module. On the other
> side, unless it is known for sure (say, from the HW architecture) that it
> is always enabled, we should not force everyone to build it into the kernel.
> 

This is the case: the HW enable the watchdog with default timings
(min_heartbeat = 0 secs max_heartbeat = 16 secs) after a Reset or a
Power On
Reset.

The enable/disable bit is part of the config and thus can only be
configured once.

You then have these 2 use cases:

1) The ROMMON does not reconfigure the watchdog
   => The linux kernel must reset the watchdog counter within 16 secs.
2) The ROMMON reconfigure the watchdog
   a) The ROMMON enable the watchdog with different timings
      => The linux kernel must reset the watchdog counter within X secs
(according
         to the ROMMON config).
   b) The ROMMON disable the watchdog
      => The watchdog is unusable and the linux watchdog driver is
useless


BTW, 16 seconds should be enough to
 - load the kernel
 - mount a rootfs
 - load the at91 sam9 watchdog module



> Other drivers deal with that condition by only resetting and re-initializing
> the watchdog if it is already running. This driver is a bit of an exception,
> as it always enables the watchdog during initialization. Which is actually
> another reason to be able to build it as module: If the watchdog was not
> enabled by ROMMON, this ensures that it only starts running when the module
> is loaded.
> 
> Thanks,
> Guenter
> 
>> >
>> >You really have two times to deal with:
>> >- Time from ROMMON handoff to loading the driver
>> >   Nothing you can do there. If the watchdog fires before the driver is loaded,
>> >   you are lost. Only way t handle this situation is to increase the timeout
>> >   in the ROMMON.
>> >- Time from loading driver to watchdog device open. This is really the time
>> >   you are increasing with your change.
>>
>> This is where it gets a bit tricky.
>>
>> The heartbeat I'm talking about is not the "user space" heartbeat
>> (struct watchdog_device timeout field), it's the "hardware watchdog
>> counter reset"
>> heartbeat (struct at91wdt heartbeat field).
>>
>> Actually the at91 sam9 wdt driver does not provide a direct access
>> to the watchdog reset
>> counter functionnality.
>> Instead, it periodically reset the watchdog counter (based on the
>> timing config retrieved from
>> the hw registers), and eventually, if the user open a the watchdog
>> dev file and fails to reset
>> the watchdog (using the user space API), the drivers stops resetting
>> the hw counter, which leads
>> to a watchdog reset.
>>
>> I hope I'm clear enough, cause it's quite complicated to explain.
>>
>> Best Regards,
>>
>> Boris
>>
>> >Thanks,
>> >Guenter
>> >
>> >>If I use the heartbeat value when configuring the first expiration
>> >>of the timer,
>> >>it might be too late to reset the watchdog counter.
>> >>
>> >>I'll try to find a proper to explain this use case :-).
>> >>
>> >>>>+	mod_timer(&wdt->timer, jiffies + min_heartbeat);
>> >>>>  	/* Try to set timeout from device tree first */
>> >>>>  	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
>> >>>>--
>> >>>>1.7.9.5
>> >>>>
>> >>>>--
>> >>>>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> >>>>the body of a message to majordomo at vger.kernel.org
>> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>>>
>> >>--
>> >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> >>the body of a message to majordomo at vger.kernel.org
>> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>
>>
>>

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:27 Tue 29 Oct     , Guenter Roeck wrote:
> On Tue, Oct 29, 2013 at 06:22:47PM +0100, boris brezillon wrote:
> > On 29/10/2013 17:43, Guenter Roeck wrote:
> > >On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
> > >>On 29/10/2013 16:45, Guenter Roeck wrote:
> > >>>On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
> > >>>>Fix the secs_to_ticks macro in case 0 is passed as an argument.
> > >>>>
> > >>>>Rework the heartbeat calculation to increase the security margin of the
> > >>>>watchdog reset timer.
> > >>>>
> > >>>>Use the min_heartbeat value instead of the calculated heartbeat value for
> > >>>>the first watchdog reset.
> > >>>>
> > >>>>Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> > >>>Hi Boris,
> > >>>
> > >>>can you possibly split the three changes into separate patches ?
> > >>Sure. My first idea was to split this in 3 patches, but, as the
> > >>buggy at91 watchdog series was
> > >>already applied to linux-watchdog-next, I thought it would be faster
> > >>to only provide one
> > >>patch to fix all the issues at once.
> > >>
> > >>>The first is a no-brainer. Gives my opinion of my code review capabilities
> > >>>a slight damper ;-).
> > >>>
> > >>>For the other two problems, it might make sense to describe
> > >>>the problems you are trying to solve.
> > >>>
> > >>>Couple of comments inline.
> > >>>
> > >>>Thanks,
> > >>>Guenter
> > >>>
> > >>>
> > >>>>---
> > >>>>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
> > >>>>  1 file changed, 24 insertions(+), 11 deletions(-)
> > >>>>
> > >>>>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> > >>>>index 9bd089e..f1b59f1 100644
> > >>>>--- a/drivers/watchdog/at91sam9_wdt.c
> > >>>>+++ b/drivers/watchdog/at91sam9_wdt.c
> > >>>>@@ -51,7 +51,7 @@
> > >>>>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
> > >>>>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
> > >>>>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
> > >>>>-#define secs_to_ticks(s)		(((s) << 8) - 1)
> > >>>>+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
> > >>>	(s)
> > >>>
> > >>>>  #define WDT_MR_RESET	0x3FFF2FFF
> > >>>>@@ -61,6 +61,11 @@
> > >>>>  /* Watchdog max delta/value in secs */
> > >>>>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
> > >>>>+/* Watchdog heartbeat shift used for security margin:
> > >>>>+ * we'll try to rshift the heartbeat value with this value to secure
> > >>>>+ * the watchdog reset. */
> > >>>>+#define WDT_HEARTBEAT_SHIFT	2
> > >>>>+
> > >>>>  /* Hardware timeout in seconds */
> > >>>>  #define WDT_HW_TIMEOUT 2
> > >>>>@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> > >>>>  	int err;
> > >>>>  	u32 mask = wdt->mr_mask;
> > >>>>  	unsigned long min_heartbeat = 1;
> > >>>>+	unsigned long max_heartbeat;
> > >>>>  	struct device *dev = &pdev->dev;
> > >>>>+	int shift;
> > >>>>  	tmp = wdt_read(wdt, AT91_WDT_MR);
> > >>>>  	if ((tmp & mask) != (wdt->mr & mask)) {
> > >>>>@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> > >>>>  	if (delta < value)
> > >>>>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
> > >>>>-	wdt->heartbeat = ticks_to_hz_rounddown(value);
> > >>>>-	if (!wdt->heartbeat) {
> > >>>>+	max_heartbeat = ticks_to_hz_rounddown(value);
> > >>>>+	if (!max_heartbeat) {
> > >>>>  		dev_err(dev,
> > >>>>  			"heartbeat is too small for the system to handle it correctly\n");
> > >>>>  		return -EINVAL;
> > >>>>  	}
> > >>>>-	if (wdt->heartbeat < min_heartbeat + 4) {
> > >>>>+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
> > >>>>+		if ((max_heartbeat >> shift) < min_heartbeat)
> > >>>>+			continue;
> > >>>>+
> > >>>>+		wdt->heartbeat = max_heartbeat >> shift;
> > >>>>+		break;
> > >>>>+	}
> > >>>>+
> > >>>>+	if (!shift)
> > >>>>  		wdt->heartbeat = min_heartbeat;
> > >>>Correct me if I am wrong, but it seems to me that
> > >>>
> > >>>	if ((max_heartbeat >> 2) >= min_heartbeat)
> > >>>		 wdt->heartbeat = max_heartbeat >> 2;
> > >>>	else if ((max_heartbeat >> 1) >= min_heartbeat)
> > >>>		wdt->heartbeat = max_heartbeat >> 1;
> > >>>	else
> > >>>		wdt->heartbeat = min_heartbeat;
> > >>>
> > >>>would accomplish the same and be easier to understand.
> > >>This is exactly what I'm trying to accomplish.
> > >>I used the for loop in case we ever want to change the
> > >>WDT_HEARTBEAT_SHIFT value
> > >>(which is unlikely to happen).
> > >>
> > >>I'll move to your proposition.
> > >>
> > >>>However, given that, I wonder if it is really necessary to bail out above if
> > >>>max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
> > >>>in this case.
> > >>Yes it is necessary. The max_heartbeat is a configuration that
> > >>cannot be changed once configured.
> > >>We have to inform the user that his max_heartbeat value is too small
> > >>to be handled correctly by the Linux kernel.
> > >>
> > >>If we simply use the min_heartbeat value as heartbeat and ignore the
> > >>wrong max_heartbeat value,
> > >>the watchdog will reset the SoC before we can ever reset the
> > >>watchdog counter.
> > >>
> > >>>>+
> > >>>>+	if (max_heartbeat < min_heartbeat + 4)
> > >>>>  		dev_warn(dev,
> > >>>>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> > >>>>-		if (wdt->heartbeat < 4)
> > >>>>-			dev_warn(dev,
> > >>>>-				 "heartbeat might be too small for the system to handle it correctly\n");
> > >>>>-	} else {
> > >>>>-		wdt->heartbeat -= 4;
> > >>>>-	}
> > >>>>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> > >>>>  		err = request_irq(wdt->irq, wdt_interrupt,
> > >>>>@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> > >>>>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> > >>>>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> > >>>>-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> > >>>>+	/* Use min_heartbeat the first time because the watchdog timer might
> > >>>>+	 * be running for a long time when we reach this init function. */
> > >>>	/*
> > >>>	 * Multi-line comment style
> > >>>	 *
> > >>>	 * Not really sure I understand what this accomplishes. What problem
> > >>>	 * are you trying to solve here ?
> > >>>	 */
> > >>Sure, I'll change the comment style.
> > >>
> > >>What, I'm trying to explain, is that the watchdog might (or should)
> > >>have been resetted
> > >>before loading the linux kernel. But loading the kernel takes some
> > >>time (uncompressing,
> > >>low level init, ...), and if we take the heartbeat (or max_heartbeat
> > >>/ 4 in the common case) value as
> > >>the next trigger to reset the watchdog counter, the watchdog timer
> > >>might have already expired.
> > >>
> > >But increasing anything in the watchdog driver itself won't help you there.
> > >You can not execute any kernel code before that kernel code is running.
> > 
> > Of course, but you can at least try to minimize the time between the
> > watchdog driver init
> > and the first wathdog counter reset.
> > 
> Sure.
> 
> > >>Here is an example:
> > >>  - max_heartbeat configured to 8 seconds
> > >>  - min_heartbeat configured to 1 second
> > >>  => heartbeat = 2s
> > >>  - kernel boot time (before at91 watchdog is loaded) = 6 secs
> > >>
> > >Guess that is where I got lost. Do you mean the time from loading the driver
> > >to starting the watchdog application ? Because the init function is only
> > >executed after the driver is loaded, so nothing will help you if it takes
> > >too long for the driver to load.
> > 
> > I think there is another bug here: the driver should not be compiled
> > as a module.
> > 
> > Here is why:
> > 
> > At POR the at91 SoC configure its watchdog with these default values:
> >  - enabled
> >  - min heartbeat = 0 ticks
> >  - max heartbeat = 0xfff ticks <=> 16 secs
> >  - some reset options
> > After a POR the watchdog can only be reconfigured once (and only once).
> > This configuration oftenly takes place in the the bootstrap (or bootloader),
> > but can eventually be done by the Linux kernel.
> > 
> > If the watchdog is kept enabled by the bootstrap (or bootloader), this means
> > the linux kernel will have to reset the watchdog counter as soon as
> > possible to avoid
> > a possible watchdog reset.
> > 
> > => If we authorize the at91 sam9 watchdog to be compiled as a
> > module, we're not sure
> > the module will be loaded soon enough to be able to reset the
> > watchdog counter.
> > 
> Agreed, but that is only an issue _if_ the watchdog is enabled from ROMMON,
> which we don't know. This makes it a configuration issue: If the watchdog 
> is enabled by ROMMON, the driver should not be built as module. On the other
> side, unless it is known for sure (say, from the HW architecture) that it
> is always enabled, we should not force everyone to build it into the kernel.
> 
> Other drivers deal with that condition by only resetting and re-initializing
> the watchdog if it is already running. This driver is a bit of an exception,
> as it always enables the watchdog during initialization. Which is actually
> another reason to be able to build it as module: If the watchdog was not
> enabled by ROMMON, this ensures that it only starts running when the module
> is loaded.

this make no sense on at91 as the watchdog is always ON and can be disable
only once

so always enable as builtin is the right way to do

Best Regards
J.

[PATCH] watchdog: at91sam9_wdt: various fixes

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:27 Tue 29 Oct     , Guenter Roeck wrote:
> On Tue, Oct 29, 2013 at 06:22:47PM +0100, boris brezillon wrote:
> > On 29/10/2013 17:43, Guenter Roeck wrote:
> > >On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
> > >>On 29/10/2013 16:45, Guenter Roeck wrote:
> > >>>On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
> > >>>>Fix the secs_to_ticks macro in case 0 is passed as an argument.
> > >>>>
> > >>>>Rework the heartbeat calculation to increase the security margin of the
> > >>>>watchdog reset timer.
> > >>>>
> > >>>>Use the min_heartbeat value instead of the calculated heartbeat value for
> > >>>>the first watchdog reset.
> > >>>>
> > >>>>Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> > >>>Hi Boris,
> > >>>
> > >>>can you possibly split the three changes into separate patches ?
> > >>Sure. My first idea was to split this in 3 patches, but, as the
> > >>buggy at91 watchdog series was
> > >>already applied to linux-watchdog-next, I thought it would be faster
> > >>to only provide one
> > >>patch to fix all the issues at once.
> > >>
> > >>>The first is a no-brainer. Gives my opinion of my code review capabilities
> > >>>a slight damper ;-).
> > >>>
> > >>>For the other two problems, it might make sense to describe
> > >>>the problems you are trying to solve.
> > >>>
> > >>>Couple of comments inline.
> > >>>
> > >>>Thanks,
> > >>>Guenter
> > >>>
> > >>>
> > >>>>---
> > >>>>  drivers/watchdog/at91sam9_wdt.c |   35 ++++++++++++++++++++++++-----------
> > >>>>  1 file changed, 24 insertions(+), 11 deletions(-)
> > >>>>
> > >>>>diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> > >>>>index 9bd089e..f1b59f1 100644
> > >>>>--- a/drivers/watchdog/at91sam9_wdt.c
> > >>>>+++ b/drivers/watchdog/at91sam9_wdt.c
> > >>>>@@ -51,7 +51,7 @@
> > >>>>  #define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
> > >>>>  #define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
> > >>>>  #define ticks_to_secs(t)		(((t) + 1) >> 8)
> > >>>>-#define secs_to_ticks(s)		(((s) << 8) - 1)
> > >>>>+#define secs_to_ticks(s)		(s ? (((s) << 8) - 1) : 0)
> > >>>	(s)
> > >>>
> > >>>>  #define WDT_MR_RESET	0x3FFF2FFF
> > >>>>@@ -61,6 +61,11 @@
> > >>>>  /* Watchdog max delta/value in secs */
> > >>>>  #define WDT_COUNTER_MAX_SECS	ticks_to_secs(WDT_COUNTER_MAX_TICKS)
> > >>>>+/* Watchdog heartbeat shift used for security margin:
> > >>>>+ * we'll try to rshift the heartbeat value with this value to secure
> > >>>>+ * the watchdog reset. */
> > >>>>+#define WDT_HEARTBEAT_SHIFT	2
> > >>>>+
> > >>>>  /* Hardware timeout in seconds */
> > >>>>  #define WDT_HW_TIMEOUT 2
> > >>>>@@ -158,7 +163,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> > >>>>  	int err;
> > >>>>  	u32 mask = wdt->mr_mask;
> > >>>>  	unsigned long min_heartbeat = 1;
> > >>>>+	unsigned long max_heartbeat;
> > >>>>  	struct device *dev = &pdev->dev;
> > >>>>+	int shift;
> > >>>>  	tmp = wdt_read(wdt, AT91_WDT_MR);
> > >>>>  	if ((tmp & mask) != (wdt->mr & mask)) {
> > >>>>@@ -181,23 +188,27 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> > >>>>  	if (delta < value)
> > >>>>  		min_heartbeat = ticks_to_hz_roundup(value - delta);
> > >>>>-	wdt->heartbeat = ticks_to_hz_rounddown(value);
> > >>>>-	if (!wdt->heartbeat) {
> > >>>>+	max_heartbeat = ticks_to_hz_rounddown(value);
> > >>>>+	if (!max_heartbeat) {
> > >>>>  		dev_err(dev,
> > >>>>  			"heartbeat is too small for the system to handle it correctly\n");
> > >>>>  		return -EINVAL;
> > >>>>  	}
> > >>>>-	if (wdt->heartbeat < min_heartbeat + 4) {
> > >>>>+	for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
> > >>>>+		if ((max_heartbeat >> shift) < min_heartbeat)
> > >>>>+			continue;
> > >>>>+
> > >>>>+		wdt->heartbeat = max_heartbeat >> shift;
> > >>>>+		break;
> > >>>>+	}
> > >>>>+
> > >>>>+	if (!shift)
> > >>>>  		wdt->heartbeat = min_heartbeat;
> > >>>Correct me if I am wrong, but it seems to me that
> > >>>
> > >>>	if ((max_heartbeat >> 2) >= min_heartbeat)
> > >>>		 wdt->heartbeat = max_heartbeat >> 2;
> > >>>	else if ((max_heartbeat >> 1) >= min_heartbeat)
> > >>>		wdt->heartbeat = max_heartbeat >> 1;
> > >>>	else
> > >>>		wdt->heartbeat = min_heartbeat;
> > >>>
> > >>>would accomplish the same and be easier to understand.
> > >>This is exactly what I'm trying to accomplish.
> > >>I used the for loop in case we ever want to change the
> > >>WDT_HEARTBEAT_SHIFT value
> > >>(which is unlikely to happen).
> > >>
> > >>I'll move to your proposition.
> > >>
> > >>>However, given that, I wonder if it is really necessary to bail out above if
> > >>>max_heartbeat is 0. After all, you set heartbeat to min_heartbeat anyway
> > >>>in this case.
> > >>Yes it is necessary. The max_heartbeat is a configuration that
> > >>cannot be changed once configured.
> > >>We have to inform the user that his max_heartbeat value is too small
> > >>to be handled correctly by the Linux kernel.
> > >>
> > >>If we simply use the min_heartbeat value as heartbeat and ignore the
> > >>wrong max_heartbeat value,
> > >>the watchdog will reset the SoC before we can ever reset the
> > >>watchdog counter.
> > >>
> > >>>>+
> > >>>>+	if (max_heartbeat < min_heartbeat + 4)
> > >>>>  		dev_warn(dev,
> > >>>>  			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> > >>>>-		if (wdt->heartbeat < 4)
> > >>>>-			dev_warn(dev,
> > >>>>-				 "heartbeat might be too small for the system to handle it correctly\n");
> > >>>>-	} else {
> > >>>>-		wdt->heartbeat -= 4;
> > >>>>-	}
> > >>>>  	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> > >>>>  		err = request_irq(wdt->irq, wdt_interrupt,
> > >>>>@@ -213,7 +224,9 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> > >>>>  			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> > >>>>  	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> > >>>>-	mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
> > >>>>+	/* Use min_heartbeat the first time because the watchdog timer might
> > >>>>+	 * be running for a long time when we reach this init function. */
> > >>>	/*
> > >>>	 * Multi-line comment style
> > >>>	 *
> > >>>	 * Not really sure I understand what this accomplishes. What problem
> > >>>	 * are you trying to solve here ?
> > >>>	 */
> > >>Sure, I'll change the comment style.
> > >>
> > >>What, I'm trying to explain, is that the watchdog might (or should)
> > >>have been resetted
> > >>before loading the linux kernel. But loading the kernel takes some
> > >>time (uncompressing,
> > >>low level init, ...), and if we take the heartbeat (or max_heartbeat
> > >>/ 4 in the common case) value as
> > >>the next trigger to reset the watchdog counter, the watchdog timer
> > >>might have already expired.
> > >>
> > >But increasing anything in the watchdog driver itself won't help you there.
> > >You can not execute any kernel code before that kernel code is running.
> > 
> > Of course, but you can at least try to minimize the time between the
> > watchdog driver init
> > and the first wathdog counter reset.
> > 
> Sure.
> 
> > >>Here is an example:
> > >>  - max_heartbeat configured to 8 seconds
> > >>  - min_heartbeat configured to 1 second
> > >>  => heartbeat = 2s
> > >>  - kernel boot time (before at91 watchdog is loaded) = 6 secs
> > >>
> > >Guess that is where I got lost. Do you mean the time from loading the driver
> > >to starting the watchdog application ? Because the init function is only
> > >executed after the driver is loaded, so nothing will help you if it takes
> > >too long for the driver to load.
> > 
> > I think there is another bug here: the driver should not be compiled
> > as a module.
> > 
> > Here is why:
> > 
> > At POR the at91 SoC configure its watchdog with these default values:
> >  - enabled
> >  - min heartbeat = 0 ticks
> >  - max heartbeat = 0xfff ticks <=> 16 secs
> >  - some reset options
> > After a POR the watchdog can only be reconfigured once (and only once).
> > This configuration oftenly takes place in the the bootstrap (or bootloader),
> > but can eventually be done by the Linux kernel.
> > 
> > If the watchdog is kept enabled by the bootstrap (or bootloader), this means
> > the linux kernel will have to reset the watchdog counter as soon as
> > possible to avoid
> > a possible watchdog reset.
> > 
> > => If we authorize the at91 sam9 watchdog to be compiled as a
> > module, we're not sure
> > the module will be loaded soon enough to be able to reset the
> > watchdog counter.
> > 
> Agreed, but that is only an issue _if_ the watchdog is enabled from ROMMON,
> which we don't know. This makes it a configuration issue: If the watchdog 
> is enabled by ROMMON, the driver should not be built as module. On the other
> side, unless it is known for sure (say, from the HW architecture) that it
> is always enabled, we should not force everyone to build it into the kernel.
> 
> Other drivers deal with that condition by only resetting and re-initializing
> the watchdog if it is already running. This driver is a bit of an exception,
> as it always enables the watchdog during initialization. Which is actually
> another reason to be able to build it as module: If the watchdog was not
> enabled by ROMMON, this ensures that it only starts running when the module
> is loaded.

this make no sense on at91 as the watchdog is always ON and can be disable
only once

so always enable as builtin is the right way to do

Best Regards
J.

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[boris brezillon]

Hello Guenter,

Are you okay with the 3 fixes/improvements provided by this patch,
and the explanation I gave regarding the reason for these changes ?

If so, I will submit a new series splitting the patch and including your 
suggestions.

Best Regards,

Boris

On 29/10/2013 18:22, boris brezillon wrote:
> On 29/10/2013 17:43, Guenter Roeck wrote:
>> On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
>>> On 29/10/2013 16:45, Guenter Roeck wrote:
>>>> On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
>>>>> Fix the secs_to_ticks macro in case 0 is passed as an argument.
>>>>>
>>>>> Rework the heartbeat calculation to increase the security margin 
>>>>> of the
>>>>> watchdog reset timer.
>>>>>
>>>>> Use the min_heartbeat value instead of the calculated heartbeat 
>>>>> value for
>>>>> the first watchdog reset.
>>>>>
>>>>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>>>> Hi Boris,
>>>>
>>>> can you possibly split the three changes into separate patches ?
>>> Sure. My first idea was to split this in 3 patches, but, as the
>>> buggy at91 watchdog series was
>>> already applied to linux-watchdog-next, I thought it would be faster
>>> to only provide one
>>> patch to fix all the issues at once.
>>>
>>>> The first is a no-brainer. Gives my opinion of my code review 
>>>> capabilities
>>>> a slight damper ;-).
>>>>
>>>> For the other two problems, it might make sense to describe
>>>> the problems you are trying to solve.
>>>>
>>>> Couple of comments inline.
>>>>
>>>> Thanks,
>>>> Guenter
>>>>
>>>>
>>>>> ---
>>>>>   drivers/watchdog/at91sam9_wdt.c |   35 
>>>>> ++++++++++++++++++++++++-----------
>>>>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/at91sam9_wdt.c 
>>>>> b/drivers/watchdog/at91sam9_wdt.c
>>>>> index 9bd089e..f1b59f1 100644
>>>>> --- a/drivers/watchdog/at91sam9_wdt.c
>>>>> +++ b/drivers/watchdog/at91sam9_wdt.c
>>>>> @@ -51,7 +51,7 @@
>>>>>   #define ticks_to_hz_rounddown(t)    ((((t) + 1) * HZ) >> 8)
>>>>>   #define ticks_to_hz_roundup(t)        (((((t) + 1) * HZ) + 255) 
>>>>> >> 8)
>>>>>   #define ticks_to_secs(t)        (((t) + 1) >> 8)
>>>>> -#define secs_to_ticks(s)        (((s) << 8) - 1)
>>>>> +#define secs_to_ticks(s)        (s ? (((s) << 8) - 1) : 0)
>>>>     (s)
>>>>
>>>>>   #define WDT_MR_RESET    0x3FFF2FFF
>>>>> @@ -61,6 +61,11 @@
>>>>>   /* Watchdog max delta/value in secs */
>>>>>   #define WDT_COUNTER_MAX_SECS ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>>>>> +/* Watchdog heartbeat shift used for security margin:
>>>>> + * we'll try to rshift the heartbeat value with this value to secure
>>>>> + * the watchdog reset. */
>>>>> +#define WDT_HEARTBEAT_SHIFT    2
>>>>> +
>>>>>   /* Hardware timeout in seconds */
>>>>>   #define WDT_HW_TIMEOUT 2
>>>>> @@ -158,7 +163,9 @@ static int at91_wdt_init(struct 
>>>>> platform_device *pdev, struct at91wdt *wdt)
>>>>>       int err;
>>>>>       u32 mask = wdt->mr_mask;
>>>>>       unsigned long min_heartbeat = 1;
>>>>> +    unsigned long max_heartbeat;
>>>>>       struct device *dev = &pdev->dev;
>>>>> +    int shift;
>>>>>       tmp = wdt_read(wdt, AT91_WDT_MR);
>>>>>       if ((tmp & mask) != (wdt->mr & mask)) {
>>>>> @@ -181,23 +188,27 @@ static int at91_wdt_init(struct 
>>>>> platform_device *pdev, struct at91wdt *wdt)
>>>>>       if (delta < value)
>>>>>           min_heartbeat = ticks_to_hz_roundup(value - delta);
>>>>> -    wdt->heartbeat = ticks_to_hz_rounddown(value);
>>>>> -    if (!wdt->heartbeat) {
>>>>> +    max_heartbeat = ticks_to_hz_rounddown(value);
>>>>> +    if (!max_heartbeat) {
>>>>>           dev_err(dev,
>>>>>               "heartbeat is too small for the system to handle it 
>>>>> correctly\n");
>>>>>           return -EINVAL;
>>>>>       }
>>>>> -    if (wdt->heartbeat < min_heartbeat + 4) {
>>>>> +    for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
>>>>> +        if ((max_heartbeat >> shift) < min_heartbeat)
>>>>> +            continue;
>>>>> +
>>>>> +        wdt->heartbeat = max_heartbeat >> shift;
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>> +    if (!shift)
>>>>>           wdt->heartbeat = min_heartbeat;
>>>> Correct me if I am wrong, but it seems to me that
>>>>
>>>>     if ((max_heartbeat >> 2) >= min_heartbeat)
>>>>          wdt->heartbeat = max_heartbeat >> 2;
>>>>     else if ((max_heartbeat >> 1) >= min_heartbeat)
>>>>         wdt->heartbeat = max_heartbeat >> 1;
>>>>     else
>>>>         wdt->heartbeat = min_heartbeat;
>>>>
>>>> would accomplish the same and be easier to understand.
>>> This is exactly what I'm trying to accomplish.
>>> I used the for loop in case we ever want to change the
>>> WDT_HEARTBEAT_SHIFT value
>>> (which is unlikely to happen).
>>>
>>> I'll move to your proposition.
>>>
>>>> However, given that, I wonder if it is really necessary to bail out 
>>>> above if
>>>> max_heartbeat is 0. After all, you set heartbeat to min_heartbeat 
>>>> anyway
>>>> in this case.
>>> Yes it is necessary. The max_heartbeat is a configuration that
>>> cannot be changed once configured.
>>> We have to inform the user that his max_heartbeat value is too small
>>> to be handled correctly by the Linux kernel.
>>>
>>> If we simply use the min_heartbeat value as heartbeat and ignore the
>>> wrong max_heartbeat value,
>>> the watchdog will reset the SoC before we can ever reset the
>>> watchdog counter.
>>>
>>>>> +
>>>>> +    if (max_heartbeat < min_heartbeat + 4)
>>>>>           dev_warn(dev,
>>>>>                "min heartbeat and max heartbeat might be too close 
>>>>> for the system to handle it correctly\n");
>>>>> -        if (wdt->heartbeat < 4)
>>>>> -            dev_warn(dev,
>>>>> -                 "heartbeat might be too small for the system to 
>>>>> handle it correctly\n");
>>>>> -    } else {
>>>>> -        wdt->heartbeat -= 4;
>>>>> -    }
>>>>>       if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>>>>>           err = request_irq(wdt->irq, wdt_interrupt,
>>>>> @@ -213,7 +224,9 @@ static int at91_wdt_init(struct 
>>>>> platform_device *pdev, struct at91wdt *wdt)
>>>>>                tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>>>>>       setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>>>>> -    mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>>>>> +    /* Use min_heartbeat the first time because the watchdog 
>>>>> timer might
>>>>> +     * be running for a long time when we reach this init 
>>>>> function. */
>>>>     /*
>>>>      * Multi-line comment style
>>>>      *
>>>>      * Not really sure I understand what this accomplishes. What 
>>>> problem
>>>>      * are you trying to solve here ?
>>>>      */
>>> Sure, I'll change the comment style.
>>>
>>> What, I'm trying to explain, is that the watchdog might (or should)
>>> have been resetted
>>> before loading the linux kernel. But loading the kernel takes some
>>> time (uncompressing,
>>> low level init, ...), and if we take the heartbeat (or max_heartbeat
>>> / 4 in the common case) value as
>>> the next trigger to reset the watchdog counter, the watchdog timer
>>> might have already expired.
>>>
>> But increasing anything in the watchdog driver itself won't help you 
>> there.
>> You can not execute any kernel code before that kernel code is running.
>
> Of course, but you can at least try to minimize the time between the 
> watchdog driver init
> and the first wathdog counter reset.
>
>>> Here is an example:
>>>   - max_heartbeat configured to 8 seconds
>>>   - min_heartbeat configured to 1 second
>>>   => heartbeat = 2s
>>>   - kernel boot time (before at91 watchdog is loaded) = 6 secs
>>>
>> Guess that is where I got lost. Do you mean the time from loading the 
>> driver
>> to starting the watchdog application ? Because the init function is only
>> executed after the driver is loaded, so nothing will help you if it 
>> takes
>> too long for the driver to load.
>
> I think there is another bug here: the driver should not be compiled 
> as a module.
>
> Here is why:
>
> At POR the at91 SoC configure its watchdog with these default values:
>  - enabled
>  - min heartbeat = 0 ticks
>  - max heartbeat = 0xfff ticks <=> 16 secs
>  - some reset options
> After a POR the watchdog can only be reconfigured once (and only once).
> This configuration oftenly takes place in the the bootstrap (or 
> bootloader),
> but can eventually be done by the Linux kernel.
>
> If the watchdog is kept enabled by the bootstrap (or bootloader), this 
> means
> the linux kernel will have to reset the watchdog counter as soon as 
> possible to avoid
> a possible watchdog reset.
>
> => If we authorize the at91 sam9 watchdog to be compiled as a module, 
> we're not sure
> the module will be loaded soon enough to be able to reset the watchdog 
> counter.
>
>>
>> You really have two times to deal with:
>> - Time from ROMMON handoff to loading the driver
>>    Nothing you can do there. If the watchdog fires before the driver 
>> is loaded,
>>    you are lost. Only way t handle this situation is to increase the 
>> timeout
>>    in the ROMMON.
>> - Time from loading driver to watchdog device open. This is really 
>> the time
>>    you are increasing with your change.
>
> This is where it gets a bit tricky.
>
> The heartbeat I'm talking about is not the "user space" heartbeat
> (struct watchdog_device timeout field), it's the "hardware watchdog 
> counter reset"
> heartbeat (struct at91wdt heartbeat field).
>
> Actually the at91 sam9 wdt driver does not provide a direct access to 
> the watchdog reset
> counter functionnality.
> Instead, it periodically reset the watchdog counter (based on the 
> timing config retrieved from
> the hw registers), and eventually, if the user open a the watchdog dev 
> file and fails to reset
> the watchdog (using the user space API), the drivers stops resetting 
> the hw counter, which leads
> to a watchdog reset.
>
> I hope I'm clear enough, cause it's quite complicated to explain.
>
> Best Regards,
>
> Boris
>
>> Thanks,
>> Guenter
>>
>>> If I use the heartbeat value when configuring the first expiration
>>> of the timer,
>>> it might be too late to reset the watchdog counter.
>>>
>>> I'll try to find a proper to explain this use case :-).
>>>
>>>>> +    mod_timer(&wdt->timer, jiffies + min_heartbeat);
>>>>>       /* Try to set timeout from device tree first */
>>>>>       if (watchdog_init_timeout(&wdt->wdd, 0, dev))
>>>>> -- 
>>>>> 1.7.9.5
>>>>>
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe 
>>>>> linux-watchdog" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe 
>>> linux-watchdog" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>

[PATCH] watchdog: at91sam9_wdt: various fixes

[boris brezillon]

Hello Guenter,

Are you okay with the 3 fixes/improvements provided by this patch,
and the explanation I gave regarding the reason for these changes ?

If so, I will submit a new series splitting the patch and including your 
suggestions.

Best Regards,

Boris

On 29/10/2013 18:22, boris brezillon wrote:
> On 29/10/2013 17:43, Guenter Roeck wrote:
>> On Tue, Oct 29, 2013 at 05:22:50PM +0100, boris brezillon wrote:
>>> On 29/10/2013 16:45, Guenter Roeck wrote:
>>>> On Tue, Oct 29, 2013 at 11:37:33AM +0100, Boris BREZILLON wrote:
>>>>> Fix the secs_to_ticks macro in case 0 is passed as an argument.
>>>>>
>>>>> Rework the heartbeat calculation to increase the security margin 
>>>>> of the
>>>>> watchdog reset timer.
>>>>>
>>>>> Use the min_heartbeat value instead of the calculated heartbeat 
>>>>> value for
>>>>> the first watchdog reset.
>>>>>
>>>>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>>>> Hi Boris,
>>>>
>>>> can you possibly split the three changes into separate patches ?
>>> Sure. My first idea was to split this in 3 patches, but, as the
>>> buggy at91 watchdog series was
>>> already applied to linux-watchdog-next, I thought it would be faster
>>> to only provide one
>>> patch to fix all the issues at once.
>>>
>>>> The first is a no-brainer. Gives my opinion of my code review 
>>>> capabilities
>>>> a slight damper ;-).
>>>>
>>>> For the other two problems, it might make sense to describe
>>>> the problems you are trying to solve.
>>>>
>>>> Couple of comments inline.
>>>>
>>>> Thanks,
>>>> Guenter
>>>>
>>>>
>>>>> ---
>>>>>   drivers/watchdog/at91sam9_wdt.c |   35 
>>>>> ++++++++++++++++++++++++-----------
>>>>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/at91sam9_wdt.c 
>>>>> b/drivers/watchdog/at91sam9_wdt.c
>>>>> index 9bd089e..f1b59f1 100644
>>>>> --- a/drivers/watchdog/at91sam9_wdt.c
>>>>> +++ b/drivers/watchdog/at91sam9_wdt.c
>>>>> @@ -51,7 +51,7 @@
>>>>>   #define ticks_to_hz_rounddown(t)    ((((t) + 1) * HZ) >> 8)
>>>>>   #define ticks_to_hz_roundup(t)        (((((t) + 1) * HZ) + 255) 
>>>>> >> 8)
>>>>>   #define ticks_to_secs(t)        (((t) + 1) >> 8)
>>>>> -#define secs_to_ticks(s)        (((s) << 8) - 1)
>>>>> +#define secs_to_ticks(s)        (s ? (((s) << 8) - 1) : 0)
>>>>     (s)
>>>>
>>>>>   #define WDT_MR_RESET    0x3FFF2FFF
>>>>> @@ -61,6 +61,11 @@
>>>>>   /* Watchdog max delta/value in secs */
>>>>>   #define WDT_COUNTER_MAX_SECS ticks_to_secs(WDT_COUNTER_MAX_TICKS)
>>>>> +/* Watchdog heartbeat shift used for security margin:
>>>>> + * we'll try to rshift the heartbeat value with this value to secure
>>>>> + * the watchdog reset. */
>>>>> +#define WDT_HEARTBEAT_SHIFT    2
>>>>> +
>>>>>   /* Hardware timeout in seconds */
>>>>>   #define WDT_HW_TIMEOUT 2
>>>>> @@ -158,7 +163,9 @@ static int at91_wdt_init(struct 
>>>>> platform_device *pdev, struct at91wdt *wdt)
>>>>>       int err;
>>>>>       u32 mask = wdt->mr_mask;
>>>>>       unsigned long min_heartbeat = 1;
>>>>> +    unsigned long max_heartbeat;
>>>>>       struct device *dev = &pdev->dev;
>>>>> +    int shift;
>>>>>       tmp = wdt_read(wdt, AT91_WDT_MR);
>>>>>       if ((tmp & mask) != (wdt->mr & mask)) {
>>>>> @@ -181,23 +188,27 @@ static int at91_wdt_init(struct 
>>>>> platform_device *pdev, struct at91wdt *wdt)
>>>>>       if (delta < value)
>>>>>           min_heartbeat = ticks_to_hz_roundup(value - delta);
>>>>> -    wdt->heartbeat = ticks_to_hz_rounddown(value);
>>>>> -    if (!wdt->heartbeat) {
>>>>> +    max_heartbeat = ticks_to_hz_rounddown(value);
>>>>> +    if (!max_heartbeat) {
>>>>>           dev_err(dev,
>>>>>               "heartbeat is too small for the system to handle it 
>>>>> correctly\n");
>>>>>           return -EINVAL;
>>>>>       }
>>>>> -    if (wdt->heartbeat < min_heartbeat + 4) {
>>>>> +    for (shift = WDT_HEARTBEAT_SHIFT; shift > 0; shift--) {
>>>>> +        if ((max_heartbeat >> shift) < min_heartbeat)
>>>>> +            continue;
>>>>> +
>>>>> +        wdt->heartbeat = max_heartbeat >> shift;
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>> +    if (!shift)
>>>>>           wdt->heartbeat = min_heartbeat;
>>>> Correct me if I am wrong, but it seems to me that
>>>>
>>>>     if ((max_heartbeat >> 2) >= min_heartbeat)
>>>>          wdt->heartbeat = max_heartbeat >> 2;
>>>>     else if ((max_heartbeat >> 1) >= min_heartbeat)
>>>>         wdt->heartbeat = max_heartbeat >> 1;
>>>>     else
>>>>         wdt->heartbeat = min_heartbeat;
>>>>
>>>> would accomplish the same and be easier to understand.
>>> This is exactly what I'm trying to accomplish.
>>> I used the for loop in case we ever want to change the
>>> WDT_HEARTBEAT_SHIFT value
>>> (which is unlikely to happen).
>>>
>>> I'll move to your proposition.
>>>
>>>> However, given that, I wonder if it is really necessary to bail out 
>>>> above if
>>>> max_heartbeat is 0. After all, you set heartbeat to min_heartbeat 
>>>> anyway
>>>> in this case.
>>> Yes it is necessary. The max_heartbeat is a configuration that
>>> cannot be changed once configured.
>>> We have to inform the user that his max_heartbeat value is too small
>>> to be handled correctly by the Linux kernel.
>>>
>>> If we simply use the min_heartbeat value as heartbeat and ignore the
>>> wrong max_heartbeat value,
>>> the watchdog will reset the SoC before we can ever reset the
>>> watchdog counter.
>>>
>>>>> +
>>>>> +    if (max_heartbeat < min_heartbeat + 4)
>>>>>           dev_warn(dev,
>>>>>                "min heartbeat and max heartbeat might be too close 
>>>>> for the system to handle it correctly\n");
>>>>> -        if (wdt->heartbeat < 4)
>>>>> -            dev_warn(dev,
>>>>> -                 "heartbeat might be too small for the system to 
>>>>> handle it correctly\n");
>>>>> -    } else {
>>>>> -        wdt->heartbeat -= 4;
>>>>> -    }
>>>>>       if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>>>>>           err = request_irq(wdt->irq, wdt_interrupt,
>>>>> @@ -213,7 +224,9 @@ static int at91_wdt_init(struct 
>>>>> platform_device *pdev, struct at91wdt *wdt)
>>>>>                tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
>>>>>       setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>>>>> -    mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
>>>>> +    /* Use min_heartbeat the first time because the watchdog 
>>>>> timer might
>>>>> +     * be running for a long time when we reach this init 
>>>>> function. */
>>>>     /*
>>>>      * Multi-line comment style
>>>>      *
>>>>      * Not really sure I understand what this accomplishes. What 
>>>> problem
>>>>      * are you trying to solve here ?
>>>>      */
>>> Sure, I'll change the comment style.
>>>
>>> What, I'm trying to explain, is that the watchdog might (or should)
>>> have been resetted
>>> before loading the linux kernel. But loading the kernel takes some
>>> time (uncompressing,
>>> low level init, ...), and if we take the heartbeat (or max_heartbeat
>>> / 4 in the common case) value as
>>> the next trigger to reset the watchdog counter, the watchdog timer
>>> might have already expired.
>>>
>> But increasing anything in the watchdog driver itself won't help you 
>> there.
>> You can not execute any kernel code before that kernel code is running.
>
> Of course, but you can at least try to minimize the time between the 
> watchdog driver init
> and the first wathdog counter reset.
>
>>> Here is an example:
>>>   - max_heartbeat configured to 8 seconds
>>>   - min_heartbeat configured to 1 second
>>>   => heartbeat = 2s
>>>   - kernel boot time (before at91 watchdog is loaded) = 6 secs
>>>
>> Guess that is where I got lost. Do you mean the time from loading the 
>> driver
>> to starting the watchdog application ? Because the init function is only
>> executed after the driver is loaded, so nothing will help you if it 
>> takes
>> too long for the driver to load.
>
> I think there is another bug here: the driver should not be compiled 
> as a module.
>
> Here is why:
>
> At POR the at91 SoC configure its watchdog with these default values:
>  - enabled
>  - min heartbeat = 0 ticks
>  - max heartbeat = 0xfff ticks <=> 16 secs
>  - some reset options
> After a POR the watchdog can only be reconfigured once (and only once).
> This configuration oftenly takes place in the the bootstrap (or 
> bootloader),
> but can eventually be done by the Linux kernel.
>
> If the watchdog is kept enabled by the bootstrap (or bootloader), this 
> means
> the linux kernel will have to reset the watchdog counter as soon as 
> possible to avoid
> a possible watchdog reset.
>
> => If we authorize the at91 sam9 watchdog to be compiled as a module, 
> we're not sure
> the module will be loaded soon enough to be able to reset the watchdog 
> counter.
>
>>
>> You really have two times to deal with:
>> - Time from ROMMON handoff to loading the driver
>>    Nothing you can do there. If the watchdog fires before the driver 
>> is loaded,
>>    you are lost. Only way t handle this situation is to increase the 
>> timeout
>>    in the ROMMON.
>> - Time from loading driver to watchdog device open. This is really 
>> the time
>>    you are increasing with your change.
>
> This is where it gets a bit tricky.
>
> The heartbeat I'm talking about is not the "user space" heartbeat
> (struct watchdog_device timeout field), it's the "hardware watchdog 
> counter reset"
> heartbeat (struct at91wdt heartbeat field).
>
> Actually the at91 sam9 wdt driver does not provide a direct access to 
> the watchdog reset
> counter functionnality.
> Instead, it periodically reset the watchdog counter (based on the 
> timing config retrieved from
> the hw registers), and eventually, if the user open a the watchdog dev 
> file and fails to reset
> the watchdog (using the user space API), the drivers stops resetting 
> the hw counter, which leads
> to a watchdog reset.
>
> I hope I'm clear enough, cause it's quite complicated to explain.
>
> Best Regards,
>
> Boris
>
>> Thanks,
>> Guenter
>>
>>> If I use the heartbeat value when configuring the first expiration
>>> of the timer,
>>> it might be too late to reset the watchdog counter.
>>>
>>> I'll try to find a proper to explain this use case :-).
>>>
>>>>> +    mod_timer(&wdt->timer, jiffies + min_heartbeat);
>>>>>       /* Try to set timeout from device tree first */
>>>>>       if (watchdog_init_timeout(&wdt->wdd, 0, dev))
>>>>> -- 
>>>>> 1.7.9.5
>>>>>
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe 
>>>>> linux-watchdog" in
>>>>> the body of a message to majordomo at vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe 
>>> linux-watchdog" in
>>> the body of a message to majordomo at vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[Guenter Roeck]

On 11/03/2013 06:42 AM, boris brezillon wrote:
> Hello Guenter,
>
> Are you okay with the 3 fixes/improvements provided by this patch,
> and the explanation I gave regarding the reason for these changes ?
>
> If so, I will submit a new series splitting the patch and including your suggestions.
>
Yes, sorry, I was waiting for your patches.

Thanks,
Guenter

[PATCH] watchdog: at91sam9_wdt: various fixes

[Guenter Roeck]

On 11/03/2013 06:42 AM, boris brezillon wrote:
> Hello Guenter,
>
> Are you okay with the 3 fixes/improvements provided by this patch,
> and the explanation I gave regarding the reason for these changes ?
>
> If so, I will submit a new series splitting the patch and including your suggestions.
>
Yes, sorry, I was waiting for your patches.

Thanks,
Guenter

Re: [PATCH] watchdog: at91sam9_wdt: various fixes

[boris brezillon]

On 03/11/2013 17:22, Guenter Roeck wrote:
> On 11/03/2013 06:42 AM, boris brezillon wrote:
>> Hello Guenter,
>>
>> Are you okay with the 3 fixes/improvements provided by this patch,
>> and the explanation I gave regarding the reason for these changes ?
>>
>> If so, I will submit a new series splitting the patch and including 
>> your suggestions.
>>
> Yes, sorry, I was waiting for your patches.
Done.

BTW, thanks for your review :-).
>
> Thanks,
> Guenter
>

[PATCH] watchdog: at91sam9_wdt: various fixes

[boris brezillon]

On 03/11/2013 17:22, Guenter Roeck wrote:
> On 11/03/2013 06:42 AM, boris brezillon wrote:
>> Hello Guenter,
>>
>> Are you okay with the 3 fixes/improvements provided by this patch,
>> and the explanation I gave regarding the reason for these changes ?
>>
>> If so, I will submit a new series splitting the patch and including 
>> your suggestions.
>>
> Yes, sorry, I was waiting for your patches.
Done.

BTW, thanks for your review :-).
>
> Thanks,
> Guenter
>