On Tue, Feb 11, 2020 at 05:25:33PM +0100, Jean Delvare wrote: > On Tue, 11 Feb 2020 15:59:44 +0200, Mika Westerberg wrote: > > If the default timeout is short then that might happen but I think WDAT > > spec had some "reasonable" lower limit. > > Could you please point me to the WDAT specification? Somehow my web > search failed to spot it. You can find it here: http://msdn.microsoft.com/en-us/windows/hardware/gg463320.aspx Most of the ACPI related documents not part of the spec itself are listed in the following page: https://uefi.org/acpi > > > You may set bigger default timeout during the probe by doing something > > like below. This should give some 30s time before the system is rebooted > > after the device is opened. > > > > diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c > > index b069349b52f5..24351afe2718 100644 > > --- a/drivers/watchdog/wdat_wdt.c > > +++ b/drivers/watchdog/wdat_wdt.c > > @@ -439,6 +439,10 @@ static int wdat_wdt_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, wdat); > > > > watchdog_set_nowayout(&wdat->wdd, nowayout); > > + > > + /* Increase default timeout */ > > + wdat_wdt_set_timeout(&wdat->wdd, 30 * 1000); > > + > > return devm_watchdog_register_device(dev, &wdat->wdd); > > } > > That is a very valid point. We know that the device works fine when > using the iTCO_wdt driver, and the iTCO_wdt driver *does* set a timeout > value at probe time, it does not rely on the BIOS having set a sane > value beforehand. So maybe that's the problem. > > Guenter, what is considered best practice for watchdog drivers in this > respect? Trust the BIOS or set an arbitrary timeout value? > > Maybe we should read the timeout value before enabling the device, and > if it is unreasonably low (< 5 seconds), log a warning and reset the > value to a sane default (30 seconds)? Yes, that would work. > Alternatively, or in addition to that, maybe the wdat_wdt driver should > have a module parameter to set the default timeout value, as the > iTCO_wdt driver has? Or is this deprecated in favor of the > WDIOC_SETTIMEOUT ioctl? Problem with the ioctl is that it requires the > device node to be opened, which starts the count down (I think?) Indeed it seems to be the case if I understand watchdog core code correctly.
