[RFC PATCH 0/5] watchdog: Add reboot API

[RFC PATCH 0/5] watchdog: Add reboot API

[Guenter Roeck]

Some hardware implements reboot through its watchdog hardware, for example
by triggering a watchdog timeout or by writing into its watchdog register
set. Platform specific code starts to spread into watchdog drivers,
typically by setting pointers to a callback function which is then called
from the architecture's reset handler.

While global and exported callback function pointers (such as arm_pm_restart)
may be acceptable as long as they are used from platform and/or architecture
code, using such a mechanism across subsystems and drivers is less than
desirable. Ultimately, we'll need a better solution.

This patch series is an attempt to provide such a solution. It extends
the watchdog subsystem to support reboot functionality, provides an
API function call to trigger reboots, adds support for the new API
to arm and arm64, and converts the drivers providing reboot functionality
to use the new infrastructure.

The first patch in the series implements the new API. The second and third
patch modify the arm and arm64 architecture reset handlers to call the
added API function. The final two patches register the reboot handlers
in the sunxi and moxart watchdog drivers with the watchdog subsystem.

The sunxi patche depends on the most recent patch series sumitted by
Maxime Ripard.

Compile tested only.

[RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Guenter Roeck]

Some hardware implements reboot through its watchdog hardware,
for example by triggering a watchdog timeout. Platform specific
code starts to spread into watchdog drivers, typically by setting
pointers to a callback functions which is then called from the
platform reset handler.

To simplify code and provide a unified API to trigger reboots by
watchdog drivers, provide a single API to trigger such reboots
through the watchdog subsystem.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
 include/linux/watchdog.h         |   11 +++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index cec9b55..4ec6e2f 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -43,6 +43,17 @@
 static DEFINE_IDA(watchdog_ida);
 static struct class *watchdog_class;
 
