[PATCH v2] watchdog: sbsa: Adjust keepalive timeout to avoid MediaTek WS0 race condition

[PATCH v2] watchdog: sbsa: Adjust keepalive timeout to avoid MediaTek WS0 race condition

[Aaron Plattner]

The MediaTek implementation of the sbsa_gwdt watchdog has a race
condition where a write to SBSA_GWDT_WRR is ignored if it occurs while
the hardware is processing a timeout refresh that asserts WS0.

Detect this based on the hardware implementer and adjust
wdd->min_hw_heartbeat_ms to avoid the race by forcing the keepalive ping
to be one second later.

Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
Acked-by: Timur Tabi <ttabi@nvidia.com>
---
Thanks for the suggestion of adjusting wdd->min_hw_heartbeat_ms, Guenter. I
confirmed that doing this causes the keepalive to fire at the later time and
this avoids the race:

 [  137.500462] sbsa-gwdt sbsa-gwdt.0: version: 1, impl: 0x426, need_ws0_race_workaround: 1
 [  137.500473] sbsa-gwdt sbsa-gwdt.0: Set min_hw_heartbeat_ms to 6000
 [  137.500944] sbsa-gwdt sbsa-gwdt.0: Initialized with 10s timeout @ 1000000000 Hz, action=0. [enabled]
 [  143.501475] sbsa-gwdt sbsa-gwdt.0: ping
 [  149.502044] sbsa-gwdt sbsa-gwdt.0: ping
 [  155.502351] sbsa-gwdt sbsa-gwdt.0: ping

I also confirmed that this no longer exposes the adjusted keepalive time to
userspace:

# wdctl
Device:        /dev/watchdog0
Identity:      SBSA Generic Watchdog [version 0]
Timeout:       10 seconds

(in v1 this would have reported 8 seconds)

-- Aaron

 drivers/watchdog/sbsa_gwdt.c | 50 +++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index 5f23913ce3b49..6ce1bfb390641 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c
@@ -75,11 +75,17 @@
 #define SBSA_GWDT_VERSION_MASK  0xF
 #define SBSA_GWDT_VERSION_SHIFT 16
 
+#define SBSA_GWDT_IMPL_MASK	0x7FF
+#define SBSA_GWDT_IMPL_SHIFT	0
+#define SBSA_GWDT_IMPL_MEDIATEK	0x426
+
 /**
  * struct sbsa_gwdt - Internal representation of the SBSA GWDT
  * @wdd:		kernel watchdog_device structure
  * @clk:		store the System Counter clock frequency, in Hz.
  * @version:            store the architecture version
+ * @need_ws0_race_workaround:
+ *			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
  */
@@ -87,6 +93,7 @@ struct sbsa_gwdt {
 	struct watchdog_device	wdd;
 	u32			clk;
 	int			version;
+	bool			need_ws0_race_workaround;
 	void __iomem		*refresh_base;
 	void __iomem		*control_base;
 };
@@ -161,6 +168,31 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
 		 */
 		sbsa_gwdt_reg_write(((u64)gwdt->clk / 2) * timeout, gwdt);
 
+	/*
+	 * Some watchdog hardware has a race condition where it will ignore
+	 * sbsa_gwdt_keepalive() if it is called at the exact moment that a
+	 * timeout occurs and WS0 is being asserted. Unfortunately, the default
+	 * behavior of the watchdog core is very likely to trigger this race
+	 * when action=0 because it programs WOR to be half of the desired
+	 * timeout, and watchdog_next_keepalive() chooses the exact same time to
+	 * send keepalive pings.
+	 *
+	 * This triggers a race where sbsa_gwdt_keepalive() can be called right
+	 * as WS0 is being asserted, and affected hardware will ignore that
+	 * write and continue to assert WS0. After another (timeout / 2)
+	 * seconds, the same race happens again. If the driver wins then the
+	 * explicit refresh will reset WS0 to false but if the hardware wins,
+	 * then WS1 is asserted and the system resets.
+	 *
+	 * Avoid the problem by scheduling keepalive heartbeats one second later
+	 * than the WOR timeout.
+	 *
+	 * This workaround might not be needed in a future revision of the
+	 * hardware.
+	 */
+	if (gwdt->need_ws0_race_workaround)
+		wdd->min_hw_heartbeat_ms = timeout * 500 + 1000;
+
 	return 0;
 }
 
