[PATCH] watchdog: iTCO_wdt: Set NO_REBOOT if the watchdog is not already running

[PATCH] watchdog: iTCO_wdt: Set NO_REBOOT if the watchdog is not already running

[Mika Westerberg]

Daniel reported that the commit 1ae3e78c0820 ("watchdog: iTCO_wdt: No
need to stop the timer in probe") makes QEMU implementation of the iTCO
watchdog not to trigger reboot anymore when NO_REBOOT flag is initially
cleared using this option (in QEMU command line):

  -global ICH9-LPC.noreboot=false

The problem with the commit is that it left the unconditional setting of
NO_REBOOT that is not cleared anymore when the kernel keeps pinging the
watchdog (as opposed to the previous code that called iTCO_wdt_stop()
that cleared it).

Fix this so that we only set NO_REBOOT if the watchdog was not initially
running.

Fixes: 1ae3e78c0820 ("watchdog: iTCO_wdt: No need to stop the timer in probe")
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Hi,

I tested this with the command line Daniel provided:

      -watchdog-action poweroff \
      -global ICH9-LPC.noreboot=false

With the patch QEMU shuts down after ~60s and without it keeps running.

 drivers/watchdog/iTCO_wdt.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 34693f11385f..e937b4dd28be 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -423,14 +423,18 @@ static unsigned int iTCO_wdt_get_timeleft(struct watchdog_device *wd_dev)
 	return time_left;
 }
 
-static void iTCO_wdt_set_running(struct iTCO_wdt_private *p)
+/* Returns true if the watchdog was running */
+static bool iTCO_wdt_set_running(struct iTCO_wdt_private *p)
 {
 	u16 val;
 
-	/* Bit 11: TCO Timer Halt -> 0 = The TCO timer is * enabled */
+	/* Bit 11: TCO Timer Halt -> 0 = The TCO timer is enabled */
 	val = inw(TCO1_CNT(p));
-	if (!(val & BIT(11)))
+	if (!(val & BIT(11))) {
 		set_bit(WDOG_HW_RUNNING, &p->wddev.status);
+		return true;
+	}
+	return false;
 }
 
 /*
@@ -518,9 +522,6 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
 	}
 
-	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	p->update_no_reboot_bit(p->no_reboot_priv, true);
-
 	if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
 		/*
 		 * Bit 13: TCO_EN -> 0
@@ -572,7 +573,13 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	watchdog_set_drvdata(&p->wddev, p);
 	platform_set_drvdata(pdev, p);
 
-	iTCO_wdt_set_running(p);
+	if (!iTCO_wdt_set_running(p)) {
+		/*
+		 * If the watchdog was not running set NO_REBOOT now to
+		 * prevent later reboots.
+		 */
+		p->update_no_reboot_bit(p->no_reboot_priv, true);
+	}
 
 	/* Check that the heartbeat value is within it's range;
 	   if not reset to the default */
-- 
2.35.1

Re: [PATCH] watchdog: iTCO_wdt: Set NO_REBOOT if the watchdog is not already running

[Guenter Roeck]

On 10/27/22 23:27, Mika Westerberg wrote:
> Daniel reported that the commit 1ae3e78c0820 ("watchdog: iTCO_wdt: No
> need to stop the timer in probe") makes QEMU implementation of the iTCO
> watchdog not to trigger reboot anymore when NO_REBOOT flag is initially
> cleared using this option (in QEMU command line):
> 
>    -global ICH9-LPC.noreboot=false
> 
> The problem with the commit is that it left the unconditional setting of
> NO_REBOOT that is not cleared anymore when the kernel keeps pinging the
> watchdog (as opposed to the previous code that called iTCO_wdt_stop()
> that cleared it).
> 
> Fix this so that we only set NO_REBOOT if the watchdog was not initially
> running.
> 
> Fixes: 1ae3e78c0820 ("watchdog: iTCO_wdt: No need to stop the timer in probe")
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

> ---
> Hi,
> 
> I tested this with the command line Daniel provided:
> 
>        -watchdog-action poweroff \
>        -global ICH9-LPC.noreboot=false
> 
> With the patch QEMU shuts down after ~60s and without it keeps running.
> 
>   drivers/watchdog/iTCO_wdt.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 34693f11385f..e937b4dd28be 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -423,14 +423,18 @@ static unsigned int iTCO_wdt_get_timeleft(struct watchdog_device *wd_dev)
>   	return time_left;
>   }
>   
> -static void iTCO_wdt_set_running(struct iTCO_wdt_private *p)
> +/* Returns true if the watchdog was running */
> +static bool iTCO_wdt_set_running(struct iTCO_wdt_private *p)
>   {
>   	u16 val;
>   
> -	/* Bit 11: TCO Timer Halt -> 0 = The TCO timer is * enabled */
> +	/* Bit 11: TCO Timer Halt -> 0 = The TCO timer is enabled */
>   	val = inw(TCO1_CNT(p));
> -	if (!(val & BIT(11)))
> +	if (!(val & BIT(11))) {
>   		set_bit(WDOG_HW_RUNNING, &p->wddev.status);
> +		return true;
> +	}
> +	return false;
>   }
>   
>   /*
> @@ -518,9 +522,6 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>   		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
>   	}
>   
> -	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	p->update_no_reboot_bit(p->no_reboot_priv, true);
> -
>   	if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
>   		/*
>   		 * Bit 13: TCO_EN -> 0
> @@ -572,7 +573,13 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>   	watchdog_set_drvdata(&p->wddev, p);
>   	platform_set_drvdata(pdev, p);
>   
> -	iTCO_wdt_set_running(p);
> +	if (!iTCO_wdt_set_running(p)) {
> +		/*
> +		 * If the watchdog was not running set NO_REBOOT now to
> +		 * prevent later reboots.
> +		 */
> +		p->update_no_reboot_bit(p->no_reboot_priv, true);
> +	}
>   
>   	/* Check that the heartbeat value is within it's range;
>   	   if not reset to the default */

Re: [PATCH] watchdog: iTCO_wdt: Set NO_REBOOT if the watchdog is not already running

[Daniel P. Berrangé]

On Fri, Oct 28, 2022 at 09:27:50AM +0300, Mika Westerberg wrote:
> Daniel reported that the commit 1ae3e78c0820 ("watchdog: iTCO_wdt: No
> need to stop the timer in probe") makes QEMU implementation of the iTCO
> watchdog not to trigger reboot anymore when NO_REBOOT flag is initially
> cleared using this option (in QEMU command line):
> 
>   -global ICH9-LPC.noreboot=false
> 
> The problem with the commit is that it left the unconditional setting of
> NO_REBOOT that is not cleared anymore when the kernel keeps pinging the
> watchdog (as opposed to the previous code that called iTCO_wdt_stop()
> that cleared it).
> 
> Fix this so that we only set NO_REBOOT if the watchdog was not initially
> running.
> 
> Fixes: 1ae3e78c0820 ("watchdog: iTCO_wdt: No need to stop the timer in probe")
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Tested-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|