+static struct watchdog_device *wdd_reboot_dev;
+
+void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
+{
+	if (wdd_reboot_dev) {
+		if (wdd_reboot_dev->ops->reboot)
+			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
+	}
+}
+EXPORT_SYMBOL(watchdog_do_reboot);
+
 static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
 {
 	/*
@@ -162,6 +173,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
 		return ret;
 	}
 
+	if (wdd->ops->reboot)
+		wdd_reboot_dev = wdd;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(watchdog_register_device);
@@ -181,6 +195,9 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
 	if (wdd == NULL)
 		return;
 
+	if (wdd == wdd_reboot_dev)
+		wdd_reboot_dev = NULL;
+
 	devno = wdd->cdev.dev;
 	ret = watchdog_dev_unregister(wdd);
 	if (ret)
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 2a3038e..0d204af 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -12,6 +12,7 @@
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/reboot.h>
 #include <uapi/linux/watchdog.h>
 
 struct watchdog_ops;
@@ -23,6 +24,7 @@ struct watchdog_device;
  * @start:	The routine for starting the watchdog device.
  * @stop:	The routine for stopping the watchdog device.
  * @ping:	The routine that sends a keepalive ping to the watchdog device.
+ * @reboot:	The routine for rebooting the system
  * @status:	The routine that shows the status of the watchdog device.
  * @set_timeout:The routine for setting the watchdog devices timeout value.
  * @get_timeleft:The routine that get's the time that's left before a reset.
@@ -42,6 +44,8 @@ struct watchdog_ops {
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
+	void (*reboot)(struct watchdog_device *, enum reboot_mode,
+		       const char *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
 	unsigned int (*get_timeleft)(struct watchdog_device *);
@@ -142,4 +146,11 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
 extern int watchdog_register_device(struct watchdog_device *);
 extern void watchdog_unregister_device(struct watchdog_device *);
 
+#ifdef CONFIG_WATCHDOG_CORE
+extern void watchdog_do_reboot(enum reboot_mode mode, const char *cmd);
+#else
+static inline void watchdog_do_reboot(enum reboot_mode mode,
+				      const char *cmd) { }
+#endif
+
 #endif  /* ifndef _LINUX_WATCHDOG_H */
-- 
1.7.9.7

[RFC PATCH 2/5] arm64: Support reboot through watchdog subsystem

[Guenter Roeck]

The watchdog subsystem provides an API to perform a system reboot.
Use it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm64/kernel/process.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 6391485..29c2bc0 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -42,6 +42,7 @@
 #include <linux/hw_breakpoint.h>
 #include <linux/personality.h>
 #include <linux/notifier.h>
+#include <linux/watchdog.h>
 
 #include <asm/compat.h>
 #include <asm/cacheflush.h>
@@ -144,6 +145,8 @@ void machine_restart(char *cmd)
 	if (arm_pm_restart)
 		arm_pm_restart(reboot_mode, cmd);
 
+	watchdog_do_reboot(reboot_mode, cmd);
+
 	/*
 	 * Whoops - the architecture was unable to reboot.
 	 */
-- 
1.7.9.7

[RFC PATCH 3/5] arm: Support reboot through watchdog subsystem

[Guenter Roeck]

The watchdog subsystem provides an API to perform a system reboot.
Use it.

With this change, the arm_pm_restart callback is now optional,
so check if it is set before calling it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm/kernel/process.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 81ef686..c3b7b5e 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -32,6 +32,7 @@
 #include <linux/hw_breakpoint.h>
 #include <linux/leds.h>
 #include <linux/reboot.h>
+#include <linux/watchdog.h>
 
 #include <asm/cacheflush.h>
 #include <asm/idmap.h>
@@ -230,7 +231,10 @@ void machine_restart(char *cmd)
 	local_irq_disable();
 	smp_send_stop();
 
-	arm_pm_restart(reboot_mode, cmd);
+	if (arm_pm_restart)
+		arm_pm_restart(reboot_mode, cmd);
+
+	watchdog_do_reboot(reboot_mode, cmd);
 
 	/* Give a grace period for failure to restart of 1s */
 	mdelay(1000);
-- 
1.7.9.7

[RFC PATCH 4/5] watchdog: moxart: Register reboot handler with watchdog subsystem

[Guenter Roeck]

The watchdog subsystem now provides an API to trigger a system reboot.
Register with it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/moxart_wdt.c |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/moxart_wdt.c b/drivers/watchdog/moxart_wdt.c
index 4aa3a8a..b83646b 100644
--- a/drivers/watchdog/moxart_wdt.c
+++ b/drivers/watchdog/moxart_wdt.c
@@ -19,8 +19,6 @@
 #include <linux/watchdog.h>
 #include <linux/moduleparam.h>
 
-#include <asm/system_misc.h>
-
 #define REG_COUNT			0x4
 #define REG_MODE			0x8
 #define REG_ENABLE			0xC
@@ -31,15 +29,16 @@ struct moxart_wdt_dev {
 	unsigned int clock_frequency;
 };
 
-static struct moxart_wdt_dev *moxart_restart_ctx;
-
 static int heartbeat;
 
-static void moxart_wdt_restart(enum reboot_mode reboot_mode, const char *cmd)
+static void moxart_wdt_reboot(struct watchdog_device *wdt_dev,
+			      enum reboot_mode mode, const char *cmd)
 {
-	writel(1, moxart_restart_ctx->base + REG_COUNT);
-	writel(0x5ab9, moxart_restart_ctx->base + REG_MODE);
-	writel(0x03, moxart_restart_ctx->base + REG_ENABLE);
+	struct moxart_wdt_dev *moxart_wdt = watchdog_get_drvdata(wdt_dev);
+
+	writel(1, moxart_wdt->base + REG_COUNT);
+	writel(0x5ab9, moxart_wdt->base + REG_MODE);
+	writel(0x03, moxart_wdt->base + REG_ENABLE);
 }
 
 static int moxart_wdt_stop(struct watchdog_device *wdt_dev)
@@ -81,6 +80,7 @@ static const struct watchdog_ops moxart_wdt_ops = {
 	.owner          = THIS_MODULE,
 	.start          = moxart_wdt_start,
 	.stop           = moxart_wdt_stop,
+	.reboot		= moxart_wdt_reboot,
 	.set_timeout    = moxart_wdt_set_timeout,
 };
 
@@ -136,9 +136,6 @@ static int moxart_wdt_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	moxart_restart_ctx = moxart_wdt;
-	arm_pm_restart = moxart_wdt_restart;
-
 	dev_dbg(dev, "Watchdog enabled (heartbeat=%d sec, nowayout=%d)\n",
 		moxart_wdt->dev.timeout, nowayout);
 
@@ -149,7 +146,6 @@ static int moxart_wdt_remove(struct platform_device *pdev)
 {
 	struct moxart_wdt_dev *moxart_wdt = platform_get_drvdata(pdev);
 
-	arm_pm_restart = NULL;
 	moxart_wdt_stop(&moxart_wdt->dev);
 	watchdog_unregister_device(&moxart_wdt->dev);
 
-- 
1.7.9.7

[RFC PATCH 5/5] watchdog: sunxi: Register reboot handler with watchdog subsystem

[Guenter Roeck]

The watchdog subsystem now provides an API to trigger a system reboot.
Register with it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/sunxi_wdt.c |   22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index c9ca343..744b6f5 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -27,8 +27,6 @@
 #include <linux/types.h>
 #include <linux/watchdog.h>
 
-#include <asm/system_misc.h>
-
 #define WDT_MAX_TIMEOUT         16
 #define WDT_MIN_TIMEOUT         1
 #define WDT_MODE_TIMEOUT(n)     ((n) << 3)
@@ -74,27 +72,25 @@ static const int wdt_timeout_map[] = {
 	[16] = 0xB, /* 16s */
 };
 
-static struct sunxi_wdt_dev *sunxi_restart_ctx;
-
-static void sun4i_wdt_restart(enum reboot_mode mode, const char *cmd)
+static void sunxi_wdt_reboot(struct watchdog_device *wdt_dev,
+			     enum reboot_mode mode, const char *cmd)
 {
-	if (!sunxi_restart_ctx)
-		return;
+	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
 
 	/* Enable timer and set reset bit in the watchdog */
 	writel(WDT_MODE_EN | WDT_MODE_RST_EN,
-	       sunxi_restart_ctx->wdt_base + WDT_MODE);
+	       sunxi_wdt->wdt_base + WDT_MODE);
 
 	/*
 	 * Restart the watchdog. The default (and lowest) interval
 	 * value for the watchdog is 0.5s.
 	 */
-	writel(WDT_CTRL_RELOAD, sunxi_restart_ctx->wdt_base + WDT_CTRL);
+	writel(WDT_CTRL_RELOAD, sunxi_wdt->wdt_base + WDT_CTRL);
 
 	while (1) {
 		mdelay(5);
 		writel(WDT_MODE_EN | WDT_MODE_RST_EN,
-		       sunxi_restart_ctx->wdt_base + WDT_MODE);
+		       sunxi_wdt->wdt_base + WDT_MODE);
 	}
 }
 
