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

René Rebe
2025-11-17T09:51:49Z

Good morning,

> On 17. Nov 2025, at 01:16, Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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.

You previously only did not believe me it has a broken BIOS option, you did
not yet articulate the information is not relevant. Many patches carry a Why?
motivation, and at least for me it is relevant why we even bothered with this.

>> 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.

I don’t recall you mentioning that before, but your level not nitpicking
here about context and hints is honestly rather strange to me.

>> 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 ]

Pity you suggest that change now after making me return the actual time multiplied
by the 60s config bit initially. But more than happy to change. Saves some ~28 clock
cycles on my i386 ;-)

>> +}
>> +
>> 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.

Yes, indeed, I only moved it down as originally we needed the configured timeout
to pass it to your initially suggested max_hw_heartbeat_ms, too. Will move it up
and send v4.

Best
René

>> + 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)
>

--
https://exactco.de - https://t2linux.com - https://rene.rebe.de