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

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

[Ivy Lopez]

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.

Add a pm_runtime_put_sync() call in err_exit, gated on
pm_runtime_enabled(), to balance the earlier get.

Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/watchdog/starfive-wdt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index af55adc4a3c6..f87f8d5a3dab 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);
 	pm_runtime_disable(&pdev->dev);
 
 	return ret;
-- 
2.55.0

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

[sashiko-bot]

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

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

[Ivy Lopez]

pm_runtime_resume_and_get() increments the runtime PM usage counter
and, via starfive_wdt_runtime_resume(), enables the clocks. 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 balances the
earlier get, leaking the PM usage count.

Track whether a PM reference is currently held (pm_ref_held) and
whether the clocks are currently enabled (clocks_on), and use them
in err_exit to release exactly what was acquired:

 - if a PM reference is held, put it via pm_runtime_put_sync();
   only fall back to a manual clock disable if the resulting
   suspend transition did not actually run (put failure), since
   the runtime_suspend callback already disables the clocks on a
   successful put and a redundant call would double-disable them
   and underflow the clock enable count.
 - if no PM reference is held but the clocks were enabled directly
   (runtime PM disabled case), disable them manually.

Also propagate this accounting to the existing pm_runtime_put_sync()
call at the end of a successful probe: its usage-count decrement
happens regardless of the call's return value, so pm_ref_held must
be cleared immediately after calling it to avoid a second,
unbalanced put_sync() call if a later step in that path fails and
falls through to err_exit.

Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
v2:
 - Fix double clock-disable / clock-enable-count underflow when a
   PM reference was already released via a successful
   pm_runtime_put_sync() in err_exit
 - Fix double pm_runtime_put_sync() / usage-count underflow when
   falling through from the tail put failure to err_unregister_wdt

 drivers/watchdog/starfive-wdt.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index af55adc4a3c6..4ad587732bda 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -429,6 +429,8 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 {
 	struct starfive_wdt *wdt;
 	int ret;
+	bool pm_ref_held = false;
+	bool clocks_on = false;
 
 	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
@@ -449,11 +451,14 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 		ret = pm_runtime_resume_and_get(&pdev->dev);
 		if (ret < 0)
 			return ret;
+		pm_ref_held = true;
+		clocks_on = true;
 	} else {
 		/* runtime PM is disabled but clocks need to be enabled */
 		ret = starfive_wdt_enable_clock(wdt);
 		if (ret)
 			return ret;
+		clocks_on = true;
 	}
 
 	ret = starfive_wdt_reset_init(&pdev->dev);
@@ -499,6 +504,9 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 	if (!early_enable) {
 		if (pm_runtime_enabled(&pdev->dev)) {
 			ret = pm_runtime_put_sync(&pdev->dev);
+			pm_ref_held = false;
+			if (ret == 0)
+				clocks_on = false;
 			if (ret)
 				goto err_unregister_wdt;
 		}
@@ -509,7 +517,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);
+	if (pm_ref_held) {
+		if (pm_runtime_put_sync(&pdev->dev))
+			starfive_wdt_disable_clock(wdt);
+	} else if (clocks_on) {
+		starfive_wdt_disable_clock(wdt);
+	}
 	pm_runtime_disable(&pdev->dev);
 
 	return ret;
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pm: desynchronize PM state by manually disabling clocks on put failure
--

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

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

This commit addresses a PM reference leak in the probe error path by tracking
whether a PM reference is held and whether clocks are enabled, ensuring they
are properly released on exit.

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a3c69..4ad587732bdaa 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c

[ ... ]

> @@ -509,7 +517,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);
> +	if (pm_ref_held) {
> +		if (pm_runtime_put_sync(&pdev->dev))
> +			starfive_wdt_disable_clock(wdt);

[Severity: High]
Does manually disabling the clock here risk desynchronizing the PM state if
pm_runtime_put_sync() intentionally fails?

If a concurrent operation (like userspace sysfs interaction) bumps the PM
usage count during this window, pm_runtime_put_sync() in starfive_wdt_probe()
returns an error such as -EAGAIN. This indicates the PM core has decided to
keep the device active.

By manually disabling the clocks here, the hardware is turned off while the
PM core still considers the device to be RPM_ACTIVE.

If a concurrent thread or a subsequent driver reprobe accesses the peripheral
registers while unclocked (since a later pm_runtime_resume_and_get() would
find the device already active and skip the resume callback), could this lead
to a bus error or system crash?

> +	} else if (clocks_on) {
> +		starfive_wdt_disable_clock(wdt);
> +	}
>  	pm_runtime_disable(&pdev->dev);
>  
>  	return ret;

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

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

[Guenter Roeck]

