[PATCH 7/10 v2] Generic Watchdog Timer Driver

[PATCH 7/10 v2] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

watchdog: WatchDog Timer Driver Core - Part 7

Add support for the nowayout feature to the
WatchDog Timer Driver Core framework.
This feature prevents the watchdog timer from being
stopped.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

diff -urN linux-2.6.38-generic-part6/Documentation/watchdog/src/watchdog-with-timer-example.c linux-2.6.38-generic-part7/Documentation/watchdog/src/watchdog-with-timer-example.c
--- linux-2.6.38-generic-part6/Documentation/watchdog/src/watchdog-with-timer-example.c	2011-06-16 21:04:06.283940741 +0200
+++ linux-2.6.38-generic-part7/Documentation/watchdog/src/watchdog-with-timer-example.c	2011-06-17 09:52:32.866632635 +0200
@@ -45,6 +45,11 @@
 MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
 	"(default = " __MODULE_STRING(WDT_TIMEOUT) ")");
 
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started. "
+	"(default = " __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
 static struct watchdog_device wdt_dev;
 static void wdt_timer_tick(unsigned long data);
 static DEFINE_TIMER(timer, wdt_timer_tick, 0, 0);
@@ -152,6 +157,8 @@
 
 	/* Set watchdog_device parameters */
 	wdt_dev.timeout = timeout;
+	if (nowayout)
+		set_bit(WDOG_NO_WAY_OUT, &wdt_dev.status);
 
 	/* Register the watchdog timer device */
 	res = watchdog_register_device(&wdt_dev);
diff -urN linux-2.6.38-generic-part6/Documentation/watchdog/watchdog-kernel-api.txt linux-2.6.38-generic-part7/Documentation/watchdog/watchdog-kernel-api.txt
--- linux-2.6.38-generic-part6/Documentation/watchdog/watchdog-kernel-api.txt	2011-06-16 21:04:06.287940722 +0200
+++ linux-2.6.38-generic-part7/Documentation/watchdog/watchdog-kernel-api.txt	2011-06-17 09:58:20.754629453 +0200
@@ -57,8 +57,8 @@
   WDIOF_* status bits).
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the watchdog timer
-  running/active, is the device opened via the /dev/watchdog interface or not,
-  ...)
+  running/active, is the nowayout bit set, is the device opened via
+  the /dev/watchdog interface or not, ...)
 
 The list of watchdog operations is defined as:
 
@@ -129,7 +129,9 @@
 * WDOG_EXPECT_RELEASE: this bit stores whether or not the magic close character
   has been sent (so that we can support the magic close feature).
   (This bit should only be used by the WatchDog Timer Driver Core).
+* WDOG_NO_WAY_OUT: this bit stores the nowayout setting for the watchdog.
+  If this bit is set then the watchdog timer will not be able to stop.
 
-Note: The WatchDog Timer Driver Core supports the magic close feature. To use
-the magic close feature you must set the WDIOF_MAGICCLOSE bit in the options
-field of the watchdog's info structure.
+Note: The WatchDog Timer Driver Core supports the magic close feature and
+the nowayout feature. To use the magic close feature you must set the
+WDIOF_MAGICCLOSE bit in the options field of the watchdog's info structure.
diff -urN linux-2.6.38-generic-part6/drivers/watchdog/core/watchdog_dev.c linux-2.6.38-generic-part7/drivers/watchdog/core/watchdog_dev.c
--- linux-2.6.38-generic-part6/drivers/watchdog/core/watchdog_dev.c	2011-06-16 22:52:56.763937624 +0200
+++ linux-2.6.38-generic-part7/drivers/watchdog/core/watchdog_dev.c	2011-06-17 09:52:32.870632731 +0200
@@ -126,11 +126,15 @@
  *	Stop the watchdog if it is still active and unmark it active.
  *	This function returns zero on success or a negative errno code for
  *	failure.
+ *	If the 'nowayout' feature was set, the watchdog cannot be stopped.
  */
 
 static int watchdog_stop(struct watchdog_device *wddev)
 {
-	int err;
+	int err = -1;
+
+	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status))
+		return err;
 
 	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
 		err = wddev->ops->stop(wddev);
@@ -151,7 +155,7 @@
  *
  *	A write to a watchdog device is defined as a keepalive ping.
  *	Writing the magic 'V' sequence allows the next close to turn
