[PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start

[PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start

[manushprajwal]

starfive_wdt_pm_start() calls pm_runtime_get_sync() and returns its
error code directly on failure. Per Documentation/power/runtime_pm.rst,
pm_runtime_get_sync() does not drop the usage counter on error, so the
caller is expected to release the reference itself. The early return
here skips that, leaking a runtime PM usage count on wdd->parent on
every failed resume, which prevents the device from being suspended
correctly afterwards.

Fix it by calling pm_runtime_put_noidle() before returning, mirroring
the same balance the counterpart starfive_wdt_pm_stop() already
maintains with pm_runtime_put_sync().

Signed-off-by: manushprajwal <manushprajwal555@gmail.com>
---
 drivers/watchdog/starfive-wdt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index af55adc4a..ecb704b2e 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -373,8 +373,10 @@ 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);
 
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put_noidle(wdd->parent);
 		return ret;
+	}
 
 	return starfive_wdt_start(wdt);
 }
-- 
2.46.2.windows.1

Re: [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unprotected hardware access in get_timeleft sysfs callback allows unprivileged local DoS.
- [High] The patch fixes a runtime PM usage counter leak when pm_runtime_get_sync() fails, but leaves an identical leak when the subsequent starfive_wdt_start() fails.
- [High] Unclocked hardware register accesses in suspend, shutdown, and remove paths cause bus faults.
- [High] Leaked runtime PM usage counter and missing pm_runtime_disable() in starfive_wdt_probe() error paths.
- [High] Runtime PM usage counter and clocks are permanently leaked in remove if early_enable is true and the watchdog was never opened.
- [High] The hardware watchdog is not restarted on system resume if early_enable is true but the device was not opened.
--

--- Patch [1]: [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start ---
commit 6f33b73ef6f14abdd5d861e7b791ec5774bc787e
Author: manushprajwal <manushprajwal555@gmail.com>

watchdog: starfive: fix runtime PM usage counter leak in pm_start

This commit fixes a runtime PM usage counter leak in starfive_wdt_pm_start().
It ensures pm_runtime_put_noidle() is called when pm_runtime_get_sync()
returns an error.

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a3c69..ecb704b2e9a3d 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -373,8 +373,10 @@ 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);
>  
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(wdd->parent);
>  		return ret;
> +	}
>  
>  	return starfive_wdt_start(wdt);
>  }

[Severity: High]
This is a pre-existing issue, but does this still leak the runtime PM usage
counter if starfive_wdt_start() fails?

If starfive_wdt_start() returns a negative error (for example, if it
times out waiting for an interrupt clear), the error is returned directly.
Since the pm_runtime_get_sync() succeeded, doesn't this return path skip
dropping the PM reference, permanently preventing the device from suspending?


[Severity: High]
This is a pre-existing issue, but do the suspend, remove, and shutdown paths
access hardware registers while the device is suspended?

In starfive_wdt_suspend(), the registers are accessed unconditionally:

    drivers/watchdog/starfive-wdt.c:starfive_wdt_suspend() {
        ...
        wdt->reload = starfive_wdt_get_count(wdt);
        /* Note that WTCNT doesn't need to be saved. */
        starfive_wdt_stop(wdt);
        ...
    }

If the watchdog was not opened by userspace and early_enable is false, the
device resides in a runtime-suspended state with clocks disabled. Could
accessing its registers in this state trigger a synchronous external abort
(bus fault)? Similarly, the remove and shutdown paths also call
starfive_wdt_stop() directly.


[Severity: High]
This is a pre-existing issue, but do the error paths in starfive_wdt_probe()
leak the runtime PM usage counter or miss disabling PM?

If pm_runtime_resume_and_get() fails:

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

Should this call pm_runtime_disable() before returning?

Additionally, if a subsequent step fails, it jumps to err_exit:

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

        return ret;
    }

Does this path need to call pm_runtime_put_noidle() to drop the usage
counter before disabling PM?


[Severity: High]
This is a pre-existing issue, but does starfive_wdt_remove() leak the
runtime PM reference and clocks if early_enable is true?

If early_enable is set, the probe function leaves the PM usage count at 1.
If userspace never opens the watchdog, WDOG_ACTIVE remains false.
During remove:

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

Since watchdog_unregister_device() bypasses the stop callback when
WDOG_ACTIVE is false, and remove() only disables runtime PM without dropping
the usage counter, does this prevent the device from ever suspending and
leave the clocks running indefinitely?


[Severity: High]
This is a pre-existing issue, but does starfive_wdt_resume() fail to
restart the hardware if early_enable is true but the device is unopened?

    drivers/watchdog/starfive-wdt.c:starfive_wdt_resume() {
        ...
        if (watchdog_active(&wdt->wdd))
            return starfive_wdt_start(wdt);

        return 0;
    }

If the watchdog was started via early_enable but not opened by userspace,
watchdog_active() is false. Since starfive_wdt_suspend() unconditionally
stops the hardware, does this leave the system without hardware watchdog
protection after resuming?


[Severity: Critical]
This is a pre-existing issue, but does starfive_wdt_get_timeleft()
allow an unprivileged local denial of service?

    drivers/watchdog/starfive-wdt.c:starfive_wdt_get_timeleft() {
        ...
        count = starfive_wdt_get_count(wdt);
        ...
    }

This function is called when a user reads the 0444 sysfs attribute
timeleft. If the watchdog is closed and the device is runtime-suspended,
its clocks are disabled. Could an unprivileged user reading this sysfs
attribute cause an immediate bus fault (kernel panic) by accessing
unclocked registers?

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

Re: [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start

[Guenter Roeck]

On 8/23/26 04:22, manushprajwal wrote:
> starfive_wdt_pm_start() calls pm_runtime_get_sync() and returns its
> error code directly on failure. Per Documentation/power/runtime_pm.rst,
> pm_runtime_get_sync() does not drop the usage counter on error, so the
> caller is expected to release the reference itself. The early return
> here skips that, leaking a runtime PM usage count on wdd->parent on
> every failed resume, which prevents the device from being suspended
> correctly afterwards.
> 
> Fix it by calling pm_runtime_put_noidle() before returning, mirroring
> the same balance the counterpart starfive_wdt_pm_stop() already
> maintains with pm_runtime_put_sync().
> 
> Signed-off-by: manushprajwal <manushprajwal555@gmail.com>
> ---
>   drivers/watchdog/starfive-wdt.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a..ecb704b2e 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -373,8 +373,10 @@ 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);
>   
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(wdd->parent);
>   		return ret;
> +	}
>   
>   	return starfive_wdt_start(wdt);
>   }

As Sashiko points out, there is still a runtime PM leak if starfive_wdt_start()
fails (plus all the other issues in the driver, but those can be fixed with
separate patches).

Thanks,
Guenter