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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Using a regmap configured with `use_raw_spinlock = true` in the watchdog restart handler introduces a deadlock if a panic occurs while another CPU holds the regmap lock.
- [High] The watchdog restart handler inappropriately aborts on regmap errors, silently preventing system reboot.

Pre-existing issues:
- [High] Calling `clk_enable()` from `rzv2h_wdt_restart()` can cause deadlocks if another CPU holds the corresponding framework spinlocks during a panic.
--

--- Patch [2]: [PATCH v3 2/3] watchdog: rzv2h: Convert WDTDCR handling to regmap ---
commit 81d528488e87b317129eaa95c0076e2e770f3265
Author: Lad Prabhakar <[email protected]>
Subject: watchdog: rzv2h: Convert WDTDCR handling to regmap

This commit converts the WDTDCR register access from raw readl/writel
variants over to the regmap framework using devm_regmap_init_mmio(). This
refactoring allows the driver to subsequently support syscon-based system
controllers natively.

> diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
> index b770226f785ea..cf28caef31e8e 100644
> --- a/drivers/watchdog/rzv2h_wdt.c
> +++ b/drivers/watchdog/rzv2h_wdt.c
[ ... ]
> @@ -91,20 +92,12 @@ static int rzv2h_wdt_ping(struct watchdog_device *wdev)
>  
>  static int rzt2h_wdt_wdtdcr_count_stop(struct rzv2h_wdt_priv *priv)
>  {
> -	u32 reg = readl(priv->wdtdcr + WDTDCR);
> -
> -	writel(reg | WDTDCR_WDTSTOPCTRL, priv->wdtdcr + WDTDCR);
> -
> -	return 0;
> +	return regmap_set_bits(priv->wdtdcr_regmap, WDTDCR, WDTDCR_WDTSTOPCTRL);
>  }
>  
>  static int rzt2h_wdt_wdtdcr_count_start(struct rzv2h_wdt_priv *priv)
>  {
> -	u32 reg = readl(priv->wdtdcr + WDTDCR);
> -
> -	writel(reg & ~WDTDCR_WDTSTOPCTRL, priv->wdtdcr + WDTDCR);
> -
> -	return 0;
> +	return regmap_clear_bits(priv->wdtdcr_regmap, WDTDCR, WDTDCR_WDTSTOPCTRL);
>  }

[Severity: High]
Since rzt2h_wdt_wdtdcr_count_start() is called by rzv2h_wdt_restart() during
a system panic, returning regmap_clear_bits() here means the restart handler
can now fail if the regmap access fails.

If rzt2h_wdt_wdtdcr_count_start() returns an error, the restart handler aborts
early:

    rzv2h_wdt_restart()
        if (priv->of_data->wdtdcr) {
            ret = rzt2h_wdt_wdtdcr_count_start(priv);
            if (ret) {
                ...
                return ret;
            }
        }
        rzv2h_wdt_ping(wdev);

Does this silently prevent the watchdog ping and hang the system indefinitely
during a panic instead of rebooting? Should the restart handler ignore the
error to ensure the watchdog ping always occurs?

[Severity: High]
This is a pre-existing issue, but I noticed the restart handler calls
clk_enable() when the watchdog is not active:

    rzv2h_wdt_restart()
        if (!active) {
            ret = clk_enable(priv->pclk);
            if (ret)
                return ret;
        }

Since rzv2h_wdt_restart() is called in an atomic panic context where other CPUs
are halted via smp_send_stop(), can this cause a deadlock? If another CPU is
halted while holding the clock framework's global enable_lock spinlock, the
panicking CPU will spin indefinitely waiting for it.

[ ... ]
> @@ -280,14 +273,30 @@ 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,
> +	.use_raw_spinlock = true,
> +};

[Severity: High]
Since rzt2h_wdt_wdtdcr_count_start() uses regmap_clear_bits() and is called
by the panic restart handler, can this raw spinlock introduce a deadlock?

If a panic occurs while another CPU is executing rzv2h_wdt_start() or
rzv2h_wdt_stop() and holds this regmap's raw spinlock, the other CPU is
halted by smp_send_stop(). When the panicking CPU executes rzv2h_wdt_restart(),
it will attempt to acquire the exact same raw spinlock and hang forever
instead of rebooting.

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