Re: [PATCH] watchdog: starfive-wdt: fix PM reference leak in probe error path

[sashiko-bot@kernel.org]

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

New issues:
- [High] Double clock disable in the probe error path leads to clock underflow.
- [High] Double pm_runtime_put_sync() and usage counter underflow when falling through from err_unregister_wdt.

Pre-existing issues:
- [High] Missing `pm_runtime_disable()` when `probe()` fails during clock enablement or PM resume.
- [High] `starfive_wdt_set_timeout()` unconditionally enables the watchdog hardware, which can unexpectedly start a stopped watchdog.
- [High] Unbalanced PM usage count decrement in shutdown callback.
- [High] Hardware access while runtime suspended in remove() and shutdown().
- [Medium] The driver incorrectly treats a return value of 1 from pm_runtime_put_sync() as an error.
- [Medium] The driver permanently leaks the PM usage count across driver unbinds if early_enable is true.
--

commit bf7d8a19365abbc34b40d8597c85a439ec80777c
Author: Ivy Lopez <skunkolee@gmail.com>

watchdog: starfive-wdt: fix PM reference leak in probe error path

pm_runtime_resume_and_get() increments the runtime PM usage counter
on success. If a later step in probe() fails (reset_init, clock rate
check, watchdog_start, or watchdog_register_device), control reaches
err_exit, which calls pm_runtime_disable() but never puts the
reference, leaking the PM usage count.

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a3c69..f87f8d5a3dabb 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -510,6 +510,8 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>  	watchdog_unregister_device(&wdt->wdd);
>  err_exit:
>  	starfive_wdt_disable_clock(wdt);
> +	if (pm_runtime_enabled(&pdev->dev))
> +		pm_runtime_put_sync(&pdev->dev);

[Severity: High]
The probe function correctly calls pm_runtime_resume_and_get(&pdev->dev),
which implicitly enables the clocks via starfive_wdt_runtime_resume().

On failure, the code jumps to err_exit and explicitly calls
starfive_wdt_disable_clock(wdt).

By adding pm_runtime_put_sync(&pdev->dev) here, it synchronously invokes
starfive_wdt_runtime_suspend(), which also calls
starfive_wdt_disable_clock(wdt) a second time.

Does this code double-disable the clocks and cause an underflow in the Common
Clock Framework?

[Severity: High]
At the end of starfive_wdt_probe(), if !early_enable, the driver invokes
pm_runtime_put_sync(&pdev->dev) to drop the PM reference.

Because pm_runtime_put_sync() unconditionally decrements the usage counter
regardless of its return value, any failure still results in a decremented
count.

If that first call returns an error, the code checks if (ret) and executes
goto err_unregister_wdt;, which falls through to err_exit:

drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
    ...
	if (!early_enable) {
		if (pm_runtime_enabled(&pdev->dev)) {
			ret = pm_runtime_put_sync(&pdev->dev);
			if (ret)
				goto err_unregister_wdt;
		}
	}

	return 0;

err_unregister_wdt:
	watchdog_unregister_device(&wdt->wdd);
err_exit:
    ...
}

Could falling through to err_exit drop the usage counter below zero and severely
corrupt the runtime PM state, because the patch adds a second
pm_runtime_put_sync() call in err_exit?

>  	pm_runtime_disable(&pdev->dev);
>  
>  	return ret;

[Severity: High]
This is a pre-existing issue, but if pm_runtime_resume_and_get() or
starfive_wdt_enable_clock() fail early in starfive_wdt_probe(), the function
immediately executes return ret;, bypassing the err_exit cleanup block:

drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
    ...
	pm_runtime_enable(&pdev->dev);
	if (pm_runtime_enabled(&pdev->dev)) {
		ret = pm_runtime_resume_and_get(&pdev->dev);
		if (ret < 0)
			return ret;
    ...
}

Because pm_runtime_disable(&pdev->dev) is skipped on these paths, the disable
depth remains unbalanced.

Will this leak the PM enablement state and cause an
"Unbalanced pm_runtime_enable!" warning upon a subsequent re-probe?

[Severity: High]
This is a pre-existing issue, but starfive_wdt_set_timeout() unconditionally
calls starfive_wdt_enable() after updating the reload count:

drivers/watchdog/starfive-wdt.c:starfive_wdt_set_timeout() {
    ...
	starfive_wdt_disable(wdt);
	starfive_wdt_set_reload_count(wdt, wdt->count);
	starfive_wdt_enable(wdt);
    ...
}

If a user explicitly stops the watchdog (using WDIOC_SETOPTIONS with
WDIOS_DISABLECARD) and then sets a new timeout, this will silently turn the
hardware back on.

Can this break the watchdog core's state tracking and result in an
unexpected system reboot when the hardware watchdog times out?

[Severity: Medium]
This isn't a bug introduced by this patch, but according to the Power
Management subsystem guidelines, pm_runtime_put_sync() returns 1 when the
device is already suspended.

This is a success condition, not an error.

drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
    ...
			ret = pm_runtime_put_sync(&pdev->dev);
			if (ret)
				goto err_unregister_wdt;
    ...
}

Does treating any non-zero value as a failure violate the API contract and
potentially trigger the error rollback path incorrectly?

[Severity: Medium]
This is a pre-existing issue, but if early_enable is true,
starfive_wdt_probe() intentionally skips pm_runtime_put_sync(), leaving
the device's PM usage_count incremented by 1.

When starfive_wdt_remove() unregisters the device and disables PM, it fails
to drop this initial reference:

drivers/watchdog/starfive-wdt.c:starfive_wdt_remove() {
    ...
	if (pm_runtime_enabled(&pdev->dev))
		pm_runtime_disable(&pdev->dev);
    ...
}

Because the platform device structure is not destroyed on unbind, its
usage_count is not reset.

Will a subsequent driver bind start with usage_count >= 1 and leak another
reference?

[Severity: High]
This isn't a bug introduced by this patch, but starfive_wdt_shutdown()
unconditionally calls starfive_wdt_pm_stop(&wdt->wdd), which calls
pm_runtime_put_sync().

If the watchdog was not running (e.g., userspace closed it, or it was never
started and early_enable was false), the PM usage count is already 0.

Because watchdog_stop_on_reboot() was called during probe, the watchdog core
may have already stopped the watchdog via a reboot notifier before the driver
core calls shutdown.

Will this unconditional decrement drop the usage count to -1 and cause a PM
core warning and state corruption?

[Severity: High]
This is a pre-existing issue, but if the watchdog device enters RPM_SUSPENDED
state, its clocks are disabled.

In starfive_wdt_remove() and starfive_wdt_shutdown(), starfive_wdt_stop() is
called directly without ensuring the device is active.

In starfive_wdt_suspend(), the driver unconditionally calls
starfive_wdt_get_count() and starfive_wdt_stop():

drivers/watchdog/starfive-wdt.c:starfive_wdt_suspend() {
    ...
	wdt->reload = starfive_wdt_get_count(wdt);
    ...
}

Accessing peripheral registers while the APB and core clocks are disabled
triggers a bus fault and hangs the system on many platforms.

Does the driver need to check watchdog_active() or resume the device first
before accessing these registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914165022.169877-1-skunkolee@gmail.com?part=1