Re: [PATCH 5/5] watchdog: starfive-wdt: Drop runtime PM reference before registering watchdog

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Wed, Nov 04, 2026 at 08:44:45AM -0800, Guenter Roeck wrote:
> In starfive_wdt_probe(), watchdog_register_device() is called before the
> probe-time runtime PM reference is released via pm_runtime_put_sync(), and
> any non-zero return value from pm_runtime_put_sync() is treated as a fatal
> probe error.
> 
> If userspace (such as udev or a watchdog daemon) opens /dev/watchdog
> immediately after watchdog_register_device() exposes the character device,
> starfive_wdt_pm_start() increments the runtime PM usage count. The
> subsequent pm_runtime_put_sync() in probe then decrements the usage count
> to 1 and returns -EAGAIN (it may also return 1, -EBUSY, or -EPERM in
> non-error idle conditions), causing starfive_wdt_probe() to unregister the
> working watchdog device and fail probe.
> 
> When early_enable is not set, no hardware registers are accessed after
> starfive_wdt_stop(). Release the probe-time runtime PM reference right
> after starfive_wdt_stop() and before watchdog_register_device(), without
> treating pm_runtime_put_sync() idle return codes as probe errors.
> 
> Fixes: 8bc22a2f1bf0 ("watchdog: starfive: Check pm_runtime_enabled() before decrementing usage counter")
> Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> Cc: Ziv Xu <ziv.xu@starfivetech.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

The patch looks correct to me.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

> @@ -499,28 +499,25 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>  		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>  	} else {
>  		starfive_wdt_stop(wdt);
> +		if (pm_runtime_enabled(&pdev->dev))
> +			pm_runtime_put_sync(&pdev->dev);
>  	}
>  
>  	ret = watchdog_register_device(&wdt->wdd);
> -	if (ret)
> -		goto err_exit;
> -
> -	if (!early_enable) {
> -		if (pm_runtime_enabled(&pdev->dev)) {
> -			ret = pm_runtime_put_sync(&pdev->dev);
> -			if (ret)
> -				goto err_unregister_wdt;
> +	if (ret) {
> +		if (watchdog_hw_running(&wdt->wdd)) {
> +			starfive_wdt_stop(wdt);
> +			if (pm_runtime_enabled(&pdev->dev))
> +				pm_runtime_put_sync(&pdev->dev);
>  		}
> +		if (!pm_runtime_enabled(&pdev->dev))
> +			starfive_wdt_disable_clock(wdt);
> +		goto err_disable_pm;
>  	}
>  
>  	return 0;
>  
> -err_unregister_wdt:
> -	watchdog_unregister_device(&wdt->wdd);
> -	goto err_disable_pm;
>  err_exit:
> -	if (watchdog_hw_running(&wdt->wdd))
> -		starfive_wdt_stop(wdt);
>  	if (pm_runtime_enabled(&pdev->dev))
>  		pm_runtime_put_sync(&pdev->dev);
>  	else

Would this improve readability?

	ret = watchdog_register_device(&wdt->wdd);
	if (ret) {
		if (early_enable) {
			starfive_wdt_stop(wdt);
			goto err_exit;
		} else {
			if (!pm_runtime_enabled(&pdev->dev))
				starfive_wdt_disable_clock(wdt);
		}
		goto err_disable_pm;
	}