[PATCH] watchdog: sbsa_gwdt: stop the watchdog across the whole system-sleep transition

[David Cemin <dcemin@nvidia.com>]

The driver stops a running watchdog in its own device suspend callback
and restarts it in its resume callback. That leaves the watchdog armed,
with nobody refreshing it, for the entire early part of suspend entry:
userspace freeze, kernel thread freeze, and every device suspend
callback that runs before this device's own. The same window exists at
the tail end of resume.

When the watchdog is already running when the driver binds (started by
firmware, 10 s default timeout) and any device stalls its suspend
callback past the timeout, the watchdog resets the system in the middle
of suspend entry. On an arm64 laptop platform this fired on about 7% of
suspend attempts in a randomized suspend stress run (9 resets in 124
suspends, with the watchdog reset status set in the SoC's reset status
register). Two elimination runs confirm the mechanism: the identical
stress matrix with the watchdog stopped produced zero resets in 118
suspends, and with the first version of this change (notifier plus the
original device callbacks) applied, zero resets in 198 suspends across
four runs, where the baseline rate predicts about 14. The version here
keeps that mechanism, removes the device resume callback and adds the
locking described below; it went through a further 120 suspends (60
s2idle, 60 S3, randomized order, console recorded through every entry)
with the watchdog armed from boot and zero resets.

Stop the watchdog from a PM notifier at the *_PREPARE events, before
tasks are frozen and device callbacks run, and restart it at the
PM_POST_* events, after everything has resumed. The driver state (armed,
stopped for sleep) lives under a lock shared with the watchdog ops, so a
userspace stop or magic close after thaw cannot race the restart, and a
start requested while the transition is in progress is deferred until
PM_POST_* instead of arming hardware nobody can refresh; the transition
is recorded at *_PREPARE whether or not the watchdog was armed at that
point, so a start between *_PREPARE and task freezing is deferred as
well. The notifier is
registered before anything can arm the watchdog and its failure fails
the probe. A suspend-only device callback remains as the final guard for
a device whose probe overlapped the *_PREPARE event; it has no resume
counterpart, so nothing re-arms the watchdog during device resume,
before PM_POST_SUSPEND. The initial hardware state is adopted under the
same lock, so a firmware-started watchdog discovered by a probe that
lost the race with *_PREPARE is stopped at once and armed again at
PM_POST_*; the stop path is idempotent so the device callback remains
an effective fallback whatever the ordering.

Fixes: 57d2caaabfc7 ("Watchdog: introduce ARM SBSA watchdog driver")
Signed-off-by: David Cemin <dcemin@nvidia.com>
---
 drivers/watchdog/sbsa_gwdt.c | 191 +++++++++++++++++++++++++++++------
 1 file changed, 159 insertions(+), 32 deletions(-)

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index e04d42cc7774..399a8bcb3c6f 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c
@@ -46,6 +46,8 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/suspend.h>
 #include <linux/uaccess.h>
 #include <linux/watchdog.h>
 #include <asm/arch_timer.h>
@@ -87,6 +89,13 @@
  *			indicate whether to adjust wdd->timeout to avoid a race with WS0
  * @refresh_base:	Virtual address of the watchdog refresh frame
  * @control_base:	Virtual address of the watchdog control frame
+ * @lock:		Serializes the watchdog ops against the system sleep hooks
+ * @hw_armed:		The watchdog is logically running (started by firmware,
+ *			or userspace); the hardware follows it except
+ *			while a system sleep transition is in progress
+ * @sleeping:		A system sleep transition is in progress: the hardware
+ *			stays stopped and hw_armed is applied at PM_POST_*
+ * @pm_nb:		PM notifier stopping the watchdog across system sleep
  */
 struct sbsa_gwdt {
 	struct watchdog_device	wdd;
@@ -95,6 +104,10 @@ struct sbsa_gwdt {
 	bool			need_ws0_race_workaround;
 	void __iomem		*refresh_base;
 	void __iomem		*control_base;
+	spinlock_t		lock; /* hw_armed, sleeping */
+	bool			hw_armed;
+	bool			sleeping;
+	struct notifier_block	pm_nb;
 };
 
 #define DEFAULT_TIMEOUT		10 /* seconds */
@@ -244,12 +257,33 @@ static void sbsa_gwdt_get_version(struct watchdog_device *wdd)
 		!action && (impl == SBSA_GWDT_IMPL_MEDIATEK);
 }
 
