[PATCH v3] watchdog: msc313e: Propagate error code in resume()

[PATCH v3] 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 and print a message if msc313e_wdt_start()
fails.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v3:
- Don't clear WDOG_HW_RUNNING and WDOG_ACTIVE flags if
  msc313e_wdt_start() fails in msc313e_wdt_resume() which hides the dead
  state (Sashiko reported).

v2: https://lore.kernel.org/all/20260828161348.13212-9-tzungbi@kernel.org
- New to the series.
---
 drivers/watchdog/msc313e_wdt.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index f5a5afd91b98..dc7bdd5660e6 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -191,11 +191,15 @@ static int msc313e_wdt_suspend(struct device *dev)
 static int 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;
+	return ret;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(msc313e_wdt_pm_ops, msc313e_wdt_suspend, msc313e_wdt_resume);
-- 
2.53.0

Re: [PATCH v3] watchdog: msc313e: Propagate error code in resume()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] If `msc313e_wdt_start()` fails during system resume, the device's clock remains disabled, but the watchdog framework state flags remain unchanged. Subsequent watchdog core operations (e.g., ping, stop, set_timeout) will attempt to access memory-mapped registers without the clock enabled, leading to an external bus abort (kernel panic). Additionally, a subsequent `stop` operation will cause a clock reference count underflow.
--

commit be9325a33db039f8043de74057aabb9bbaef3593
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. Propagate the error code and print a message if
msc313e_wdt_start() fails.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index f5a5afd91b987..dc7bdd5660e65 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -191,11 +191,15 @@ static int msc313e_wdt_suspend(struct device *dev)
>  static int 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);

[Severity: High]
This is a pre-existing issue, but does leaving the watchdog framework state
flags intact when the clock fails to enable cause a system crash?

If msc313e_wdt_start() fails, the clock remains disabled, but WDOG_ACTIVE
and WDOG_HW_RUNNING are still set. The Linux PM core ignores device resume 
errors and proceeds with system resume.

The watchdog core's PM notifier then calls watchdog_dev_resume(), which
immediately invokes msc313e_wdt_ping(). This unconditionally writes to a 
memory-mapped register:

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_ping() {
	...
	writew(1, priv->base + REG_WDT_CLR);
	return 0;
}

Can accessing these registers with the bus clock disabled on this platform 
trigger a synchronous external bus abort and a kernel panic?

Additionally, a subsequent stop operation unconditionally disables the clock:

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_stop() {
	...
	writew(0, priv->base + REG_WDT_CLR);
	clk_disable_unprepare(priv->clk);
	return 0;
}

Since the clock enable previously failed, would this extra 
clk_disable_unprepare() cause a clock reference count underflow warning?

> +	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(msc313e_wdt_pm_ops, msc313e_wdt_suspend, msc313e_wdt_resume);

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