[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

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

[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

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

[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

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 = {

[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

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 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

[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

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 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

[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

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

[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

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

[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

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);

[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 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 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