@@ -171,6 +167,7 @@ static const struct watchdog_ops sunxi_wdt_ops = {
 	.start		= sunxi_wdt_start,
 	.stop		= sunxi_wdt_stop,
 	.ping		= sunxi_wdt_ping,
+	.reboot		= sunxi_wdt_reboot,
 	.set_timeout	= sunxi_wdt_set_timeout,
 };
 
@@ -209,9 +206,6 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
 	if (unlikely(err))
 		return err;
 
-	sunxi_restart_ctx = sunxi_wdt;
-	arm_pm_restart = sun4i_wdt_restart;
-
 	dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
 			sunxi_wdt->wdt_dev.timeout, nowayout);
 
@@ -222,8 +216,6 @@ static int sunxi_wdt_remove(struct platform_device *pdev)
 {
 	struct sunxi_wdt_dev *sunxi_wdt = platform_get_drvdata(pdev);
 
-	arm_pm_restart = NULL;
-
 	watchdog_unregister_device(&sunxi_wdt->wdt_dev);
 	watchdog_set_drvdata(&sunxi_wdt->wdt_dev, NULL);
 
-- 
1.7.9.7

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Will Deacon]

Hi Guenter,

This looks pretty sensible to me (and the arm/arm64 bits look fine too), but
I have one question below...

On Thu, May 01, 2014 at 04:41:29PM +0100, Guenter Roeck wrote:
> Some hardware implements reboot through its watchdog hardware,
> for example by triggering a watchdog timeout. Platform specific
> code starts to spread into watchdog drivers, typically by setting
> pointers to a callback functions which is then called from the
> platform reset handler.
> 
> To simplify code and provide a unified API to trigger reboots by
> watchdog drivers, provide a single API to trigger such reboots
> through the watchdog subsystem.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
>  include/linux/watchdog.h         |   11 +++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..4ec6e2f 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -43,6 +43,17 @@
>  static DEFINE_IDA(watchdog_ida);
>  static struct class *watchdog_class;
>  
> +static struct watchdog_device *wdd_reboot_dev;
> +
> +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
> +{
> +	if (wdd_reboot_dev) {
> +		if (wdd_reboot_dev->ops->reboot)
> +			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
> +	}
> +}

What reboot_mode values would you expect a watchdog to support other than
REBOOT_HARD? Also, is the cmd even useful here?

Will

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Guenter Roeck]

