watchdog: msc313e: Propagate error code in resume()

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

Quoted 7 lines · 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.

Quoted 23 lines

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

Quoted 2 lines

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.

drivers/watchdog/msc313e_wdt.c +9 -1

index 4a5cce2a16b1..437b8ace0a00 100644
@@ -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?

Quoted 6 lines · Triet Hoang

Quoted 4 lines · 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?

Quoted 3 lines

Quoted 3 lines

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.

Another example that Sashiko complains either way. See also v2 [1].

[1] https://lore.kernel.org/all/[email protected]

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 <[email protected]>