[PATCH 0/5] watchdog: starfive-wdt: Fix runtime PM and unclocked register access bugs

[PATCH 0/5] watchdog: starfive-wdt: Fix runtime PM and unclocked register access bugs

[Guenter Roeck]

The StarFive watchdog driver (drivers/watchdog/starfive-wdt.c) gates its
APB and core clocks via runtime PM callbacks, but has several runtime PM
reference-counting, ordering, and unclocked MMIO access bugs across
probe, remove, start/stop, shutdown, and system sleep paths:

1. In starfive_wdt_pm_start(), if starfive_wdt_start() fails after
   pm_runtime_resume_and_get() succeeds, the acquired runtime PM
   reference is leaked because the watchdog core does not call the stop
   callback when start fails.
2. In starfive_wdt_probe(), early returns on pm_runtime_resume_and_get()
   or starfive_wdt_enable_clock() failure bypass pm_runtime_disable(),
   leaving the runtime PM disable_depth counter unbalanced.
3. When runtime PM has suspended an idle watchdog and gated its clocks,
   starfive_wdt_remove(), starfive_wdt_shutdown(), starfive_wdt_suspend(),
   and starfive_wdt_resume() unconditionally read and write hardware
   registers, causing unclocked MMIO bus faults. In addition,
   starfive_wdt_pm_stop() propagates non-zero idle return codes from
   pm_runtime_put_sync() as errors to watchdog_stop().
4. Probe error paths after pm_runtime_resume_and_get() disable clocks
   manually without releasing the runtime PM reference (and without
   stopping an early_enable watchdog if registration fails), and
   starfive_wdt_remove() never drops the active runtime PM reference
   retained when watchdog_hw_running() is still set (e.g. early_enable).
5. In starfive_wdt_probe(), watchdog_register_device() is called before
   pm_runtime_put_sync() drops the probe-time reference, and non-zero
   idle return values such as -EAGAIN (triggered if userspace opens
   /dev/watchdog immediately upon registration) are treated as fatal
   probe errors.

These bugs have been reported by Sashiko.

This 5-patch series fixes each of these issues cleanly in sequence.

Note on tooling and testing:
This series was generated with an AI coding assistant (Jetski / Gemini)
following analysis of reported runtime PM and lifecycle bugs in
starfive-wdt.c. All patches were build-tested with make allmodconfig and
make W=1 with zero warnings, verified with scripts/checkpatch.pl
--strict, and validated against a state-machine simulation harness
covering runtime PM, clock gating, and concurrent open paths; they were
not tested on physical StarFive JH7100/JH7110 hardware.

Guenter Roeck (5):
  watchdog: starfive-wdt: Drop runtime PM reference if starting watchdog
    fails
  watchdog: starfive-wdt: Balance pm_runtime_enable() on probe error
    paths
  watchdog: starfive-wdt: Avoid unclocked register access in remove and
    PM paths
  watchdog: starfive-wdt: Fix runtime PM usage count leaks in probe and
    remove
  watchdog: starfive-wdt: Drop runtime PM reference before registering
    watchdog

 drivers/watchdog/starfive-wdt.c | 75 +++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 27 deletions(-)

Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
Cc: Ziv Xu <ziv.xu@starfivetech.com>
-- 
2.56.0

[PATCH 1/5] watchdog: starfive-wdt: Drop runtime PM reference if starting watchdog fails

[Guenter Roeck]

Commit 8f0ca55016a7 ("watchdog: starfive-wdt: Fix runtime PM leak in
starfive_wdt_pm_start()") switched starfive_wdt_pm_start() to
pm_runtime_resume_and_get() so the runtime PM usage counter is not
leaked when resuming the device fails. However, if starfive_wdt_start()
subsequently fails (for example, on a timeout in starfive_wdt_int_clr()),
starfive_wdt_pm_start() returns the error without releasing the runtime
PM reference acquired by pm_runtime_resume_and_get(). Because the
watchdog core does not invoke the stop callback when start fails, the
usage counter remains permanently elevated.

Drop the runtime PM reference with pm_runtime_put_sync() if
starfive_wdt_start() fails.

Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
Cc: Ziv Xu <ziv.xu@starfivetech.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/starfive-wdt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 044b4fdb7350..ab6920b8d7d8 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -372,12 +372,17 @@ 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_resume_and_get(wdd->parent);
+	int ret;
 
+	ret = pm_runtime_resume_and_get(wdd->parent);
 	if (ret < 0)
 		return ret;
 
-	return starfive_wdt_start(wdt);
+	ret = starfive_wdt_start(wdt);
+	if (ret)
+		pm_runtime_put_sync(wdd->parent);
+
+	return ret;
 }
 
 static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