@@ -202,12 +234,15 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
 static void sbsa_gwdt_get_version(struct watchdog_device *wdd)
 {
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
-	int ver;
+	int iidr, ver, impl;
 
-	ver = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
-	ver = (ver >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
+	iidr = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
+	ver = (iidr >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
+	impl = (iidr >> SBSA_GWDT_IMPL_SHIFT) & SBSA_GWDT_IMPL_MASK;
 
 	gwdt->version = ver;
+	gwdt->need_ws0_race_workaround =
+		!action && (impl == SBSA_GWDT_IMPL_MEDIATEK);
 }
 
 static int sbsa_gwdt_start(struct watchdog_device *wdd)
@@ -299,6 +334,15 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	else
 		wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
 
+	if (gwdt->need_ws0_race_workaround) {
+		/*
+		 * A timeout of 3 seconds means that WOR will be set to 1.5
+		 * seconds and the heartbeat will be scheduled every 2.5
+		 * seconds.
+		 */
+		wdd->min_timeout = 3;
+	}
+
 	status = readl(cf_base + SBSA_GWDT_WCS);
 	if (status & SBSA_GWDT_WCS_WS1) {
 		dev_warn(dev, "System reset by WDT.\n");
-- 
2.50.1

Re: [PATCH v2] watchdog: sbsa: Adjust keepalive timeout to avoid MediaTek WS0 race condition

[Guenter Roeck]

On 7/21/25 16:06, Aaron Plattner wrote:
> The MediaTek implementation of the sbsa_gwdt watchdog has a race
> condition where a write to SBSA_GWDT_WRR is ignored if it occurs while
> the hardware is processing a timeout refresh that asserts WS0.
> 
> Detect this based on the hardware implementer and adjust
> wdd->min_hw_heartbeat_ms to avoid the race by forcing the keepalive ping
> to be one second later.
> 
> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
> Acked-by: Timur Tabi <ttabi@nvidia.com>
> ---
> Thanks for the suggestion of adjusting wdd->min_hw_heartbeat_ms, Guenter. I
> confirmed that doing this causes the keepalive to fire at the later time and
> this avoids the race:
> 
>   [  137.500462] sbsa-gwdt sbsa-gwdt.0: version: 1, impl: 0x426, need_ws0_race_workaround: 1
>   [  137.500473] sbsa-gwdt sbsa-gwdt.0: Set min_hw_heartbeat_ms to 6000
>   [  137.500944] sbsa-gwdt sbsa-gwdt.0: Initialized with 10s timeout @ 1000000000 Hz, action=0. [enabled]
>   [  143.501475] sbsa-gwdt sbsa-gwdt.0: ping
>   [  149.502044] sbsa-gwdt sbsa-gwdt.0: ping
>   [  155.502351] sbsa-gwdt sbsa-gwdt.0: ping
> 
> I also confirmed that this no longer exposes the adjusted keepalive time to
> userspace:
> 
> # wdctl
> Device:        /dev/watchdog0
> Identity:      SBSA Generic Watchdog [version 0]
> Timeout:       10 seconds
> 
> (in v1 this would have reported 8 seconds)
> 

That is much better!

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

> -- Aaron
> 
>   drivers/watchdog/sbsa_gwdt.c | 50 +++++++++++++++++++++++++++++++++---
>   1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index 5f23913ce3b49..6ce1bfb390641 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -75,11 +75,17 @@
>   #define SBSA_GWDT_VERSION_MASK  0xF
>   #define SBSA_GWDT_VERSION_SHIFT 16
>   
> +#define SBSA_GWDT_IMPL_MASK	0x7FF
> +#define SBSA_GWDT_IMPL_SHIFT	0
> +#define SBSA_GWDT_IMPL_MEDIATEK	0x426
> +
>   /**
>    * struct sbsa_gwdt - Internal representation of the SBSA GWDT
>    * @wdd:		kernel watchdog_device structure
>    * @clk:		store the System Counter clock frequency, in Hz.
>    * @version:            store the architecture version
> + * @need_ws0_race_workaround:
> + *			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
>    */
> @@ -87,6 +93,7 @@ struct sbsa_gwdt {
>   	struct watchdog_device	wdd;
>   	u32			clk;
>   	int			version;
> +	bool			need_ws0_race_workaround;
>   	void __iomem		*refresh_base;
>   	void __iomem		*control_base;
>   };
> @@ -161,6 +168,31 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
>   		 */
>   		sbsa_gwdt_reg_write(((u64)gwdt->clk / 2) * timeout, gwdt);
>   
> +	/*
> +	 * Some watchdog hardware has a race condition where it will ignore
> +	 * sbsa_gwdt_keepalive() if it is called at the exact moment that a
> +	 * timeout occurs and WS0 is being asserted. Unfortunately, the default
> +	 * behavior of the watchdog core is very likely to trigger this race
> +	 * when action=0 because it programs WOR to be half of the desired
> +	 * timeout, and watchdog_next_keepalive() chooses the exact same time to
> +	 * send keepalive pings.
> +	 *
> +	 * This triggers a race where sbsa_gwdt_keepalive() can be called right
> +	 * as WS0 is being asserted, and affected hardware will ignore that
> +	 * write and continue to assert WS0. After another (timeout / 2)
> +	 * seconds, the same race happens again. If the driver wins then the
> +	 * explicit refresh will reset WS0 to false but if the hardware wins,
> +	 * then WS1 is asserted and the system resets.
> +	 *
> +	 * Avoid the problem by scheduling keepalive heartbeats one second later
> +	 * than the WOR timeout.
> +	 *
> +	 * This workaround might not be needed in a future revision of the
> +	 * hardware.
> +	 */
> +	if (gwdt->need_ws0_race_workaround)
> +		wdd->min_hw_heartbeat_ms = timeout * 500 + 1000;
> +
>   	return 0;
>   }
>   
> @@ -202,12 +234,15 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
>   static void sbsa_gwdt_get_version(struct watchdog_device *wdd)
>   {
>   	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
> -	int ver;
> +	int iidr, ver, impl;
>   
> -	ver = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
> -	ver = (ver >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
> +	iidr = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
> +	ver = (iidr >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
> +	impl = (iidr >> SBSA_GWDT_IMPL_SHIFT) & SBSA_GWDT_IMPL_MASK;
>   
>   	gwdt->version = ver;
> +	gwdt->need_ws0_race_workaround =
> +		!action && (impl == SBSA_GWDT_IMPL_MEDIATEK);
>   }
>   
>   static int sbsa_gwdt_start(struct watchdog_device *wdd)
> @@ -299,6 +334,15 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>   	else
>   		wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
>   
> +	if (gwdt->need_ws0_race_workaround) {
> +		/*
> +		 * A timeout of 3 seconds means that WOR will be set to 1.5
> +		 * seconds and the heartbeat will be scheduled every 2.5
> +		 * seconds.
> +		 */
> +		wdd->min_timeout = 3;
> +	}
> +
>   	status = readl(cf_base + SBSA_GWDT_WCS);
>   	if (status & SBSA_GWDT_WCS_WS1) {
>   		dev_warn(dev, "System reset by WDT.\n");