[PATCH 0/2] [RFC] watchdog reboot timeout

[PATCH 0/2] [RFC] watchdog reboot timeout

[Bjorn Helgaas]

I'm looking for comments on these patches.  I'm not really happy with
them because they add basically identical code in each driver.  I also
don't like the fact that the parameter is not generic, so I have to know
which driver will be used on my machine.

Bjorn


Bjorn Helgaas (2):
      watchdog: iTCO_wdt: optionally leave watchdog enabled during restart
      watchdog: nv_tco: optionally leave watchdog enabled during restart


 drivers/watchdog/iTCO_wdt.c |   13 +++++++++++++
 drivers/watchdog/nv_tco.c   |   13 +++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

[PATCH 1/2] watchdog: iTCO_wdt: optionally leave watchdog enabled during restart

[Bjorn Helgaas]

A system reboot may fail, especially when rebooting via kexec, which can
result in a hung system that requires manual intervention.

This patch adds support for a "reboot_timeout", so we can program the
watchdog to reset the system if a reboot hangs.

On the way down, we schedule a watchdog reset for some time in the future:
long enough for the new kernel to boot and set up the watchdog timer itself.

If we reboot via BIOS, BIOS should disable the watchdog itself, so this
shouldn't cause unintended resets, even if the user interrupts the boot.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/watchdog/iTCO_wdt.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 5fd020d..66f800c 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -507,6 +507,11 @@ MODULE_PARM_DESC(heartbeat, "Watchdog timeout in seconds. "
 	"5..76 (TCO v1) or 3..614 (TCO v2), default="
 				__MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
 
+static int reboot_timeout;
+module_param(reboot_timeout, int, 0);
+MODULE_PARM_DESC(reboot_timeout,
+	"Schedule reset N seconds after reboot (default=0 (no reset))");
+
 static int nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, int, 0);
 MODULE_PARM_DESC(nowayout,
@@ -1050,6 +1055,14 @@ static int __devexit iTCO_wdt_remove(struct platform_device *dev)
 static void iTCO_wdt_shutdown(struct platform_device *dev)
 {
 	iTCO_wdt_stop();
+
+	if (system_state == SYSTEM_RESTART && reboot_timeout) {
+		printk(KERN_CRIT PFX
+		       "scheduling watchdog reset %d seconds from now\n",
+		       reboot_timeout);
+		iTCO_wdt_set_heartbeat(reboot_timeout);
+		iTCO_wdt_start();
+	}
 }
 
 #define iTCO_wdt_suspend NULL

Re: [PATCH 1/2] watchdog: iTCO_wdt: optionally leave watchdog enabled during restart

[Valdis.Kletnieks]

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

On Wed, 06 Jul 2011 10:09:36 MDT, Bjorn Helgaas said:

> If we reboot via BIOS, BIOS should disable the watchdog itself, so this
> shouldn't cause unintended resets, even if the user interrupts the boot.

Yes, but didn't Linus say something about BIOS code authors being
crack-addicted monkeys? :)

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

Re: [PATCH 1/2] watchdog: iTCO_wdt: optionally leave watchdog enabled during restart

[Pádraig Brady]

On 07/07/11 16:39, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 06 Jul 2011 10:09:36 MDT, Bjorn Helgaas said:
> 
>> If we reboot via BIOS, BIOS should disable the watchdog itself, so this
>> shouldn't cause unintended resets, even if the user interrupts the boot.
> 
> Yes, but didn't Linus say something about BIOS code authors being
> crack-addicted monkeys? :)

Yes as I said in a round about way in another mail,
one can't depend on that at all.
Some reset, some don't, some behave weirdly,
iTCO is unusual as kernel resets early at boot, ...

If using this, one would have to set the timeout large enough,
to encompass a full reboot

Re: [PATCH 1/2] watchdog: iTCO_wdt: optionally leave watchdog enabled during restart

[Bjorn Helgaas]

On Thu, Jul 7, 2011 at 9:53 AM, Pádraig Brady <P@draigbrady.com> wrote:
> On 07/07/11 16:39, Valdis.Kletnieks@vt.edu wrote:
>> On Wed, 06 Jul 2011 10:09:36 MDT, Bjorn Helgaas said:
>>
>>> If we reboot via BIOS, BIOS should disable the watchdog itself, so this
>>> shouldn't cause unintended resets, even if the user interrupts the boot.
>>
>> Yes, but didn't Linus say something about BIOS code authors being
>> crack-addicted monkeys? :)