- *	off the watchdog.
+ *	off the watchdog (if 'nowayout' is not set).
  */
 
 static ssize_t watchdog_write(struct file *file, const char __user *data,
diff -urN linux-2.6.38-generic-part6/include/linux/watchdog.h linux-2.6.38-generic-part7/include/linux/watchdog.h
--- linux-2.6.38-generic-part6/include/linux/watchdog.h	2011-06-17 12:17:15.285063678 +0200
+++ linux-2.6.38-generic-part7/include/linux/watchdog.h	2011-06-17 12:17:39.205063756 +0200
@@ -86,6 +86,7 @@
 #define WDOG_DEV_OPEN		1	/* is the watchdog opened via
 					 * /dev/watchdog */
 #define WDOG_EXPECT_RELEASE	2	/* did we receive the magic char ? */
+#define WDOG_NO_WAY_OUT		3	/* is 'nowayout' feature set ? */
 };
 
 /* drivers/watchdog/core/watchdog_core.c */

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Arnd Bergmann]

On Saturday 18 June 2011 19:25:37 Wim Van Sebroeck wrote:
> diff -urN linux-2.6.38-generic-part6/Documentation/watchdog/src/watchdog-with-timer-example.c linux-2.6.38-generic-part7/Documentation/watchdog/src/watchdog-with-timer-example.c
> --- linux-2.6.38-generic-part6/Documentation/watchdog/src/watchdog-with-timer-example.c 2011-06-16 21:04:06.283940741 +0200
> +++ linux-2.6.38-generic-part7/Documentation/watchdog/src/watchdog-with-timer-example.c 2011-06-17 09:52:32.866632635 +0200
> @@ -45,6 +45,11 @@
>  MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
>         "(default = " __MODULE_STRING(WDT_TIMEOUT) ")");
>  
> +static int nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, int, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started. "
> +       "(default = " __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
>  static struct watchdog_device wdt_dev;
>  static void wdt_timer_tick(unsigned long data);
>  static DEFINE_TIMER(timer, wdt_timer_tick, 0, 0);
> @@ -152,6 +157,8 @@
>  
>         /* Set watchdog_device parameters */
>         wdt_dev.timeout = timeout;
> +       if (nowayout)
> +               set_bit(WDOG_NO_WAY_OUT, &wdt_dev.status);
>  
>         /* Register the watchdog timer device */
>         res = watchdog_register_device(&wdt_dev);

Why is this a feature of the individual drivers and not of the core?

Maybe it can be in both, so existing drivers don't need to change the user
interface, but new drivers don't have to provide the option individually
when you can simply set it for the base module.

	Arnd

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Alan Cox]

> >         /* Register the watchdog timer device */
> >         res = watchdog_register_device(&wdt_dev);
> 
> Why is this a feature of the individual drivers and not of the core?

It's a hardware dependant feature - some watchdogs cannot be stopped once
initialized.

> Maybe it can be in both, so existing drivers don't need to change the user
> interface, but new drivers don't have to provide the option individually
> when you can simply set it for the base module.

Then you'd need an additional interface to specify which watchdog as soon
as we support multiple watchdogs.

Alan

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Arnd Bergmann]

On Sunday 19 June 2011 12:03:28 Alan Cox wrote:
> > >         /* Register the watchdog timer device */
> > >         res = watchdog_register_device(&wdt_dev);
> > 
> > Why is this a feature of the individual drivers and not of the core?
> 
> It's a hardware dependant feature - some watchdogs cannot be stopped once
> initialized.

Ah, I see. OTOH, 80 of the 107 watchdogs in the kernel do provide it as a
module option, which indicates that there is some room for consolidation,
even if it might not be appropriate for every one of them.

> > Maybe it can be in both, so existing drivers don't need to change the user
> > interface, but new drivers don't have to provide the option individually
> > when you can simply set it for the base module.
> 
> Then you'd need an additional interface to specify which watchdog as soon
> as we support multiple watchdogs.

You can always have multiple ways of setting nowayout -- hardware requirements,
global module option, local module option, and a new ioctl command -- but
what is being used is then the logical OR of all of them.

	Arnd

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Alan Cox]

> > Then you'd need an additional interface to specify which watchdog as soon
> > as we support multiple watchdogs.
> 
> You can always have multiple ways of setting nowayout -- hardware requirements,
> global module option, local module option, and a new ioctl command -- but
> what is being used is then the logical OR of all of them.

