[PATCH v2 0/9] watchdog: msc313e: Fix issues Sashiko reported

[PATCH v2 0/9] watchdog: msc313e: Fix issues Sashiko reported

[Tzung-Bi Shih]

The series mainly fixes issues reported by Sashiko in [1][2][3].

Patch 1 fixes a report in [1]:
- "[High] Missing platform_set_drvdata() or dev_set_drvdata() in the
  probe function leads to a guaranteed NULL pointer dereference during
  suspend/resume operations."

Patch 2 fixes reports in both [2][3]:
- "[High] Potential division by zero during `max_timeout` calculation."
- "[High] Potential division by zero if clk_get_rate() returns 0."

Patch 3 fixes reports in both [2][3]:
- "Clock reference count leak and unintended hardware activation in
  `msc313e_wdt_settimeout`."
- "[High] Repeatedly setting the watchdog timeout leaks clock prepare
  and enable references, potentially leading to refcount overflow."

Patch 4 fixes a report in [1]:
- "[High] The driver accesses hardware registers without ensuring the
  required clock is enabled, which can lead to a synchronous bus fault."
It also fixes a further report in [3]:
- "[Medium] Clock reference count is leaked on the probe error path if
  watchdog registration fails."

Patch 5 fixes reports in both [2][3]:
- "[High] Hardware watchdog is not properly stopped during system
  suspend if it was started by the bootloader but not opened by
  userspace."
- "[High] System will unexpectedly reset during suspend if the hardware
  watchdog is running but was never opened by userspace."

Patch 6 fixes an undefined behavior while I was reviewing the code.

Patch 7 fixes a report from a local AI tool.  If WDT was running at
boot, the timeout value could be inconsistent with what the driver has.
Sync the value.

Patch 8 fixes a report in [2]:
- "[High] The resume callback ignores the return value of
  msc313e_wdt_start()."

Patch 9 fixes a report in [2]:
- "[Low] Struct initialization uses commas instead of semicolons."

[1] https://lore.kernel.org/all/20260826062035.7645D1F000E9@smtp.kernel.org/
[2] https://lore.kernel.org/all/20260827045746.C79091F000E9@smtp.kernel.org/
[3] https://lore.kernel.org/all/20260827050107.9AD441F000E9@smtp.kernel.org/
---
Patch 1 seems to be applied[4].  But the tree isn't available yet.  The
series still bases on current watchdog-next branch.

[4] https://lore.kernel.org/all/666397f1-ac63-4644-9de1-9f675d2a7f3a@roeck-us.net
---
v2:
- Add some more fixes to the series.

v1: https://lore.kernel.org/r/20260827044700.554333-1-tzungbi@kernel.org

Tzung-Bi Shih (9):
  watchdog: msc313e: Fix NULL pointer dereference in PM callbacks
  watchdog: msc313e: Avoid division by zero
  watchdog: msc313e: Fix clock leak and spurious timer in settimeout()
  watchdog: msc313e: Enable clock before accessing hardware registers
  watchdog: msc313e: Fix spurious reset on suspend
  watchdog: msc313e: Fix undefined behavior
  watchdog: msc313e: Sync timeout value if WDT was running at boot
  watchdog: msc313e: Propagate error code in resume()
  watchdog: msc313e: Replace commas with semicolons in probe()

 drivers/watchdog/msc313e_wdt.c | 90 +++++++++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 18 deletions(-)

-- 
2.53.0

[PATCH v2 1/9] watchdog: msc313e: Fix NULL pointer dereference in PM callbacks

[Tzung-Bi Shih]

msc313e_wdt_probe() doesn't set the driver data for the platform device.
As a result, dev_get_drvdata() in msc313e_wdt_suspend() and
msc313e_wdt_resume() will return NULL, leading to a NULL pointer
dereference afterward.

Set the platform device driver data in msc313e_wdt_probe().

Fixes: e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- No changes.

v1: https://lore.kernel.org/all/20260827044700.554333-2-tzungbi@kernel.org
---
 drivers/watchdog/msc313e_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index d962589e2c55..f69d66971c41 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -124,6 +124,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
 
 	watchdog_set_drvdata(&priv->wdev, priv);
+	platform_set_drvdata(pdev, priv);
 
 	watchdog_init_timeout(&priv->wdev, timeout, dev);
 	watchdog_stop_on_reboot(&priv->wdev);
-- 
2.53.0

[PATCH v2 2/9] watchdog: msc313e: Avoid division by zero

[Tzung-Bi Shih]

clk_get_rate() could return 0.  Avoid a division by zero panic.

Fixes: e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Simply browsed code under drivers/watchdog/, the following drivers also
use clk_get_rate() as a denominator directly:
- drivers/watchdog/digicolor_wdt.c
- drivers/watchdog/rtd119x_wdt.c
- drivers/watchdog/rzv2h_wdt.c