-- 
2.45.2

Re: [PATCH 1/5] watchdog: starfive-wdt: Drop runtime PM reference if starting watchdog fails

[Tzung-Bi Shih]

On Wed, Nov 04, 2026 at 08:44:41AM -0800, Guenter Roeck wrote:
> Commit 8f0ca55016a7 ("watchdog: starfive-wdt: Fix runtime PM leak in
> starfive_wdt_pm_start()") switched starfive_wdt_pm_start() to
> pm_runtime_resume_and_get() so the runtime PM usage counter is not
> leaked when resuming the device fails. However, if starfive_wdt_start()
> subsequently fails (for example, on a timeout in starfive_wdt_int_clr()),
> starfive_wdt_pm_start() returns the error without releasing the runtime
> PM reference acquired by pm_runtime_resume_and_get(). Because the
> watchdog core does not invoke the stop callback when start fails, the
> usage counter remains permanently elevated.
> 
> Drop the runtime PM reference with pm_runtime_put_sync() if
> starfive_wdt_start() fails.
> 
> Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> Cc: Ziv Xu <ziv.xu@starfivetech.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

With a nit,

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

> @@ -372,12 +372,17 @@ 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_resume_and_get(wdd->parent);
> +	int ret;
>  
> +	ret = pm_runtime_resume_and_get(wdd->parent);
>  	if (ret < 0)
>  		return ret;
>  
> -	return starfive_wdt_start(wdt);
> +	ret = starfive_wdt_start(wdt);
> +	if (ret)
> +		pm_runtime_put_sync(wdd->parent);
> +
> +	return ret;

Or just `return 0;`.

Re: [PATCH 1/5] watchdog: starfive-wdt: Drop runtime PM reference if starting watchdog fails

[Guenter Roeck]

On Thu, Nov 05, 2026 at 01:46:40PM +0000, Tzung-Bi Shih wrote:
> On Wed, Nov 04, 2026 at 08:44:41AM -0800, Guenter Roeck wrote:
> > Commit 8f0ca55016a7 ("watchdog: starfive-wdt: Fix runtime PM leak in
> > starfive_wdt_pm_start()") switched starfive_wdt_pm_start() to
> > pm_runtime_resume_and_get() so the runtime PM usage counter is not
> > leaked when resuming the device fails. However, if starfive_wdt_start()
> > subsequently fails (for example, on a timeout in starfive_wdt_int_clr()),
> > starfive_wdt_pm_start() returns the error without releasing the runtime
> > PM reference acquired by pm_runtime_resume_and_get(). Because the
> > watchdog core does not invoke the stop callback when start fails, the
> > usage counter remains permanently elevated.
> > 
> > Drop the runtime PM reference with pm_runtime_put_sync() if
> > starfive_wdt_start() fails.
> > 
> > Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> > Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> > Cc: Ziv Xu <ziv.xu@starfivetech.com>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> With a nit,
> 
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> 
> > @@ -372,12 +372,17 @@ 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_resume_and_get(wdd->parent);
> > +	int ret;
> >  
> > +	ret = pm_runtime_resume_and_get(wdd->parent);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	return starfive_wdt_start(wdt);
> > +	ret = starfive_wdt_start(wdt);
> > +	if (ret)
> > +		pm_runtime_put_sync(wdd->parent);
> > +
> > +	return ret;
> 
> Or just `return 0;`.
> 

Good idea. I'll change that.

Thanks,
Guenter

Re: [PATCH 1/5] watchdog: starfive-wdt: Drop runtime PM reference if starting watchdog fails

[Guenter Roeck]

On Thu, Nov 05, 2026 at 06:49:26AM -0800, Guenter Roeck wrote:
> On Thu, Nov 05, 2026 at 01:46:40PM +0000, Tzung-Bi Shih wrote:
> > On Wed, Nov 04, 2026 at 08:44:41AM -0800, Guenter Roeck wrote:
> > > Commit 8f0ca55016a7 ("watchdog: starfive-wdt: Fix runtime PM leak in
> > > starfive_wdt_pm_start()") switched starfive_wdt_pm_start() to
> > > pm_runtime_resume_and_get() so the runtime PM usage counter is not
> > > leaked when resuming the device fails. However, if starfive_wdt_start()
> > > subsequently fails (for example, on a timeout in starfive_wdt_int_clr()),
> > > starfive_wdt_pm_start() returns the error without releasing the runtime
> > > PM reference acquired by pm_runtime_resume_and_get(). Because the
> > > watchdog core does not invoke the stop callback when start fails, the
> > > usage counter remains permanently elevated.
> > > 
> > > Drop the runtime PM reference with pm_runtime_put_sync() if
> > > starfive_wdt_start() fails.
> > > 
> > > Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> > > Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > Cc: Ziv Xu <ziv.xu@starfivetech.com>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > 
> > With a nit,
> > 
> > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > 
> > > @@ -372,12 +372,17 @@ 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_resume_and_get(wdd->parent);
> > > +	int ret;
> > >  
> > > +	ret = pm_runtime_resume_and_get(wdd->parent);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > -	return starfive_wdt_start(wdt);
> > > +	ret = starfive_wdt_start(wdt);
> > > +	if (ret)
> > > +		pm_runtime_put_sync(wdd->parent);
> > > +
> > > +	return ret;
> > 
> > Or just `return 0;`.
> > 
> 
> Good idea. I'll change that.

Ah, no, we still want to return the error. Difference is that
pm_runtime_put_sync() is only called if there was an error,
but the error must still be returned to the caller.

Thanks,
Guenter

Re: [PATCH 1/5] watchdog: starfive-wdt: Drop runtime PM reference if starting watchdog fails

[Tzung-Bi Shih]

On Thu, Nov 05, 2026 at 06:52:29AM -0800, Guenter Roeck wrote:
> On Thu, Nov 05, 2026 at 06:49:26AM -0800, Guenter Roeck wrote:
> > On Thu, Nov 05, 2026 at 01:46:40PM +0000, Tzung-Bi Shih wrote:
> > > On Wed, Nov 04, 2026 at 08:44:41AM -0800, Guenter Roeck wrote:
> > > > Commit 8f0ca55016a7 ("watchdog: starfive-wdt: Fix runtime PM leak in
> > > > starfive_wdt_pm_start()") switched starfive_wdt_pm_start() to
> > > > pm_runtime_resume_and_get() so the runtime PM usage counter is not
> > > > leaked when resuming the device fails. However, if starfive_wdt_start()
> > > > subsequently fails (for example, on a timeout in starfive_wdt_int_clr()),
> > > > starfive_wdt_pm_start() returns the error without releasing the runtime
> > > > PM reference acquired by pm_runtime_resume_and_get(). Because the
> > > > watchdog core does not invoke the stop callback when start fails, the
> > > > usage counter remains permanently elevated.
> > > > 
> > > > Drop the runtime PM reference with pm_runtime_put_sync() if
> > > > starfive_wdt_start() fails.
> > > > 
> > > > Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> > > > Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > Cc: Ziv Xu <ziv.xu@starfivetech.com>
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > 
> > > With a nit,
> > > 
> > > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > > 
> > > > @@ -372,12 +372,17 @@ 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_resume_and_get(wdd->parent);
> > > > +	int ret;
> > > >  
> > > > +	ret = pm_runtime_resume_and_get(wdd->parent);
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > >  
> > > > -	return starfive_wdt_start(wdt);
> > > > +	ret = starfive_wdt_start(wdt);
> > > > +	if (ret)
> > > > +		pm_runtime_put_sync(wdd->parent);
> > > > +
> > > > +	return ret;
> > > 
> > > Or just `return 0;`.
> > > 
> > 
> > Good idea. I'll change that.
> 
> Ah, no, we still want to return the error. Difference is that
> pm_runtime_put_sync() is only called if there was an error,
> but the error must still be returned to the caller.

Ah...correct, I overlooked that.

[PATCH 2/5] watchdog: starfive-wdt: Balance pm_runtime_enable() on probe error paths

[Guenter Roeck]

In starfive_wdt_probe(), pm_runtime_enable() is called before attempting
to resume the device via pm_runtime_resume_and_get() or enable its clocks
via starfive_wdt_enable_clock(). If either call fails, probe returns
directly without calling pm_runtime_disable(), leaving the runtime PM
disable_depth counter unbalanced (and triggering "Unbalanced
pm_runtime_enable!" warnings on deferred probe retries).

Add an err_disable_pm error label and jump to it so pm_runtime_disable()
is called on these error paths.

Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
Cc: Ziv Xu <ziv.xu@starfivetech.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/starfive-wdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index ab6920b8d7d8..4724fccbfe63 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -454,12 +454,12 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 	if (pm_runtime_enabled(&pdev->dev)) {
 		ret = pm_runtime_resume_and_get(&pdev->dev);
 		if (ret < 0)
-			return ret;
+			goto err_disable_pm;
 	} else {
 		/* runtime PM is disabled but clocks need to be enabled */
 		ret = starfive_wdt_enable_clock(wdt);
 		if (ret)
-			return ret;
+			goto err_disable_pm;
 	}
 
 	ret = starfive_wdt_reset_init(&pdev->dev);
@@ -516,6 +516,7 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 	watchdog_unregister_device(&wdt->wdd);
 err_exit:
 	starfive_wdt_disable_clock(wdt);
+err_disable_pm:
 	pm_runtime_disable(&pdev->dev);
 
 	return ret;
-- 
2.45.2

Re: [PATCH 2/5] watchdog: starfive-wdt: Balance pm_runtime_enable() on probe error paths

[Tzung-Bi Shih]

On Wed, Nov 04, 2026 at 08:44:42AM -0800, Guenter Roeck wrote:
> In starfive_wdt_probe(), pm_runtime_enable() is called before attempting
> to resume the device via pm_runtime_resume_and_get() or enable its clocks
> via starfive_wdt_enable_clock(). If either call fails, probe returns
> directly without calling pm_runtime_disable(), leaving the runtime PM
> disable_depth counter unbalanced (and triggering "Unbalanced
> pm_runtime_enable!" warnings on deferred probe retries).
> 
> Add an err_disable_pm error label and jump to it so pm_runtime_disable()
> is called on these error paths.
> 
> Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> Cc: Ziv Xu <ziv.xu@starfivetech.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

[PATCH 3/5] watchdog: starfive-wdt: Avoid unclocked register access in remove and PM paths

[Guenter Roeck]

When runtime PM is enabled and the watchdog is inactive, the device enters
runtime suspend and starfive_wdt_runtime_suspend() disables core_clk and
apb_clk.

However, starfive_wdt_remove(), starfive_wdt_shutdown(),
starfive_wdt_suspend(), and starfive_wdt_resume() unconditionally read and
write watchdog MMIO registers without checking whether the watchdog is
running or clocks are enabled, causing a bus fault when the device is
runtime-suspended:

- In starfive_wdt_remove(), starfive_wdt_stop() is called before
  watchdog_unregister_device() while clocks may be gated off. Unregister
  the watchdog device first (which stops an active watchdog via
  watchdog_stop_on_unregister while its runtime PM reference keeps clocks
  enabled), and only call starfive_wdt_stop() if watchdog_hw_running() is
  still set.
- In starfive_wdt_pm_stop(), ignore non-zero idle return codes from
  pm_runtime_put_sync() so watchdog_stop() does not falsely treat a
  stopped watchdog as still running.
- In starfive_wdt_shutdown(), skip starfive_wdt_pm_stop() if the device is
  already runtime-suspended.
- In starfive_wdt_suspend() and starfive_wdt_resume(), only save/stop and
  restore/restart the hardware watchdog when watchdog_hw_running() is set
  (in which case an active runtime PM reference ensures clocks are enabled
  before suspend and re-enabled by pm_runtime_force_resume()).

Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
Cc: Ziv Xu <ziv.xu@starfivetech.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/starfive-wdt.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 4724fccbfe63..18301f5d1144 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -390,7 +390,10 @@ static int starfive_wdt_pm_stop(struct watchdog_device *wdd)
 	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
 
 	starfive_wdt_stop(wdt);
-	return pm_runtime_put_sync(wdd->parent);
+	if (pm_runtime_enabled(wdd->parent))
+		pm_runtime_put_sync(wdd->parent);
+
+	return 0;
 }
 
 static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
@@ -526,9 +529,11 @@ static void starfive_wdt_remove(struct platform_device *pdev)
 {
 	struct starfive_wdt *wdt = platform_get_drvdata(pdev);
 
-	starfive_wdt_stop(wdt);
 	watchdog_unregister_device(&wdt->wdd);
 
+	if (watchdog_hw_running(&wdt->wdd))
+		starfive_wdt_stop(wdt);
+
 	if (pm_runtime_enabled(&pdev->dev))
 		pm_runtime_disable(&pdev->dev);
 	else
@@ -540,18 +545,21 @@ static void starfive_wdt_shutdown(struct platform_device *pdev)
 {
 	struct starfive_wdt *wdt = platform_get_drvdata(pdev);
 
-	starfive_wdt_pm_stop(&wdt->wdd);
+	if (!pm_runtime_suspended(&pdev->dev))
+		starfive_wdt_pm_stop(&wdt->wdd);
 }
 
 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);
+	if (watchdog_hw_running(&wdt->wdd)) {
+		/* 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);
+		/* Note that WTCNT doesn't need to be saved. */
+		starfive_wdt_stop(wdt);
+	}
 
 	return pm_runtime_force_suspend(dev);
 }
@@ -565,13 +573,14 @@ static int starfive_wdt_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	starfive_wdt_unlock(wdt);
-	/* Restore watchdog state. */
-	starfive_wdt_set_reload_count(wdt, wdt->reload);
-	starfive_wdt_lock(wdt);
+	if (watchdog_hw_running(&wdt->wdd)) {
+		starfive_wdt_unlock(wdt);
+		/* Restore watchdog state. */
+		starfive_wdt_set_reload_count(wdt, wdt->reload);
+		starfive_wdt_lock(wdt);
 
-	if (watchdog_active(&wdt->wdd))
 		return starfive_wdt_start(wdt);
+	}
 
 	return 0;
 }
-- 
2.45.2

Re: [PATCH 3/5] watchdog: starfive-wdt: Avoid unclocked register access in remove and PM paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] watchdog: starfive-wdt: unbalanced runtime PM put in starfive_wdt_shutdown()
--

commit 79e745d33ef2e011dcb21c8530361f6c5a3a1d9a
Author: Guenter Roeck <linux@roeck-us.net>

watchdog: starfive-wdt: Avoid unclocked register access in remove and PM paths

This commit avoids unclocked MMIO register accesses in the remove, shutdown,
suspend, and resume paths when the StarFive watchdog device is runtime
suspended with its clocks gated off. It also ignores non-zero return codes
from pm_runtime_put_sync() in starfive_wdt_pm_stop().

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index 4724fccbfe637..18301f5d1144c 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c

[ ... ]

> @@ -540,18 +545,21 @@ static void starfive_wdt_shutdown(struct platform_device *pdev)
>  {
>  	struct starfive_wdt *wdt = platform_get_drvdata(pdev);
>  
> -	starfive_wdt_pm_stop(&wdt->wdd);
> +	if (!pm_runtime_suspended(&pdev->dev))
> +		starfive_wdt_pm_stop(&wdt->wdd);
>  }

[Severity: Medium]
Can calling starfive_wdt_pm_stop() here cause an unbalanced
pm_runtime_put_sync() or an unclocked register access?

In starfive_wdt_probe(), the driver registers a reboot notifier via
watchdog_stop_on_reboot(&wdt->wdd):

drivers/watchdog/starfive-wdt.c:starfive_wdt_probe()
	watchdog_set_nowayout(&wdt->wdd, nowayout);
	watchdog_stop_on_reboot(&wdt->wdd);
	watchdog_stop_on_unregister(&wdt->wdd);

During system reboot, halt, or power-off, kernel_restart_prepare() and
kernel_shutdown_prepare() run reboot_notifier_list before device_shutdown().
When the watchdog is running, watchdog_reboot_notifier() in
drivers/watchdog/watchdog_core.c already calls wdd->ops->stop(wdd), which is
starfive_wdt_pm_stop():

drivers/watchdog/starfive-wdt.c:starfive_wdt_pm_stop()
	starfive_wdt_stop(wdt);
	if (pm_runtime_enabled(wdd->parent))
		pm_runtime_put_sync(wdd->parent);

That call stops the hardware and drops the runtime PM reference acquired by
starfive_wdt_pm_start().

When device_shutdown() subsequently calls starfive_wdt_shutdown(), if the
device is not in RPM_SUSPENDED state (for example, if runtime suspend was
forbidden via sysfs power/control or blocked by a PM QoS constraint, either
when the watchdog was never started or after watchdog_reboot_notifier()
already stopped it), !pm_runtime_suspended(&pdev->dev) is true and
starfive_wdt_pm_stop() is called without a matching runtime PM reference held
by the driver.

Also, because pm_runtime_suspended(dev) checks both
dev->power.runtime_status == RPM_SUSPENDED and !dev->power.disable_depth, if
runtime PM is disabled while the device is suspended with core_clk and
apb_clk gated off (such as after pm_runtime_force_suspend() during
hibernation shutdown), !pm_runtime_suspended(&pdev->dev) evaluates to true
and starfive_wdt_stop() will access unclocked registers.

Since watchdog_stop_on_reboot() is already set in starfive_wdt_probe(),
should starfive_wdt_shutdown() check watchdog_hw_running(&wdt->wdd) (or be
removed so it also respects the watchdog.stop_on_reboot=0 module parameter)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104164445.3209560-1-linux@roeck-us.net?part=3

Re: [PATCH 3/5] watchdog: starfive-wdt: Avoid unclocked register access in remove and PM paths

[Guenter Roeck]

On 11/4/26 09:02, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] watchdog: starfive-wdt: unbalanced runtime PM put in starfive_wdt_shutdown()
> --

I really love/hate Sashiko :-). A manual run did not report any problems.
On top of the issue reported here, it also found a number of additional
bugs (reported in its bug database, not here) in the driver, related to
MMIO access with clocks turned off. I'll get all those addressed, but
I'll wait a day of two before sending out v2 of the patch series.

