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

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

[René Rebe]

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

--- 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>
  *
  *	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);
+	if (max_units > 255)
+		ret |= superio_inb(WDTVALMSB) << 8;
+	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");
+		wdt_dev.max_hw_heartbeat_ms = timeout * 1000;
+		set_bit(WDOG_HW_RUNNING, &wdt_dev.status);
+		_wdt_update_timeout(timeout);
+	}
+
+	superio_exit();
+
 	watchdog_stop_on_reboot(&wdt_dev);
 	rc = watchdog_register_device(&wdt_dev);
 	if (rc)

-- 
  René Rebe, ExactCODE GmbH, Berlin, Germany
  https://exactcode.com | https://t2linux.com | https://rene.rebe.de

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

[Guenter Roeck]

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

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

[René Rebe]

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.

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

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

> >    *
> >    *	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 ;-)

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

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

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

-- 
  René Rebe, ExactCODE GmbH, Berlin, Germany
  https://exactco.de | https://t2linux.com | https://rene.rebe.de

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

[Guenter Roeck]

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

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

[René Rebe]

On Sun, 16 Nov 2025 08:22:39 -0800,
Guenter Roeck <linux@roeck-us.net> wrote:

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

After debugging, reading the core source and RTFM apparently not. As
the time can be changed and thus .max_hw_heartbeat_ms should
apparently not be set at all unlike you initially suggested. AFAICS it
is the regular .timeout and .max_timeout we want to keep setting.

Best regards,
     René

-- 
  René Rebe, ExactCODE GmbH, Berlin, Germany
  https://exactco.de | https://t2linux.com | https://rene.rebe.de

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

[René Rebe]

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
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 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;
+}
+
 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) {
+		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)

-- 
  René Rebe, ExactCODE GmbH, Berlin, Germany
  https://exactco.de | https://t2linux.com | https://rene.rebe.de

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

[Guenter Roeck]

On 11/16/25 11:42, René Rebe wrote:
> On Sun, 16 Nov 2025 08:22:39 -0800,
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> 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.
> 
> After debugging, reading the core source and RTFM apparently not. As
> the time can be changed and thus .max_hw_heartbeat_ms should
> apparently not be set at all unlike you initially suggested. AFAICS it
> is the regular .timeout and .max_timeout we want to keep setting.
> 

You are correct here. My understanding wqas wrong - I though the problem
was that updating the timeout was disabled by the kernel, but the problem
was just that the timeout was enabled by the BIOS. So all that needs to
be done is what was done in w83627hf_wdt.c - mark the watchdog as running
if that is the case.

Guenter

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

[Guenter Roeck]

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

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

[René Rebe]

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