[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

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

[Triet Hoang]

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

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

[Tzung-Bi Shih]

On Wed, Sep 16, 2026 at 11:45:57AM +0700, Triet Hoang wrote:
> 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.

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

[1] https://lore.kernel.org/all/20260828162703.9F9DF1F00A3E@smtp.kernel.org

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

Hi Triet,

Thanks for the suggestion.

Fundamentally, if msc313e_wdt_start() fails due to a clk_prepare_enable()
failure, it indicates a much more severe system-level issue.  In such a
scenario, propagating the error to the PM core is likely our best bet rather
than trying to gracefully manage it with additional state tracking.

Also, regarding Sashiko's concern about the bus abort and kernel panic:
the driver enables and disables this clock primarily for CCF reference
counting.  It is actually a "fixed-clock" (see xtal_div2 in mstar-v7.dtsi).
Because it is a fixed clock, there is no physical clock gating that would
block bus access and cause the synchronous external bus abort that the bot
is warning about.

Given that, I think it's better to avoid adding extra state complexity to
the driver just to silence the warning.

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

[Guenter Roeck]

On 9/16/26 00:10, Tzung-Bi Shih wrote:
> On Wed, Sep 16, 2026 at 11:45:57AM +0700, Triet Hoang wrote:
>> 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.
> 
> Another example that Sashiko complains either way.  See also v2 [1].
> 
> [1] https://lore.kernel.org/all/20260828162703.9F9DF1F00A3E@smtp.kernel.org
> 

I'll just take v3. If the watchdog fails to start there isn't much we can do
anyway, so we should not bother too much about it.

Guenter

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

[Guenter Roeck]

On Sun, Sep 13, 2026 at 12:33:34AM +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 and print a message if msc313e_wdt_start()
> fails.
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Applied.

Thanks,
Guenter