Re: wdat_wdt: access width inconsistency

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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.