Guenter

[PATCH 4/5] watchdog: starfive-wdt: Fix runtime PM usage count leaks in probe and remove

[Guenter Roeck]

When runtime PM is enabled, starfive_wdt_probe() acquires a runtime PM
reference via pm_runtime_resume_and_get(). On subsequent probe failures,
err_exit manually disables clocks via starfive_wdt_disable_clock() and
calls pm_runtime_disable(), without dropping the acquired runtime PM
reference (and leaving the hardware timer running if early_enable was
enabled and watchdog_register_device() failed).

Similarly, when early_enable is set (or the watchdog was left running
on close), the active runtime PM reference is retained after probe, but
starfive_wdt_remove() never releases it before calling
pm_runtime_disable().

Release the runtime PM reference with pm_runtime_put_sync() on probe
error paths (stopping the watchdog first if running) and in
starfive_wdt_remove() when watchdog_hw_running() is still set.

Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
Cc: Ziv Xu <ziv.xu@starfivetech.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/starfive-wdt.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 18301f5d1144..817f84cc2807 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -517,8 +517,14 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 
 err_unregister_wdt:
 	watchdog_unregister_device(&wdt->wdd);
+	goto err_disable_pm;
 err_exit:
-	starfive_wdt_disable_clock(wdt);
+	if (watchdog_hw_running(&wdt->wdd))
+		starfive_wdt_stop(wdt);
+	if (pm_runtime_enabled(&pdev->dev))
+		pm_runtime_put_sync(&pdev->dev);
+	else
+		starfive_wdt_disable_clock(wdt);
 err_disable_pm:
 	pm_runtime_disable(&pdev->dev);
 
