Message-ID: <20220806000706.3eeafc9c@endymion.delvare>
Date:   Sat, 6 Aug 2022 00:07:06 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     linux-watchdog@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Cc:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: [PATCH] watchdog: wdat_wdt: Set the min and max timeout values
 properly

The wdat_wdt driver is misusing the min_hw_heartbeat_ms field. This
field should only be used when the hardware watchdog device should not
be pinged more frequently than a specific period. The ACPI WDAT
"Minimum Count" field, on the other hand, specifies the minimum
timeout value that can be set. This corresponds to the min_timeout
field in Linux's watchdog infrastructure.

Setting min_hw_heartbeat_ms instead can cause pings to the hardware
to be delayed when there is no reason for that, eventually leading to
unexpected firing of the watchdog timer (and thus unexpected reboot).

I'm also changing max_hw_heartbeat_ms to max_timeout for symmetry,
although the use of this one isn't fundamentally wrong, but there is
also no reason to enable the software-driven ping mechanism for the
wdat_wdt driver.

Signed-off-by: Jean Delvare <jdelvare@...e.de>
Fixes: 058dfc767008 ("ACPI / watchdog: Add support for WDAT hardware watchdog")
Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>
Cc: Guenter Roeck <linux@...ck-us.net>
Cc! Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
---
Untested, as I have no supported hardware at hand.

Note to the watchdog subsystem maintainers: I must say I find the
whole thing pretty confusing.

First of all, the name symmetry between min_hw_heartbeat_ms and
max_hw_heartbeat_ms, while these properties are completely unrelated,
is heavily misleading. max_hw_heartbeat_ms is really max_hw_timeout
and should be renamed to that IMHO, if we keep it at all.

Secondly, the coexistence of max_timeout and max_hw_heartbeat_ms is
also making the code pretty hard to understand and get right.
Historically, max_timeout was already supposed to be the maximum
hardware timeout value. I don't understand why a new field with that
meaning was introduced, subsequently changing the original meaning of
max_timeout to become a software-only limit... but only if
max_hw_heartbeat_ms is set.

To be honest, I'm not sold to the idea of a software-emulated
maximum timeout value above what the hardware can do, but if doing
that makes sense in certain situations, then I believe it should be
implemented as a boolean flag (named emulate_large_timeout, for
example) to complement max_timeout instead of a separate time value.
Is there a reason I'm missing, why it was not done that way?

Currently, a comment in watchdog.h claims that max_timeout is ignored
when max_hw_heartbeat_ms is set. However in watchdog_dev.c, sysfs
attribute max_timeout is created unconditionally, and
max_hw_heartbeat_ms doesn't have a sysfs attribute. So userspace has
no way to know if max_timeout is the hardware limit, or whether
software emulation will kick in for a specified timeout value. Also,
there is no complaint if both max_hw_heartbeat_ms and max_timeout
are set.

 drivers/watchdog/wdat_wdt.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux-5.18.orig/drivers/watchdog/wdat_wdt.c	2022-07-27 07:32:33.336928967 +0200
+++ linux-5.18/drivers/watchdog/wdat_wdt.c	2022-08-05 19:49:19.607793835 +0200
@@ -342,8 +342,8 @@ static int wdat_wdt_probe(struct platfor
 		return -EINVAL;
 
 	wdat->period = tbl->timer_period;
-	wdat->wdd.min_hw_heartbeat_ms = wdat->period * tbl->min_count;
-	wdat->wdd.max_hw_heartbeat_ms = wdat->period * tbl->max_count;
+	wdat->wdd.min_timeout = DIV_ROUND_UP(wdat->period * tbl->min_count, 1000);
+	wdat->wdd.max_timeout = wdat->period * tbl->max_count / 1000;
 	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
 	wdat->wdd.info = &wdat_wdt_info;
 	wdat->wdd.ops = &wdat_wdt_ops;
@@ -450,8 +450,8 @@ static int wdat_wdt_probe(struct platfor
 	 * watchdog properly after it has opened the device. In some cases
 	 * the BIOS default is too short and causes immediate reboot.
 	 */
-	if (timeout * 1000 < wdat->wdd.min_hw_heartbeat_ms ||
-	    timeout * 1000 > wdat->wdd.max_hw_heartbeat_ms) {
+	if (timeout < wdat->min_timeout ||
+	    timeout > wdat->max_timeout) {
 		dev_warn(dev, "Invalid timeout %d given, using %d\n",
 			 timeout, WDAT_DEFAULT_TIMEOUT);
 		timeout = WDAT_DEFAULT_TIMEOUT;


-- 
Jean Delvare
SUSE L3 Support

Powered by blists - more mailing lists