[PATCH v3 0/2] watchdog: w83627hf_wdt: Add NCT6126 support and improve handling of running watchdog

[PATCH v3 0/2] watchdog: w83627hf_wdt: Add NCT6126 support and improve handling of running watchdog

[Paul Louvel]

This series brings two small improvements to the w83627hf_wdt driver.

The first patch adds support for the Nuvoton NCT6126 Super I/O chip. The
NCT6126 is compatible with the existing driver but requires reading the
full 16-bit chip ID, as it shares its high-byte ID with the NCT6116.

The second patch improves the handling of watchdogs that are already
running when the driver probes. Instead of issuing a single keepalive
using the default timeout, it relies on the watchdog core's
WDOG_HW_RUNNING support to automatically maintain the watchdog until
userspace opens the device.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
Changes in v3:
- Forgot to add case in w83627hf_init() with the chip. Not detected
  during testing because the BIOS probably configure it anyway on boot.
- Link to v2: https://patch.msgid.link/20260706-w83627hf_wdt-nct6126d-v2-0-77af74af88ae@bootlin.com

Changes in v2:
- Small mistake in the first patch. The NCT6116_ID macro is redefined.
  Fix it.

To: Wim Van Sebroeck <wim@linux-watchdog.org>
To: Guenter Roeck <linux@roeck-us.net>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Paul Louvel (2):
      watchdog: w83627hf_wdt: Add support for Nuvoton NCT6126
      watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

 drivers/watchdog/w83627hf_wdt.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
---
base-commit: 028ef9c96e96197026887c0f092424679298aae8
change-id: 20260705-w83627hf_wdt-nct6126d-9a016bf7c936

Best regards,
--  
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 1/2] watchdog: w83627hf_wdt: Add support for Nuvoton NCT6126

[Paul Louvel]

Add support for the hardware watchdog integrated in the Nuvoton NCT6126
Super I/O chip. This device is used on a number of x86 single-board
computers and is compatible with the w83627hf_wdt driver.

Unlike most supported chips, the NCT6126 shares the same high-byte chip
ID as the NCT6116. Read the low byte of the chip ID as well to
distinguish between the two devices and identify the NCT6126 correctly.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/watchdog/w83627hf_wdt.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
index bc33b63c5a5d..a6dfa9d01702 100644
--- a/drivers/watchdog/w83627hf_wdt.c
+++ b/drivers/watchdog/w83627hf_wdt.c
@@ -49,7 +49,7 @@ static int wdt_cfg_leave = 0xAA;/* key to lock configuration space */
 enum chips { w83627hf, w83627s, w83697hf, w83697ug, w83637hf, w83627thf,
 	     w83687thf, w83627ehf, w83627dhg, w83627uhg, w83667hg, w83627dhg_p,
 	     w83667hg_b, nct6775, nct6776, nct6779, nct6791, nct6792, nct6793,
-	     nct6795, nct6796, nct6102, nct6116 };
+	     nct6795, nct6796, nct6102, nct6116, nct6126 };
 
 static int timeout;			/* in seconds */
 module_param(timeout, int, 0);
@@ -94,7 +94,9 @@ MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=0)");
 #define NCT6775_ID		0xb4
 #define NCT6776_ID		0xc3
 #define NCT6102_ID		0xc4
-#define NCT6116_ID		0xd2
+#define NCT6116_ID		0xd2	/* also NCT6126D */
+#define NCT6126_VER_A_LOW_ID	0x83	/* ... version A */
+#define NCT6126_VER_B_LOW_ID	0x84	/* ... version B */
 #define NCT6779_ID		0xc5
 #define NCT6791_ID		0xc8
 #define NCT6792_ID		0xc9
@@ -217,6 +219,7 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
 	case nct6796:
 	case nct6102:
 	case nct6116:
+	case nct6126:
 		/*
 		 * These chips have a fixed WDTO# output pin (W83627UHG),
 		 * or support more than one WDTO# output pin.
@@ -428,7 +431,12 @@ static int wdt_find(int addr)
 		cr_wdt_csr = NCT6102D_WDT_CSR;
 		break;
 	case NCT6116_ID:
-		ret = nct6116;
+		val = superio_inb(0x21);
+		if (val == NCT6126_VER_A_LOW_ID || val == NCT6126_VER_B_LOW_ID)
+			ret = nct6126;
+		else
+			ret = nct6116;
+
 		cr_wdt_timeout = NCT6102D_WDT_TIMEOUT;
 		cr_wdt_control = NCT6102D_WDT_CONTROL;
 		cr_wdt_csr = NCT6102D_WDT_CSR;
@@ -499,6 +507,7 @@ static int __init wdt_init(void)
 		"NCT6796",
 		"NCT6102",
 		"NCT6116",
+		"NCT6126"
 	};
 
 	/* Apply system-specific quirks */

-- 
2.55.0

[PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[Paul Louvel]

If the watchdog is already running when the driver is initialized, the
driver currently refreshes it once using the default timeout of 60
seconds.

Since commit ee142889e32f ("watchdog: Introduce WDOG_HW_RUNNING flag"),
the watchdog core can keep an already running watchdog alive even when
the watchdog device is not open. This is more robust than issuing a
single manual keepalive during initialization.

Replace the manual watchdog refresh with the WDOG_HW_RUNNING flag and
let the watchdog core handle periodic keepalives until user space takes
over.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/watchdog/w83627hf_wdt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
index a6dfa9d01702..27ef1cce3a14 100644
--- a/drivers/watchdog/w83627hf_wdt.c
+++ b/drivers/watchdog/w83627hf_wdt.c
@@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
 			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
 			superio_outb(cr_wdt_timeout, 0);
 		} else {
-			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
-				wdog->timeout);
-			superio_outb(cr_wdt_timeout, wdog->timeout);
+			pr_info("Watchdog already running.\n");
+			set_bit(WDOG_HW_RUNNING, &wdog->status);
 		}
 	}
 

-- 
2.55.0

