[PATCH v3 4/6] watchdog: starfive: guard system suspend/resume hardware access

[William Theesfeld]

starfive_wdt_suspend() reads from STARFIVE_WDT_VALUE and writes to
WDOGCONTROL via starfive_wdt_get_count() and starfive_wdt_stop()
before calling pm_runtime_force_suspend().  If the device was already
runtime-suspended when system suspend began, the apb and core clocks
are gated and those register accesses produce a synchronous external
abort.

starfive_wdt_resume() has the mirror problem: it unconditionally
restores the WDOGLOAD value after pm_runtime_force_resume() returns,
but pm_runtime_force_resume() leaves the device in whichever runtime
PM state it was in pre-suspend.  If the device stays suspended, the
restore writes race with gated clocks.

Gate both halves with pm_runtime_status_suspended() so the hardware
is only touched when it is actually active.  When the watchdog was
suspended at runtime PM level there is no state to save or restore.

Additionally, restart the hardware on resume for the early_enable
case (WDOG_HW_RUNNING set without WDOG_ACTIVE).  Without this, a
system suspend stops the early-enabled hardware via the suspend
callback above, but the resume callback only restarts when
watchdog_active() is true, leaving the watchdog permanently stopped
after the suspend/resume cycle.

Signed-off-by: William Theesfeld <william@xxxxxxxxxxxxx>
---
 drivers/watchdog/starfive-wdt.c | 34 ++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
index 856e55f04..e3a47c0c3 100644
--- a/drivers/watchdog/starfive-wdt.c
+++ b/drivers/watchdog/starfive-wdt.c
@@ -564,11 +564,16 @@ static int starfive_wdt_suspend(struct device *dev)
 {
 	struct starfive_wdt *wdt = dev_get_drvdata(dev);
 
-	/* Save watchdog state, and turn it off. */
-	wdt->reload = starfive_wdt_get_count(wdt);
-
-	/* Note that WTCNT doesn't need to be saved. */
-	starfive_wdt_stop(wdt);
+	/*
+	 * Only access the hardware while the device is runtime-resumed; if
+	 * runtime PM has already suspended the device, its clocks are gated
+	 * and a register read/write would trigger a synchronous external
+	 * abort.  WTCNT does not need to be saved.
+	 */
+	if (!pm_runtime_status_suspended(dev)) {
+		wdt->reload = starfive_wdt_get_count(wdt);
+		starfive_wdt_stop(wdt);
+	}
 
 	return pm_runtime_force_suspend(dev);
 }
@@ -582,12 +587,27 @@ static int starfive_wdt_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	/*
+	 * pm_runtime_force_resume() leaves the device in whichever runtime
+	 * PM state it was in before system suspend.  Only restore hardware
+	 * state when the device is actually resumed; otherwise the register
+	 * writes below would race with gated clocks.
+	 */
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
 	starfive_wdt_unlock(wdt);
-	/* Restore watchdog state. */
 	starfive_wdt_set_reload_count(wdt, wdt->reload);
 	starfive_wdt_lock(wdt);
 
-	if (watchdog_active(&wdt->wdd))
+	/*
+	 * Restart the hardware on resume for both userspace-opened
+	 * watchdogs (WDOG_ACTIVE) and watchdogs started via early_enable
+	 * (WDOG_HW_RUNNING).  Checking only WDOG_ACTIVE leaves the
+	 * early_enable case stopped after a suspend/resume cycle even
+	 * though the framework still considers the hardware running.
+	 */
+	if (watchdog_active(&wdt->wdd) || watchdog_hw_running(&wdt->wdd))
 		return starfive_wdt_start(wdt);
 
 	return 0;
-- 
2.54.0