+static void sbsa_gwdt_hw_start(struct sbsa_gwdt *gwdt)
+{
+	/* writing WCS will cause an explicit watchdog refresh */
+	writel(SBSA_GWDT_WCS_EN, gwdt->control_base + SBSA_GWDT_WCS);
+}
+
+static void sbsa_gwdt_hw_stop(struct sbsa_gwdt *gwdt)
+{
+	/* Simply write 0 to WCS to clean WCS_EN bit */
+	writel(0, gwdt->control_base + SBSA_GWDT_WCS);
+}
+
 static int sbsa_gwdt_start(struct watchdog_device *wdd)
 {
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
+	unsigned long flags;
 
-	/* writing WCS will cause an explicit watchdog refresh */
-	writel(SBSA_GWDT_WCS_EN, gwdt->control_base + SBSA_GWDT_WCS);
+	spin_lock_irqsave(&gwdt->lock, flags);
+	gwdt->hw_armed = true;
+	/*
+	 * While a system sleep transition is in progress nobody can refresh
+	 * the watchdog: leave the hardware stopped and let the PM_POST_*
+	 * notifier arm it once everything has resumed.
+	 */
+	if (!gwdt->sleeping)
+		sbsa_gwdt_hw_start(gwdt);
+	spin_unlock_irqrestore(&gwdt->lock, flags);
 
 	return 0;
 }
@@ -257,9 +291,12 @@ static int sbsa_gwdt_start(struct watchdog_device *wdd)
 static int sbsa_gwdt_stop(struct watchdog_device *wdd)
 {
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
+	unsigned long flags;
 
-	/* Simply write 0 to WCS to clean WCS_EN bit */
-	writel(0, gwdt->control_base + SBSA_GWDT_WCS);
+	spin_lock_irqsave(&gwdt->lock, flags);
+	gwdt->hw_armed = false;
+	sbsa_gwdt_hw_stop(gwdt);
+	spin_unlock_irqrestore(&gwdt->lock, flags);
 
 	return 0;
 }
@@ -288,12 +325,101 @@ static const struct watchdog_ops sbsa_gwdt_ops = {
 	.get_timeleft	= sbsa_gwdt_get_timeleft,
 };
 
