[PATCH 5.10.y-cip 0/2] watchdog: iTCO_wdt: Backport patches to enable handle_boot_enabled=0

[PATCH 5.10.y-cip 0/2] watchdog: iTCO_wdt: Backport patches to enable handle_boot_enabled=0

[Jan Kiszka]

These patches are needed in order to make iTCO_wdt respect
handle_boot_enabled=0, i.e. to step back from a running watchdog until
userspace picks it up. Up to 5.16, the driver simply stopped the
watchdog which could have created a non-monitored window during boot-up
in the handover from the firmware starting it to userspace (typically
sytemd) safely driving. E.g., any infinite loops in the initramfs would
could have created a locked-up system.

These patches were proposed to the watchdog maintainers for official
stable integration  [1] but received no reaction so far. As the issue is critical for
CIP's OTA software update integration, let's use the CIP path instead.

I don't have any test setup for 4.19, so I'm only proposing this for
5.10 here.

Jan

[1] https://lore.kernel.org/linux-watchdog/e9c2d0a5-9618-b616-3be3-826cc0665388@siemens.com/T/#t

Mika Westerberg (2):
  watchdog: iTCO_wdt: No need to stop the timer in probe
  watchdog: iTCO_wdt: Set NO_REBOOT if the watchdog is not already
    running

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

-- 
2.35.3

[PATCH 5.10.y-cip 1/2] watchdog: iTCO_wdt: No need to stop the timer in probe

[Jan Kiszka]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Commit 1ae3e78c08209ac657c59f6f7ea21bbbd7f6a1d4 upstream.

The watchdog core can handle pinging of the watchdog before userspace
opens the device. For this reason instead of stopping the timer, just
mark it as running and let the watchdog core take care of it.

Cc: Malin Jonsson <malin.jonsson@ericsson.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/r/20210921102900.61586-1-mika.westerberg@linux.intel.com
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@linux-watchdog.org>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/watchdog/iTCO_wdt.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index a370a185a41c..9048fa44897f 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -426,6 +426,16 @@ 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)
+{
+	u16 val;
+
+	/* Bit 11: TCO Timer Halt -> 0 = The TCO timer is * enabled */
+	val = inw(TCO1_CNT(p));
+	if (!(val & BIT(11)))
+		set_bit(WDOG_HW_RUNNING, &p->wddev.status);
+}
+
 /*
  *	Kernel Interfaces
  */
@@ -568,8 +578,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	watchdog_set_drvdata(&p->wddev, p);
 	platform_set_drvdata(pdev, p);
 
-	/* Make sure the watchdog is not running */
-	iTCO_wdt_stop(&p->wddev);
+	iTCO_wdt_set_running(p);
 
 	/* Check that the heartbeat value is within it's range;
 	   if not reset to the default */
-- 
2.35.3

[PATCH 5.10.y-cip 2/2] watchdog: iTCO_wdt: Set NO_REBOOT if the watchdog is not already running

[Jan Kiszka]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Commit ef9b7bf52c2f47f0a9bf988543c577b92c92d15e upstream.

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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/r/20221028062750.45451-1-mika.westerberg@linux.intel.com
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Wim Van Sebroeck <wim@linux-watchdog.org>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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 9048fa44897f..50c874d48860 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -426,14 +426,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;
 }
 
 /*
@@ -524,9 +528,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
@@ -578,7 +579,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.3

RE: [PATCH 5.10.y-cip 0/2] watchdog: iTCO_wdt: Backport patches to enable handle_boot_enabled=0

[nobuhiro1.iwamatsu]

Hi Jan,

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com>
> Sent: Wednesday, July 19, 2023 2:01 AM
> To: cip-dev@lists.cip-project.org; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□
> ＤＩＴ○ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek
> <pavel@denx.de>
> Subject: [PATCH 5.10.y-cip 0/2] watchdog: iTCO_wdt: Backport patches to
> enable handle_boot_enabled=0
> 
> These patches are needed in order to make iTCO_wdt respect
> handle_boot_enabled=0, i.e. to step back from a running watchdog until
> userspace picks it up. Up to 5.16, the driver simply stopped the watchdog
> which could have created a non-monitored window during boot-up in the
> handover from the firmware starting it to userspace (typically
> sytemd) safely driving. E.g., any infinite loops in the initramfs would could have
> created a locked-up system.
> 
> These patches were proposed to the watchdog maintainers for official stable
> integration  [1] but received no reaction so far. As the issue is critical for CIP's
> OTA software update integration, let's use the CIP path instead.

Feedback from the watchdog maintainers is reassuring, but you can also submit
backported patches directly to stable. How about sending a patch to stable first?

> 
> I don't have any test setup for 4.19, so I'm only proposing this for
> 5.10 here.
> 
> Jan
> 
> [1]
> https://lore.kernel.org/linux-watchdog/e9c2d0a5-9618-b616-3be3-826cc066
> 5388@siemens.com/T/#t
> 
> Mika Westerberg (2):
>   watchdog: iTCO_wdt: No need to stop the timer in probe
>   watchdog: iTCO_wdt: Set NO_REBOOT if the watchdog is not already
>     running
> 
>  drivers/watchdog/iTCO_wdt.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> --
> 2.35.3

Best regard,
  Nobuhiro

Re: [PATCH 5.10.y-cip 0/2] watchdog: iTCO_wdt: Backport patches to enable handle_boot_enabled=0

[Pavel Machek]

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

Hi!

> These patches are needed in order to make iTCO_wdt respect
> handle_boot_enabled=0, i.e. to step back from a running watchdog until
> userspace picks it up. Up to 5.16, the driver simply stopped the
> watchdog which could have created a non-monitored window during boot-up
> in the handover from the firmware starting it to userspace (typically
> sytemd) safely driving. E.g., any infinite loops in the initramfs would
> could have created a locked-up system.
> 
> These patches were proposed to the watchdog maintainers for official
> stable integration  [1] but received no reaction so far. As the issue is critical for
> CIP's OTA software update integration, let's use the CIP path
> instead.

So... these are mainline, but -stable failed to react. Rules say we
can take them.

Both patches are in 6.1, so we don't need to port it there.

And I don't see anything horribly wrong; I propose we apply them if
they pass testing.

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]