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

[PATCH v2 0/8] 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, ioctl/sysfs, 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_shutdown() duplicates watchdog_stop_on_reboot() (causing
   unbalanced runtime PM puts when not runtime-suspended and ignoring
   watchdog.stop_on_reboot=0), and 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.
6. In starfive_wdt_get_timeleft(), reading the sysfs timeleft attribute
   or issuing ioctl(WDIOC_GETTIMELEFT) while the watchdog is inactive
   reads hardware registers while clocks are gated off.
7. In starfive_wdt_probe(), omitting watchdog_stop_ping_on_suspend()
   leaves the non-freezable watchdog ping worker active across system
   suspend when early_enable=1 is used, risking unclocked MMIO in
   starfive_wdt_keepalive() after starfive_wdt_suspend() gates clocks.
8. In starfive_wdt_set_timeout(), issuing ioctl(WDIOC_SETTIMEOUT) after
   stopping the watchdog via WDIOS_DISABLECARD accesses unclocked MMIO
   registers (when runtime-suspended) or unexpectedly re-enables the
   hardware counter while inactive (when runtime PM is disabled).

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

Changes in v2:
- Patch 1, 2, 4, and 5: Added Reviewed-by: tags
- Patch 3: Remove the redundant starfive_wdt_shutdown() callback instead
  of guarding it with !pm_runtime_suspended(), avoiding unbalanced
  pm_runtime_put_sync() calls when runtime suspend is forbidden or after
  watchdog_reboot_notifier() already stopped the watchdog, avoiding
  unclocked MMIO during hibernation shutdown, and respecting the
  watchdog.stop_on_reboot module parameter.
- Patch 6 (new): Return 0 in starfive_wdt_get_timeleft() when
  !watchdog_hw_running() to avoid unclocked MMIO reads.
- Patch 7 (new): Call watchdog_stop_ping_on_suspend() in
  starfive_wdt_probe() so the ping worker does not call
  starfive_wdt_keepalive() while clocks are gated during suspend.
- Patch 8 (new): Only program hardware registers in
  starfive_wdt_set_timeout() when watchdog_hw_running() is set, and
  reload the preset counter in starfive_wdt_start() when starting from
  probe or after WDIOS_DISABLECARD.

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, ioctl/sysfs, suspend/shutdown, and
concurrent open paths; they were not tested on physical StarFive
JH7100/JH7110 hardware.

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

Guenter Roeck (8):
  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
  watchdog: starfive-wdt: Avoid unclocked MMIO read in
    starfive_wdt_get_timeleft()
  watchdog: starfive-wdt: Stop ping worker on system suspend
  watchdog: starfive-wdt: Avoid unclocked MMIO access in
    starfive_wdt_set_timeout()

 drivers/watchdog/starfive-wdt.c | 102 ++++++++++++++++++--------------
 1 file changed, 58 insertions(+), 44 deletions(-)

-- 
2.56.0

[PATCH v2 1/8] 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>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add Reviewed-by: tag

 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

[PATCH v2 2/8] 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>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add Reviewed-by: tag

 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

[PATCH v2 3/8] 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(), check pm_runtime_enabled() and ignore
  non-zero idle return codes from pm_runtime_put_sync() so watchdog_stop()
  does not falsely treat a stopped watchdog as still running.
- Remove the redundant starfive_wdt_shutdown() callback. Because
  starfive_wdt_probe() already sets watchdog_stop_on_reboot(),
  watchdog_reboot_notifier() stops a running watchdog on reboot, halt, or
  power-off (while respecting the watchdog.stop_on_reboot parameter)
  before device_shutdown() runs; calling starfive_wdt_pm_stop() again in
  .shutdown causes unclocked MMIO when suspended and an unbalanced
  pm_runtime_put_sync() when inactive or already stopped.