I shouldn't have written anything about what BIOS "should" do.  That's
not very useful because, as you suggest, there is room for variation
there.

The risk I was alluding to was this:
  - User boots with "reboot_timeout=X"
  - User reboots normally (non-kexec)
  - BIOS does some reinitialization
  - Machine doesn't autoboot, e.g., because user interrupted boot
  - Watchdog resets machine -- this may be unexpected by the user

On the machines I tested, the unexpected reset doesn't happen because
the BIOS reinit includes disabling the watchdog.  But obviously, that
depends on BIOS details, so there's no guarantee.

I should have just written something along the lines of:

  The reboot_timeout option is intended for kexec reboots, which do
not involve BIOS.
  In this case, the reboot_timeout covers the interval between shutdown of the
  watchdog driver in the old kernel and startup of the driver in the new kernel.

  For normal reboots (via the BIOS), the behavior depends on the BIOS
implementation.
  Some BIOSes disable the watchdog timer, so the reboot_timeout only covers the
  interval until the BIOS disable.  Others leave the timer running, so
the reboot_timeout
  may cause a reset if the machine doesn't autoboot, e.g., if the user
interrupts the boot.

I think the *option* of using a reboot_timeout is still useful,
especially in clusters of unattended machines where it's expensive to
deal with boot failures.

> Yes as I said in a round about way in another mail,
> one can't depend on that at all.
> Some reset, some don't, some behave weirdly,
> iTCO is unusual as kernel resets early at boot, ...

You mentioned an unexplained iTCO reset in your other mail.  That
sounds like a kernel or iTCO_wdt bug, but I think it's unrelated to
this patch.

> If using this, one would have to set the timeout large enough,
> to encompass a full reboot

