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

Guenter Roeck
2025-11-16T16:22:42Z

On 11/16/25 05:59, 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 defautl time or turn it off. This makes installing

default. Also, claiming that the BIOS would be broken is inappropriate:
For all we know this is on purpose.

Unless I am missing something, this makes it impossible to stop the watchdog.
Is there a configuration bit indicating that the timer can not be updated ?
If so, the watchdog core needs to be told that the watchdog can not be stopped.
Otherwise userspace could try to stop the watchdog and the system would reset.

> Linux difficult and annoyingly rebooting early in an installer; unless
> one loads and starts a watchdogd in the installer environment.
>
> Change it87_wdt to report a running timer to the watchdog core using
> the user supplied or module's default time and set it as the timer
> might already be up and trigger soon.
>
> Signed-off-by: René Rebe <rene@exactco.de>
>

---
and change log goes here.

> --- a/drivers/watchdog/it87_wdt.c 2025-09-28 23:39:22.000000000 +0200
> +++ b/drivers/watchdog/it87_wdt.c 2025-11-16 14:18:24.759115286 +0100
> @@ -4,6 +4,7 @@
> * for ITE IT87xx Environment Control - Low Pin Count Input / Output
> *
> * (c) Copyright 2007 Oliver Schuster <olivers137@aol.com>
> + * (c) Copyright 2025 René Rebe <rene@exactco.de>

For a couple of lines of code ? Really ? I changed 75 lines in that driver
and don't claim that.

> *
> * Based on softdog.c by Alan Cox,
> * 83977f_wdt.c by Jose Goncalves,
> @@ -188,6 +189,15 @@
> superio_outb(t >> 8, WDTVALMSB);
> }
>
> +/* Internal function, should be called after superio_select(GPIO) */
> +static unsigned int _wdt_get_timeout(void)
> +{
> + unsigned int ret = superio_inb(WDTVALLSB);

Empty line after variable declarations.

> + if (max_units > 255)
> + ret |= superio_inb(WDTVALMSB) << 8;

There is a configuration bit which determines if the timeout is counted
in minutes or in seconds. That needs to be taken into account as well.

> + return ret;
> +}
> +
> static int wdt_update_timeout(unsigned int t)
> {
> int ret;
> @@ -292,6 +302,7 @@
> u8 ctrl;
> int quirks = 0;
> int rc;
> + unsigned int _timeout;
>
> rc = superio_enter();
> if (rc)
> @@ -374,8 +385,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 +397,17 @@
> wdt_dev.timeout = timeout;
> wdt_dev.max_timeout = max_units * 60;
>
> + /* wdt already left running by fw bios? */
> + _timeout = _wdt_get_timeout();
> + if (_timeout) {
> + pr_warn("Left running by firmware.\n");

This is not a reason for a log message, much less for a warning.

> + wdt_dev.max_hw_heartbeat_ms = timeout * 1000;

This should be set instead of setting max_timeout.

> + set_bit(WDOG_HW_RUNNING, &wdt_dev.status);
> + _wdt_update_timeout(timeout);

The watchdog core does that already when the watchdog is registered.

> + }
> +
> + superio_exit();
> +
> watchdog_stop_on_reboot(&wdt_dev);
> rc = watchdog_register_device(&wdt_dev);
> if (rc)
>