Re: [PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the manual hardware timeout refresh while still unconditionally switching the hardware timer base from minutes to seconds drastically truncates the remaining timeout if the BIOS originally configured it in minutes, risking unexpected system reboots.
--

commit 0014048ec369e2a09098aa033aeb725449e2fd67
Author: Paul Louvel <paul.louvel@bootlin.com>

watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

This patch updates the w83627hf watchdog driver to rely on the watchdog
core's WDOG_HW_RUNNING flag to keep a running watchdog alive during boot.
It replaces the previous behavior of manually issuing a single keepalive
during initialization.

> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c
[ ... ]
> @@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>  			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
>  			superio_outb(cr_wdt_timeout, 0);
>  		} else {
> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
> -				wdog->timeout);
> -			superio_outb(cr_wdt_timeout, wdog->timeout);
> +			pr_info("Watchdog already running.\n");
> +			set_bit(WDOG_HW_RUNNING, &wdog->status);
>  		}
>  	}
>  

[Severity: High]
Does removing this explicit timeout reset create a risk of unexpected reboots?

If the BIOS originally configured the watchdog with a timeout in minutes
(for example, a timeout of 5 minutes), the hardware counter cr_wdt_timeout
will hold the value 5.

Because this patch removes the explicit write of wdog->timeout (which is
typically 60) to cr_wdt_timeout, the counter retains the raw value 5.
Just after this block in w83627hf_init(), the driver unconditionally
switches the timebase to seconds:

w83627hf_init() {
    ...
    /* set second mode & disable keyboard turning off watchdog */
    t = superio_inb(cr_wdt_control) & ~0x0C;
    superio_outb(cr_wdt_control, t);
    ...
}

This immediately shrinks the remaining timeout from 5 minutes to 5 seconds.

If the watchdog core's kworker is delayed, or if the system is configured
not to ping the watchdog before user space takes over, could this unusually
short timeout cause the system to reboot unexpectedly during early boot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-w83627hf_wdt-nct6126d-v3-0-fa5ad8d486bc@bootlin.com?part=2

Re: [PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[Paul Louvel]

On Mon Jul 6, 2026 at 12:40 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Removing the manual hardware timeout refresh while still unconditionally switching the hardware timer base from minutes to seconds drastically truncates the remaining timeout if the BIOS originally configured it in minutes, risking unexpected system reboots.
> --
>
> commit 0014048ec369e2a09098aa033aeb725449e2fd67
> Author: Paul Louvel <paul.louvel@bootlin.com>
>
> watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot
>
> This patch updates the w83627hf watchdog driver to rely on the watchdog
> core's WDOG_HW_RUNNING flag to keep a running watchdog alive during boot.
> It replaces the previous behavior of manually issuing a single keepalive
> during initialization.
>
>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>> --- a/drivers/watchdog/w83627hf_wdt.c
>> +++ b/drivers/watchdog/w83627hf_wdt.c
> [ ... ]
>> @@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>  			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
>>  			superio_outb(cr_wdt_timeout, 0);
>>  		} else {
>> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
>> -				wdog->timeout);
>> -			superio_outb(cr_wdt_timeout, wdog->timeout);
>> +			pr_info("Watchdog already running.\n");
>> +			set_bit(WDOG_HW_RUNNING, &wdog->status);
>>  		}
>>  	}
>>  
>
> [Severity: High]
> Does removing this explicit timeout reset create a risk of unexpected reboots?
>
> If the BIOS originally configured the watchdog with a timeout in minutes
> (for example, a timeout of 5 minutes), the hardware counter cr_wdt_timeout
> will hold the value 5.

First of all, the driver does not support minute mode for now.

>
> Because this patch removes the explicit write of wdog->timeout (which is
> typically 60) to cr_wdt_timeout, the counter retains the raw value 5.
> Just after this block in w83627hf_init(), the driver unconditionally
> switches the timebase to seconds:
>
> w83627hf_init() {
>     ...
>     /* set second mode & disable keyboard turning off watchdog */
>     t = superio_inb(cr_wdt_control) & ~0x0C;
>     superio_outb(cr_wdt_control, t);
>     ...
> }
>
> This immediately shrinks the remaining timeout from 5 minutes to 5 seconds.

True, but in those 5 seconds, the watchdog core will kick the watchdog
immediately. by calling start(), and thus refreshing the watchdog to 60 seconds.

>
> If the watchdog core's kworker is delayed, or if the system is configured
> not to ping the watchdog before user space takes over, could this unusually
> short timeout cause the system to reboot unexpectedly during early boot?

Yes, but expected. If WATCHDOG is enabled, then WATCHDOG_HANDLE_BOOT_ENABLED is
enabled by default unless explicitly disabled when configuring the kernel.




-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 1/2] watchdog: w83627hf_wdt: Add support for Nuvoton NCT6126

[Guenter Roeck]

On 7/6/26 03:29, Paul Louvel wrote:
> Add support for the hardware watchdog integrated in the Nuvoton NCT6126
> Super I/O chip. This device is used on a number of x86 single-board
> computers and is compatible with the w83627hf_wdt driver.
> 
> Unlike most supported chips, the NCT6126 shares the same high-byte chip
> ID as the NCT6116. Read the low byte of the chip ID as well to
> distinguish between the two devices and identify the NCT6126 correctly.
> 

Doesn't that (and the code below) mean that the driver already supports
the chip, only it identifies it as NCT6116 instead of NCT6126 ?

This means that the commit message is misleading: It should say that
the chip is already supported but misidentified, and that the added
code helps to correctly identify the chip.

Guenter

> Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
> ---
>   drivers/watchdog/w83627hf_wdt.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index bc33b63c5a5d..a6dfa9d01702 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c
> @@ -49,7 +49,7 @@ static int wdt_cfg_leave = 0xAA;/* key to lock configuration space */
>   enum chips { w83627hf, w83627s, w83697hf, w83697ug, w83637hf, w83627thf,
>   	     w83687thf, w83627ehf, w83627dhg, w83627uhg, w83667hg, w83627dhg_p,
>   	     w83667hg_b, nct6775, nct6776, nct6779, nct6791, nct6792, nct6793,
> -	     nct6795, nct6796, nct6102, nct6116 };
> +	     nct6795, nct6796, nct6102, nct6116, nct6126 };
>   
>   static int timeout;			/* in seconds */
>   module_param(timeout, int, 0);
> @@ -94,7 +94,9 @@ MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=0)");
>   #define NCT6775_ID		0xb4
>   #define NCT6776_ID		0xc3
>   #define NCT6102_ID		0xc4
> -#define NCT6116_ID		0xd2
> +#define NCT6116_ID		0xd2	/* also NCT6126D */
> +#define NCT6126_VER_A_LOW_ID	0x83	/* ... version A */
> +#define NCT6126_VER_B_LOW_ID	0x84	/* ... version B */
>   #define NCT6779_ID		0xc5
>   #define NCT6791_ID		0xc8
>   #define NCT6792_ID		0xc9
> @@ -217,6 +219,7 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>   	case nct6796:
>   	case nct6102:
>   	case nct6116:
> +	case nct6126:
>   		/*
>   		 * These chips have a fixed WDTO# output pin (W83627UHG),
>   		 * or support more than one WDTO# output pin.
> @@ -428,7 +431,12 @@ static int wdt_find(int addr)
>   		cr_wdt_csr = NCT6102D_WDT_CSR;
>   		break;
>   	case NCT6116_ID:
> -		ret = nct6116;
> +		val = superio_inb(0x21);
> +		if (val == NCT6126_VER_A_LOW_ID || val == NCT6126_VER_B_LOW_ID)
> +			ret = nct6126;
> +		else
> +			ret = nct6116;
> +
>   		cr_wdt_timeout = NCT6102D_WDT_TIMEOUT;
>   		cr_wdt_control = NCT6102D_WDT_CONTROL;
>   		cr_wdt_csr = NCT6102D_WDT_CSR;
> @@ -499,6 +507,7 @@ static int __init wdt_init(void)
>   		"NCT6796",
>   		"NCT6102",
>   		"NCT6116",
> +		"NCT6126"
>   	};
>   
>   	/* Apply system-specific quirks */
> 

Re: [PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[Guenter Roeck]

On 7/6/26 04:15, Paul Louvel wrote:
> On Mon Jul 6, 2026 at 12:40 PM CEST, sashiko-bot wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] Removing the manual hardware timeout refresh while still unconditionally switching the hardware timer base from minutes to seconds drastically truncates the remaining timeout if the BIOS originally configured it in minutes, risking unexpected system reboots.
>> --
>>
>> commit 0014048ec369e2a09098aa033aeb725449e2fd67
>> Author: Paul Louvel <paul.louvel@bootlin.com>
>>
>> watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot
>>
>> This patch updates the w83627hf watchdog driver to rely on the watchdog
>> core's WDOG_HW_RUNNING flag to keep a running watchdog alive during boot.
>> It replaces the previous behavior of manually issuing a single keepalive
>> during initialization.
>>
>>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>>> --- a/drivers/watchdog/w83627hf_wdt.c
>>> +++ b/drivers/watchdog/w83627hf_wdt.c
>> [ ... ]
>>> @@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>>   			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
>>>   			superio_outb(cr_wdt_timeout, 0);
>>>   		} else {
>>> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
>>> -				wdog->timeout);
>>> -			superio_outb(cr_wdt_timeout, wdog->timeout);
>>> +			pr_info("Watchdog already running.\n");
>>> +			set_bit(WDOG_HW_RUNNING, &wdog->status);
>>>   		}
>>>   	}
>>>   
>>
>> [Severity: High]
>> Does removing this explicit timeout reset create a risk of unexpected reboots?
>>
>> If the BIOS originally configured the watchdog with a timeout in minutes
>> (for example, a timeout of 5 minutes), the hardware counter cr_wdt_timeout
>> will hold the value 5.
> 
> First of all, the driver does not support minute mode for now.

The above feedback doesn't claim otherwise. The BIOS may still have configured
the timeout to 5 minutes.

> 
>>
>> Because this patch removes the explicit write of wdog->timeout (which is
>> typically 60) to cr_wdt_timeout, the counter retains the raw value 5.
>> Just after this block in w83627hf_init(), the driver unconditionally
>> switches the timebase to seconds:
>>
>> w83627hf_init() {
>>      ...
>>      /* set second mode & disable keyboard turning off watchdog */
>>      t = superio_inb(cr_wdt_control) & ~0x0C;
>>      superio_outb(cr_wdt_control, t);
>>      ...
>> }
>>
>> This immediately shrinks the remaining timeout from 5 minutes to 5 seconds.
> 
> True, but in those 5 seconds, the watchdog core will kick the watchdog
> immediately. by calling start(), and thus refreshing the watchdog to 60 seconds.

... unless below.

> 
>>
>> If the watchdog core's kworker is delayed, or if the system is configured
>> not to ping the watchdog before user space takes over, could this unusually
>> short timeout cause the system to reboot unexpectedly during early boot?
> 
> Yes, but expected. If WATCHDOG is enabled, then WATCHDOG_HANDLE_BOOT_ENABLED is
> enabled by default unless explicitly disabled when configuring the kernel.
> 

It is still an unexpected and undocumented change in behavior. Previously
in that situation the watchdog timeout would happen much later and give
user space time to start the watchdog daemon. This is no longer the case.

Guenter

Re: [PATCH v3 1/2] watchdog: w83627hf_wdt: Add support for Nuvoton NCT6126

[Paul Louvel]

On Mon Jul 6, 2026 at 4:14 PM CEST, Guenter Roeck wrote:
> On 7/6/26 03:29, Paul Louvel wrote:
>> Add support for the hardware watchdog integrated in the Nuvoton NCT6126
>> Super I/O chip. This device is used on a number of x86 single-board
>> computers and is compatible with the w83627hf_wdt driver.
>> 
>> Unlike most supported chips, the NCT6126 shares the same high-byte chip
>> ID as the NCT6116. Read the low byte of the chip ID as well to
>> distinguish between the two devices and identify the NCT6126 correctly.
>> 
>
> Doesn't that (and the code below) mean that the driver already supports
> the chip, only it identifies it as NCT6116 instead of NCT6126 ?

Yes, because they have the same high-byte chip ID.

>
> This means that the commit message is misleading: It should say that
> the chip is already supported but misidentified, and that the added
> code helps to correctly identify the chip.

Indeed. My bad !

