Re: [PATCH v2 3/6] watchdog: starfive: balance PM refcount and disable in probe error paths

[sashiko-bot@kernel.org]

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

New issues:
- [High] Treating pm_runtime_put_sync() return value as a fatal error causes spurious probe failures and a double PM refcount drop.

Pre-existing issues:
- [High] Missing runtime PM synchronization in remove() leads to unclocked hardware access and potential kernel panic.
--

commit b65fb45d5687c6419949ad39d8553aec8eec2e77
Author: William Theesfeld <william@theesfeld.net>

watchdog: starfive: balance PM refcount and disable in probe error paths

The commit restructures the error handling in the probe function of the
starfive watchdog driver. It adds specific labels to ensure runtime PM
refcounts and clocks are properly balanced and disabled during failures.

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index c821b38d26d22..6539bf0893583 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
[ ... ]
> @@ -506,7 +506,7 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>  
>  	ret = watchdog_register_device(&wdt->wdd);
>  	if (ret)
> -		goto err_exit;
> +		goto err_put_pm;
>  
>  	if (!early_enable) {
>  		if (pm_runtime_enabled(&pdev->dev)) {
> @@ -520,8 +520,12 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>  
>  err_unregister_wdt:
>  	watchdog_unregister_device(&wdt->wdd);
> -err_exit:
> -	starfive_wdt_disable_clock(wdt);
> +err_put_pm:
> +	if (pm_runtime_enabled(&pdev->dev))
> +		pm_runtime_put_sync(&pdev->dev);

[Severity: High]
Is there a risk of a double put and refcount underflow here?

If pm_runtime_put_sync() returns a negative value like -EAGAIN 
because another thread incremented the usage count before the idle check, 
it successfully decremented the refcount.

If we jump to err_unregister_wdt, the code will fall through to err_put_pm
and call pm_runtime_put_sync() a second time.

Does this cause an unbalanced put and spurious probe failures?

[Severity: High]
This isn't a bug introduced by this patch, but looking at the PM handling
in starfive_wdt_remove():

drivers/watchdog/starfive-wdt.c:starfive_wdt_remove() {
    ...
    starfive_wdt_stop(wdt);
    watchdog_unregister_device(&wdt->wdd);
    ...
}

Is there missing runtime PM synchronization here?

starfive_wdt_remove() unconditionally calls starfive_wdt_stop(), which
accesses hardware registers. If the watchdog is not open, the device might
be runtime-suspended with its APB and core clocks gated.

Could accessing unclocked hardware registers here trigger a synchronous
external abort and a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780664848.git.william@theesfeld.net?part=3