@@ -531,8 +537,11 @@ static void starfive_wdt_remove(struct platform_device *pdev)
 
 	watchdog_unregister_device(&wdt->wdd);
 
-	if (watchdog_hw_running(&wdt->wdd))
+	if (watchdog_hw_running(&wdt->wdd)) {
 		starfive_wdt_stop(wdt);
+		if (pm_runtime_enabled(&pdev->dev))
+			pm_runtime_put_sync(&pdev->dev);
+	}
 
 	if (pm_runtime_enabled(&pdev->dev))
 		pm_runtime_disable(&pdev->dev);
-- 
2.45.2

Re: [PATCH 4/5] watchdog: starfive-wdt: Fix runtime PM usage count leaks in probe and remove

[Tzung-Bi Shih]

On Wed, Nov 04, 2026 at 08:44:44AM -0800, Guenter Roeck wrote:
> When runtime PM is enabled, starfive_wdt_probe() acquires a runtime PM
> reference via pm_runtime_resume_and_get(). On subsequent probe failures,
> err_exit manually disables clocks via starfive_wdt_disable_clock() and
> calls pm_runtime_disable(), without dropping the acquired runtime PM
> reference (and leaving the hardware timer running if early_enable was
> enabled and watchdog_register_device() failed).
> 
> Similarly, when early_enable is set (or the watchdog was left running
> on close), the active runtime PM reference is retained after probe, but
> starfive_wdt_remove() never releases it before calling
> pm_runtime_disable().
> 
> Release the runtime PM reference with pm_runtime_put_sync() on probe
> error paths (stopping the watchdog first if running) and in
> starfive_wdt_remove() when watchdog_hw_running() is still set.
> 
> Fixes: db728ea9c7be ("drivers: watchdog: Add StarFive Watchdog driver")
> Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> Cc: Ziv Xu <ziv.xu@starfivetech.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