>
> Guenter
>
>> Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
>> ---
>>   drivers/watchdog/w83627hf_wdt.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>> index bc33b63c5a5d..a6dfa9d01702 100644
>> --- a/drivers/watchdog/w83627hf_wdt.c
>> +++ b/drivers/watchdog/w83627hf_wdt.c
>> @@ -49,7 +49,7 @@ static int wdt_cfg_leave = 0xAA;/* key to lock configuration space */
>>   enum chips { w83627hf, w83627s, w83697hf, w83697ug, w83637hf, w83627thf,
>>   	     w83687thf, w83627ehf, w83627dhg, w83627uhg, w83667hg, w83627dhg_p,
>>   	     w83667hg_b, nct6775, nct6776, nct6779, nct6791, nct6792, nct6793,
>> -	     nct6795, nct6796, nct6102, nct6116 };
>> +	     nct6795, nct6796, nct6102, nct6116, nct6126 };
>>   
>>   static int timeout;			/* in seconds */
>>   module_param(timeout, int, 0);
>> @@ -94,7 +94,9 @@ MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=0)");
>>   #define NCT6775_ID		0xb4
>>   #define NCT6776_ID		0xc3
>>   #define NCT6102_ID		0xc4
>> -#define NCT6116_ID		0xd2
>> +#define NCT6116_ID		0xd2	/* also NCT6126D */
>> +#define NCT6126_VER_A_LOW_ID	0x83	/* ... version A */
>> +#define NCT6126_VER_B_LOW_ID	0x84	/* ... version B */
>>   #define NCT6779_ID		0xc5
>>   #define NCT6791_ID		0xc8
>>   #define NCT6792_ID		0xc9
>> @@ -217,6 +219,7 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>   	case nct6796:
>>   	case nct6102:
>>   	case nct6116:
>> +	case nct6126:
>>   		/*
>>   		 * These chips have a fixed WDTO# output pin (W83627UHG),
>>   		 * or support more than one WDTO# output pin.
>> @@ -428,7 +431,12 @@ static int wdt_find(int addr)
>>   		cr_wdt_csr = NCT6102D_WDT_CSR;
>>   		break;
>>   	case NCT6116_ID:
>> -		ret = nct6116;
>> +		val = superio_inb(0x21);
>> +		if (val == NCT6126_VER_A_LOW_ID || val == NCT6126_VER_B_LOW_ID)
>> +			ret = nct6126;
>> +		else
>> +			ret = nct6116;
>> +
>>   		cr_wdt_timeout = NCT6102D_WDT_TIMEOUT;
>>   		cr_wdt_control = NCT6102D_WDT_CONTROL;
>>   		cr_wdt_csr = NCT6102D_WDT_CSR;
>> @@ -499,6 +507,7 @@ static int __init wdt_init(void)
>>   		"NCT6796",
>>   		"NCT6102",
>>   		"NCT6116",
>> +		"NCT6126"
>>   	};
>>   
>>   	/* Apply system-specific quirks */
>> 

Paul.




-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[Paul Louvel]

On Mon Jul 6, 2026 at 4:22 PM CEST, Guenter Roeck wrote:
> On 7/6/26 04:15, Paul Louvel wrote:
>> On Mon Jul 6, 2026 at 12:40 PM CEST, sashiko-bot wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [High] Removing the manual hardware timeout refresh while still unconditionally switching the hardware timer base from minutes to seconds drastically truncates the remaining timeout if the BIOS originally configured it in minutes, risking unexpected system reboots.
>>> --
>>>
>>> commit 0014048ec369e2a09098aa033aeb725449e2fd67
>>> Author: Paul Louvel <paul.louvel@bootlin.com>
>>>
>>> watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot
>>>
>>> This patch updates the w83627hf watchdog driver to rely on the watchdog
>>> core's WDOG_HW_RUNNING flag to keep a running watchdog alive during boot.
>>> It replaces the previous behavior of manually issuing a single keepalive
>>> during initialization.
>>>
>>>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>>>> --- a/drivers/watchdog/w83627hf_wdt.c
>>>> +++ b/drivers/watchdog/w83627hf_wdt.c
>>> [ ... ]
>>>> @@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>>>   			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
>>>>   			superio_outb(cr_wdt_timeout, 0);
>>>>   		} else {
>>>> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
>>>> -				wdog->timeout);
>>>> -			superio_outb(cr_wdt_timeout, wdog->timeout);
>>>> +			pr_info("Watchdog already running.\n");
>>>> +			set_bit(WDOG_HW_RUNNING, &wdog->status);
>>>>   		}
>>>>   	}
>>>>   
>>>
>>> [Severity: High]
>>> Does removing this explicit timeout reset create a risk of unexpected reboots?
>>>
>>> If the BIOS originally configured the watchdog with a timeout in minutes
>>> (for example, a timeout of 5 minutes), the hardware counter cr_wdt_timeout
>>> will hold the value 5.
>> 
>> First of all, the driver does not support minute mode for now.
>
> The above feedback doesn't claim otherwise. The BIOS may still have configured
> the timeout to 5 minutes.
>
>> 
>>>
>>> Because this patch removes the explicit write of wdog->timeout (which is
>>> typically 60) to cr_wdt_timeout, the counter retains the raw value 5.
>>> Just after this block in w83627hf_init(), the driver unconditionally
>>> switches the timebase to seconds:
>>>
>>> w83627hf_init() {
>>>      ...
>>>      /* set second mode & disable keyboard turning off watchdog */
>>>      t = superio_inb(cr_wdt_control) & ~0x0C;
>>>      superio_outb(cr_wdt_control, t);
>>>      ...
>>> }
>>>
>>> This immediately shrinks the remaining timeout from 5 minutes to 5 seconds.
>> 
>> True, but in those 5 seconds, the watchdog core will kick the watchdog
>> immediately. by calling start(), and thus refreshing the watchdog to 60 seconds.
>
> ... unless below.
>
>> 
>>>
>>> If the watchdog core's kworker is delayed, or if the system is configured



