| Message ID | 20221028062750.45451-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | watchdog: iTCO_wdt: Set NO_REBOOT if the watchdog is not already running | expand |
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 */
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
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 */
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(-)