Re: [PATCH 2/3] watchdog: rzv2h: Convert WDTDCR handling to regmap

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The regmap framework allocates a standard spinlock by default, which causes deadlocks or 'scheduling while atomic' bugs when called from the atomic watchdog restart handler.
- [High] Improper error rollback in `rzv2h_wdt_stop()` leaves the watchdog unconfigured but out of reset, which can lead to spurious reboots.
- [High] The watchdog restart handler aborts the system reboot if the regmap update fails.
- [Medium] Asynchronous `pm_runtime_put()` is followed by synchronous `pm_runtime_disable()` in the initialization error path, which cancels the power-down and leaves the hardware powered on.
--

commit 3a9f06deb3b8a110dd1799630b20292ab0c85c6e
Author: Lad Prabhakar <[email protected]>
Date:   Thu Jul 2 16:04:56 2026 +0000

    watchdog: rzv2h: Convert WDTDCR handling to regmap

    This commit converts the WDTDCR register access from raw readl/writel
    variants over to the regmap framework. This acts as a preparatory step
    to allow the driver to natively support syscon-based system controllers.

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

[ ... ]

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

[Severity: High]
Does this error path leave the watchdog out of reset without restoring its
required configuration?

If rzt2h_wdt_wdtdcr_count_stop() fails, the rollback deasserts the reset, but
the hardware reset wiped all watchdog configurations like the timeout value
in WDTCR.

Since the core sees the stop failed, won't it assume the watchdog is still
running and resume pinging it?

A subsequent ping would start the down-counter with uninitialized default
registers, which could cause a near-immediate spurious system reboot.

> +		}
> +	}
>  
>  	pm_runtime_put(wdev->parent);
>  

[ ... ]

> @@ -242,8 +249,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;

[Severity: High]
Should a failure in an auxiliary register write abort an emergency system
reboot?

The restart handler is used to forcefully reboot the system in situations
like a kernel panic. Returning early here skips the rzv2h_wdt_ping() step,
which is what actually triggers the hardware to reset the system.

> +		}
> +	}
>  
>  	rzv2h_wdt_ping(wdev);
>  
> @@ -261,24 +277,39 @@ static const struct watchdog_ops rzv2h_wdt_ops = {
>  	.restart = rzv2h_wdt_restart,
>  };
>  
> +static const struct regmap_config rzv2h_wdtdcr_regmap_config = {
> +	.name = "wdtdcr",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = WDTDCR,
> +	.fast_io = true,
> +};

[Severity: High]
Will this regmap configuration cause deadlocks or scheduling while atomic bugs
when called from the atomic watchdog restart handler?

The regmap framework allocates a standard spinlock by default because the
configuration omits use_raw_spinlock = true or disable_locking = true.

Since rzv2h_wdt_restart() runs in atomic context (and potentially NMI) during
system panic or reboot, attempting to acquire a sleeping spinlock on
PREEMPT_RT could cause a crash. On all systems, if the lock was held by
another thread during a panic, the restart handler could deadlock and prevent
the system from rebooting.

> +
>  static int rzt2h_wdt_wdtdcr_init(struct platform_device *pdev,
>  				 struct rzv2h_wdt_priv *priv)
>  {
> +	void __iomem *wdtdcr;
>  	int ret;
>  
> -	priv->wdtdcr = devm_platform_ioremap_resource(pdev, 1);
> -	if (IS_ERR(priv->wdtdcr))
> -		return PTR_ERR(priv->wdtdcr);
> +	wdtdcr = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(wdtdcr))
> +		return PTR_ERR(wdtdcr);
> +
> +	priv->wdtdcr_regmap = devm_regmap_init_mmio(&pdev->dev, wdtdcr,
> +						    &rzv2h_wdtdcr_regmap_config);
> +	if (IS_ERR(priv->wdtdcr_regmap))
> +		return PTR_ERR(priv->wdtdcr_regmap);
>  
>  	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;

[Severity: Medium]
Is it safe to use an asynchronous pm_runtime_put() here right before
returning a potential error code?

If rzt2h_wdt_wdtdcr_count_stop() fails, the error propagates back to
rzv2h_wdt_probe(), triggering devm cleanup which immediately calls
pm_runtime_disable().

As noted in the power management subsystem guidelines, calling
pm_runtime_disable() immediately after an async put cancels the pending
async operations. This would permanently leave the device hardware in an
active power state.

Should pm_runtime_put_sync() be used instead?

>  }
>  
>  static int rzv2h_wdt_probe(struct platform_device *pdev)

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