>>> not to ping the watchdog before user space takes over, could this unusually
>>> short timeout cause the system to reboot unexpectedly during early boot?
>> 
>> Yes, but expected. If WATCHDOG is enabled, then WATCHDOG_HANDLE_BOOT_ENABLED is
>> enabled by default unless explicitly disabled when configuring the kernel.
>> 
>
> It is still an unexpected and undocumented change in behavior. Previously
> in that situation the watchdog timeout would happen much later and give
> user space time to start the watchdog daemon. This is no longer the case.

Do you mean that, in the case WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the
driver now doesn't refresh the watchdog, this is unexpected ? I was relying on
the fact that this entry is enabled by default.
In this case, should the driver refresh the watchdog itself, and set the
WDOG_HW_RUNNING bit (for the added feature that it does it repeatedly) ?
Or document the fact that the driver will not refresh the watchdog if
WATCHDOG_HANDLE_BOOT_ENABLED is disabled ?

>
> Guenter

Thanks,
Paul.



-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[Guenter Roeck]

On 7/6/26 08:44, Paul Louvel wrote:
> On Mon Jul 6, 2026 at 4:22 PM CEST, Guenter Roeck wrote:
>> On 7/6/26 04:15, Paul Louvel wrote:
>>> On Mon Jul 6, 2026 at 12:40 PM CEST, sashiko-bot wrote:
>>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>>> - [High] Removing the manual hardware timeout refresh while still unconditionally switching the hardware timer base from minutes to seconds drastically truncates the remaining timeout if the BIOS originally configured it in minutes, risking unexpected system reboots.
>>>> --
>>>>
>>>> commit 0014048ec369e2a09098aa033aeb725449e2fd67
>>>> Author: Paul Louvel <paul.louvel@bootlin.com>
>>>>
>>>> watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot
>>>>
>>>> This patch updates the w83627hf watchdog driver to rely on the watchdog
>>>> core's WDOG_HW_RUNNING flag to keep a running watchdog alive during boot.
>>>> It replaces the previous behavior of manually issuing a single keepalive
>>>> during initialization.
>>>>
>>>>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>>>>> --- a/drivers/watchdog/w83627hf_wdt.c
>>>>> +++ b/drivers/watchdog/w83627hf_wdt.c
>>>> [ ... ]
>>>>> @@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>>>>    			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
>>>>>    			superio_outb(cr_wdt_timeout, 0);
>>>>>    		} else {
>>>>> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
>>>>> -				wdog->timeout);
>>>>> -			superio_outb(cr_wdt_timeout, wdog->timeout);
>>>>> +			pr_info("Watchdog already running.\n");
>>>>> +			set_bit(WDOG_HW_RUNNING, &wdog->status);
>>>>>    		}
>>>>>    	}
>>>>>    
>>>>
>>>> [Severity: High]
>>>> Does removing this explicit timeout reset create a risk of unexpected reboots?
>>>>
>>>> If the BIOS originally configured the watchdog with a timeout in minutes
>>>> (for example, a timeout of 5 minutes), the hardware counter cr_wdt_timeout
>>>> will hold the value 5.
>>>
>>> First of all, the driver does not support minute mode for now.
>>
>> The above feedback doesn't claim otherwise. The BIOS may still have configured
>> the timeout to 5 minutes.
>>
>>>
>>>>
>>>> Because this patch removes the explicit write of wdog->timeout (which is
>>>> typically 60) to cr_wdt_timeout, the counter retains the raw value 5.
>>>> Just after this block in w83627hf_init(), the driver unconditionally
>>>> switches the timebase to seconds:
>>>>
>>>> w83627hf_init() {
>>>>       ...
>>>>       /* set second mode & disable keyboard turning off watchdog */
>>>>       t = superio_inb(cr_wdt_control) & ~0x0C;
>>>>       superio_outb(cr_wdt_control, t);
>>>>       ...
>>>> }
>>>>
>>>> This immediately shrinks the remaining timeout from 5 minutes to 5 seconds.
>>>
>>> True, but in those 5 seconds, the watchdog core will kick the watchdog
>>> immediately. by calling start(), and thus refreshing the watchdog to 60 seconds.
>>
>> ... unless below.
>>
>>>
>>>>
>>>> If the watchdog core's kworker is delayed, or if the system is configured
> 
> 
> 
>>>> not to ping the watchdog before user space takes over, could this unusually
>>>> short timeout cause the system to reboot unexpectedly during early boot?
>>>
>>> Yes, but expected. If WATCHDOG is enabled, then WATCHDOG_HANDLE_BOOT_ENABLED is
>>> enabled by default unless explicitly disabled when configuring the kernel.
>>>
>>
>> It is still an unexpected and undocumented change in behavior. Previously
>> in that situation the watchdog timeout would happen much later and give
>> user space time to start the watchdog daemon. This is no longer the case.
> 
> Do you mean that, in the case WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the
> driver now doesn't refresh the watchdog, this is unexpected ? I was relying on
> the fact that this entry is enabled by default.

It can be disabled with a configuration option, and with a module parameter.

> In this case, should the driver refresh the watchdog itself, and set the
> WDOG_HW_RUNNING bit (for the added feature that it does it repeatedly) ?
> Or document the fact that the driver will not refresh the watchdog if
> WATCHDOG_HANDLE_BOOT_ENABLED is disabled ?
> 

The difference is that the timeout used to be 60 seconds, or whatever the
default driver timeout is set to. If the BIOS enables the hardware timeout
and sets it to <n> minutes, your patch changes that to <n> seconds. That
is unexpected.

I see no reason to drop "superio_outb(cr_wdt_timeout, wdog->timeout);"
in your patch. Keeping it would retain existing behavior. Changing that
is unacceptable unless you provide a really good reason for it that I
currently can not see.

Thanks,
Guenter


	

