Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Jean Delvare <jdelvare@suse.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>,
	linux-acpi@vger.kernel.org,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	linux-watchdog@vger.kernel.org, Tom Abraham <tabraham@suse.com>
Subject: Re: wdat_wdt: access width inconsistency
Date: Tue, 11 Feb 2020 08:45:50 -0800
Message-ID: <20200211164550.GB2975@roeck-us.net> (raw)
In-Reply-To: <20200211172533.08b27181@endymion>

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 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?
> 
Drivers should either set a valid timeout or read the timeout from
the chip. That does not mean that they always do that. Either case,
trusting the BIOS/ROMMON is not a good idea, because it may change.
When enabling the watchdog, the driver should make sure that the chip
timeout matches what it believes it should be, and that the watchdog
doesn't trigger immediately but only after that timeout period
expires. Not all drivers do that either.

> 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)?
> 
> 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?)
> 
No, the module parameter hasn't been deprecated.

Guenter

  parent reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-10 10:16 Jean Delvare
2020-02-10 11:23 ` Mika Westerberg
2020-02-11 13:11   ` Jean Delvare
2020-02-11 13:59     ` Mika Westerberg
2020-02-11 16:25       ` Jean Delvare
2020-02-11 16:37         ` Mika Westerberg
2020-02-11 17:03           ` Jean Delvare
2020-02-12 11:05             ` [PATCH 1/3] ACPICA: Introduce ACPI_ACCESS_BIT_WIDTH() macro Mika Westerberg
2020-02-12 11:05               ` [PATCH 2/3] ACPI / watchdog: Fix gas->access_width usage Mika Westerberg
2020-02-12 11:56                 ` Jean Delvare
2020-02-12 12:10                   ` Mika Westerberg
2020-02-12 11:05               ` [PATCH 3/3] ACPI / watchdog: Set default timeout in probe Mika Westerberg
2020-02-12 12:07                 ` Jean Delvare
2020-02-12 12:13                   ` Mika Westerberg
2020-02-12 11:52               ` [PATCH 1/3] ACPICA: Introduce ACPI_ACCESS_BIT_WIDTH() macro Jean Delvare
2020-02-12 12:08                 ` Mika Westerberg
2020-02-11 16:45         ` Guenter Roeck [this message]
2020-02-12 10:30   ` wdat_wdt: access width inconsistency Jean Delvare
2020-02-12 10:47     ` Mika Westerberg
2020-02-12 11:05       ` Jean Delvare

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200211164550.GB2975@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=jdelvare@suse.de \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=rjw@rjwysocki.net \
    --cc=tabraham@suse.com \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git