[PATCHv2] watchdog: Add stop_on_reboot parameter to control reboot policy

[PATCHv2] watchdog: Add stop_on_reboot parameter to control reboot policy

[Dmitry Safonov]

Many watchdog drivers use watchdog_stop_on_reboot() helper in order
to stop the watchdog on system reboot. Unfortunately, this logic is
coded in driver's probe function and doesn't allows user to decide what
to do during shutdown/reboot.

On the other side, Xen and Qemu watchdog drivers (xen_wdt and i6300esb)
may be configured to either send NMI or turn off/reboot VM as
the watchdog action. As the kernel may stuck at any state, sending NMIs
can't reliably reboot the VM.

At Arista, we benefited from the following set-up: the emulated watchdogs
trigger VM reset and softdog is set to catch less severe conditions to
generate vmcore. Just before reboot watchdog's timeout is increased
to some good-enough value (3 mins). That keeps watchdog always running
and guarantees that VM doesn't stuck.

Provide new stop_on_reboot module parameter to let user control
watchdog's reboot policy.

Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: linux-watchdog@vger.kernel.org
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
Changes v1 => v2: Add module parameter instead of ioctl()

 drivers/watchdog/watchdog_core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 861daf4f37b2..5ead96199a0b 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -39,6 +39,10 @@
 
 static DEFINE_IDA(watchdog_ida);
 
+static int stop_on_reboot = -1;
+module_param(stop_on_reboot, int, 0644);
+MODULE_PARM_DESC(stop_on_reboot, "Stop watchdogs on reboot (0=keep watching, 1=stop)");
+
 /*
  * Deferred Registration infrastructure.
  *
@@ -254,6 +258,14 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		}
 	}
 
+	/* Module parameter to force watchdog policy on reboot. */
+	if (stop_on_reboot != -1) {
+		if (stop_on_reboot)
+			set_bit(WDOG_STOP_ON_REBOOT, &wdd->status);
+		else
+			clear_bit(WDOG_STOP_ON_REBOOT, &wdd->status);
+	}
+
 	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
 		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
 
-- 
2.25.0

Re: [PATCHv2] watchdog: Add stop_on_reboot parameter to control reboot policy

[Guenter Roeck]

On Fri, Feb 14, 2020 at 04:22:09PM +0000, Dmitry Safonov wrote:
> Many watchdog drivers use watchdog_stop_on_reboot() helper in order
> to stop the watchdog on system reboot. Unfortunately, this logic is
> coded in driver's probe function and doesn't allows user to decide what
> to do during shutdown/reboot.
> 
> On the other side, Xen and Qemu watchdog drivers (xen_wdt and i6300esb)
> may be configured to either send NMI or turn off/reboot VM as
> the watchdog action. As the kernel may stuck at any state, sending NMIs
> can't reliably reboot the VM.
> 
> At Arista, we benefited from the following set-up: the emulated watchdogs
> trigger VM reset and softdog is set to catch less severe conditions to
> generate vmcore. Just before reboot watchdog's timeout is increased
> to some good-enough value (3 mins). That keeps watchdog always running
> and guarantees that VM doesn't stuck.
> 
> Provide new stop_on_reboot module parameter to let user control
> watchdog's reboot policy.
> 
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: linux-watchdog@vger.kernel.org
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
> Changes v1 => v2: Add module parameter instead of ioctl()
> 
>  drivers/watchdog/watchdog_core.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 861daf4f37b2..5ead96199a0b 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -39,6 +39,10 @@
>  
>  static DEFINE_IDA(watchdog_ida);
>  
> +static int stop_on_reboot = -1;
> +module_param(stop_on_reboot, int, 0644);
> +MODULE_PARM_DESC(stop_on_reboot, "Stop watchdogs on reboot (0=keep watching, 1=stop)");
> +

My major concern is that this is writeable at runtime.
Changing the value won't change the behavior of already loaded
drivers. Unloading and reloading the driver will change its behavior
after the value was changed. This would be confusing, and it is hard
to imagine for anyone to expect such a behavior. Does this have to be
writeable ?

Guenter

>  /*
>   * Deferred Registration infrastructure.
>   *
> @@ -254,6 +258,14 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		}
>  	}
>  
> +	/* Module parameter to force watchdog policy on reboot. */
> +	if (stop_on_reboot != -1) {
> +		if (stop_on_reboot)
> +			set_bit(WDOG_STOP_ON_REBOOT, &wdd->status);
> +		else
> +			clear_bit(WDOG_STOP_ON_REBOOT, &wdd->status);
> +	}
> +
>  	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
>  		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
>  

Re: [PATCHv2] watchdog: Add stop_on_reboot parameter to control reboot policy

[Dmitry Safonov]

Hi Guenter,

On 2/22/20 4:06 PM, Guenter Roeck wrote:
> On Fri, Feb 14, 2020 at 04:22:09PM +0000, Dmitry Safonov wrote:
[..]
>> +static int stop_on_reboot = -1;
>> +module_param(stop_on_reboot, int, 0644);
>> +MODULE_PARM_DESC(stop_on_reboot, "Stop watchdogs on reboot (0=keep watching, 1=stop)");
>> +
> 
> My major concern is that this is writeable at runtime.
> Changing the value won't change the behavior of already loaded
> drivers. Unloading and reloading the driver will change its behavior
> after the value was changed. This would be confusing, and it is hard
> to imagine for anyone to expect such a behavior. Does this have to be
> writeable ?

No, it wasn't. I've messed it up by thinking about fours in 0644, but
for some reason failed to recognize that it allows root writes.

I'll follow up with v3, sorry for simple-minded typo.

Thanks,
          Dmitry