Re: [PATCH v3 1/3] watchdog: rzv2h_wdt: Propagate WDTDCR access errors

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Error path unwinding in `rzv2h_wdt_stop()` leaves the watchdog hardware unconfigured, causing an unintended immediate system reboot.
- [High] Aborting the `restart` handler upon register access failure prevents system reboot and hangs the system.
- [Medium] Using asynchronous `pm_runtime_put()` on the probe error path leaves the device permanently powered on.
--

--- Patch [1]: [PATCH v3 1/3] watchdog: rzv2h_wdt: Propagate WDTDCR access errors ---
commit ffabc27a1b8e9074dcd4cdaba7bc802a6d661fcc
Author: Lad Prabhakar <[email protected]>

watchdog: rzv2h_wdt: Propagate WDTDCR access errors

The WDTDCR helpers currently access the register directly using readl()/writel()
and therefore cannot report failures to their callers. Make the helpers return
an error so their callers can propagate failures.

> diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
> index 3b6abb66a1da0..b770226f785ea 100644
> --- a/drivers/watchdog/rzv2h_wdt.c
> +++ b/drivers/watchdog/rzv2h_wdt.c

[ ... ]

> @@ -171,8 +181,13 @@ static int rzv2h_wdt_stop(struct watchdog_device *wdev)
>  	if (ret)
>  		return ret;
>  
> -	if (priv->of_data->wdtdcr)
> -		rzt2h_wdt_wdtdcr_count_stop(priv);
> +	if (priv->of_data->wdtdcr) {
> +		ret = rzt2h_wdt_wdtdcr_count_stop(priv);
> +		if (ret) {
> +			reset_control_deassert(priv->rstc);
> +			return ret;
> +		}
> +	}
>  
>  	pm_runtime_put(wdev->parent);

[Severity: High]
If rzt2h_wdt_wdtdcr_count_stop() fails, this unwinding leaves the hardware
out of reset but unconfigured. 

If the stop operation returns an error, the watchdog core assumes the
hardware is still running and immediately issues a fallback ping. Will this
ping against zeroed, unconfigured registers start a zero-length timeout and
cause an unintended immediate system reboot?

[ ... ]

> @@ -234,8 +250,19 @@ static int rzv2h_wdt_restart(struct watchdog_device *wdev,
>  	rzv2h_wdt_setup(wdev, priv->of_data->cks_min | WDTCR_RPSS_25 |
>  			WDTCR_RPES_75 | WDTCR_TOPS_1024);
>  
> -	if (priv->of_data->wdtdcr)
> -		rzt2h_wdt_wdtdcr_count_start(priv);
> +	if (priv->of_data->wdtdcr) {
> +		ret = rzt2h_wdt_wdtdcr_count_start(priv);
> +		if (ret) {
> +			if (!active) {
> +				reset_control_assert(priv->rstc);
> +				clk_disable(priv->oscclk);
> +				clk_disable(priv->pclk);
> +			} else {
> +				reset_control_reset(priv->rstc);
> +			}
> +			return ret;
> +		}
> +	}
>  
>  	rzv2h_wdt_ping(wdev);

[Severity: High]
Does returning an error here prevent the system from resetting?

The watchdog core's restart notifier wrapper translates an error return into
NOTIFY_BAD. Since NOTIFY_BAD includes the NOTIFY_STOP_MASK bit, this aborts
the restart notifier chain entirely, skipping any fallback handlers and
causing the system to hang indefinitely.

Should this restart handler make a best-effort attempt (perhaps ignoring the
WDTDCR start failure) instead of returning an error?

[ ... ]

> @@ -266,11 +293,11 @@ static int rzt2h_wdt_wdtdcr_init(struct platform_device *pdev,
>  	if (ret)
>  		return ret;
>  
> -	rzt2h_wdt_wdtdcr_count_stop(priv);
> +	ret = rzt2h_wdt_wdtdcr_count_stop(priv);
>  
>  	pm_runtime_put(&pdev->dev);
>  
> -	return 0;
> +	return ret;
>  }

[Severity: Medium]
When rzt2h_wdt_wdtdcr_count_stop() fails, this returns an error that fails
the probe function.

Does using the asynchronous pm_runtime_put() here leave the device powered
on?

Since probe failed, devres immediately calls pm_runtime_disable(), which
cancels any pending asynchronous power-management work. Should this be
pm_runtime_put_sync() instead, to ensure the device powers down before the
disable occurs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1