On 05/02/2014 03:01 AM, Will Deacon wrote:
> Hi Guenter,
>
> This looks pretty sensible to me (and the arm/arm64 bits look fine too), but
> I have one question below...
>
> On Thu, May 01, 2014 at 04:41:29PM +0100, Guenter Roeck wrote:
>> Some hardware implements reboot through its watchdog hardware,
>> for example by triggering a watchdog timeout. Platform specific
>> code starts to spread into watchdog drivers, typically by setting
>> pointers to a callback functions which is then called from the
>> platform reset handler.
>>
>> To simplify code and provide a unified API to trigger reboots by
>> watchdog drivers, provide a single API to trigger such reboots
>> through the watchdog subsystem.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
>>   include/linux/watchdog.h         |   11 +++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
>> index cec9b55..4ec6e2f 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -43,6 +43,17 @@
>>   static DEFINE_IDA(watchdog_ida);
>>   static struct class *watchdog_class;
>>
>> +static struct watchdog_device *wdd_reboot_dev;
>> +
>> +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
>> +{
>> +	if (wdd_reboot_dev) {
>> +		if (wdd_reboot_dev->ops->reboot)
>> +			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
>> +	}
>> +}
>
> What reboot_mode values would you expect a watchdog to support other than
> REBOOT_HARD? Also, is the cmd even useful here?
>

Answer is "I don't know" in both cases. I thought about dropping the parameters,
but then I thought it does not hurt to have them around either. I am open to
suggestions and to dropping the parameters if everyone agrees that they are
useless.

Guenter

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Maxime Ripard]

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

Hi Guenter,