On 9/14/26 20:20, Ivy Lopez wrote:
> pm_runtime_resume_and_get() increments the runtime PM usage counter
> and, via starfive_wdt_runtime_resume(), enables the clocks. 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 balances the
> earlier get, leaking the PM usage count.
> 
> Track whether a PM reference is currently held (pm_ref_held) and
> whether the clocks are currently enabled (clocks_on), and use them
> in err_exit to release exactly what was acquired:
> 
>   - if a PM reference is held, put it via pm_runtime_put_sync();
>     only fall back to a manual clock disable if the resulting
>     suspend transition did not actually run (put failure), since
>     the runtime_suspend callback already disables the clocks on a
>     successful put and a redundant call would double-disable them
>     and underflow the clock enable count.
>   - if no PM reference is held but the clocks were enabled directly
>     (runtime PM disabled case), disable them manually.
> 
> Also propagate this accounting to the existing pm_runtime_put_sync()
> call at the end of a successful probe: its usage-count decrement
> happens regardless of the call's return value, so pm_ref_held must
> be cleared immediately after calling it to avoid a second,
> unbalanced put_sync() call if a later step in that path fails and
> falls through to err_exit.
> 
> Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> Signed-off-by: Ivy Lopez <skunkolee@gmail.com>

I really don't get it. People keep sending new patch revisions as response
to previous patch revisions, even though that is discouraged, but no one
admits where they get the idea from.

I am going to just ignore such submissions in the future. Last warning.

Guenter

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

[Guenter Roeck]

On 9/14/26 20:20, Ivy Lopez wrote:
> pm_runtime_resume_and_get() increments the runtime PM usage counter
> and, via starfive_wdt_runtime_resume(), enables the clocks. 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 balances the
> earlier get, leaking the PM usage count.
> 
> Track whether a PM reference is currently held (pm_ref_held) and
> whether the clocks are currently enabled (clocks_on), and use them
> in err_exit to release exactly what was acquired:
> 
>   - if a PM reference is held, put it via pm_runtime_put_sync();
>     only fall back to a manual clock disable if the resulting
>     suspend transition did not actually run (put failure), since
>     the runtime_suspend callback already disables the clocks on a
>     successful put and a redundant call would double-disable them
>     and underflow the clock enable count.
>   - if no PM reference is held but the clocks were enabled directly
>     (runtime PM disabled case), disable them manually.
> 
> Also propagate this accounting to the existing pm_runtime_put_sync()
> call at the end of a successful probe: its usage-count decrement
> happens regardless of the call's return value, so pm_ref_held must
> be cleared immediately after calling it to avoid a second,
> unbalanced put_sync() call if a later step in that path fails and
> falls through to err_exit.
> 
> Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
> ---
> v2:
>   - Fix double clock-disable / clock-enable-count underflow when a
>     PM reference was already released via a successful
>     pm_runtime_put_sync() in err_exit
>   - Fix double pm_runtime_put_sync() / usage-count underflow when
>     falling through from the tail put failure to err_unregister_wdt
> 
>   drivers/watchdog/starfive-wdt.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a3c6..4ad587732bda 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -429,6 +429,8 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>   {
>   	struct starfive_wdt *wdt;
>   	int ret;
> +	bool pm_ref_held = false;
> +	bool clocks_on = false;
>   
>   	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>   	if (!wdt)
> @@ -449,11 +451,14 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>   		ret = pm_runtime_resume_and_get(&pdev->dev);
>   		if (ret < 0)
>   			return ret;
> +		pm_ref_held = true;
> +		clocks_on = true;
>   	} else {
>   		/* runtime PM is disabled but clocks need to be enabled */
>   		ret = starfive_wdt_enable_clock(wdt);
>   		if (ret)
>   			return ret;
> +		clocks_on = true;
>   	}
>   
>   	ret = starfive_wdt_reset_init(&pdev->dev);
> @@ -499,6 +504,9 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>   	if (!early_enable) {
>   		if (pm_runtime_enabled(&pdev->dev)) {
>   			ret = pm_runtime_put_sync(&pdev->dev);
> +			pm_ref_held = false;
> +			if (ret == 0)
> +				clocks_on = false;
>   			if (ret)
>   				goto err_unregister_wdt;
>   		}
> @@ -509,7 +517,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);
> +	if (pm_ref_held) {
> +		if (pm_runtime_put_sync(&pdev->dev))

The return value from pm_runtime_put_sync() is almost never checked.
I wonder if checking it creates more trouble than it is worth.

Guenter

> +			starfive_wdt_disable_clock(wdt);
> +	} else if (clocks_on) {
> +		starfive_wdt_disable_clock(wdt);
> +	}
>   	pm_runtime_disable(&pdev->dev);
>   
>   	return ret;