An ioctl for it would make a lot of sense as watchdogs are often compiled
in so currently there isn't a good way to runtime set this.

Alan

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Mark Lord]

On 11-06-19 10:19 AM, Alan Cox wrote:
>>> Then you'd need an additional interface to specify which watchdog as soon
>>> as we support multiple watchdogs.
>>
>> You can always have multiple ways of setting nowayout -- hardware requirements,
>> global module option, local module option, and a new ioctl command -- but
>> what is being used is then the logical OR of all of them.
> 
> An ioctl for it would make a lot of sense as watchdogs are often compiled
> in so currently there isn't a good way to runtime set this.

I wouldn't mind a kernel parameter to enable a hardware watchdog timer at boot.
Currently, there's a window at startup where the watchdog is not enabled,
and the system could lock up and die in there without it being triggered.

Cheers

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi Mark,

> >>> Then you'd need an additional interface to specify which watchdog as soon
> >>> as we support multiple watchdogs.
> >>
> >> You can always have multiple ways of setting nowayout -- hardware requirements,
> >> global module option, local module option, and a new ioctl command -- but
> >> what is being used is then the logical OR of all of them.
> > 
> > An ioctl for it would make a lot of sense as watchdogs are often compiled
> > in so currently there isn't a good way to runtime set this.
> 
> I wouldn't mind a kernel parameter to enable a hardware watchdog timer at boot.
> Currently, there's a window at startup where the watchdog is not enabled,
> and the system could lock up and die in there without it being triggered.

This is another tricky thing were developers will always discuss about.
What you don't want to happen is that the watchdog reboots your system when it does
an fsck at bootup (for instance because the system rebooted by the watchdog and left
the filesystem in a dirty state...).

So it's more complex if you look at the overal system...

Kind regards,
Wim.

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Mark Lord]

On 11-06-22 04:13 PM, Wim Van Sebroeck wrote:
> Hi Mark,
> 
>>>>> Then you'd need an additional interface to specify which watchdog as soon
>>>>> as we support multiple watchdogs.
>>>>
>>>> You can always have multiple ways of setting nowayout -- hardware requirements,
>>>> global module option, local module option, and a new ioctl command -- but
>>>> what is being used is then the logical OR of all of them.
>>>
>>> An ioctl for it would make a lot of sense as watchdogs are often compiled
>>> in so currently there isn't a good way to runtime set this.
>>
>> I wouldn't mind a kernel parameter to enable a hardware watchdog timer at boot.
>> Currently, there's a window at startup where the watchdog is not enabled,
>> and the system could lock up and die in there without it being triggered.
> 
> This is another tricky thing were developers will always discuss about.
> What you don't want to happen is that the watchdog reboots your system when it does
> an fsck at bootup (for instance because the system rebooted by the watchdog and left
> the filesystem in a dirty state...).
> 
> So it's more complex if you look at the overal system...

Sure, but that's got little to do with wanting a kernel parameter to OPTIONALLY
enable a hardware watchdog timer at boot.

Filesystem checks are a separate issue, easily worked around in practice.

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Arnd Bergmann]

On Thursday 23 June 2011 16:13:29 Mark Lord wrote:
> On 11-06-22 04:13 PM, Wim Van Sebroeck wrote:
> > 
> > This is another tricky thing were developers will always discuss about.
> > What you don't want to happen is that the watchdog reboots your system when it does
> > an fsck at bootup (for instance because the system rebooted by the watchdog and left
> > the filesystem in a dirty state...).
> > 
> > So it's more complex if you look at the overal system...
> 
> Sure, but that's got little to do with wanting a kernel parameter to OPTIONALLY
> enable a hardware watchdog timer at boot.
> 
> Filesystem checks are a separate issue, easily worked around in practice.

I agree, it's nice to give system integrators the option to enable the watchdog
very early, the problems that Wim mentioned need to be solved in user space
but are not a serious limitation.

	Arnd

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi All,

> > > This is another tricky thing were developers will always discuss about.
> > > What you don't want to happen is that the watchdog reboots your system when it does
> > > an fsck at bootup (for instance because the system rebooted by the watchdog and left
> > > the filesystem in a dirty state...).
> > > 
> > > So it's more complex if you look at the overal system...
> > 
> > Sure, but that's got little to do with wanting a kernel parameter to OPTIONALLY
> > enable a hardware watchdog timer at boot.
> > 
> > Filesystem checks are a separate issue, easily worked around in practice.
> 
> I agree, it's nice to give system integrators the option to enable the watchdog
> very early, the problems that Wim mentioned need to be solved in user space
> but are not a serious limitation.

