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

Guenter Roeck
2025-11-16T18:43:17Z

On 11/16/25 09:14, René Rebe wrote:
> On Sun, 16 Nov 2025 08:22:39 -0800,
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> 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
>
> Sorry for the typo, English is not my native language. I will try
> harder next time.
>

No worries. Happens to native speakers as well.

>> inappropriate:
>> For all we know this is on purpose.
>
> The BIOS is broken in that it has an option configure the time and
> disable it but it does not save and thus have no effect. Thus that
> part is as broken as I described it, ...
>
>> 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 ?
>
> No, I only meant the BIOS configuration is broken.
>

Ah, ok, then the scope is wrong to start with: The problem is not that
the BIOS is broken, the problem is that there are BIOSes out there which
enable the watchdog. The driver should support that case, broken BIOS or
not.

In other words, the driver should work fine if the BIOS _does_ enable
the watchdog. It should not be necessary to disable it for the driver to
work.

>> 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.
>
> No it is fine, setting the time to 0 disabled it, I tested.
>
>>> 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.
>
> sorry, oversight, I'll make a mental note to never forget it again Sir.
>
>>> --- 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.
>
> To be fair, for my inital one-liner fixed I did not have that, but as
> I now spent hours adjusting it to your suggestions, if not
> requirements, debugging and testing, I thought that is more than a one
> liner. But my life does not depend on it though legally it probably
> might be a thing.
>

I'd have a copyright claim on hundreds of files in the kernel if I would
use that logic. Let's see...

$ git show --name-only --format="" $(git log --oneline --author="Guenter Roeck" --format=%h) | sort -u | wc
1401 1401 42814
$ git grep "Copyright.*Guenter Roeck" | cut -f1 -d: | sort -u | wc
32 32 883

Guess I have some copyrighting work cut out for me. Oh, and that would
include this driver.

Seriously, if you rework the driver to work as platform device, a copyright
claim might be warranted if you insist. If you rework a watchdog driver to
use the watchdog subsystem core (not an issue here, I did that already),
you would have a valid claim. Adding minor functionality such as detecting
if the watchdog is running ? Not really.

As for time spent ... I sometimes spend several days (not just hours) to track
down a problem in the Linux kernel. Does that make me own a copyright on the
file(s) that include the fix ? Not really.

I assume that there is some legal definition on who has a copyright on kernel
code. If so, that is automatic and does not need to be spelled out. By convention
the copyright listed on the top of Linux kernel files is used to indicate major
contributors. I have no interest in changing that - if minor contributors would
be listed, the entire kernel would be cluttered with copyright lines. I find
that misleading and unacceptable, and will not agree to it.

>>> *
>>> * 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.
>
> Oh man, guess time to re-read the kernel style guide and run all
> changes thru some verifier ;-)
>

You might possibly consider running checkpatch --strict.

>>> + 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.
>
> I had this at the caller but can of course move it here.
>
>>> + 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.
>
> I often daily waste time with something random, because of missing
> diagnostics. Often something as stupid as this. Should the user not
> know the FW left a watchdog running that you insisted should be left
> running? I certainly would like to know that there is something I did
> not explicity asked for especially if a user might wonder why the
> system might sometimes randomly reboot. Few will see this message.
>

Not an argument. This is not something left running, it is perfectly
valid configuration - even more so if the BIOS supports enabling the
watchdog. It is also not "left running". I'd accept an informational
"timer is running\n" (hpwdt) or even "Watchdog already running. Resetting
timeout to %d sec\n" (w83627hf_wdt), but not as warning and it has to be
neutral (no claim or suggestion that this is somehow wrong).

> The kernel has much more random nonsene diagnstic, like MCE missing on
> on Intel and AMD CPUs etc.
>

That is a much worse argument. You can find pretty much everything in the kernel.
Many people drive faster than the speed limit. That doesn't make it a good idea,
much less legal.

>>> + wdt_dev.max_hw_heartbeat_ms = timeout * 1000;
>>
>> This should be set instead of setting max_timeout.
>
> I thought that might not yet work here that early, and IIRC grep found
> other drivers doing it liek this, but will of course adapt.
>
>>> + set_bit(WDOG_HW_RUNNING, &wdt_dev.status);
>>> + _wdt_update_timeout(timeout);
>>
>> The watchdog core does that already when the watchdog is registered.
>
> Ok, instantly without delay? Will double check.
>
> Thank you for your review, will send v3 soon.
>
> René
>
>>> + }
>>> +
>>> + superio_exit();
>>> +
>>> watchdog_stop_on_reboot(&wdt_dev);
>>> rc = watchdog_register_device(&wdt_dev);
>>> if (rc)
>>>
>>
>