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

["Paul Louvel" <paul.louvel@bootlin.com>]

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