Right.  In the case of iTCO, I think the range is up to about 10
minutes, which is enough in my case (things like fsck may take longer,
but that's OK as long as the watchdog driver is built in statically).

Bjorn

[PATCH 2/2] watchdog: nv_tco: optionally leave watchdog enabled during restart

[Bjorn Helgaas]

A system reboot may fail, especially when rebooting via kexec, which can
result in a hung system that requires manual intervention.

This patch adds support for a "reboot_timeout", so we can program the
watchdog to reset the system if a reboot hangs.

On the way down, we schedule a watchdog reset for some time in the future:
long enough for the new kernel to boot and set up the watchdog timer itself.

If we reboot via BIOS, BIOS should disable the watchdog itself, so this
shouldn't cause unintended resets, even if the user interrupts the boot.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/watchdog/nv_tco.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/nv_tco.c b/drivers/watchdog/nv_tco.c
index afa78a5..90eac80 100644
--- a/drivers/watchdog/nv_tco.c
+++ b/drivers/watchdog/nv_tco.c
@@ -60,6 +60,11 @@ module_param(heartbeat, int, 0);
 MODULE_PARM_DESC(heartbeat, "Watchdog heartbeat in seconds. (2<heartbeat<39, "
 			    "default=" __MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
 
+static int reboot_timeout;
+module_param(reboot_timeout, int, 0);
+MODULE_PARM_DESC(reboot_timeout,
+	"Schedule reset N seconds after reboot (default=0 (no reset))");
+
 static int nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, int, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started"
@@ -459,6 +464,14 @@ static int __devexit nv_tco_remove(struct platform_device *dev)
 static void nv_tco_shutdown(struct platform_device *dev)
 {
 	tco_timer_stop();
+
+	if (system_state == SYSTEM_RESTART && reboot_timeout) {
+		printk(KERN_CRIT PFX
+		       "scheduling watchdog reset %d seconds from now\n",
+		       reboot_timeout);
+		tco_timer_set_heartbeat(reboot_timeout);
+		tco_timer_start();
+	}
 }
 
 static struct platform_driver nv_tco_driver = {

Re: [PATCH 0/2] [RFC] watchdog reboot timeout

[Pádraig Brady]

On 06/07/11 17:09, Bjorn Helgaas wrote:
> I'm looking for comments on these patches.  I'm not really happy with
> them because they add basically identical code in each driver.  I also
> don't like the fact that the parameter is not generic, so I have to know
> which driver will be used on my machine.

Hmm, so this is protecting the window between watchdog_stop and machine_reset.
It would be nice to have this as generic code that did WDIOC_SETTIMEOUT
immediately after (or instead of) shutdown.

Leaving a watchdog enabled across boot does have issues on some systems.
It was OK for winboond based watchdogs I tested, but for some IBM
server systems, "weird stuff" happened if the watchdog was left enabled
across boot.

Just to mention the other watchdog boot scenario I often use
(which does _not_ protect your window above), which is to enable the
watchdog in the BIOS.  This is more general protection for the
boot _up_ process.  On a netboot system for example this will mean
that the system will keep rebooting until whatever switch is
down etc. is restored. Many systems I've used had support
for this in the BIOS and those that didn't were quickly updated
by vendors to do so.

A related issue I noticed with iTCO_wdt is that something
else in the kernel disables the iTCO watchdog (which my BIOS had started).
This is undesirable for obvious reasons.

cheers,
Pádraig.

Re: [PATCH 0/2] [RFC] watchdog reboot timeout

[Bjorn Helgaas]

On Thu, Jul 7, 2011 at 7:51 AM, Pádraig Brady <P@draigbrady.com> wrote:
> On 06/07/11 17:09, Bjorn Helgaas wrote:
>> I'm looking for comments on these patches.  I'm not really happy with
>> them because they add basically identical code in each driver.  I also
>> don't like the fact that the parameter is not generic, so I have to know
>> which driver will be used on my machine.
>
> Hmm, so this is protecting the window between watchdog_stop and machine_reset.

That's part of it, but I'm really interested in reboots using kexec,
and in that case, there is no machine reset, and BIOS isn't involved
at all.  If the kexec fails or the new kernel hangs for some reason,
I'd like the watchdog to reset the machine.

Currently, we call all the driver .shutdown methods() on the way down,
and the watchdog shutdown methods turn off the watchdog, so we have no
protection after that.

> It would be nice to have this as generic code that did WDIOC_SETTIMEOUT
> immediately after (or instead of) shutdown.

Yes.  I'd prefer this to be more generic.  I've skimmed the new
generic watchdog code a bit, hoping for a nice way to do this, but the
actual .shutdown() method is still in each driver, so making it
generic doesn't seem trivial.  So consider this a plea for future
development.

> Leaving a watchdog enabled across boot does have issues on some systems.
> It was OK for winboond based watchdogs I tested, but for some IBM
> server systems, "weird stuff" happened if the watchdog was left enabled
> across boot.

Point taken: it's unwise to assume consistency across all BIOSes, so
it was a mistake to write the last paragraph ("If we reboot via BIOS,
BIOS should disable the watchdog ...").

My reboot_timeout is disabled by default and is only enabled if the
user specifies the module option.  I don't think it's possible to
enable something like this by default, because in general we don't
know anything about the new kernel.  It may not have a watchdog driver
at all.

But in many cases, we *do* know the machine doesn't do "weird stuff,"
the new kernel has a driver, and we can bound the expected boot time.
In those cases, I think this option provides useful protection.

> Just to mention the other watchdog boot scenario I often use
> (which does _not_ protect your window above), which is to enable the
> watchdog in the BIOS.  This is more general protection for the
> boot _up_ process.

Yes, that makes good sense.  In my case, I'm interested in kexec, so
the BIOS isn't involved, so enabling it in the BIOS wouldn't help.

> A related issue I noticed with iTCO_wdt is that something
> else in the kernel disables the iTCO watchdog (which my BIOS had started).
> This is undesirable for obvious reasons.

This sounds related to iTCO, but unrelated to my patch.  Right?

Thanks a lot for your comments!

Bjorn

Re: [PATCH 0/2] [RFC] watchdog reboot timeout

[Pádraig Brady]

On 12/07/11 17:25, Bjorn Helgaas wrote:
> On Thu, Jul 7, 2011 at 7:51 AM, Pádraig Brady <P@draigbrady.com> wrote:
>> A related issue I noticed with iTCO_wdt is that something
>> else in the kernel disables the iTCO watchdog (which my BIOS had started).
>> This is undesirable for obvious reasons.
> 
> This sounds related to iTCO, but unrelated to my patch.  Right?

Right, unrelated to your patch but it could affect you.
As the kernel comes up it may reinit the iTCO, disabling
the watchdog until the iTCO_wdt module is loaded and patted.

cheers,
Pádraig.