I'm definitely not against it and I am sure that we all agree that this is a valid
option for some drivers (certainly in embedded environments). But for me it has to
stay an option.
FYI: We allready have one driver that does this: w83697hf_wdt.c See commit
6fd656012bb8d5c5a4570adc2e630668b0109cb0.

Kind regards,
Wim.

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Arnd Bergmann]

On Friday 24 June 2011, Wim Van Sebroeck wrote:
> > > > This is another tricky thing were developers will always discuss about.
> > > > What you don't want to happen is that the watchdog reboots your system when it does
> > > > an fsck at bootup (for instance because the system rebooted by the watchdog and left
> > > > the filesystem in a dirty state...).
> > > > 
> > > > So it's more complex if you look at the overal system...
> > > 
> > > Sure, but that's got little to do with wanting a kernel parameter to OPTIONALLY
> > > enable a hardware watchdog timer at boot.
> > > 
> > > Filesystem checks are a separate issue, easily worked around in practice.
> > 
> > I agree, it's nice to give system integrators the option to enable the watchdog
> > very early, the problems that Wim mentioned need to be solved in user space
> > but are not a serious limitation.
> 
> I'm definitely not against it and I am sure that we all agree that this is a valid
> option for some drivers (certainly in embedded environments). But for me it has to
> stay an option.

I certainly wouldn't make it the default to enable watchdogs on boot, but having
a generic command line argument in the core seems more appropriate to me than
doing it in the drivers, which would require knowing which driver is responsible.

> FYI: We allready have one driver that does this: w83697hf_wdt.c See commit
> 6fd656012bb8d5c5a4570adc2e630668b0109cb0.

This one is somewhat different because the watchdog is already enabled in
this case on bootup, and the command line option tells the driver not
to disable it if it's already on. I can also see why that may be useful,
but an option to enable it independent of its current state would be more
logical to other drivers that always come up disabled.

Anyway, it's certainly not an important functionality to have, and getting
your patches merged shouldn't wait for it.

	Arnd

Re: [PATCH 7/10 v2] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi Arnd,

> On Saturday 18 June 2011 19:25:37 Wim Van Sebroeck wrote:
> > diff -urN linux-2.6.38-generic-part6/Documentation/watchdog/src/watchdog-with-timer-example.c linux-2.6.38-generic-part7/Documentation/watchdog/src/watchdog-with-timer-example.c
> > --- linux-2.6.38-generic-part6/Documentation/watchdog/src/watchdog-with-timer-example.c 2011-06-16 21:04:06.283940741 +0200
> > +++ linux-2.6.38-generic-part7/Documentation/watchdog/src/watchdog-with-timer-example.c 2011-06-17 09:52:32.866632635 +0200
> > @@ -45,6 +45,11 @@
> >  MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
> >         "(default = " __MODULE_STRING(WDT_TIMEOUT) ")");
> >  
> > +static int nowayout = WATCHDOG_NOWAYOUT;
> > +module_param(nowayout, int, 0);
> > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started. "
> > +       "(default = " __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> >  static struct watchdog_device wdt_dev;
> >  static void wdt_timer_tick(unsigned long data);
> >  static DEFINE_TIMER(timer, wdt_timer_tick, 0, 0);
> > @@ -152,6 +157,8 @@
> >  
> >         /* Set watchdog_device parameters */
> >         wdt_dev.timeout = timeout;
> > +       if (nowayout)
> > +               set_bit(WDOG_NO_WAY_OUT, &wdt_dev.status);
> >  
> >         /* Register the watchdog timer device */
> >         res = watchdog_register_device(&wdt_dev);
> 
> Why is this a feature of the individual drivers and not of the core?

Some drivers/devices will need it (like Alan allready explained), where
others don't.

> Maybe it can be in both, so existing drivers don't need to change the user
> interface, but new drivers don't have to provide the option individually
> when you can simply set it for the base module.

the CONFIG_WATCHDOG_NOWAYOUT option in Kconfig allready stores the default
behaviour for all your drivers. the module param is initialized with it
during compilation ans allows to have it changed when you load the module.

Kind regards,
Wim.