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

[Guenter Roeck <linux@roeck-us.net>]

On 9/12/26 11:21, David Cemin wrote:
> 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.
> 

Isn't this a problem that affects _all_ watchdog drivers ?

Thanks,
Guenter

> 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