- 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>
---
v2: Remove the redundant starfive_wdt_shutdown() callback instead
    of guarding it with !pm_runtime_suspended(), avoiding unbalanced
    pm_runtime_put_sync() calls when runtime suspend is forbidden or after
    watchdog_reboot_notifier() already stopped the watchdog, avoiding
    unclocked MMIO during hibernation shutdown, and respecting the
    watchdog.stop_on_reboot module parameter.

 drivers/watchdog/starfive-wdt.c | 38 ++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 4724fccbfe63..c5f044c9d03c 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
@@ -536,22 +541,17 @@ static void starfive_wdt_remove(struct platform_device *pdev)
 		starfive_wdt_disable_clock(wdt);
 }
 
-static void starfive_wdt_shutdown(struct platform_device *pdev)
-{
-	struct starfive_wdt *wdt = platform_get_drvdata(pdev);
-
-	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 +565,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;
 }
@@ -607,7 +608,6 @@ MODULE_DEVICE_TABLE(of, starfive_wdt_match);
 static struct platform_driver starfive_wdt_driver = {
 	.probe = starfive_wdt_probe,
 	.remove = starfive_wdt_remove,
-	.shutdown = starfive_wdt_shutdown,
 	.driver = {
 		.name = "starfive-wdt",
 		.pm = pm_ptr(&starfive_wdt_pm_ops),
-- 
2.45.2

[PATCH v2 4/8] 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>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add Reviewed-by: tag

 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 c5f044c9d03c..859232fa55a3 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

[PATCH v2 5/8] 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>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add Reviewed-by: tag

 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 859232fa55a3..4142e1502038 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

[PATCH v2 6/8] watchdog: starfive-wdt: Avoid unclocked MMIO read in starfive_wdt_get_timeleft()

[Guenter Roeck]

When the watchdog is inactive, runtime PM suspends the device and gates
off apb_clk and core_clk via starfive_wdt_runtime_suspend(). However,
reading the sysfs timeleft attribute via timeleft_show() or issuing
ioctl(WDIOC_GETTIMELEFT) calls starfive_wdt_get_timeleft() unconditionally.

Because starfive_wdt_get_timeleft() reads the counter and interrupt status
registers without checking whether the hardware watchdog is running,
querying timeleft while inactive performs unclocked MMIO reads and triggers
a bus fault.

Return 0 immediately in starfive_wdt_get_timeleft() when
watchdog_hw_running() is false. When watchdog_hw_running() is true, the
driver holds an active runtime PM reference keeping the clocks enabled.

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>
---
v2: New patch

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

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 4142e1502038..60a97ffa940a 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -312,6 +312,9 @@ static unsigned int starfive_wdt_get_timeleft(struct watchdog_device *wdd)
 	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
 	u32 count;
 
+	if (!watchdog_hw_running(wdd))
+		return 0;
+
 	/*
 	 * If the watchdog takes twice timeout and set half count value,
 	 * timeleft value should add the count value before first timeout.
-- 
2.45.2

[PATCH v2 7/8] watchdog: starfive-wdt: Stop ping worker on system suspend

[Guenter Roeck]

When early_enable=1 is set and CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is
enabled, watchdog_register_device() starts the watchdog core hrtimer so the
non-freezable watchdogd kthread worker periodically pings the hardware
before userspace opens /dev/watchdog.

Because starfive_wdt_probe() does not call watchdog_stop_ping_on_suspend(),
WDOG_NO_PING_ON_SUSPEND is not set and the watchdog core does not cancel
its hrtimer and ping work across system suspend. After
starfive_wdt_suspend() stops the counter and calls
pm_runtime_force_suspend() to gate core_clk and apb_clk, WDOG_HW_RUNNING
remains set; if the timer fires before starfive_wdt_resume() re-enables
the clocks, watchdog_ping_work() invokes starfive_wdt_keepalive() with
gated clocks and causes an unclocked MMIO bus fault.

Call watchdog_stop_ping_on_suspend() in starfive_wdt_probe() so the
watchdog core cancels its ping timer and work in PM_SUSPEND_PREPARE before
clocks are gated and resumes pinging in PM_POST_SUSPEND after clocks are
restored.

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>
---
v2: New patch

 drivers/watchdog/starfive-wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 60a97ffa940a..ae2d84ccc76f 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -494,6 +494,7 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 	watchdog_set_nowayout(&wdt->wdd, nowayout);
 	watchdog_stop_on_reboot(&wdt->wdd);
 	watchdog_stop_on_unregister(&wdt->wdd);
+	watchdog_stop_ping_on_suspend(&wdt->wdd);
 
 	if (early_enable) {
 		ret = starfive_wdt_start(wdt);
-- 
2.45.2

[PATCH v2 8/8] watchdog: starfive-wdt: Avoid unclocked MMIO access in starfive_wdt_set_timeout()

[Guenter Roeck]

When the watchdog is stopped via ioctl(WDIOC_SETOPTIONS, WDIOS_DISABLECARD)
while /dev/watchdog remains open, watchdog_stop() clears WDOG_HW_RUNNING
and WDOG_ACTIVE, and starfive_wdt_pm_stop() halts the hardware counter and
drops the runtime PM reference, gating core_clk and apb_clk.

A subsequent ioctl(WDIOC_SETTIMEOUT) invokes starfive_wdt_set_timeout()
regardless of whether the watchdog is running. Because
starfive_wdt_set_timeout() unconditionally accesses MMIO registers and
calls starfive_wdt_enable(), updating the timeout while stopped triggers an
unclocked MMIO bus fault when runtime-suspended, or unexpectedly starts the
hardware counter without WDOG_ACTIVE or WDOG_HW_RUNNING set when runtime PM
is disabled.

Only program the hardware registers in starfive_wdt_set_timeout() when
watchdog_hw_running() is set. In addition, reload the initial counter value
via starfive_wdt_set_reload_count() in starfive_wdt_start() (preserving
wdt->reload on resume) so JH7100 loads the configured timeout into the
counter when started from probe or after WDIOS_DISABLECARD.

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>
---
v2: New patch

 drivers/watchdog/starfive-wdt.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index ae2d84ccc76f..758030ad9c65 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -344,7 +344,7 @@ static int starfive_wdt_keepalive(struct watchdog_device *wdd)
 	return ret;
 }
 
-static int starfive_wdt_start(struct starfive_wdt *wdt)
+static int starfive_wdt_start(struct starfive_wdt *wdt, u32 count)
 {
 	int ret;
 
@@ -357,7 +357,9 @@ static int starfive_wdt_start(struct starfive_wdt *wdt)
 	if (ret)
 		goto exit;
 
-	starfive_wdt_set_count(wdt, wdt->count);
+	starfive_wdt_set_reload_count(wdt, count);
+	if (count != wdt->count)
+		starfive_wdt_set_count(wdt, wdt->count);
 	starfive_wdt_enable(wdt);
 
 exit:
@@ -381,7 +383,7 @@ static int starfive_wdt_pm_start(struct watchdog_device *wdd)
 	if (ret < 0)
 		return ret;
 
-	ret = starfive_wdt_start(wdt);
+	ret = starfive_wdt_start(wdt, wdt->count);
 	if (ret)
 		pm_runtime_put_sync(wdd->parent);
 
@@ -412,11 +414,13 @@ static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
 	wdt->count = count;
 	wdd->timeout = timeout;
 
-	starfive_wdt_unlock(wdt);
-	starfive_wdt_disable(wdt);
-	starfive_wdt_set_reload_count(wdt, wdt->count);
-	starfive_wdt_enable(wdt);
-	starfive_wdt_lock(wdt);
+	if (watchdog_hw_running(wdd)) {
+		starfive_wdt_unlock(wdt);
+		starfive_wdt_disable(wdt);
+		starfive_wdt_set_reload_count(wdt, wdt->count);
+		starfive_wdt_enable(wdt);
+		starfive_wdt_lock(wdt);
+	}
 
 	return 0;
 }
@@ -497,7 +501,7 @@ static int starfive_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_ping_on_suspend(&wdt->wdd);
 
 	if (early_enable) {
-		ret = starfive_wdt_start(wdt);
+		ret = starfive_wdt_start(wdt, wdt->count);
 		if (ret)
 			goto err_exit;
 		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
@@ -575,14 +579,8 @@ static int starfive_wdt_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	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);
-
-		return starfive_wdt_start(wdt);
-	}
+	if (watchdog_hw_running(&wdt->wdd))
+		return starfive_wdt_start(wdt, wdt->reload);
 
 	return 0;
 }
-- 
2.45.2

Re: [PATCH v2 8/8] watchdog: starfive-wdt: Avoid unclocked MMIO access in starfive_wdt_set_timeout()

[Tzung-Bi Shih]

On Thu, Nov 05, 2026 at 11:05:29AM -0800, Guenter Roeck wrote:
> Only program the hardware registers in starfive_wdt_set_timeout() when
> watchdog_hw_running() is set. In addition, reload the initial counter value
> via starfive_wdt_set_reload_count() in starfive_wdt_start() (preserving

Is the additional change relevant to this patch (i.e., avoid unclocked
MMIO access)?

> wdt->reload on resume) so JH7100 loads the configured timeout into the
> counter when started from probe or after WDIOS_DISABLECARD.

Is it really JH7100 specific (not for JH7110)?

> @@ -344,7 +344,7 @@ static int starfive_wdt_keepalive(struct watchdog_device *wdd)
>  	return ret;
>  }
>  
> -static int starfive_wdt_start(struct starfive_wdt *wdt)
> +static int starfive_wdt_start(struct starfive_wdt *wdt, u32 count)
>  {
>  	int ret;
>  
> @@ -357,7 +357,9 @@ static int starfive_wdt_start(struct starfive_wdt *wdt)
>  	if (ret)
>  		goto exit;
>  
> -	starfive_wdt_set_count(wdt, wdt->count);
> +	starfive_wdt_set_reload_count(wdt, count);
> +	if (count != wdt->count)
> +		starfive_wdt_set_count(wdt, wdt->count);
>  	starfive_wdt_enable(wdt);
>  
>  exit:
> @@ -381,7 +383,7 @@ static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = starfive_wdt_start(wdt);
> +	ret = starfive_wdt_start(wdt, wdt->count);
>  	if (ret)
>  		pm_runtime_put_sync(wdd->parent);
>  
> @@ -412,11 +414,13 @@ static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
>  	wdt->count = count;
>  	wdd->timeout = timeout;
>  
> -	starfive_wdt_unlock(wdt);
> -	starfive_wdt_disable(wdt);
> -	starfive_wdt_set_reload_count(wdt, wdt->count);
> -	starfive_wdt_enable(wdt);
> -	starfive_wdt_lock(wdt);
> +	if (watchdog_hw_running(wdd)) {
> +		starfive_wdt_unlock(wdt);
> +		starfive_wdt_disable(wdt);
> +		starfive_wdt_set_reload_count(wdt, wdt->count);
> +		starfive_wdt_enable(wdt);
> +		starfive_wdt_lock(wdt);
> +	}
>  
>  	return 0;
>  }
> @@ -497,7 +501,7 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>  	watchdog_stop_ping_on_suspend(&wdt->wdd);
>  
>  	if (early_enable) {
> -		ret = starfive_wdt_start(wdt);
> +		ret = starfive_wdt_start(wdt, wdt->count);
>  		if (ret)
>  			goto err_exit;
>  		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> @@ -575,14 +579,8 @@ static int starfive_wdt_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	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);
> -
> -		return starfive_wdt_start(wdt);
> -	}
> +	if (watchdog_hw_running(&wdt->wdd))
> +		return starfive_wdt_start(wdt, wdt->reload);
>  
>  	return 0;
>  }

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

[Tzung-Bi Shih]

On Thu, Nov 05, 2026 at 11:05:24AM -0800, Guenter Roeck wrote:
> 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(), check pm_runtime_enabled() and ignore
>   non-zero idle return codes from pm_runtime_put_sync() so watchdog_stop()
>   does not falsely treat a stopped watchdog as still running.
> - Remove the redundant starfive_wdt_shutdown() callback. Because
>   starfive_wdt_probe() already sets watchdog_stop_on_reboot(),
>   watchdog_reboot_notifier() stops a running watchdog on reboot, halt, or
>   power-off (while respecting the watchdog.stop_on_reboot parameter)
>   before device_shutdown() runs; calling starfive_wdt_pm_stop() again in
>   .shutdown causes unclocked MMIO when suspended and an unbalanced
>   pm_runtime_put_sync() when inactive or already stopped.
> - 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>

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

Re: [PATCH v2 6/8] watchdog: starfive-wdt: Avoid unclocked MMIO read in starfive_wdt_get_timeleft()

[Tzung-Bi Shih]

On Thu, Nov 05, 2026 at 11:05:27AM -0800, Guenter Roeck wrote:
> When the watchdog is inactive, runtime PM suspends the device and gates
> off apb_clk and core_clk via starfive_wdt_runtime_suspend(). However,
> reading the sysfs timeleft attribute via timeleft_show() or issuing
> ioctl(WDIOC_GETTIMELEFT) calls starfive_wdt_get_timeleft() unconditionally.
> 
> Because starfive_wdt_get_timeleft() reads the counter and interrupt status
> registers without checking whether the hardware watchdog is running,
> querying timeleft while inactive performs unclocked MMIO reads and triggers
> a bus fault.
> 
> Return 0 immediately in starfive_wdt_get_timeleft() when
> watchdog_hw_running() is false. When watchdog_hw_running() is true, the
> driver holds an active runtime PM reference keeping the clocks enabled.
> 
> 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>

Re: [PATCH v2 7/8] watchdog: starfive-wdt: Stop ping worker on system suspend

[Tzung-Bi Shih]

On Thu, Nov 05, 2026 at 11:05:28AM -0800, Guenter Roeck wrote:
> When early_enable=1 is set and CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is
> enabled, watchdog_register_device() starts the watchdog core hrtimer so the
> non-freezable watchdogd kthread worker periodically pings the hardware
> before userspace opens /dev/watchdog.
> 
> Because starfive_wdt_probe() does not call watchdog_stop_ping_on_suspend(),
> WDOG_NO_PING_ON_SUSPEND is not set and the watchdog core does not cancel
> its hrtimer and ping work across system suspend. After
> starfive_wdt_suspend() stops the counter and calls
> pm_runtime_force_suspend() to gate core_clk and apb_clk, WDOG_HW_RUNNING
> remains set; if the timer fires before starfive_wdt_resume() re-enables
> the clocks, watchdog_ping_work() invokes starfive_wdt_keepalive() with
> gated clocks and causes an unclocked MMIO bus fault.
> 
> Call watchdog_stop_ping_on_suspend() in starfive_wdt_probe() so the
> watchdog core cancels its ping timer and work in PM_SUSPEND_PREPARE before
> clocks are gated and resumes pinging in PM_POST_SUSPEND after clocks are
> restored.
> 
> 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>

Re: [PATCH v2 8/8] watchdog: starfive-wdt: Avoid unclocked MMIO access in starfive_wdt_set_timeout()

[Guenter Roeck]

On 11/6/26 06:21, Tzung-Bi Shih wrote:
> On Thu, Nov 05, 2026 at 11:05:29AM -0800, Guenter Roeck wrote:
>> Only program the hardware registers in starfive_wdt_set_timeout() when
>> watchdog_hw_running() is set. In addition, reload the initial counter value
>> via starfive_wdt_set_reload_count() in starfive_wdt_start() (preserving
> 
> Is the additional change relevant to this patch (i.e., avoid unclocked
> MMIO access)?

I think so, since reload_count is no longer set in starfive_wdt_set_timeout()
if the watchdog is not running.

> 
>> wdt->reload on resume) so JH7100 loads the configured timeout into the
>> counter when started from probe or after WDIOS_DISABLECARD.
> 
> Is it really JH7100 specific (not for JH7110)?
> 

Yes, the other chip doesn't have a reload count register.
See "if (wdt->variant->reload)" in starfive_wdt_set_reload_count().

Thanks,
Guenter

>> @@ -344,7 +344,7 @@ static int starfive_wdt_keepalive(struct watchdog_device *wdd)
>>   	return ret;
>>   }
>>   
>> -static int starfive_wdt_start(struct starfive_wdt *wdt)
>> +static int starfive_wdt_start(struct starfive_wdt *wdt, u32 count)
>>   {
>>   	int ret;
>>   
>> @@ -357,7 +357,9 @@ static int starfive_wdt_start(struct starfive_wdt *wdt)
>>   	if (ret)
>>   		goto exit;
>>   
>> -	starfive_wdt_set_count(wdt, wdt->count);
>> +	starfive_wdt_set_reload_count(wdt, count);
>> +	if (count != wdt->count)
>> +		starfive_wdt_set_count(wdt, wdt->count);
>>   	starfive_wdt_enable(wdt);
>>   
>>   exit:
>> @@ -381,7 +383,7 @@ static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	ret = starfive_wdt_start(wdt);
>> +	ret = starfive_wdt_start(wdt, wdt->count);
>>   	if (ret)
>>   		pm_runtime_put_sync(wdd->parent);
>>   
>> @@ -412,11 +414,13 @@ static int starfive_wdt_set_timeout(struct watchdog_device *wdd,
>>   	wdt->count = count;
>>   	wdd->timeout = timeout;
>>   
>> -	starfive_wdt_unlock(wdt);
>> -	starfive_wdt_disable(wdt);
>> -	starfive_wdt_set_reload_count(wdt, wdt->count);
>> -	starfive_wdt_enable(wdt);
>> -	starfive_wdt_lock(wdt);
>> +	if (watchdog_hw_running(wdd)) {
>> +		starfive_wdt_unlock(wdt);
>> +		starfive_wdt_disable(wdt);
>> +		starfive_wdt_set_reload_count(wdt, wdt->count);
>> +		starfive_wdt_enable(wdt);
>> +		starfive_wdt_lock(wdt);
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -497,7 +501,7 @@ static int starfive_wdt_probe(struct platform_device *pdev)
>>   	watchdog_stop_ping_on_suspend(&wdt->wdd);
>>   
>>   	if (early_enable) {
>> -		ret = starfive_wdt_start(wdt);
>> +		ret = starfive_wdt_start(wdt, wdt->count);
>>   		if (ret)
>>   			goto err_exit;
>>   		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>> @@ -575,14 +579,8 @@ static int starfive_wdt_resume(struct device *dev)
>>   	if (ret)
>>   		return ret;
>>   
>> -	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);
>> -
>> -		return starfive_wdt_start(wdt);
>> -	}
>> +	if (watchdog_hw_running(&wdt->wdd))
>> +		return starfive_wdt_start(wdt, wdt->reload);
>>   
>>   	return 0;
>>   }

Re: [PATCH v2 8/8] watchdog: starfive-wdt: Avoid unclocked MMIO access in starfive_wdt_set_timeout()

[Tzung-Bi Shih]

On Fri, Nov 06, 2026 at 07:20:32AM -0800, Guenter Roeck wrote:
> On 11/6/26 06:21, Tzung-Bi Shih wrote:
> > On Thu, Nov 05, 2026 at 11:05:29AM -0800, Guenter Roeck wrote:
> > > Only program the hardware registers in starfive_wdt_set_timeout() when
> > > watchdog_hw_running() is set. In addition, reload the initial counter value
> > > via starfive_wdt_set_reload_count() in starfive_wdt_start() (preserving
> > 
> > Is the additional change relevant to this patch (i.e., avoid unclocked
> > MMIO access)?
> 
> I think so, since reload_count is no longer set in starfive_wdt_set_timeout()
> if the watchdog is not running.

Ah, I see.

> 
> > 
> > > wdt->reload on resume) so JH7100 loads the configured timeout into the
> > > counter when started from probe or after WDIOS_DISABLECARD.
> > 
> > Is it really JH7100 specific (not for JH7110)?
> > 
> 
> Yes, the other chip doesn't have a reload count register.
> See "if (wdt->variant->reload)" in starfive_wdt_set_reload_count().

I see.

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