On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote:
> Some hardware implements reboot through its watchdog hardware,
> for example by triggering a watchdog timeout. Platform specific
> code starts to spread into watchdog drivers, typically by setting
> pointers to a callback functions which is then called from the
> platform reset handler.
> 
> To simplify code and provide a unified API to trigger reboots by
> watchdog drivers, provide a single API to trigger such reboots
> through the watchdog subsystem.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
>  include/linux/watchdog.h         |   11 +++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..4ec6e2f 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -43,6 +43,17 @@
>  static DEFINE_IDA(watchdog_ida);
>  static struct class *watchdog_class;
>  
> +static struct watchdog_device *wdd_reboot_dev;
> +
> +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
> +{
> +	if (wdd_reboot_dev) {
> +		if (wdd_reboot_dev->ops->reboot)
> +			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
> +	}
> +}
> +EXPORT_SYMBOL(watchdog_do_reboot);
> +
>  static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>  {
>  	/*
> @@ -162,6 +173,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
>  		return ret;
>  	}
>  
> +	if (wdd->ops->reboot)
> +		wdd_reboot_dev = wdd;
> +

Overall, it looks really great, but I guess we can make it a
list. Otherwise, we might end up in a situation where we could not
reboot anymore, like this one for example:
  - a first watchdog is probed, registers a reboot function
  - a second watchdog is probed, registers a reboot function that
    overwrites the first one.
  - then, the second watchdog disappears for some reason, and the
    reboot is set to NULL

Or maybe we can just use the start callback, with the min timeout already
registered, and prevent the user to kick the watchdog.

Maxime

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

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

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Guenter Roeck]

On Fri, May 02, 2014 at 06:22:43PM -0700, Maxime Ripard wrote:
> Hi Guenter,
> 
> On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote:
> > Some hardware implements reboot through its watchdog hardware,
> > for example by triggering a watchdog timeout. Platform specific
> > code starts to spread into watchdog drivers, typically by setting
> > pointers to a callback functions which is then called from the
> > platform reset handler.
> > 
> > To simplify code and provide a unified API to trigger reboots by
> > watchdog drivers, provide a single API to trigger such reboots
> > through the watchdog subsystem.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
> >  include/linux/watchdog.h         |   11 +++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> > index cec9b55..4ec6e2f 100644
> > --- a/drivers/watchdog/watchdog_core.c
> > +++ b/drivers/watchdog/watchdog_core.c
> > @@ -43,6 +43,17 @@
> >  static DEFINE_IDA(watchdog_ida);
> >  static struct class *watchdog_class;
> >  
> > +static struct watchdog_device *wdd_reboot_dev;
> > +
> > +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
> > +{
> > +	if (wdd_reboot_dev) {
> > +		if (wdd_reboot_dev->ops->reboot)
> > +			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
> > +	}
> > +}
> > +EXPORT_SYMBOL(watchdog_do_reboot);
> > +
> >  static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
> >  {
> >  	/*
> > @@ -162,6 +173,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
> >  		return ret;
> >  	}
> >  
> > +	if (wdd->ops->reboot)
> > +		wdd_reboot_dev = wdd;
> > +
> 
> Overall, it looks really great, but I guess we can make it a
> list. Otherwise, we might end up in a situation where we could not
> reboot anymore, like this one for example:
>   - a first watchdog is probed, registers a reboot function
>   - a second watchdog is probed, registers a reboot function that
>     overwrites the first one.
>   - then, the second watchdog disappears for some reason, and the
>     reboot is set to NULL
> 
I thought about that, but how likely (or unlikely) is that to ever happen ?
So I figured it is not worth the effort, and would just add complexity without
real gain. We could always add the list later if we ever encounter a situation
where two watchdogs in the same system provide a reboot callback.

> Or maybe we can just use the start callback, with the min timeout already
> registered, and prevent the user to kick the watchdog.
> 
Doesn't always work, unfortunately, even now. The moxart driver causes 
an explicit and immediate reset. Also, some watchdogs don't reset the system
directly but get an interrupt, which then calls the reset handler. Which,
in our case, would call the start callback again, and you would have an endless
loop.

Guenter

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Maxime Ripard]

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

On Fri, May 02, 2014 at 09:29:25PM -0700, Guenter Roeck wrote:
> > > +	if (wdd->ops->reboot)
> > > +		wdd_reboot_dev = wdd;
> > > +
> > 
> > Overall, it looks really great, but I guess we can make it a
> > list. Otherwise, we might end up in a situation where we could not
> > reboot anymore, like this one for example:
> >   - a first watchdog is probed, registers a reboot function
> >   - a second watchdog is probed, registers a reboot function that
> >     overwrites the first one.
> >   - then, the second watchdog disappears for some reason, and the
> >     reboot is set to NULL
> > 
> I thought about that, but how likely (or unlikely) is that to ever happen ?
> So I figured it is not worth the effort, and would just add complexity without
> real gain. We could always add the list later if we ever encounter a situation
> where two watchdogs in the same system provide a reboot callback.

The A31 we were discussing about in the other thread that doesn't have
a watchdog driver yet has four, identical, watchogs. I'm not really
concerned about the mentionned issue, since they will never be
removed, but the situation might happen with an on-SoC watchdog and an
i2c one (if that even exists).

But yes, right, that can be postponed.

> > Or maybe we can just use the start callback, with the min timeout already
> > registered, and prevent the user to kick the watchdog.
> > 
> Doesn't always work, unfortunately, even now. The moxart driver causes 
> an explicit and immediate reset. Also, some watchdogs don't reset the system
> directly but get an interrupt, which then calls the reset handler. Which,
> in our case, would call the start callback again, and you would have an endless
> loop.

Ok. You have my Acked-by then.

Thanks!
Maxime

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

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

Re: [RFC PATCH 0/5] watchdog: Add reboot API

[Arnd Bergmann]

On Thursday 01 May 2014, Guenter Roeck wrote:
> 
> Some hardware implements reboot through its watchdog hardware, for example
> by triggering a watchdog timeout or by writing into its watchdog register
> set. Platform specific code starts to spread into watchdog drivers,
> typically by setting pointers to a callback function which is then called
> from the architecture's reset handler.

Looks very nice to me, thanks for starting this!

Acked-by: Arnd Bergmann <arnd@arndb.de>

	Arnd

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Felipe Balbi]

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

On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote:
> Some hardware implements reboot through its watchdog hardware,
> for example by triggering a watchdog timeout. Platform specific
> code starts to spread into watchdog drivers, typically by setting
> pointers to a callback functions which is then called from the
> platform reset handler.
> 
> To simplify code and provide a unified API to trigger reboots by
> watchdog drivers, provide a single API to trigger such reboots
> through the watchdog subsystem.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
>  include/linux/watchdog.h         |   11 +++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..4ec6e2f 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -43,6 +43,17 @@
>  static DEFINE_IDA(watchdog_ida);
>  static struct class *watchdog_class;
>  
> +static struct watchdog_device *wdd_reboot_dev;
> +
> +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
> +{
> +	if (wdd_reboot_dev) {
> +		if (wdd_reboot_dev->ops->reboot)

you can decrease one level of indentation:

	if (wdd_reboot_dev && wdd_reboot_dev->ops->reboot)

also, shouldn't you test if 'ops' is valid too ? In that case:

	if (wdd_reboot_dev && wdd_reboot_dev->ops &&
		wdd_reboot_dev->ops->reboot)

-- 
balbi

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

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Guenter Roeck]

On Mon, May 05, 2014 at 01:36:06PM -0500, Felipe Balbi wrote:
> On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote:
> > Some hardware implements reboot through its watchdog hardware,
> > for example by triggering a watchdog timeout. Platform specific
> > code starts to spread into watchdog drivers, typically by setting
> > pointers to a callback functions which is then called from the
> > platform reset handler.
> > 
> > To simplify code and provide a unified API to trigger reboots by
> > watchdog drivers, provide a single API to trigger such reboots
> > through the watchdog subsystem.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
> >  include/linux/watchdog.h         |   11 +++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> > index cec9b55..4ec6e2f 100644
> > --- a/drivers/watchdog/watchdog_core.c
> > +++ b/drivers/watchdog/watchdog_core.c
> > @@ -43,6 +43,17 @@
> >  static DEFINE_IDA(watchdog_ida);
> >  static struct class *watchdog_class;
> >  
> > +static struct watchdog_device *wdd_reboot_dev;
> > +
> > +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
> > +{
> > +	if (wdd_reboot_dev) {
> > +		if (wdd_reboot_dev->ops->reboot)
> 
> you can decrease one level of indentation:
> 
> 	if (wdd_reboot_dev && wdd_reboot_dev->ops->reboot)
> 
> also, shouldn't you test if 'ops' is valid too ? In that case:
> 
> 	if (wdd_reboot_dev && wdd_reboot_dev->ops &&
> 		wdd_reboot_dev->ops->reboot)
> 
Hmmm ... makes me think.

If ops->reboot is not set, wdd_reboot_dev should be NULL
to start with, since it is only initialized if ops->reboot
is not NULL.

It is not common in the Linux kernel to re-check pointers
if the condition can not occur, so I don't think it should
be done here.

In other words, I should probably remove the ops check as well.

Guenter

Re: [RFC PATCH 0/5] watchdog: Add reboot API

[Jonas Jensen]

Thanks,

Applied in series: 1, 3, and 4

Patches work on moxa UC-7112-LX hardware.

Tested-by: Jonas Jensen <jonas.jensen@gmail.com>

Re: [RFC PATCH 0/5] watchdog: Add reboot API

[Heiko Stübner]

Am Donnerstag, 1. Mai 2014, 08:41:28 schrieb Guenter Roeck:
> Some hardware implements reboot through its watchdog hardware, for example
> by triggering a watchdog timeout or by writing into its watchdog register
> set. Platform specific code starts to spread into watchdog drivers,
> typically by setting pointers to a callback function which is then called
> from the architecture's reset handler.
> 
> While global and exported callback function pointers (such as
> arm_pm_restart) may be acceptable as long as they are used from platform
> and/or architecture code, using such a mechanism across subsystems and
> drivers is less than desirable. Ultimately, we'll need a better solution.
> 
> This patch series is an attempt to provide such a solution. It extends
> the watchdog subsystem to support reboot functionality, provides an
> API function call to trigger reboots, adds support for the new API
> to arm and arm64, and converts the drivers providing reboot functionality
> to use the new infrastructure.
> 
> The first patch in the series implements the new API. The second and third
> patch modify the arm and arm64 architecture reset handlers to call the
> added API function. The final two patches register the reboot handlers
> in the sunxi and moxart watchdog drivers with the watchdog subsystem.
> 
> The sunxi patche depends on the most recent patch series sumitted by
> Maxime Ripard.

a lot of Samsung architectures use a watchdog based reset too (among them some 
of the s3c24xx I care about) and this series looks really great for this, so

Acked-by: Heiko Stuebner <heiko@sntech.de>

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Lucas Stach]

Hi Guenter,

Am Freitag, den 02.05.2014, 21:29 -0700 schrieb Guenter Roeck:
> On Fri, May 02, 2014 at 06:22:43PM -0700, Maxime Ripard wrote:
> > Hi Guenter,
> > 
> > On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote:
> > > Some hardware implements reboot through its watchdog hardware,
> > > for example by triggering a watchdog timeout. Platform specific
> > > code starts to spread into watchdog drivers, typically by setting
> > > pointers to a callback functions which is then called from the
> > > platform reset handler.
> > > 
> > > To simplify code and provide a unified API to trigger reboots by
> > > watchdog drivers, provide a single API to trigger such reboots
> > > through the watchdog subsystem.
> > > 
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
> > >  include/linux/watchdog.h         |   11 +++++++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> > > index cec9b55..4ec6e2f 100644
> > > --- a/drivers/watchdog/watchdog_core.c
> > > +++ b/drivers/watchdog/watchdog_core.c
> > > @@ -43,6 +43,17 @@
> > >  static DEFINE_IDA(watchdog_ida);
> > >  static struct class *watchdog_class;
> > >  
> > > +static struct watchdog_device *wdd_reboot_dev;
> > > +
> > > +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
> > > +{
> > > +	if (wdd_reboot_dev) {
> > > +		if (wdd_reboot_dev->ops->reboot)
> > > +			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL(watchdog_do_reboot);
> > > +
> > >  static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
> > >  {
> > >  	/*
> > > @@ -162,6 +173,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
> > >  		return ret;
> > >  	}
> > >  
> > > +	if (wdd->ops->reboot)
> > > +		wdd_reboot_dev = wdd;
> > > +
> > 
> > Overall, it looks really great, but I guess we can make it a
> > list. Otherwise, we might end up in a situation where we could not
> > reboot anymore, like this one for example:
> >   - a first watchdog is probed, registers a reboot function
> >   - a second watchdog is probed, registers a reboot function that
> >     overwrites the first one.
> >   - then, the second watchdog disappears for some reason, and the
> >     reboot is set to NULL
> > 
> I thought about that, but how likely (or unlikely) is that to ever happen ?
> So I figured it is not worth the effort, and would just add complexity without
> real gain. We could always add the list later if we ever encounter a situation
> where two watchdogs in the same system provide a reboot callback.
> 

While this is not directly related to the issue you are fixing with this
series, I would like to have it considered when talking about a watchdog
system reboot API.

On i.MX we have the same situation where we have to reboot through the
SoC watchdog. This works, but may leave the external components of the
system (those not integrated in the SoC) in an undefined state. So if we
have a PMIC with integrated watchdog we would rather like to this one to
reboot the system, as it the reset is then much more closer to a
power-on-reset.

This means we could have multiple watchdogs in the system, where we
really want a specific one (maybe designated through a DT property) to
do the reset. This isn't compatible with the "last watchdog that
registers a handler wins the system reset" logic in your patch.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Guenter Roeck]

On 05/07/2014 04:52 AM, Lucas Stach wrote:
> Hi Guenter,
>
> Am Freitag, den 02.05.2014, 21:29 -0700 schrieb Guenter Roeck:
>> On Fri, May 02, 2014 at 06:22:43PM -0700, Maxime Ripard wrote:
>>> Hi Guenter,
>>>
>>> On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote:
>>>> Some hardware implements reboot through its watchdog hardware,
>>>> for example by triggering a watchdog timeout. Platform specific
>>>> code starts to spread into watchdog drivers, typically by setting
>>>> pointers to a callback functions which is then called from the
>>>> platform reset handler.
>>>>
>>>> To simplify code and provide a unified API to trigger reboots by
>>>> watchdog drivers, provide a single API to trigger such reboots
>>>> through the watchdog subsystem.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>>   drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
>>>>   include/linux/watchdog.h         |   11 +++++++++++
>>>>   2 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
>>>> index cec9b55..4ec6e2f 100644
>>>> --- a/drivers/watchdog/watchdog_core.c
>>>> +++ b/drivers/watchdog/watchdog_core.c
>>>> @@ -43,6 +43,17 @@
>>>>   static DEFINE_IDA(watchdog_ida);
>>>>   static struct class *watchdog_class;
>>>>
>>>> +static struct watchdog_device *wdd_reboot_dev;
>>>> +
>>>> +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
>>>> +{
>>>> +	if (wdd_reboot_dev) {
>>>> +		if (wdd_reboot_dev->ops->reboot)
>>>> +			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
>>>> +	}
>>>> +}
>>>> +EXPORT_SYMBOL(watchdog_do_reboot);
>>>> +
>>>>   static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>>>>   {
>>>>   	/*
>>>> @@ -162,6 +173,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
>>>>   		return ret;
>>>>   	}
>>>>
>>>> +	if (wdd->ops->reboot)
>>>> +		wdd_reboot_dev = wdd;
>>>> +
>>>
>>> Overall, it looks really great, but I guess we can make it a
>>> list. Otherwise, we might end up in a situation where we could not
>>> reboot anymore, like this one for example:
>>>    - a first watchdog is probed, registers a reboot function
>>>    - a second watchdog is probed, registers a reboot function that
>>>      overwrites the first one.
>>>    - then, the second watchdog disappears for some reason, and the
>>>      reboot is set to NULL
>>>
>> I thought about that, but how likely (or unlikely) is that to ever happen ?
>> So I figured it is not worth the effort, and would just add complexity without
>> real gain. We could always add the list later if we ever encounter a situation
>> where two watchdogs in the same system provide a reboot callback.
>>
>
> While this is not directly related to the issue you are fixing with this
> series, I would like to have it considered when talking about a watchdog
> system reboot API.
>
> On i.MX we have the same situation where we have to reboot through the
> SoC watchdog. This works, but may leave the external components of the
> system (those not integrated in the SoC) in an undefined state. So if we
> have a PMIC with integrated watchdog we would rather like to this one to
> reboot the system, as it the reset is then much more closer to a
> power-on-reset.
>
> This means we could have multiple watchdogs in the system, where we
> really want a specific one (maybe designated through a DT property) to
> do the reset. This isn't compatible with the "last watchdog that
> registers a handler wins the system reset" logic in your patch.
>

Wouldn't the order in which watchdogs are configured in dt define that ?
The last one wins.

Thanks,
Guenter

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Lucas Stach]

Am Mittwoch, den 07.05.2014, 06:01 -0700 schrieb Guenter Roeck:
> On 05/07/2014 04:52 AM, Lucas Stach wrote:
> > Hi Guenter,
> >
> > Am Freitag, den 02.05.2014, 21:29 -0700 schrieb Guenter Roeck:
> >> On Fri, May 02, 2014 at 06:22:43PM -0700, Maxime Ripard wrote:
> >>> Hi Guenter,
> >>>
> >>> On Thu, May 01, 2014 at 08:41:29AM -0700, Guenter Roeck wrote:
> >>>> Some hardware implements reboot through its watchdog hardware,
> >>>> for example by triggering a watchdog timeout. Platform specific
> >>>> code starts to spread into watchdog drivers, typically by setting
> >>>> pointers to a callback functions which is then called from the
> >>>> platform reset handler.
> >>>>
> >>>> To simplify code and provide a unified API to trigger reboots by
> >>>> watchdog drivers, provide a single API to trigger such reboots
> >>>> through the watchdog subsystem.
> >>>>
> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>>> ---
> >>>>   drivers/watchdog/watchdog_core.c |   17 +++++++++++++++++
> >>>>   include/linux/watchdog.h         |   11 +++++++++++
> >>>>   2 files changed, 28 insertions(+)
> >>>>
> >>>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> >>>> index cec9b55..4ec6e2f 100644
> >>>> --- a/drivers/watchdog/watchdog_core.c
> >>>> +++ b/drivers/watchdog/watchdog_core.c
> >>>> @@ -43,6 +43,17 @@
> >>>>   static DEFINE_IDA(watchdog_ida);
> >>>>   static struct class *watchdog_class;
> >>>>
> >>>> +static struct watchdog_device *wdd_reboot_dev;
> >>>> +
> >>>> +void watchdog_do_reboot(enum reboot_mode mode, const char *cmd)
> >>>> +{
> >>>> +	if (wdd_reboot_dev) {
> >>>> +		if (wdd_reboot_dev->ops->reboot)
> >>>> +			wdd_reboot_dev->ops->reboot(wdd_reboot_dev, mode, cmd);
> >>>> +	}
> >>>> +}
> >>>> +EXPORT_SYMBOL(watchdog_do_reboot);
> >>>> +
> >>>>   static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
> >>>>   {
> >>>>   	/*
> >>>> @@ -162,6 +173,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
> >>>>   		return ret;
> >>>>   	}
> >>>>
> >>>> +	if (wdd->ops->reboot)
> >>>> +		wdd_reboot_dev = wdd;
> >>>> +
> >>>
> >>> Overall, it looks really great, but I guess we can make it a
> >>> list. Otherwise, we might end up in a situation where we could not
> >>> reboot anymore, like this one for example:
> >>>    - a first watchdog is probed, registers a reboot function
> >>>    - a second watchdog is probed, registers a reboot function that
> >>>      overwrites the first one.
> >>>    - then, the second watchdog disappears for some reason, and the
> >>>      reboot is set to NULL
> >>>
> >> I thought about that, but how likely (or unlikely) is that to ever happen ?
> >> So I figured it is not worth the effort, and would just add complexity without
> >> real gain. We could always add the list later if we ever encounter a situation
> >> where two watchdogs in the same system provide a reboot callback.
> >>
> >
> > While this is not directly related to the issue you are fixing with this
> > series, I would like to have it considered when talking about a watchdog
> > system reboot API.
> >
> > On i.MX we have the same situation where we have to reboot through the
> > SoC watchdog. This works, but may leave the external components of the
> > system (those not integrated in the SoC) in an undefined state. So if we
> > have a PMIC with integrated watchdog we would rather like to this one to
> > reboot the system, as it the reset is then much more closer to a
> > power-on-reset.
> >
> > This means we could have multiple watchdogs in the system, where we
> > really want a specific one (maybe designated through a DT property) to
> > do the reset. This isn't compatible with the "last watchdog that
> > registers a handler wins the system reset" logic in your patch.
> >
> 
> Wouldn't the order in which watchdogs are configured in dt define that ?
> The last one wins.

That sounds rather fragile to me. I would like to have a more explicit
property to control this behavior.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [RFC PATCH 1/5] watchdog: Add API to trigger reboots

[Maxime Ripard]

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

On Wed, May 07, 2014 at 06:01:31AM -0700, Guenter Roeck wrote:
> >This means we could have multiple watchdogs in the system, where we
> >really want a specific one (maybe designated through a DT property) to
> >do the reset. This isn't compatible with the "last watchdog that
> >registers a handler wins the system reset" logic in your patch.
> >
> 
> Wouldn't the order in which watchdogs are configured in dt define that ?
> The last one wins.

It doesn't work, because the DT nodes are usually ordered by physical
address.

Maxime

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

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