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

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

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");