Re: [PATCH v3] fix it87_wdt early reboot w/ FW started timer

Guenter Roeck
2025-11-17T00:16:10Z

On 11/16/25 12:09, René Rebe wrote:
> Some products, such as the Ugreen DXP4800 Plus NAS, ship with the it87
> wdt enabled by the firmware and a broken BIOS option that does not
> allow to change the default time or turn it off. This makes installing

Again, "and a broken BIOS option that does not allow to change the default
time or turn it off" is secondary. The key is that the BIOS/firmware may have
enabled the watchdog.

> Linux difficult and annoyingly rebooting early in an installer; unless
> one loads and starts a watchdogd in the installer environment.
>
Also again, please drop irrelevant information (how to work around the
problem without this patch is irrelevant) and provide a more neutral
description. Add as much additional details after "---" if you like.

> Change it87_wdt to report a running timer to the watchdog core using
> the user supplied or module's default time so it is reset before
> triggering.
>
> Signed-off-by: René Rebe <rene@exactco.de>
>
> v1:
> - just clear hw timer register
>
> v2:
> - detect running hw timer and report to watchdog core
>
> v3:
> - multiply TOV1 in _wdt_get_timeout
> - don't wrongly and superfluously set .max_hw_heartbeat_ms
> - don't call set_timeout manually
>
> --- linux-6.17/drivers/watchdog/it87_wdt.c.orig 2025-09-28 23:39:22.000000000 +0200
> +++ linux-6.17/drivers/watchdog/it87_wdt.c 2025-11-16 20:05:01.650672740 +0100
> @@ -188,6 +190,21 @@
> superio_outb(t >> 8, WDTVALMSB);
> }
>
> +/* Internal function, should be called after superio_select(GPIO) */
> +static unsigned int _wdt_get_timeout(void)
> +{
> + unsigned int t;
> + u8 cfg;
> +
> + cfg = superio_inb(WDTCFG);
> + t = superio_inb(WDTVALLSB);
> + if (max_units > 255)
> + t |= superio_inb(WDTVALMSB) << 8;
> + if (cfg & WDT_TOV1)
> + t *= 60;
> + return t;

Given the context, this can be simplified to something like

static bool _wdt_running(void)
{
return !!(superio_inb(WDTVALLSB) || max_units > 255 && superio_inb(WDTVALMSB));
}

[ where the !! isn't really necessary ]

> +}
> +
> static int wdt_update_timeout(unsigned int t)
> {
> int ret;
> @@ -292,6 +309,7 @@
> u8 ctrl;
> int quirks = 0;
> int rc;
> + unsigned int _timeout;
>
> rc = superio_enter();
> if (rc)
> @@ -374,8 +392,6 @@
> }
> }
>
> - superio_exit();
> -
> if (timeout < 1 || timeout > max_units * 60) {
> timeout = DEFAULT_TIMEOUT;
> pr_warn("Timeout value out of range, use default %d sec\n",
> @@ -388,6 +404,15 @@
> wdt_dev.timeout = timeout;
> wdt_dev.max_timeout = max_units * 60;
>
> + /* wdt already left running by fw bios? */
> + _timeout = _wdt_get_timeout();
> + if (_timeout) {

if (_wdt_running()) {
}

and the code can be moved ahead of the superio_exit() call above,
avoiding the need to move it.

> + pr_info("Left running by firmware.\n");
> + set_bit(WDOG_HW_RUNNING, &wdt_dev.status);
> + }
> +
> + superio_exit();
> +
> watchdog_stop_on_reboot(&wdt_dev);
> rc = watchdog_register_device(&wdt_dev);
> if (rc)
>