Let me know if you think we should fix them as well.
---
v2:
- New to the series.
---
 drivers/watchdog/msc313e_wdt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index f69d66971c41..c3018b970164 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -97,6 +97,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct msc313e_wdt_priv *priv;
+	unsigned long rate;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -116,7 +117,10 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 	priv->wdev.ops = &msc313e_wdt_ops,
 	priv->wdev.parent = dev;
 	priv->wdev.min_timeout = MSC313E_WDT_MIN_TIMEOUT;
-	priv->wdev.max_timeout = U32_MAX / clk_get_rate(priv->clk);
+	rate = clk_get_rate(priv->clk);
+	if (!rate)
+		return -EINVAL;
+	priv->wdev.max_timeout = U32_MAX / rate;
 	priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
 
 	/* If the period is non-zero the WDT is running */
-- 
2.53.0

[PATCH v2 3/9] watchdog: msc313e: Fix clock leak and spurious timer in settimeout()

[Tzung-Bi Shih]

msc313e_wdt_settimeout() unconditionally calls msc313e_wdt_start() which
introduces two severe bugs:

1. If the watchdog is already active, calling start() again will
   increase the reference count of the clock again.  However stop() is
   only called once, the reference count is unbalance.
2. If the watchdog is stopped, calling settimeout() will start
   the hardware timer accidentally.

Factor out the register-writing logic into a helper function.  Only call
it in settimeout() if the watchdog is running.  Otherwise, simply update
`wdev->timeout`.

Fixes: e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- New to the series.
---
 drivers/watchdog/msc313e_wdt.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index c3018b970164..8ce24df8e338 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -31,20 +31,26 @@ struct msc313e_wdt_priv {
 	struct clk *clk;
 };
 