Re: [PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[Paul Louvel]

On Mon Jul 6, 2026 at 6:12 PM CEST, Guenter Roeck wrote:
> On 7/6/26 08:44, Paul Louvel wrote:
>> On Mon Jul 6, 2026 at 4:22 PM CEST, Guenter Roeck wrote:
>>> On 7/6/26 04:15, Paul Louvel wrote:
>>>> On Mon Jul 6, 2026 at 12:40 PM CEST, sashiko-bot wrote:
>>>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>>>> - [High] Removing the manual hardware timeout refresh while still unconditionally switching the hardware timer base from minutes to seconds drastically truncates the remaining timeout if the BIOS originally configured it in minutes, risking unexpected system reboots.
>>>>> --
>>>>>
>>>>> commit 0014048ec369e2a09098aa033aeb725449e2fd67
>>>>> Author: Paul Louvel <paul.louvel@bootlin.com>
>>>>>
>>>>> watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot
>>>>>
>>>>> This patch updates the w83627hf watchdog driver to rely on the watchdog
>>>>> core's WDOG_HW_RUNNING flag to keep a running watchdog alive during boot.
>>>>> It replaces the previous behavior of manually issuing a single keepalive
>>>>> during initialization.
>>>>>
>>>>>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>>>>>> --- a/drivers/watchdog/w83627hf_wdt.c
>>>>>> +++ b/drivers/watchdog/w83627hf_wdt.c
>>>>> [ ... ]
>>>>>> @@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>>>>>    			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
>>>>>>    			superio_outb(cr_wdt_timeout, 0);
>>>>>>    		} else {
>>>>>> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
>>>>>> -				wdog->timeout);
>>>>>> -			superio_outb(cr_wdt_timeout, wdog->timeout);
>>>>>> +			pr_info("Watchdog already running.\n");
>>>>>> +			set_bit(WDOG_HW_RUNNING, &wdog->status);
>>>>>>    		}
>>>>>>    	}
>>>>>>    
>>>>>
>>>>> [Severity: High]
>>>>> Does removing this explicit timeout reset create a risk of unexpected reboots?
>>>>>
>>>>> If the BIOS originally configured the watchdog with a timeout in minutes
>>>>> (for example, a timeout of 5 minutes), the hardware counter cr_wdt_timeout
>>>>> will hold the value 5.
>>>>
>>>> First of all, the driver does not support minute mode for now.
>>>
>>> The above feedback doesn't claim otherwise. The BIOS may still have configured
>>> the timeout to 5 minutes.
>>>
>>>>
>>>>>
>>>>> Because this patch removes the explicit write of wdog->timeout (which is
>>>>> typically 60) to cr_wdt_timeout, the counter retains the raw value 5.
>>>>> Just after this block in w83627hf_init(), the driver unconditionally
>>>>> switches the timebase to seconds:
>>>>>
>>>>> w83627hf_init() {
>>>>>       ...
>>>>>       /* set second mode & disable keyboard turning off watchdog */
>>>>>       t = superio_inb(cr_wdt_control) & ~0x0C;
>>>>>       superio_outb(cr_wdt_control, t);
>>>>>       ...
>>>>> }
>>>>>
>>>>> This immediately shrinks the remaining timeout from 5 minutes to 5 seconds.
>>>>
>>>> True, but in those 5 seconds, the watchdog core will kick the watchdog
>>>> immediately. by calling start(), and thus refreshing the watchdog to 60 seconds.
>>>
>>> ... unless below.
>>>
>>>>
>>>>>
>>>>> If the watchdog core's kworker is delayed, or if the system is configured
>> 
>> 
>> 
>>>>> not to ping the watchdog before user space takes over, could this unusually
>>>>> short timeout cause the system to reboot unexpectedly during early boot?
>>>>
>>>> Yes, but expected. If WATCHDOG is enabled, then WATCHDOG_HANDLE_BOOT_ENABLED is
>>>> enabled by default unless explicitly disabled when configuring the kernel.
>>>>
>>>
>>> It is still an unexpected and undocumented change in behavior. Previously
>>> in that situation the watchdog timeout would happen much later and give
>>> user space time to start the watchdog daemon. This is no longer the case.
>> 
>> Do you mean that, in the case WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the
>> driver now doesn't refresh the watchdog, this is unexpected ? I was relying on
>> the fact that this entry is enabled by default.
>
> It can be disabled with a configuration option, and with a module parameter.
>
>> In this case, should the driver refresh the watchdog itself, and set the
>> WDOG_HW_RUNNING bit (for the added feature that it does it repeatedly) ?
>> Or document the fact that the driver will not refresh the watchdog if
>> WATCHDOG_HANDLE_BOOT_ENABLED is disabled ?
>> 
>
> The difference is that the timeout used to be 60 seconds, or whatever the
> default driver timeout is set to. If the BIOS enables the hardware timeout
> and sets it to <n> minutes, your patch changes that to <n> seconds. That
> is unexpected.

The default driver timeout is set to 60 seconds. I do not understand your point.
If the BIOS enables the hardware timeout and sets it to <n> minutes, without my
patch, the driver would still refresh the watchdog to <n> seconds (60 actually),
with "superio_outb(cr_wdt_timeout, wdog->timeout);". It forces second mode in
the control register anyway, and the driver has no way to know the previously
set timeout.
The only difference I see is that it does it immediately and does not rely on
the core to do it. It also do it only code.
Maybe I do not see something here.

>
> I see no reason to drop "superio_outb(cr_wdt_timeout, wdog->timeout);"
> in your patch. Keeping it would retain existing behavior. Changing that
> is unacceptable unless you provide a really good reason for it that I
> currently can not see.
>
> Thanks,
> Guenter
>
>
> 	

Thanks,
Paul.




-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[Guenter Roeck]