[PATCH 5/5] watchdog: starfive-wdt: Drop runtime PM reference before registering watchdog

[Guenter Roeck]

In starfive_wdt_probe(), watchdog_register_device() is called before the
probe-time runtime PM reference is released via pm_runtime_put_sync(), and
any non-zero return value from pm_runtime_put_sync() is treated as a fatal
probe error.

If userspace (such as udev or a watchdog daemon) opens /dev/watchdog
immediately after watchdog_register_device() exposes the character device,
starfive_wdt_pm_start() increments the runtime PM usage count. The
subsequent pm_runtime_put_sync() in probe then decrements the usage count
to 1 and returns -EAGAIN (it may also return 1, -EBUSY, or -EPERM in
non-error idle conditions), causing starfive_wdt_probe() to unregister the
working watchdog device and fail probe.

When early_enable is not set, no hardware registers are accessed after
starfive_wdt_stop(). Release the probe-time runtime PM reference right
after starfive_wdt_stop() and before watchdog_register_device(), without
treating pm_runtime_put_sync() idle return codes as probe errors.

Fixes: 8bc22a2f1bf0 ("watchdog: starfive: Check pm_runtime_enabled() before decrementing usage counter")
Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
Cc: Ziv Xu <ziv.xu@starfivetech.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/starfive-wdt.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 817f84cc2807..d3eb29c45e6a 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -499,28 +499,25 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
 	} else {
 		starfive_wdt_stop(wdt);
+		if (pm_runtime_enabled(&pdev->dev))
+			pm_runtime_put_sync(&pdev->dev);
 	}
 
 	ret = watchdog_register_device(&wdt->wdd);