+static void msc313e_wdt_set_hw_timeout(struct msc313e_wdt_priv *priv,
+				       unsigned int timeout)
+{
+	u32 t = timeout * clk_get_rate(priv->clk);
+
+	writew(t & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
+	writew((t >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
+	writew(1, priv->base + REG_WDT_CLR);
+}
+
 static int msc313e_wdt_start(struct watchdog_device *wdev)
 {
 	struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
-	u32 timeout;
 	int err;
 
 	err = clk_prepare_enable(priv->clk);
 	if (err)
 		return err;
 
-	timeout = wdev->timeout * clk_get_rate(priv->clk);
-	writew(timeout & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
-	writew((timeout >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
-	writew(1, priv->base + REG_WDT_CLR);
+	msc313e_wdt_set_hw_timeout(priv, wdev->timeout);
 	return 0;
 }
 
@@ -69,9 +75,13 @@ static int msc313e_wdt_stop(struct watchdog_device *wdev)
 
 static int msc313e_wdt_settimeout(struct watchdog_device *wdev, unsigned int new_time)
 {
+	struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
 	wdev->timeout = new_time;
 
-	return msc313e_wdt_start(wdev);
+	if (watchdog_hw_running(wdev) || watchdog_active(wdev))
+		msc313e_wdt_set_hw_timeout(priv, wdev->timeout);
+	return 0;
 }
 
 static const struct watchdog_info msc313e_wdt_ident = {
-- 
2.53.0

[PATCH v2 4/9] watchdog: msc313e: Enable clock before accessing hardware registers

[Tzung-Bi Shih]

msc313e_wdt_probe() reads from hardware registers without ensuring the
required clock is enabled.  Furthermore, if the bootloader leaves the
watchdog running, msc313e_wdt_probe() sets WDOG_HW_RUNNING without
increasing the clock's reference count.

While the clock is currently supplied as a fixed clock by the device
tree (`xtal_div2` in arch/arm/boot/dts/sigmastar/mstar-v7.dtsi) which
masks the physical issue, this still violates the API usage.

Call clk_prepare_enable() before reading WDT registers.  If the WDT is
running, leave the clock enabled so the CCF reference counter is
balanced.

Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- Fix a clock leak issue in the error handling path (Sashiko reported).

v1: https://lore.kernel.org/all/20260827044700.554333-3-tzungbi@kernel.org
---
 drivers/watchdog/msc313e_wdt.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index 8ce24df8e338..7c4593566781 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -108,6 +108,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct msc313e_wdt_priv *priv;
 	unsigned long rate;
+	int ret;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -133,9 +134,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 	priv->wdev.max_timeout = U32_MAX / rate;
 	priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
 
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
 	/* If the period is non-zero the WDT is running */
-	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16))
+	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16)) {
 		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
+		/*
+		 * Keep the clock enabled. The watchdog core will skip the next
+		 * start() and a future stop() will balance the CCF reference
+		 * count.
+		 */
+	} else {
+		clk_disable_unprepare(priv->clk);
+	}
 
 	watchdog_set_drvdata(&priv->wdev, priv);
 	platform_set_drvdata(pdev, priv);
@@ -144,7 +157,13 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_reboot(&priv->wdev);
 	watchdog_stop_on_unregister(&priv->wdev);
 
-	return devm_watchdog_register_device(dev, &priv->wdev);
+	ret = devm_watchdog_register_device(dev, &priv->wdev);
+
+	/* If the WDT is running and anything goes wrong, disable the clock. */
+	if (ret && test_bit(WDOG_HW_RUNNING, &priv->wdev.status))
+		clk_disable_unprepare(priv->clk);
+
+	return ret;
 }
 
 static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
-- 
2.53.0

[PATCH v2 5/9] watchdog: msc313e: Fix spurious reset on suspend

[Tzung-Bi Shih]

If the hardware watchdog was started by the bootloader and the device is
suspended before userspace opens it, the ping worker (from watchdog
core) is frozen and the active hardware timer continues running.  This
leads to a spurious system reset.

Check both watchdog_active() and watchdog_hw_running() when deciding
whether to start or stop the watchdog during suspend and resume.

Additionally, call watchdog_stop_ping_on_suspend() to ensure the ping
worker be correctly paused and restarted during suspend and resume.

Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- New to the series.
---
 drivers/watchdog/msc313e_wdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index 7c4593566781..c7d558fefc86 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -156,6 +156,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 	watchdog_init_timeout(&priv->wdev, timeout, dev);
 	watchdog_stop_on_reboot(&priv->wdev);
 	watchdog_stop_on_unregister(&priv->wdev);
+	watchdog_stop_ping_on_suspend(&priv->wdev);
 
 	ret = devm_watchdog_register_device(dev, &priv->wdev);
 
@@ -170,7 +171,7 @@ static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
 {
 	struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
 
-	if (watchdog_active(&priv->wdev))
+	if (watchdog_active(&priv->wdev) || watchdog_hw_running(&priv->wdev))
 		msc313e_wdt_stop(&priv->wdev);
 
 	return 0;
@@ -180,7 +181,7 @@ static int __maybe_unused msc313e_wdt_resume(struct device *dev)
 {
 	struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
 
-	if (watchdog_active(&priv->wdev))
+	if (watchdog_active(&priv->wdev) || watchdog_hw_running(&priv->wdev))
 		msc313e_wdt_start(&priv->wdev);
 
 	return 0;
-- 
2.53.0

[PATCH v2 6/9] watchdog: msc313e: Fix undefined behavior

[Tzung-Bi Shih]

readw() returns a u16.  Left shifting a u16 by 16 bits yields undefined
behavior.

Cast to u32 explicitly before the shift.

Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- New to the series.
---
 drivers/watchdog/msc313e_wdt.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index c7d558fefc86..e28261c7a8d4 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -31,6 +31,16 @@ struct msc313e_wdt_priv {
 	struct clk *clk;
 };
 
+static u32 msc313e_wdt_get_hw_timeout(struct msc313e_wdt_priv *priv)
+{
+	u16 low, high;
+
+	low = readw(priv->base + REG_WDT_MAX_PRD_L);
+	high = readw(priv->base + REG_WDT_MAX_PRD_H);
+
+	return ((u32)high << 16) | low;
+}
+
 static void msc313e_wdt_set_hw_timeout(struct msc313e_wdt_priv *priv,
 				       unsigned int timeout)
 {
@@ -139,7 +149,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 		return ret;
 
 	/* If the period is non-zero the WDT is running */
-	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16)) {
+	if (msc313e_wdt_get_hw_timeout(priv)) {
 		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
 		/*
 		 * Keep the clock enabled. The watchdog core will skip the next
-- 
2.53.0

[PATCH v2 7/9] watchdog: msc313e: Sync timeout value if WDT was running at boot

[Tzung-Bi Shih]

If WDT was running at boot, the hardware timeout might be set to values
other than the final software timeout.

To be consistent, set the hardware timeout to match the final software
timeout (i.e., after watchdog_init_timeout()) if WDT was running.

Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- New to the series.
---
 drivers/watchdog/msc313e_wdt.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index e28261c7a8d4..4a5cce2a16b1 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -144,12 +144,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 	priv->wdev.max_timeout = U32_MAX / rate;
 	priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
 
+	watchdog_set_drvdata(&priv->wdev, priv);
+	platform_set_drvdata(pdev, priv);
+
+	watchdog_init_timeout(&priv->wdev, timeout, dev);
+	watchdog_stop_on_reboot(&priv->wdev);
+	watchdog_stop_on_unregister(&priv->wdev);
+	watchdog_stop_ping_on_suspend(&priv->wdev);
+
 	ret = clk_prepare_enable(priv->clk);
 	if (ret)
 		return ret;
 
 	/* If the period is non-zero the WDT is running */
 	if (msc313e_wdt_get_hw_timeout(priv)) {
+		msc313e_wdt_set_hw_timeout(priv, priv->wdev.timeout);
 		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
 		/*
 		 * Keep the clock enabled. The watchdog core will skip the next
@@ -160,14 +169,6 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 		clk_disable_unprepare(priv->clk);
 	}
 
-	watchdog_set_drvdata(&priv->wdev, priv);
-	platform_set_drvdata(pdev, priv);
-
-	watchdog_init_timeout(&priv->wdev, timeout, dev);
-	watchdog_stop_on_reboot(&priv->wdev);
-	watchdog_stop_on_unregister(&priv->wdev);
-	watchdog_stop_ping_on_suspend(&priv->wdev);
-
 	ret = devm_watchdog_register_device(dev, &priv->wdev);
 
 	/* If the WDT is running and anything goes wrong, disable the clock. */
-- 
2.53.0

[PATCH v2 8/9] watchdog: msc313e: Propagate error code in resume()

[Tzung-Bi Shih]

If msc313e_wdt_start() fails during system resume, the error is
currently ignored.  Consequently, the watchdog isn't running without the
user's knowledge.

Propagate the error code, print a message, and explicitly clear both the
WDOG_HW_RUNNING and WDOG_ACTIVE flags if start fails.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- New to the series.
---
 drivers/watchdog/msc313e_wdt.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index 4a5cce2a16b1..6af865750ad0 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -191,11 +191,19 @@ static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
 static int __maybe_unused msc313e_wdt_resume(struct device *dev)
 {
 	struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
+	int ret = 0;
 
-	if (watchdog_active(&priv->wdev) || watchdog_hw_running(&priv->wdev))
-		msc313e_wdt_start(&priv->wdev);
+	if (watchdog_active(&priv->wdev) || watchdog_hw_running(&priv->wdev)) {
+		ret = msc313e_wdt_start(&priv->wdev);
+		if (ret) {
+			dev_err(dev, "Failed to restart watchdog (err=%d)\n", ret);
 
-	return 0;
+			clear_bit(WDOG_HW_RUNNING, &priv->wdev.status);
+			clear_bit(WDOG_ACTIVE, &priv->wdev.status);
+		}
+	}
+
+	return ret;
 }
 
 static SIMPLE_DEV_PM_OPS(msc313e_wdt_pm_ops, msc313e_wdt_suspend, msc313e_wdt_resume);
-- 
2.53.0

[PATCH v2 9/9] watchdog: msc313e: Replace commas with semicolons in probe()

[Tzung-Bi Shih]

Replace commas at the end of statements with semicolons in probe().

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- New to the series.
---
 drivers/watchdog/msc313e_wdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index 6af865750ad0..e375810bfc2c 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -134,8 +134,8 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->clk);
 	}
 
-	priv->wdev.info = &msc313e_wdt_ident,
-	priv->wdev.ops = &msc313e_wdt_ops,
+	priv->wdev.info = &msc313e_wdt_ident;
+	priv->wdev.ops = &msc313e_wdt_ops;
 	priv->wdev.parent = dev;
 	priv->wdev.min_timeout = MSC313E_WDT_MIN_TIMEOUT;
 	rate = clk_get_rate(priv->clk);
-- 
2.53.0

Re: [PATCH v2 0/9] watchdog: msc313e: Fix issues Sashiko reported

[Daniel Palmer]

Hi Tzung-Bi,

On Sat, 29 Aug 2026 at 01:14, Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> The series mainly fixes issues reported by Sashiko in [1][2][3].

Thanks for looking at these. I will check the patches on hardware over
the weekend.

Cheers,

Daniel

Re: [PATCH v2 7/9] watchdog: msc313e: Sync timeout value if WDT was running at boot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Non-atomic update of 32-bit hardware watchdog timeout registers while the timer is actively running, risking premature system reset.
--

commit 6f0fb889160225480fa3164d876a1cf0757c5b2c
Author: Tzung-Bi Shih <tzungbi@kernel.org>

watchdog: msc313e: Sync timeout value if WDT was running at boot

If WDT was running at boot, the hardware timeout might be set to values
other than the final software timeout. This commit syncs the hardware
timeout to match the software timeout during probe.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 72620adec641a..eea26c6c95d84 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
[ ... ]
> @@ -145,12 +145,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
[ ... ]
>  	/* If the period is non-zero the WDT is running */
>  	if (msc313e_wdt_get_hw_timeout(priv)) {
> +		msc313e_wdt_set_hw_timeout(priv, priv->wdev.timeout);

[Severity: Medium]
Could this trigger a premature system reset during boot?

This new call updates the hardware timeout while the watchdog timer is
actively running. Looking at msc313e_wdt_set_hw_timeout():

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_set_hw_timeout() {
	...
	writew(t & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
	writew((t >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
	writew(1, priv->base + REG_WDT_CLR);
}

The two 16-bit registers are updated sequentially before the counter is
cleared. If the hardware does not shadow these registers, the intermediate
32-bit max period value could fall below the running hardware counter.

This creates a window where the watchdog might immediately reset the system
before the counter is cleared.

>  		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
>  		/*
>  		 * Keep the clock enabled. The watchdog core will skip the next

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828161348.13212-1-tzungbi@kernel.org?part=7

Re: [PATCH v2 4/9] watchdog: msc313e: Enable clock before accessing hardware registers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Probe error path disables the watchdog clock without executing the hardware shutdown sequence, neutralizing the fail-safe.
--

commit 0b66291f7c8412075decd0cc0b1b00209ced44cb
Author: Tzung-Bi Shih <tzungbi@kernel.org>
watchdog: msc313e: Enable clock before accessing hardware registers

This commit fixes an issue where msc313e_wdt_probe() accesses hardware
registers before the required clock is enabled. It adds calls to properly
prepare and enable the clock, and ensures the clock reference count remains
balanced if the watchdog is already running at boot.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 5dc5e3ab7001e..86e72aea96855 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
[ ... ]
> @@ -145,7 +158,13 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	watchdog_stop_on_reboot(&priv->wdev);
>  	watchdog_stop_on_unregister(&priv->wdev);
>  
> -	return devm_watchdog_register_device(dev, &priv->wdev);
> +	ret = devm_watchdog_register_device(dev, &priv->wdev);
> +
> +	/* If the WDT is running and anything goes wrong, disable the clock. */
> +	if (ret && test_bit(WDOG_HW_RUNNING, &priv->wdev.status))
> +		clk_disable_unprepare(priv->clk);

[Severity: High]
If devm_watchdog_register_device() fails here, does disabling the clock
without properly executing the hardware shutdown sequence freeze the active
watchdog counter?

If the bootloader armed the watchdog and the probe sequence subsequently fails
(for example, due to a memory allocation failure), it seems gating the clock
without commanding the IP block to stop would neutralize the fail-safe. Does
this leave the system hanging indefinitely instead of allowing the timer to
expire and trigger a protective reset?

> +
> +	return ret;
>  }
>  
>  static int __maybe_unused msc313e_wdt_suspend(struct device *dev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828161348.13212-1-tzungbi@kernel.org?part=4

Re: [PATCH v2 5/9] watchdog: msc313e: Fix spurious reset on suspend

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling `watchdog_stop_ping_on_suspend()` opts the driver into a buggy watchdog core feature that leaks the PM notifier, causing a Use-After-Free upon device unbind or module unload.
--

commit f0550ce9d86164c3e7cb8c64fecb979c9936c149
Author: Tzung-Bi Shih <tzungbi@kernel.org>

watchdog: msc313e: Fix spurious reset on suspend
    
This commit addresses a spurious system reset on suspend when the watchdog
is started by the bootloader but not yet opened by userspace. It checks
both active and hardware running states, and stops the ping worker during
suspend.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 86e72aea96855..9cfe059fd31a7 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -157,6 +157,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	watchdog_init_timeout(&priv->wdev, timeout, dev);
>  	watchdog_stop_on_reboot(&priv->wdev);
>  	watchdog_stop_on_unregister(&priv->wdev);
> +	watchdog_stop_ping_on_suspend(&priv->wdev);

[Severity: High]
Does adding this call expose the driver to a use-after-free regression
during device unbind or module unload?

Calling watchdog_stop_ping_on_suspend() sets the WDOG_NO_PING_ON_SUSPEND
flag. The watchdog core sees this flag and registers wdd->pm_nb to the
global PM notifier chain.

However, the core unregistration path in __watchdog_unregister_device()
fails to detach the PM notifier:

drivers/watchdog/watchdog_core.c:__watchdog_unregister_device() {
    ...
    if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
        unregister_reboot_notifier(&wdd->reboot_nb);

    watchdog_dev_unregister(wdd);
    ida_free(&watchdog_ida, wdd->id);
    ...
}

When the driver is unbound (e.g. via sysfs) or the module is unloaded, the
driver's memory containing pm_nb is freed, leaving a dangling pointer in the
global PM notifier list. The next system suspend could then trigger a panic.

While the missing unregister logic is a pre-existing flaw in the watchdog
core, this patch newly exposes the msc313e_wdt driver to this issue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828161348.13212-1-tzungbi@kernel.org?part=5

Re: [PATCH v2 8/9] watchdog: msc313e: Propagate error code in resume()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver manually clears core-managed watchdog flags (WDOG_HW_RUNNING and WDOG_ACTIVE) upon resume failure, which violates subsystem boundaries and causes userspace watchdog pings to silently succeed while the hardware watchdog is actually stopped.
--

commit 671369d67bb1efa18f6207e680185bff41e40d0e
Author: Tzung-Bi Shih <tzungbi@kernel.org>

watchdog: msc313e: Propagate error code in resume()

If msc313e_wdt_start() fails during system resume, the error is
currently ignored. Consequently, the watchdog isn't running without the
user's knowledge. This commit propagates the error code, prints a
message, and explicitly clears both the WDOG_HW_RUNNING and WDOG_ACTIVE
flags if start fails.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index eea26c6c95d84..eff90ddeb3634 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -192,11 +192,19 @@ static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
>  static int __maybe_unused msc313e_wdt_resume(struct device *dev)
>  {
>  	struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
> +	int ret = 0;
>  
> -	if (watchdog_active(&priv->wdev) || watchdog_hw_running(&priv->wdev))
> -		msc313e_wdt_start(&priv->wdev);
> +	if (watchdog_active(&priv->wdev) || watchdog_hw_running(&priv->wdev)) {
> +		ret = msc313e_wdt_start(&priv->wdev);
> +		if (ret) {
> +			dev_err(dev, "Failed to restart watchdog (err=%d)\n", ret);
>  
> -	return 0;
> +			clear_bit(WDOG_HW_RUNNING, &priv->wdev.status);
> +			clear_bit(WDOG_ACTIVE, &priv->wdev.status);

[Severity: High]
Does manually clearing these core-managed flags cause userspace watchdog
pings to silently succeed while the hardware watchdog is actually stopped?

If msc313e_wdt_start() fails during system resume in msc313e_wdt_resume(),
the driver manually clears the WDOG_HW_RUNNING and WDOG_ACTIVE flags.

Later, when the userspace daemon writes to /dev/watchdog, the core
watchdog_ping() function could immediately return 0 (success) because
watchdog_hw_running() evaluates to false. This bypasses the driver's .ping
callback and returns success to userspace, hiding the dead state from
monitoring software.

> +		}
> +	}
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828161348.13212-1-tzungbi@kernel.org?part=8

Re: [PATCH v2 1/9] watchdog: msc313e: Fix NULL pointer dereference in PM callbacks

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:40AM +0800, Tzung-Bi Shih wrote:
> msc313e_wdt_probe() doesn't set the driver data for the platform device.
> As a result, dev_get_drvdata() in msc313e_wdt_suspend() and
> msc313e_wdt_resume() will return NULL, leading to a NULL pointer
> dereference afterward.
> 
> Set the platform device driver data in msc313e_wdt_probe().
> 
> Fixes: e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

Thanks,
Guenter

Re: [PATCH v2 2/9] watchdog: msc313e: Avoid division by zero

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:41AM +0800, Tzung-Bi Shih wrote:
> clk_get_rate() could return 0.  Avoid a division by zero panic.
> 
> Fixes: e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

> ---
> Simply browsed code under drivers/watchdog/, the following drivers also
> use clk_get_rate() as a denominator directly:
> - drivers/watchdog/digicolor_wdt.c
> - drivers/watchdog/rtd119x_wdt.c
> - drivers/watchdog/rzv2h_wdt.c
> 
> Let me know if you think we should fix them as well.

Yes, we should.

Thanks,
Guenter

Re: [PATCH v2 3/9] watchdog: msc313e: Fix clock leak and spurious timer in settimeout()

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:42AM +0800, Tzung-Bi Shih wrote:
> msc313e_wdt_settimeout() unconditionally calls msc313e_wdt_start() which
> introduces two severe bugs:
> 
> 1. If the watchdog is already active, calling start() again will
>    increase the reference count of the clock again.  However stop() is
>    only called once, the reference count is unbalance.
> 2. If the watchdog is stopped, calling settimeout() will start
>    the hardware timer accidentally.
> 
> Factor out the register-writing logic into a helper function.  Only call
> it in settimeout() if the watchdog is running.  Otherwise, simply update
> `wdev->timeout`.
> 
> Fixes: e9800b799464 ("watchdog: Add Mstar MSC313e WDT driver")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied. Comment below, though.

Thanks,
Guenter

> ---
> v2:
> - New to the series.
> ---
>  drivers/watchdog/msc313e_wdt.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index c3018b970164..8ce24df8e338 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -31,20 +31,26 @@ struct msc313e_wdt_priv {
>  	struct clk *clk;
>  };
>  
> +static void msc313e_wdt_set_hw_timeout(struct msc313e_wdt_priv *priv,
> +				       unsigned int timeout)
> +{
> +	u32 t = timeout * clk_get_rate(priv->clk);
> +
> +	writew(t & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
> +	writew((t >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
> +	writew(1, priv->base + REG_WDT_CLR);

I wonder if the write to REG_WDT_CLR can come first, to fix the
problem outlined by Sashiko in one of the subsequent patches.


> +}
> +
>  static int msc313e_wdt_start(struct watchdog_device *wdev)
>  {
>  	struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> -	u32 timeout;
>  	int err;
>  
>  	err = clk_prepare_enable(priv->clk);
>  	if (err)
>  		return err;
>  
> -	timeout = wdev->timeout * clk_get_rate(priv->clk);
> -	writew(timeout & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
> -	writew((timeout >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
> -	writew(1, priv->base + REG_WDT_CLR);
> +	msc313e_wdt_set_hw_timeout(priv, wdev->timeout);
>  	return 0;
>  }
>  
> @@ -69,9 +75,13 @@ static int msc313e_wdt_stop(struct watchdog_device *wdev)
>  
>  static int msc313e_wdt_settimeout(struct watchdog_device *wdev, unsigned int new_time)
>  {
> +	struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
>  	wdev->timeout = new_time;
>  
> -	return msc313e_wdt_start(wdev);
> +	if (watchdog_hw_running(wdev) || watchdog_active(wdev))
> +		msc313e_wdt_set_hw_timeout(priv, wdev->timeout);
> +	return 0;
>  }
>  
>  static const struct watchdog_info msc313e_wdt_ident = {

Re: [PATCH v2 4/9] watchdog: msc313e: Enable clock before accessing hardware registers

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:43AM +0800, Tzung-Bi Shih wrote:
> msc313e_wdt_probe() reads from hardware registers without ensuring the
> required clock is enabled.  Furthermore, if the bootloader leaves the
> watchdog running, msc313e_wdt_probe() sets WDOG_HW_RUNNING without
> increasing the clock's reference count.
> 
> While the clock is currently supplied as a fixed clock by the device
> tree (`xtal_div2` in arch/arm/boot/dts/sigmastar/mstar-v7.dtsi) which
> masks the physical issue, this still violates the API usage.
> 
> Call clk_prepare_enable() before reading WDT registers.  If the WDT is
> running, leave the clock enabled so the CCF reference counter is
> balanced.
> 
> Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

Comment below.

> ---
> v2:
> - Fix a clock leak issue in the error handling path (Sashiko reported).
> 
> v1: https://lore.kernel.org/all/20260827044700.554333-3-tzungbi@kernel.org
> ---
>  drivers/watchdog/msc313e_wdt.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 8ce24df8e338..7c4593566781 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -108,6 +108,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct msc313e_wdt_priv *priv;
>  	unsigned long rate;
> +	int ret;
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -133,9 +134,21 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	priv->wdev.max_timeout = U32_MAX / rate;
>  	priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
>  
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
>  	/* If the period is non-zero the WDT is running */
> -	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16))
> +	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16)) {
>  		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
> +		/*
> +		 * Keep the clock enabled. The watchdog core will skip the next
> +		 * start() and a future stop() will balance the CCF reference
> +		 * count.
> +		 */
> +	} else {
> +		clk_disable_unprepare(priv->clk);
> +	}
>  
>  	watchdog_set_drvdata(&priv->wdev, priv);
>  	platform_set_drvdata(pdev, priv);
> @@ -144,7 +157,13 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	watchdog_stop_on_reboot(&priv->wdev);
>  	watchdog_stop_on_unregister(&priv->wdev);
>  
> -	return devm_watchdog_register_device(dev, &priv->wdev);
> +	ret = devm_watchdog_register_device(dev, &priv->wdev);
> +
> +	/* If the WDT is running and anything goes wrong, disable the clock. */
> +	if (ret && test_bit(WDOG_HW_RUNNING, &priv->wdev.status))
> +		clk_disable_unprepare(priv->clk);

Curious. Does this mean that Sashiko complains either way ?

Thanks,
Guenter

> +
> +	return ret;
>  }
>  
>  static int __maybe_unused msc313e_wdt_suspend(struct device *dev)

Re: [PATCH v2 5/9] watchdog: msc313e: Fix spurious reset on suspend

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:44AM +0800, Tzung-Bi Shih wrote:
> If the hardware watchdog was started by the bootloader and the device is
> suspended before userspace opens it, the ping worker (from watchdog
> core) is frozen and the active hardware timer continues running.  This
> leads to a spurious system reset.
> 
> Check both watchdog_active() and watchdog_hw_running() when deciding
> whether to start or stop the watchdog during suspend and resume.
> 
> Additionally, call watchdog_stop_ping_on_suspend() to ensure the ping
> worker be correctly paused and restarted during suspend and resume.
> 
> Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

Sashiko has a point about the bug in the watchdog core, though. We'll have
to fix that at some point.

Thanks,
Guenter

Re: [PATCH v2 6/9] watchdog: msc313e: Fix undefined behavior

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:45AM +0800, Tzung-Bi Shih wrote:
> readw() returns a u16.  Left shifting a u16 by 16 bits yields undefined
> behavior.
> 
> Cast to u32 explicitly before the shift.
> 
> Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

Thanks,
Guenter

Re: [PATCH v2 7/9] watchdog: msc313e: Sync timeout value if WDT was running at boot

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:46AM +0800, Tzung-Bi Shih wrote:
> If WDT was running at boot, the hardware timeout might be set to values
> other than the final software timeout.
> 
> To be consistent, set the hardware timeout to match the final software
> timeout (i.e., after watchdog_init_timeout()) if WDT was running.
> 
> Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

We might need consider the potential problem outlined by Sashiko, though.

Thanks,
Guenter

Re: [PATCH v2 8/9] watchdog: msc313e: Propagate error code in resume()

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:47AM +0800, Tzung-Bi Shih wrote:
> If msc313e_wdt_start() fails during system resume, the error is
> currently ignored.  Consequently, the watchdog isn't running without the
> user's knowledge.
> 
> Propagate the error code, print a message, and explicitly clear both the
> WDOG_HW_RUNNING and WDOG_ACTIVE flags if start fails.
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> v2:
> - New to the series.
> ---
>  drivers/watchdog/msc313e_wdt.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 4a5cce2a16b1..6af865750ad0 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -191,11 +191,19 @@ static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
>  static int __maybe_unused msc313e_wdt_resume(struct device *dev)
>  {
>  	struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
> +	int ret = 0;
>  
> -	if (watchdog_active(&priv->wdev) || watchdog_hw_running(&priv->wdev))
> -		msc313e_wdt_start(&priv->wdev);
> +	if (watchdog_active(&priv->wdev) || watchdog_hw_running(&priv->wdev)) {
> +		ret = msc313e_wdt_start(&priv->wdev);
> +		if (ret) {
> +			dev_err(dev, "Failed to restart watchdog (err=%d)\n", ret);
>  
> -	return 0;
> +			clear_bit(WDOG_HW_RUNNING, &priv->wdev.status);
> +			clear_bit(WDOG_ACTIVE, &priv->wdev.status);

I think Sashiko has a point here. Please leave those bits alone and
just return the error.

Thanks,
Guenter

> +		}
> +	}
> +
> +	return ret;
>  }
>  
>  static SIMPLE_DEV_PM_OPS(msc313e_wdt_pm_ops, msc313e_wdt_suspend, msc313e_wdt_resume);

Re: [PATCH v2 9/9] watchdog: msc313e: Replace commas with semicolons in probe()

[Guenter Roeck]

On Sat, Aug 29, 2026 at 12:13:48AM +0800, Tzung-Bi Shih wrote:
> Replace commas at the end of statements with semicolons in probe().
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

Thanks,
Guenter

Re: [PATCH v2 5/9] watchdog: msc313e: Fix spurious reset on suspend

[Guenter Roeck]

On 9/9/26 14:16, Guenter Roeck wrote:
> On Sat, Aug 29, 2026 at 12:13:44AM +0800, Tzung-Bi Shih wrote:
>> If the hardware watchdog was started by the bootloader and the device is
>> suspended before userspace opens it, the ping worker (from watchdog
>> core) is frozen and the active hardware timer continues running.  This
>> leads to a spurious system reset.
>>
>> Check both watchdog_active() and watchdog_hw_running() when deciding
>> whether to start or stop the watchdog during suspend and resume.
>>
>> Additionally, call watchdog_stop_ping_on_suspend() to ensure the ping
>> worker be correctly paused and restarted during suspend and resume.
>>
>> Fixes: ffd264bd152c ("watchdog: msc313e: Check if the WDT was running at boot")
>> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> 
> Applied.
> 
> Sashiko has a point about the bug in the watchdog core, though. We'll have
> to fix that at some point.
> 
Never mind, that fix is already queued.

Thanks,
Guenter

Re: [PATCH v2 4/9] watchdog: msc313e: Enable clock before accessing hardware registers

[Tzung-Bi Shih]

On Wed, Sep 09, 2026 at 02:14:41PM -0700, Guenter Roeck wrote:
> On Sat, Aug 29, 2026 at 12:13:43AM +0800, Tzung-Bi Shih wrote:
> > v2:
> > - Fix a clock leak issue in the error handling path (Sashiko reported).
> > 
> > v1: https://lore.kernel.org/all/20260827044700.554333-3-tzungbi@kernel.org
...
> > @@ -144,7 +157,13 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
> >  	watchdog_stop_on_reboot(&priv->wdev);
> >  	watchdog_stop_on_unregister(&priv->wdev);
> >  
> > -	return devm_watchdog_register_device(dev, &priv->wdev);
> > +	ret = devm_watchdog_register_device(dev, &priv->wdev);
> > +
> > +	/* If the WDT is running and anything goes wrong, disable the clock. */
> > +	if (ret && test_bit(WDOG_HW_RUNNING, &priv->wdev.status))
> > +		clk_disable_unprepare(priv->clk);
> 
> Curious. Does this mean that Sashiko complains either way ?

Correct.

Sashiko complained about:
- "[Medium] Clock reference count is leaked on the probe error path if
   watchdog registration fails." in v1 [1].
- "[High] Probe error path disables the watchdog clock without executing
   the hardware shutdown sequence, neutralizing the fail-safe." in v2
   (current version) [2].

Current version makes more sense to me.  It balances the CCF reference
count correctly in both paths.

Moreover, if a system relies on the "fail-safe" WDT armed by bootloader,
the WDT shouldn't really depend on the prepared and enabled clock.  Note
that the WDT should be already running before the driver gets probed.

[1] https://lore.kernel.org/all/20260827050107.9AD441F000E9@smtp.kernel.org
[2] https://lore.kernel.org/all/20260828162531.7FA3A1F000E9@smtp.kernel.org