Re: [RFC PATCH 7/9] watchdog: rzv2h: Add syscon support for RZ/T2H and RZ/N2H WDT control register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Calling syscon `regmap_update_bits()` in the watchdog restart handler causes sleep-in-atomic BUGs and deadlocks.
- [High] Propagating non-fatal syscon errors aborts the system restart sequence.
- [High] Flawed error recovery in `rzv2h_wdt_stop()` leaves hardware active but unconfigured.
- [High] Asynchronous `pm_runtime_put()` on probe error paths leads to permanent power state leaks.
--

commit 350f84b9c5fd68e22e1aff72f286a475ec335167
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

watchdog: rzv2h: Add syscon support for RZ/T2H and RZ/N2H WDT control register

This commit adds syscon support for accessing the WDTDCR register on
RZ/T2H and RZ/N2H SoCs. It updates the WDT driver to support both a
dedicated syscon regmap interface and direct mapping for backward
compatibility.

> diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
> index e9545b8f5fd09..e14ff853a4e9f 100644
> --- a/drivers/watchdog/rzv2h_wdt.c
> +++ b/drivers/watchdog/rzv2h_wdt.c

[ ... ]

> @@ -179,8 +208,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;
> +		}
> +	}

Does error recovery here leave the hardware active but unconfigured?

Since reset_control_assert() was called earlier in this function, the
configuration registers are cleared. If we deassert the reset and return
an error, the watchdog core assumes the stop operation failed and keeps the
WDOG_HW_RUNNING flag active, causing its background worker to continue
calling rzv2h_wdt_ping().

Could pinging the unconfigured hardware start the watchdog with arbitrary
default values and trigger an immediate, unintended system reset?

[ ... ]

> @@ -242,8 +277,17 @@ 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);
> +			}
> +			return ret;
> +		}
> +	}

Does using regmap_update_bits() via rzt2h_wdt_wdtdcr_count_start() in this
restart handler cause sleep-in-atomic or deadlock issues?

Restart handlers execute in atomic context with local interrupts disabled
during the machine_restart() sequence. Syscon regmaps use spinlock_t, which
sleeps on PREEMPT_RT kernels.

Additionally, since the syscon is shared with other peripherals, if another
CPU was interrupted and stopped while holding the regmap spinlock, could
this spin indefinitely and hang the system?

Is it safe to return early on syscon errors here?

rzv2h_wdt_restart() is a system reboot handler. By returning early, it skips
rzv2h_wdt_ping(), which is the operation that actually triggers the hardware
reset. Aborting the restart sequence because a shared control register
failed to update guarantees the system will hang instead of rebooting.

Should the restart handler ignore non-fatal setup errors and make a
best-effort attempt to trigger the reset?

[ ... ]

> @@ -264,21 +308,37 @@ static const struct watchdog_ops rzv2h_wdt_ops = {
>  static int rzt2h_wdt_wdtdcr_init(struct platform_device *pdev,
>  				 struct rzv2h_wdt_priv *priv)
>  {
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret;

[ ... ]

>  	ret = pm_runtime_resume_and_get(&pdev->dev);
>  	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;
>  }

Could this asynchronous pm_runtime_put() cause a permanent power state leak
on error paths?

If rzt2h_wdt_wdtdcr_count_stop() fails, the function returns an error. This
leads to a probe failure, and the devres teardown immediately calls
pm_runtime_disable().

Since pm_runtime_disable() executes a barrier that cancels any pending
asynchronous suspend requests, the device power domain could be left
permanently powered on in the RPM_ACTIVE state.

Should this use pm_runtime_put_sync() instead when returning an error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511185058.1926869-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=7