Re: [PATCH v5 4/8] watchdog: rzv2h: Use pm_runtime_put_sync()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix: rzv2h_wdt_stop() still uses asynchronous pm_runtime_put() during driver unbind, risking a race with pm_runtime_disable().
- [High] Kernel panic from division by zero when the optional 'oscclk' is missing from the device tree.
- [High] Missing NULL check for of_device_get_match_data() return value leads to a NULL pointer dereference.
--

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

watchdog: rzv2h: Use pm_runtime_put_sync()

pm_runtime_put() may trigger the idle check after pm_runtime_disable()
is run as part of devm_pm_runtime_enable()'s cleanup action, leaving
runtime PM active.

Use pm_runtime_put_sync() to ensure the idle check runs synchronously.

> diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
> index cbf414e0c9a3c..4a5a856060e91 100644
> --- a/drivers/watchdog/rzv2h_wdt.c
> +++ b/drivers/watchdog/rzv2h_wdt.c
> @@ -270,7 +270,7 @@ static int rzt2h_wdt_wdtdcr_init(struct platform_device *pdev,
>  
>  	ret = rzt2h_wdt_wdtdcr_count_stop(priv);
>  
> -	pm_runtime_put(&pdev->dev);
> +	pm_runtime_put_sync(&pdev->dev);
>  
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but it seems there is another
instance of this same issue in rzv2h_wdt_stop(). During module unbind, the
devres cleanup triggers watchdog_unregister_device(), which calls
rzv2h_wdt_stop(). This function issues an asynchronous pm_runtime_put():

drivers/watchdog/rzv2h_wdt.c:rzv2h_wdt_stop() {
	...
	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);
	...
}

Should this also use pm_runtime_put_sync() to ensure the idle check completes
before the devres cleanup calls pm_runtime_disable()?

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference in rzv2h_wdt_probe(). The return value of
of_device_get_match_data() is not checked:

drivers/watchdog/rzv2h_wdt.c:rzv2h_wdt_probe() {
	...
	priv->of_data = of_device_get_match_data(dev);
	...
	switch (priv->of_data->count_source) {
	...
}

If the driver is bound via sysfs without device tree match data, could
priv->of_data be NULL here and cause a crash?

[Severity: High]
This is a pre-existing issue, but could the clock rate calculation result in a
division by zero?

drivers/watchdog/rzv2h_wdt.c:rzv2h_wdt_probe() {
	...
	priv->oscclk = devm_clk_get_optional_enabled(dev, "oscclk");
	...
	switch (priv->of_data->count_source) {
	case COUNT_SOURCE_LOCO:
		count_clk = priv->oscclk;
		break;
	...
	priv->wdev.max_hw_heartbeat_ms = (MILLI * priv->of_data->timeout_cycles *
					  priv->of_data->cks_div) / clk_get_rate(count_clk);
	...
}

If the optional 'oscclk' is omitted in the device tree, priv->oscclk will be
NULL. When count_clk is assigned this NULL clock, clk_get_rate(count_clk)
safely returns 0. Will this cause a kernel panic due to division by zero?

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