+/*
+ * Per-device suspend/resume callbacks alone would stop the watchdog only
+ * once this device itself is suspended, one of the last steps of suspend
+ * entry, and restart it during device resume, before tasks are thawed. A
+ * watchdog running from boot (started by firmware) would therefore be armed, with
+ * nobody refreshing it, through task freezing and every other device's
+ * suspend callback on the way down, and again from device resume until
+ * userspace runs on the way up; anything stalling past the timeout in
+ * either window resets the system.
+ *
+ * Own the transition from a PM notifier instead: stop at the *_PREPARE
+ * events, before anything is frozen, and restart at PM_POST_*, after
+ * everything has resumed. The driver state (hw_armed, sleeping) is kept
+ * under a lock shared with the watchdog ops so that a userspace stop or
+ * magic close after thaw cannot race the restart, and a start requested
+ * while the transition is in progress is deferred to PM_POST_*. The
+ * transition is recorded at *_PREPARE whether or not the watchdog was
+ * armed at that moment, so a start between *_PREPARE and task freezing
+ * is deferred as well instead of arming hardware nobody refreshes. A
+ * suspend-only device callback remains as the final guard for a device
+ * whose probe overlapped the *_PREPARE event; it has no resume
+ * counterpart, so nothing re-arms the hardware before PM_POST_*.
+ */
+static void sbsa_gwdt_sleep_stop(struct sbsa_gwdt *gwdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&gwdt->lock, flags);
+	/*
+	 * Idempotent on purpose: the *_PREPARE notifier and the device
+	 * suspend callback both land here, and a probe that adopted a
+	 * firmware-armed watchdog after *_PREPARE relies on the second
+	 * call actually stopping the hardware.
+	 */
+	gwdt->sleeping = true;
+	if (gwdt->hw_armed)
+		sbsa_gwdt_hw_stop(gwdt);
+	spin_unlock_irqrestore(&gwdt->lock, flags);
+}
+
+static void sbsa_gwdt_sleep_restart(struct sbsa_gwdt *gwdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&gwdt->lock, flags);
+	if (gwdt->sleeping) {
+		gwdt->sleeping = false;
+		if (gwdt->hw_armed)
+			sbsa_gwdt_hw_start(gwdt);
+	}
+	spin_unlock_irqrestore(&gwdt->lock, flags);
+}
+
+static int sbsa_gwdt_pm_notify(struct notifier_block *nb, unsigned long mode,
+			       void *data)
+{
+	struct sbsa_gwdt *gwdt = container_of(nb, struct sbsa_gwdt, pm_nb);
+
+	switch (mode) {
+	case PM_SUSPEND_PREPARE:
+	case PM_HIBERNATION_PREPARE:
+	case PM_RESTORE_PREPARE:
+		sbsa_gwdt_sleep_stop(gwdt);
+		break;
+	case PM_POST_SUSPEND:
+	case PM_POST_HIBERNATION:
+	case PM_POST_RESTORE:
+		sbsa_gwdt_sleep_restart(gwdt);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static void sbsa_gwdt_unregister_pm_notifier(void *data)
+{
+	unregister_pm_notifier(data);
+}
+
+static int sbsa_gwdt_suspend(struct device *dev)
+{
+	sbsa_gwdt_sleep_stop(dev_get_drvdata(dev));
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(sbsa_gwdt_pm_ops, sbsa_gwdt_suspend, NULL);
+
 static int sbsa_gwdt_probe(struct platform_device *pdev)
 {
 	void __iomem *rf_base, *cf_base;
 	struct device *dev = &pdev->dev;
 	struct watchdog_device *wdd;
 	struct sbsa_gwdt *gwdt;
+	unsigned long flags;
 	int ret, irq;
 	u32 status;
 
@@ -318,6 +444,21 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	gwdt->clk = arch_timer_get_cntfrq();
 	gwdt->refresh_base = rf_base;
 	gwdt->control_base = cf_base;
+	spin_lock_init(&gwdt->lock);
+
+	/*
+	 * Register the sleep hook before anything can arm the watchdog, and
+	 * treat its failure as fatal: without it a running watchdog would
+	 * survive into system sleep with nobody refreshing it.
+	 */
+	gwdt->pm_nb.notifier_call = sbsa_gwdt_pm_notify;
+	ret = register_pm_notifier(&gwdt->pm_nb);
+	if (!ret)
+		ret = devm_add_action_or_reset(dev,
+					       sbsa_gwdt_unregister_pm_notifier,
+					       &gwdt->pm_nb);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register PM notifier\n");
 
 	wdd = &gwdt->wdd;
 	wdd->parent = dev;
@@ -347,8 +488,20 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 		dev_warn(dev, "System reset by WDT.\n");
 		wdd->bootstatus |= WDIOF_CARDRESET;
 	}
-	if (status & SBSA_GWDT_WCS_EN)
+	if (status & SBSA_GWDT_WCS_EN) {
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
+		/*
+		 * Adopt the firmware-started watchdog under the lock: if a
+		 * system sleep transition began between notifier registration
+		 * and this point, keep the hardware stopped now and let
+		 * PM_POST_* arm it, like any other start during the transition.
+		 */
+		spin_lock_irqsave(&gwdt->lock, flags);
+		gwdt->hw_armed = true;
+		if (gwdt->sleeping)
+			sbsa_gwdt_hw_stop(gwdt);
+		spin_unlock_irqrestore(&gwdt->lock, flags);
+	}
 
 	if (action) {
 		irq = platform_get_irq(pdev, 0);
@@ -398,32 +551,6 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-/* Disable watchdog if it is active during suspend */
-static int __maybe_unused sbsa_gwdt_suspend(struct device *dev)
-{
-	struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
-
-	if (watchdog_hw_running(&gwdt->wdd))
-		sbsa_gwdt_stop(&gwdt->wdd);
-
-	return 0;
-}
-
-/* Enable watchdog if necessary */
-static int __maybe_unused sbsa_gwdt_resume(struct device *dev)
-{
-	struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
-
-	if (watchdog_hw_running(&gwdt->wdd))
-		sbsa_gwdt_start(&gwdt->wdd);
-
-	return 0;
-}
-
-static const struct dev_pm_ops sbsa_gwdt_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume)
-};
-
 static const struct of_device_id sbsa_gwdt_of_match[] = {
 	{ .compatible = "arm,sbsa-gwdt", },
 	{},
@@ -439,7 +566,7 @@ MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match);
 static struct platform_driver sbsa_gwdt_driver = {
 	.driver = {
 		.name = DRV_NAME,
-		.pm = &sbsa_gwdt_pm_ops,
+		.pm = pm_sleep_ptr(&sbsa_gwdt_pm_ops),
 		.of_match_table = sbsa_gwdt_of_match,
 	},
 	.probe = sbsa_gwdt_probe,

base-commit: 841e384b841a3d89c50b4b2d6c5bb6abab1a7e39
-- 
2.55.0