On 7/6/26 14:42, Paul Louvel wrote:
> On Mon Jul 6, 2026 at 6:12 PM CEST, Guenter Roeck wrote:
>> On 7/6/26 08:44, Paul Louvel wrote:
>>> On Mon Jul 6, 2026 at 4:22 PM CEST, Guenter Roeck wrote:
>>>> On 7/6/26 04:15, Paul Louvel wrote:
>>>>> On Mon Jul 6, 2026 at 12:40 PM CEST, sashiko-bot wrote:
>>>>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>>>>> - [High] Removing the manual hardware timeout refresh while still unconditionally switching the hardware timer base from minutes to seconds drastically truncates the remaining timeout if the BIOS originally configured it in minutes, risking unexpected system reboots.
>>>>>> --
>>>>>>
>>>>>> commit 0014048ec369e2a09098aa033aeb725449e2fd67
>>>>>> Author: Paul Louvel <paul.louvel@bootlin.com>
>>>>>>
>>>>>> watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot
>>>>>>
>>>>>> This patch updates the w83627hf watchdog driver to rely on the watchdog
>>>>>> core's WDOG_HW_RUNNING flag to keep a running watchdog alive during boot.
>>>>>> It replaces the previous behavior of manually issuing a single keepalive
>>>>>> during initialization.
>>>>>>
>>>>>>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>>>>>>> --- a/drivers/watchdog/w83627hf_wdt.c
>>>>>>> +++ b/drivers/watchdog/w83627hf_wdt.c
>>>>>> [ ... ]
>>>>>>> @@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>>>>>>     			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
>>>>>>>     			superio_outb(cr_wdt_timeout, 0);
>>>>>>>     		} else {
>>>>>>> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
>>>>>>> -				wdog->timeout);
>>>>>>> -			superio_outb(cr_wdt_timeout, wdog->timeout);
>>>>>>> +			pr_info("Watchdog already running.\n");
>>>>>>> +			set_bit(WDOG_HW_RUNNING, &wdog->status);
>>>>>>>     		}
>>>>>>>     	}
>>>>>>>     
>>>>>>
>>>>>> [Severity: High]
>>>>>> Does removing this explicit timeout reset create a risk of unexpected reboots?
>>>>>>
>>>>>> If the BIOS originally configured the watchdog with a timeout in minutes
>>>>>> (for example, a timeout of 5 minutes), the hardware counter cr_wdt_timeout
>>>>>> will hold the value 5.
>>>>>
>>>>> First of all, the driver does not support minute mode for now.
>>>>
>>>> The above feedback doesn't claim otherwise. The BIOS may still have configured
>>>> the timeout to 5 minutes.
>>>>
>>>>>
>>>>>>
>>>>>> Because this patch removes the explicit write of wdog->timeout (which is
>>>>>> typically 60) to cr_wdt_timeout, the counter retains the raw value 5.
>>>>>> Just after this block in w83627hf_init(), the driver unconditionally
>>>>>> switches the timebase to seconds:
>>>>>>
>>>>>> w83627hf_init() {
>>>>>>        ...
>>>>>>        /* set second mode & disable keyboard turning off watchdog */
>>>>>>        t = superio_inb(cr_wdt_control) & ~0x0C;
>>>>>>        superio_outb(cr_wdt_control, t);
>>>>>>        ...
>>>>>> }
>>>>>>
>>>>>> This immediately shrinks the remaining timeout from 5 minutes to 5 seconds.
>>>>>
>>>>> True, but in those 5 seconds, the watchdog core will kick the watchdog
>>>>> immediately. by calling start(), and thus refreshing the watchdog to 60 seconds.
>>>>
>>>> ... unless below.
>>>>
>>>>>
>>>>>>
>>>>>> If the watchdog core's kworker is delayed, or if the system is configured
>>>
>>>
>>>
>>>>>> not to ping the watchdog before user space takes over, could this unusually
>>>>>> short timeout cause the system to reboot unexpectedly during early boot?
>>>>>
>>>>> Yes, but expected. If WATCHDOG is enabled, then WATCHDOG_HANDLE_BOOT_ENABLED is
>>>>> enabled by default unless explicitly disabled when configuring the kernel.
>>>>>
>>>>
>>>> It is still an unexpected and undocumented change in behavior. Previously
>>>> in that situation the watchdog timeout would happen much later and give
>>>> user space time to start the watchdog daemon. This is no longer the case.
>>>
>>> Do you mean that, in the case WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the
>>> driver now doesn't refresh the watchdog, this is unexpected ? I was relying on
>>> the fact that this entry is enabled by default.
>>
>> It can be disabled with a configuration option, and with a module parameter.
>>
>>> In this case, should the driver refresh the watchdog itself, and set the
>>> WDOG_HW_RUNNING bit (for the added feature that it does it repeatedly) ?
>>> Or document the fact that the driver will not refresh the watchdog if
>>> WATCHDOG_HANDLE_BOOT_ENABLED is disabled ?
>>>
>>
>> The difference is that the timeout used to be 60 seconds, or whatever the
>> default driver timeout is set to. If the BIOS enables the hardware timeout
>> and sets it to <n> minutes, your patch changes that to <n> seconds. That
>> is unexpected.
> 
> The default driver timeout is set to 60 seconds. I do not understand your point.
> If the BIOS enables the hardware timeout and sets it to <n> minutes, without my
> patch, the driver would still refresh the watchdog to <n> seconds (60 actually),
> with "superio_outb(cr_wdt_timeout, wdog->timeout);". It forces second mode in
> the control register anyway, and the driver has no way to know the previously
> set timeout.
> The only difference I see is that it does it immediately and does not rely on
> the core to do it. It also do it only code.
> Maybe I do not see something here.
> 

Please provide the data path showing how the watchdog core sets the timeout
to 60 seconds if WATCHDOG_HANDLE_BOOT_ENABLED is not enabled.

Thanks,
Guenter

>>
>> I see no reason to drop "superio_outb(cr_wdt_timeout, wdog->timeout);"
>> in your patch. Keeping it would retain existing behavior. Changing that
>> is unacceptable unless you provide a really good reason for it that I
>> currently can not see.
>>
>> Thanks,
>> Guenter
>>
>>
>> 	
> 
> Thanks,
> Paul.
> 
> 
> 
> 

Re: [PATCH v3 2/2] watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot

[Paul Louvel]

