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

[Triet Hoang <triet.hoang.dev@gmail.com>]

Sat, 12 Sep 2026 16:42:21 +0000 Sashiko wrote:

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

Hi Tzung-Bi,

I think we can address the problem Sashiko mentioned by making the clock ownership 
explicit in the driver.

If msc313e_wdt_start() fails to enable the clock, the driver should not subsequently access
the watchdog registers or call clk_disable_unprepare() as if the clock had been successfully enabled.
In particular, we can track whether the clock was successfully acquired and make ping() and stop()
operate only when the clock is enabled.

diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
index 4a5cce2a16b1..437b8ace0a00 100644
--- a/drivers/watchdog/msc313e_wdt.c
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -29,6 +29,7 @@ struct msc313e_wdt_priv {
        void __iomem *base;
        struct watchdog_device wdev;
        struct clk *clk;
+       bool clk_enabled;
 };
 
 static u32 msc313e_wdt_get_hw_timeout(struct msc313e_wdt_priv *priv)
@@ -60,6 +61,7 @@ static int msc313e_wdt_start(struct watchdog_device *wdev)
        if (err)
                return err;
 
+       priv->clk_enabled = true;
        msc313e_wdt_set_hw_timeout(priv, wdev->timeout);
        return 0;
 }
@@ -68,6 +70,9 @@ static int msc313e_wdt_ping(struct watchdog_device *wdev)
 {
        struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
 
+       if (!priv->clk_enabled)
+               return -EIO;
+
        writew(1, priv->base + REG_WDT_CLR);
        return 0;
 }
@@ -79,7 +84,10 @@ static int msc313e_wdt_stop(struct watchdog_device *wdev)
        writew(0, priv->base + REG_WDT_MAX_PRD_L);
        writew(0, priv->base + REG_WDT_MAX_PRD_H);
        writew(0, priv->base + REG_WDT_CLR);
-       clk_disable_unprepare(priv->clk);
+       if (priv->clk_enabled) {
+               clk_disable_unprepare(priv->clk);
+               priv->clk_enabled = false;
+       }
        return 0;
 }

How do you think about this idea?

BR,
Triet