Re: [PATCH v4 6/6] watchdog: w83627hf_wdt: Add minute mode counting

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

On Tue Oct 13, 2026 at 4:08 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] watchdog: spurious hardware reboot during timeout mode transition
> --
>
> commit e9913194bed75434ff662763db21c93683fc01dc
> Author: Paul Louvel <paul.louvel@bootlin.com>
>
> watchdog: w83627hf_wdt: Add minute mode counting
>
> All of the chips supported by the driver can be set to minute mode,
> allowing timeout value up to 255 minutes. Add it to the driver.
>
>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>> index 0b9195e7e95ef..8f78fe64acde9 100644
>> --- a/drivers/watchdog/w83627hf_wdt.c
>> +++ b/drivers/watchdog/w83627hf_wdt.c
>
> [ ... ]
>
>> @@ -290,6 +284,7 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>>  static int wdt_set_time(struct watchdog_device *wdog, unsigned int timeout)
>>  {
>>  	struct w83627hf_data *data = watchdog_get_drvdata(wdog);
>> +	unsigned char ctrl;
>>  	int ret;
>>  
>>  	ret = superio_enter(data->sio->addr, data->sio->cfg_enter);
>> @@ -297,6 +292,15 @@ static int wdt_set_time(struct watchdog_device *wdog, unsigned int timeout)
>>  		return ret;
>>  
>>  	superio_select(data->sio->addr, W83627HF_LD_WDT);
>> +
>> +	ctrl = superio_inb(data->sio->addr, data->reg->control);
>> +
>> +	if (data->minute_mode)
>> +		ctrl |= WDT_CTRL_MINUTE_MODE;
>> +	else
>> +		ctrl &= ~WDT_CTRL_MINUTE_MODE;
>> +
>> +	superio_outb(data->sio->addr, data->reg->control, ctrl);
>>  	superio_outb(data->sio->addr, data->reg->timeout, timeout);
>
> [Severity: High]
> Can this sequence trigger a spurious hardware reboot during a timeout mode
> transition?
>
> In wdt_set_time(), the hardware control register is updated first to change
> between minute and second modes, immediately followed by the timeout register:
>
> 	superio_outb(data->sio->addr, data->reg->control, ctrl);
> 	superio_outb(data->sio->addr, data->reg->timeout, timeout);
>
> When transitioning from minute mode to second mode (for example, from 1 minute
> to 10 seconds), the hardware control register is updated first. The hardware
> timer momentarily retains its old value (1) but interprets it under the new
> unit (seconds), meaning it evaluates that only 1 second is left.
>
> If the thread is preempted (e.g., by an SMI or a hypervisor) between these two
> superio_outb() calls for longer than this intermediate duration, will the
> hardware watchdog fire and spuriously reboot the system?

Yes it could happen if the hardware does not take measures to avoid this. Also,
one second is a long time between those two writes, I doubt this ever happens in
practice.

>
> Would it be safer to temporarily disable the watchdog (by writing 0 to the
> timeout register) before modifying the control register's mode bit?
>

Yes.

>>  	superio_exit(data->sio->addr, data->sio->cfg_leave);
>>

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