Re: [PATCH] watchdog: starfive: use pm_runtime_resume_and_get() to fix refcount leak

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] System callbacks unconditionally access hardware registers while the device may be runtime-suspended, which will trigger a synchronous external abort.
- [High] starfive_wdt_pm_start() leaks the runtime PM usage counter if the hardware start operation fails, an edge case missed by the patch.
- [High] starfive_wdt_pm_stop() and starfive_wdt_probe() incorrectly handle a return value of 1 from pm_runtime_put_sync(), interpreting a success case as an error.
- [High] Multiple error paths in starfive_wdt_probe() leak the PM usage counter and fail to disable PM runtime.
- [High] starfive_wdt_shutdown() unconditionally drops the runtime PM usage counter, causing an underflow if the watchdog is not active.
- [High] Runtime PM usage counter leaks on module removal when early_enable is true and the device was never opened.
--

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

watchdog: starfive: use pm_runtime_resume_and_get() to fix refcount leak

starfive_wdt_pm_start() calls pm_runtime_get_sync(), but
pm_runtime_get_sync() increments the runtime PM usage counter even
when it returns an error.  On the error path this function just
returns the error code without a matching pm_runtime_put_noidle(),
so the usage counter is leaked on every failed resume.

Switch to pm_runtime_resume_and_get(), which decrements the usage
counter automatically on failure, leaving the caller with the simple
"check return value and propagate" pattern that the existing code
was already trying to follow.

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a3c69..5a3254c83d3bb 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -371,7 +371,7 @@ static void starfive_wdt_stop(struct starfive_wdt *wdt)
>  static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>  {
>  	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> -	int ret = pm_runtime_get_sync(wdd->parent);
> +	int ret = pm_runtime_resume_and_get(wdd->parent);
>  
>  	if (ret < 0)
>  		return ret;
>  
>  	return starfive_wdt_start(wdt);

[Severity: High]
This isn't a bug introduced by this patch, but does this code leak the runtime
PM usage counter if starfive_wdt_start() fails? It appears that the function
returns the error directly without calling pm_runtime_put(), keeping the
reference acquired by pm_runtime_resume_and_get().


While reviewing this file, I noticed a few other pre-existing issues.

drivers/watchdog/starfive-wdt.c:starfive_wdt_pm_stop() {
    ...
	starfive_wdt_stop(wdt);
	return pm_runtime_put_sync(wdd->parent);
}

[Severity: High]
This is a pre-existing issue, but does this break the watchdog core's
expectation of returning 0 on success?

According to Runtime PM API contracts, pm_runtime_put_sync() returns 1 if the
device is already in the RPM_SUSPENDED state, which indicates success, not an
error. By returning this raw result, it seems we force the watchdog core to
assume the stop operation failed.


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;
    ...

[Severity: High]
This is a pre-existing issue, but does this erroneously fail the probe if
pm_runtime_put_sync() returns 1?


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;
    ...

[Severity: High]
This is a pre-existing issue, but do these direct return paths bypass the
required pm_runtime_disable() cleanup after pm_runtime_enable() was called?


drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
    ...
err_exit:
	starfive_wdt_disable_clock(wdt);
	pm_runtime_disable(&pdev->dev);

	return ret;
}

[Severity: High]
This is a pre-existing issue, but does the err_exit label miss dropping the
runtime PM reference acquired via pm_runtime_resume_and_get() earlier in the
probe?


drivers/watchdog/starfive-wdt.c:starfive_wdt_remove() {
    ...
static void starfive_wdt_remove(struct platform_device *pdev)
{
	struct starfive_wdt *wdt = platform_get_drvdata(pdev);

	starfive_wdt_stop(wdt);
    ...

drivers/watchdog/starfive-wdt.c:starfive_wdt_shutdown() {
    ...
static void starfive_wdt_shutdown(struct platform_device *pdev)
{
	struct starfive_wdt *wdt = platform_get_drvdata(pdev);

	starfive_wdt_pm_stop(&wdt->wdd);
    ...

drivers/watchdog/starfive-wdt.c:starfive_wdt_suspend() {
    ...
static int starfive_wdt_suspend(struct device *dev)
{
	struct starfive_wdt *wdt = dev_get_drvdata(dev);

	/* Save watchdog state, and turn it off. */
	wdt->reload = starfive_wdt_get_count(wdt);

	/* Note that WTCNT doesn't need to be saved. */
	starfive_wdt_stop(wdt);
    ...

drivers/watchdog/starfive-wdt.c:starfive_wdt_resume() {
    ...
static int starfive_wdt_resume(struct device *dev)
{
    ...
	starfive_wdt_unlock(wdt);
	/* Restore watchdog state. */
	starfive_wdt_set_reload_count(wdt, wdt->reload);
	starfive_wdt_lock(wdt);
    ...

[Severity: Critical]
These are pre-existing issues, but do these system callbacks risk triggering a
synchronous external abort by unconditionally accessing hardware registers
while the device might be runtime-suspended and unclocked?


drivers/watchdog/starfive-wdt.c:starfive_wdt_shutdown() {
    ...
static void starfive_wdt_shutdown(struct platform_device *pdev)
{
	struct starfive_wdt *wdt = platform_get_drvdata(pdev);

	starfive_wdt_pm_stop(&wdt->wdd);
}

[Severity: High]
This is a pre-existing issue, but does unconditionally calling
starfive_wdt_pm_stop() (which contains pm_runtime_put_sync()) cause a PM
usage counter underflow if the watchdog is already inactive and its counter
is 0?


drivers/watchdog/starfive-wdt.c:starfive_wdt_remove() {
    ...
	if (pm_runtime_enabled(&pdev->dev))
		pm_runtime_disable(&pdev->dev);
	else
		/* disable clock without PM */
		starfive_wdt_disable_clock(wdt);
}

[Severity: High]
This is a pre-existing issue, but does the remove function fail to decrement
the PM usage counter if early_enable is true and the device was never opened
by userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605122336.159546-1-william@theesfeld.net?part=1