| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eae46532-57bd-40fc-9098-69c62d34571f@roeck-us.net>
Date: Mon, 21 Jul 2025 17:05:57 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Aaron Plattner <aplattner@...dia.com>,
Wim Van Sebroeck <wim@...ux-watchdog.org>
Cc: linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
Timur Tabi <ttabi@...dia.com>
Subject: Re: [PATCH v2] watchdog: sbsa: Adjust keepalive timeout to avoid
MediaTek WS0 race condition
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@...dia.com>
> Acked-by: Timur Tabi <ttabi@...dia.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@...ck-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");
Powered by blists - more mailing lists