Re: [PATCH] watchdog: sp805_wdt: fix suspend/resume handling of HW_RUNNING watchdog

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] wdt_restart() fails to reboot the system if the watchdog counter clock (wdt->clk) was previously disabled.
--

commit 5341b327de5a15d0411852abc081601a0f32ac49
Author: Li Jun <lijun01@kylinos.cn>

watchdog: sp805_wdt: fix suspend/resume handling of HW_RUNNING watchdog

sp805_wdt_suspend() and sp805_wdt_resume() only check watchdog_active(),
when the watchdog is left running by the driver sets WDOG_HW_RUNNING.
This ensures wdt_disable() is called in suspend.

> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index c2125f204a137..2ddb754f3b4a1 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -325,7 +325,8 @@ static int __maybe_unused sp805_wdt_suspend(struct device *dev)
>  {
>  	struct sp805_wdt *wdt = dev_get_drvdata(dev);
>  
> -	if (watchdog_active(&wdt->wdd))
> +	if (watchdog_active(&wdt->wdd) ||
> +	    watchdog_hw_running(&wdt->wdd))
>  		return wdt_disable(&wdt->wdd);
>  
>  	return 0;
>  }

[ ... ]

> @@ -335,7 +336,8 @@ static int __maybe_unused sp805_wdt_resume(struct device *dev)
>  {
>  	struct sp805_wdt *wdt = dev_get_drvdata(dev);
>  
> -	if (watchdog_active(&wdt->wdd))
> +	if (watchdog_active(&wdt->wdd) ||
> +	    watchdog_hw_running(&wdt->wdd))
>  		return wdt_enable(&wdt->wdd);
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does wdt_restart() fail to
reboot the system if the watchdog counter clock was previously disabled?

If userspace cleanly stops the watchdog, the core calls wdt_disable() which
disables wdt->clk. If a reboot command is subsequently issued, wdt_restart()
writes to WDTCONTROL to assert RESET_ENABLE:

drivers/watchdog/sp805_wdt.c:wdt_restart() {
        ...
        writel_relaxed(UNLOCK, wdt->base + WDTLOCK);
        writel_relaxed(0, wdt->base + WDTCONTROL);
        writel_relaxed(0, wdt->base + WDTLOAD);
        writel_relaxed(INT_ENABLE | RESET_ENABLE, wdt->base + WDTCONTROL);
        ...
}

Because the counter clock is disabled, the hardware timer will not count down,
and the hardware reset signal is never generated. Since wdt_restart() returns
0 without re-enabling the clock, does the reboot silently fail and hang the
system?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911030531.837140-1-lijun01@kylinos.cn?part=1