On Mon Jul 6, 2026 at 11:48 PM CEST, Guenter Roeck wrote:
> On 7/6/26 14:42, Paul Louvel wrote:
>> On Mon Jul 6, 2026 at 6:12 PM CEST, Guenter Roeck wrote:
>>> On 7/6/26 08:44, Paul Louvel wrote:
>>>> On Mon Jul 6, 2026 at 4:22 PM CEST, Guenter Roeck wrote:
>>>>> On 7/6/26 04:15, Paul Louvel wrote:
>>>>>> On Mon Jul 6, 2026 at 12:40 PM CEST, sashiko-bot wrote:
>>>>>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>>>>>> - [High] Removing the manual hardware timeout refresh while still unconditionally switching the hardware timer base from minutes to seconds drastically truncates the remaining timeout if the BIOS originally configured it in minutes, risking unexpected system reboots.
>>>>>>> --
>>>>>>>
>>>>>>> commit 0014048ec369e2a09098aa033aeb725449e2fd67
>>>>>>> Author: Paul Louvel <paul.louvel@bootlin.com>
>>>>>>>
>>>>>>> watchdog: w83627hf_wdt: Use WDOG_HW_RUNNING for running chip on boot
>>>>>>>
>>>>>>> This patch updates the w83627hf watchdog driver to rely on the watchdog
>>>>>>> core's WDOG_HW_RUNNING flag to keep a running watchdog alive during boot.
>>>>>>> It replaces the previous behavior of manually issuing a single keepalive
>>>>>>> during initialization.
>>>>>>>
>>>>>>>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>>>>>>>> --- a/drivers/watchdog/w83627hf_wdt.c
>>>>>>>> +++ b/drivers/watchdog/w83627hf_wdt.c
>>>>>>> [ ... ]
>>>>>>>> @@ -241,9 +241,8 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>>>>>>>     			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
>>>>>>>>     			superio_outb(cr_wdt_timeout, 0);
>>>>>>>>     		} else {
>>>>>>>> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
>>>>>>>> -				wdog->timeout);
>>>>>>>> -			superio_outb(cr_wdt_timeout, wdog->timeout);
>>>>>>>> +			pr_info("Watchdog already running.\n");
>>>>>>>> +			set_bit(WDOG_HW_RUNNING, &wdog->status);
>>>>>>>>     		}
>>>>>>>>     	}
>>>>>>>>     
>>>>>>>
>>>>>>> [Severity: High]
>>>>>>> Does removing this explicit timeout reset create a risk of unexpected reboots?
>>>>>>>
>>>>>>> If the BIOS originally configured the watchdog with a timeout in minutes
>>>>>>> (for example, a timeout of 5 minutes), the hardware counter cr_wdt_timeout
>>>>>>> will hold the value 5.
>>>>>>
>>>>>> First of all, the driver does not support minute mode for now.
>>>>>
>>>>> The above feedback doesn't claim otherwise. The BIOS may still have configured
>>>>> the timeout to 5 minutes.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Because this patch removes the explicit write of wdog->timeout (which is
>>>>>>> typically 60) to cr_wdt_timeout, the counter retains the raw value 5.
>>>>>>> Just after this block in w83627hf_init(), the driver unconditionally
>>>>>>> switches the timebase to seconds:
>>>>>>>
>>>>>>> w83627hf_init() {
>>>>>>>        ...
>>>>>>>        /* set second mode & disable keyboard turning off watchdog */
>>>>>>>        t = superio_inb(cr_wdt_control) & ~0x0C;
>>>>>>>        superio_outb(cr_wdt_control, t);
>>>>>>>        ...
>>>>>>> }
>>>>>>>
>>>>>>> This immediately shrinks the remaining timeout from 5 minutes to 5 seconds.
>>>>>>
>>>>>> True, but in those 5 seconds, the watchdog core will kick the watchdog
>>>>>> immediately. by calling start(), and thus refreshing the watchdog to 60 seconds.
>>>>>
>>>>> ... unless below.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> If the watchdog core's kworker is delayed, or if the system is configured
>>>>
>>>>
>>>>
>>>>>>> not to ping the watchdog before user space takes over, could this unusually
>>>>>>> short timeout cause the system to reboot unexpectedly during early boot?
>>>>>>
>>>>>> Yes, but expected. If WATCHDOG is enabled, then WATCHDOG_HANDLE_BOOT_ENABLED is
>>>>>> enabled by default unless explicitly disabled when configuring the kernel.
>>>>>>
>>>>>
>>>>> It is still an unexpected and undocumented change in behavior. Previously
>>>>> in that situation the watchdog timeout would happen much later and give
>>>>> user space time to start the watchdog daemon. This is no longer the case.
>>>>
>>>> Do you mean that, in the case WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the
>>>> driver now doesn't refresh the watchdog, this is unexpected ? I was relying on
>>>> the fact that this entry is enabled by default.
>>>
>>> It can be disabled with a configuration option, and with a module parameter.
>>>
>>>> In this case, should the driver refresh the watchdog itself, and set the
>>>> WDOG_HW_RUNNING bit (for the added feature that it does it repeatedly) ?
>>>> Or document the fact that the driver will not refresh the watchdog if
>>>> WATCHDOG_HANDLE_BOOT_ENABLED is disabled ?
>>>>
>>>
>>> The difference is that the timeout used to be 60 seconds, or whatever the
>>> default driver timeout is set to. If the BIOS enables the hardware timeout
>>> and sets it to <n> minutes, your patch changes that to <n> seconds. That
>>> is unexpected.
>> 
>> The default driver timeout is set to 60 seconds. I do not understand your point.
>> If the BIOS enables the hardware timeout and sets it to <n> minutes, without my
>> patch, the driver would still refresh the watchdog to <n> seconds (60 actually),
>> with "superio_outb(cr_wdt_timeout, wdog->timeout);". It forces second mode in
>> the control register anyway, and the driver has no way to know the previously
>> set timeout.
>> The only difference I see is that it does it immediately and does not rely on
>> the core to do it. It also do it only code.
>> Maybe I do not see something here.
>> 
>
> Please provide the data path showing how the watchdog core sets the timeout
> to 60 seconds if WATCHDOG_HANDLE_BOOT_ENABLED is not enabled.

If WATCHDOG_HANDLE_BOOT_ENABLED is not enabled, then I understand the issue.
Would an additional check at compile time be sufficient ?

diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
index 27ef1cce3a14..c6d470be62a8 100644
--- a/drivers/watchdog/w83627hf_wdt.c
+++ b/drivers/watchdog/w83627hf_wdt.c
@@ -242,7 +242,11 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
                        superio_outb(cr_wdt_timeout, 0);
                } else {
                        pr_info("Watchdog already running.\n");
+#ifdef CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED
                        set_bit(WDOG_HW_RUNNING, &wdog->status);
+#else
+                       superio_outb(cr_wdt_timeout, wdog->timeout);
+#endif
                }
        }

>
> Thanks,
> Guenter
>

Thanks,
Paul.


-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com