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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The conversion to the regmap API introduces a potential deadlock and sleeping in atomic context issues in the watchdog restart handler.
- [High] Deasserting the reset line in the error path of `rzv2h_wdt_stop()` leaves the watchdog running unconfigured, potentially causing spurious reboots.
- [Medium] The restart handler incorrectly aborts if the regmap operation fails, skipping the actual hardware reset trigger.
--

--- Patch [1]: [PATCH v2 1/2] watchdog: rzv2h: Convert WDTDCR handling to regmap ---
commit e599411a91a07a1899caa9c4c300e8ec3242933e
Author: Lad Prabhakar <[email protected]>

watchdog: rzv2h: Convert WDTDCR handling to regmap

This commit converts the WDTDCR register access to use the regmap framework.
It serves as a preparatory step to subsequently support syscon-based system
controllers natively without runtime architectural branching.

> diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
> --- a/drivers/watchdog/rzv2h_wdt.c
> +++ b/drivers/watchdog/rzv2h_wdt.c
[ ... ]
> @@ -171,8 +174,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]
Will deasserting the reset line in this error path leave the watchdog running
unconfigured?

Since reset_control_assert(priv->rstc) was called earlier in rzv2h_wdt_stop(),
the hardware's configuration registers, including timeout and clock division,
have been cleared.

If rzt2h_wdt_wdtdcr_count_stop() fails and this error path calls
reset_control_deassert(), it brings the hardware out of reset with default
values. The watchdog core assumes the device is still active due to the
returned error and continues to ping it, which could start it counting with
default, short timeouts and trigger a spurious system reboot.

> +		}
> +	}
>  
>  	pm_runtime_put(wdev->parent);
[ ... ]
> @@ -234,8 +243,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: Medium]
Since the restart handler executes in a best-effort emergency context, is it
safe to abort early here if a regmap operation fails?

If rzt2h_wdt_wdtdcr_count_start() returns an error, the function returns
immediately and skips the call to rzv2h_wdt_ping() below, which is the raw
register write that triggers the actual hardware reset. Skipping this might
cause the system to hang rather than successfully rebooting during a panic.

> +		}
> +	}
>  
>  	rzv2h_wdt_ping(wdev);
>  
[ ... ]
> @@ -253,24 +271,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]
Could this regmap configuration lead to deadlocks or sleeping in atomic
context when accessed from the restart handler?

The rzv2h_wdt_restart() function runs in an atomic panic context with
interrupts disabled. However, this regmap config omits .disable_locking = true
and .use_raw_spinlock = true.

By default, this creates a standard spinlock. On PREEMPT_RT kernels, standard
spinlocks are sleepable rt_mutexes, which will cause a 'sleeping in atomic
context' BUG. On other kernels, if the restart handler interrupts another CPU
that is currently holding the regmap lock, attempting to acquire it in
regmap_clear_bits() could result in a hard deadlock.

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