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

[sashiko-bot@kernel.org]

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?

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-w83627hf_wdt-improvements-v4-0-b6f6b60a9566@bootlin.com?part=6