Re: [PATCH] watchdog: npcm: Propagate errors from clock enable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] watchdog: npcm: software timeout desync on start error causes unintended reset
- [High] watchdog: npcm: aborting restart handler on clock error hangs system reboot
--

commit cc8288f5db182802c807c8255052dea000706adb
Author: Triet Hoang <triet.hoang.dev@gmail.com>

watchdog: npcm: Propagate errors from clock enable

Check and propagate errors returned by clk_prepare_enable() when starting or
restarting the watchdog. Also propagate errors from npcm_wdt_start() through
npcm_wdt_set_timeout() and probe to prevent the driver from silently
continuing when the watchdog clock cannot be enabled.

> diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
> index 51348969bf492..0976a580b9f21 100644
> --- a/drivers/watchdog/npcm_wdt.c
> +++ b/drivers/watchdog/npcm_wdt.c
> @@ -149,8 +149,11 @@ static int npcm_wdt_start(struct watchdog_device *wdd)
>  {
>  	struct npcm_wdt *wdt = to_npcm_wdt(wdd);
>  	u32 val;
> +	int err;
>  
> -	clk_prepare_enable(wdt->clk);
> +	err = clk_prepare_enable(wdt->clk);
> +	if (err)
> +		return err;
>  
>  	if (wdd->timeout < 2)
>  		val = 0x800;
[ ... ]
> @@ -216,7 +219,7 @@ static int npcm_wdt_set_timeout(struct watchdog_device *wdd,
>  		wdd->timeout = 2750;
>  
>  	if (watchdog_active(wdd))
> -		npcm_wdt_start(wdd);
> +		return npcm_wdt_start(wdd);

[Severity: High]
Does returning an error here cause a state mismatch between software and
hardware?

Earlier in npcm_wdt_set_timeout(), wdd->timeout is modified before
npcm_wdt_start() is called:

	if (timeout < 2)
		wdd->timeout = 1;
	else if (timeout < 3)
		...

If npcm_wdt_start() returns the error from clk_prepare_enable(), the new
timeout value remains in wdd->timeout while the hardware continues running
with the old timeout.

The watchdog core's keepalive worker will recalculate the ping interval
using the new (potentially much longer) wdd->timeout. The hardware
watchdog, running with a shorter timeout, could then expire and
unexpectedly reset the system.

>  
>  	return 0;
>  }
> @@ -234,9 +237,12 @@ static int npcm_wdt_restart(struct watchdog_device *wdd,
>  			    unsigned long action, void *data)
>  {
>  	struct npcm_wdt *wdt = to_npcm_wdt(wdd);
> +	int err;
>  
>  	/* For reset, we start the WDT clock and leave it running. */
> -	clk_prepare_enable(wdt->clk);
> +	err = clk_prepare_enable(wdt->clk);
> +	if (err)
> +		return err;

[Severity: High]
Could returning an error here hang the system during reboot?

npcm_wdt_restart() is invoked via watchdog_restart_notifier() in the
watchdog core to handle system reboots. The watchdog core maps any non-zero
return from a restart handler to a notification value that aborts the
notifier chain.

This causes atomic_notifier_call_chain() to immediately break out of the
restart_handler_list loop in kernel/reboot.c.

As a result, not only does the NPCM watchdog skip its attempt to write to
the reset registers (which might succeed if the clock was already enabled),
but all lower-priority fallback restart handlers are preempted. The system
will hang indefinitely instead of rebooting.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916092753.187583-1-triet.hoang.dev@gmail.com?part=1