-	if (ret)
-		goto err_exit;
-
-	if (!early_enable) {
-		if (pm_runtime_enabled(&pdev->dev)) {
-			ret = pm_runtime_put_sync(&pdev->dev);
-			if (ret)
-				goto err_unregister_wdt;
+	if (ret) {
+		if (watchdog_hw_running(&wdt->wdd)) {
+			starfive_wdt_stop(wdt);
+			if (pm_runtime_enabled(&pdev->dev))
+				pm_runtime_put_sync(&pdev->dev);
 		}
+		if (!pm_runtime_enabled(&pdev->dev))
+			starfive_wdt_disable_clock(wdt);
+		goto err_disable_pm;
 	}
 
 	return 0;
 
-err_unregister_wdt:
-	watchdog_unregister_device(&wdt->wdd);
-	goto err_disable_pm;
 err_exit:
-	if (watchdog_hw_running(&wdt->wdd))
-		starfive_wdt_stop(wdt);
 	if (pm_runtime_enabled(&pdev->dev))
 		pm_runtime_put_sync(&pdev->dev);
 	else
-- 
2.45.2

Re: [PATCH 5/5] watchdog: starfive-wdt: Drop runtime PM reference before registering watchdog

[Tzung-Bi Shih]

On Wed, Nov 04, 2026 at 08:44:45AM -0800, Guenter Roeck wrote:
> In starfive_wdt_probe(), watchdog_register_device() is called before the
> probe-time runtime PM reference is released via pm_runtime_put_sync(), and
> any non-zero return value from pm_runtime_put_sync() is treated as a fatal
> probe error.
> 
> If userspace (such as udev or a watchdog daemon) opens /dev/watchdog
> immediately after watchdog_register_device() exposes the character device,
> starfive_wdt_pm_start() increments the runtime PM usage count. The
> subsequent pm_runtime_put_sync() in probe then decrements the usage count
> to 1 and returns -EAGAIN (it may also return 1, -EBUSY, or -EPERM in
> non-error idle conditions), causing starfive_wdt_probe() to unregister the
> working watchdog device and fail probe.
> 
> When early_enable is not set, no hardware registers are accessed after
> starfive_wdt_stop(). Release the probe-time runtime PM reference right
> after starfive_wdt_stop() and before watchdog_register_device(), without
> treating pm_runtime_put_sync() idle return codes as probe errors.
> 
> Fixes: 8bc22a2f1bf0 ("watchdog: starfive: Check pm_runtime_enabled() before decrementing usage counter")
> Cc: Xingyu Wu <xingyu.wu@starfivetech.com>
> Cc: Ziv Xu <ziv.xu@starfivetech.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

The patch looks correct to me.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

> @@ -499,28 +499,25 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>  		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>  	} else {
>  		starfive_wdt_stop(wdt);
> +		if (pm_runtime_enabled(&pdev->dev))
> +			pm_runtime_put_sync(&pdev->dev);
>  	}
>  
>  	ret = watchdog_register_device(&wdt->wdd);
> -	if (ret)
> -		goto err_exit;
> -
> -	if (!early_enable) {
> -		if (pm_runtime_enabled(&pdev->dev)) {
> -			ret = pm_runtime_put_sync(&pdev->dev);
> -			if (ret)
> -				goto err_unregister_wdt;
> +	if (ret) {
> +		if (watchdog_hw_running(&wdt->wdd)) {
> +			starfive_wdt_stop(wdt);
> +			if (pm_runtime_enabled(&pdev->dev))
> +				pm_runtime_put_sync(&pdev->dev);
>  		}
> +		if (!pm_runtime_enabled(&pdev->dev))
> +			starfive_wdt_disable_clock(wdt);
> +		goto err_disable_pm;
>  	}
>  
>  	return 0;
>  
> -err_unregister_wdt:
> -	watchdog_unregister_device(&wdt->wdd);
> -	goto err_disable_pm;
>  err_exit:
> -	if (watchdog_hw_running(&wdt->wdd))
> -		starfive_wdt_stop(wdt);
>  	if (pm_runtime_enabled(&pdev->dev))
>  		pm_runtime_put_sync(&pdev->dev);
>  	else

Would this improve readability?

	ret = watchdog_register_device(&wdt->wdd);
	if (ret) {
		if (early_enable) {
			starfive_wdt_stop(wdt);
			goto err_exit;
		} else {
			if (!pm_runtime_enabled(&pdev->dev))
				starfive_wdt_disable_clock(wdt);
		}
		goto err_disable_pm;
	}