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

[sashiko-bot@kernel.org]

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