[PATCH v2] watchdog: wdat_wdt: Set the min and max timeout values properly

Jean Delvare posted 1 patch 5 days, 16 hours ago
drivers/watchdog/wdat_wdt.c |    8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
[PATCH v2] watchdog: wdat_wdt: Set the min and max timeout values properly
Posted by Jean Delvare 5 days, 16 hours ago
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@suse.de>
Fixes: 058dfc767008 ("ACPI / watchdog: Add support for WDAT hardware watchdog")
Reviewed-by! Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
Changes since v1:
 * Fix a stupid typo which broke the build. Apparently I shouldn't be
   sending out patches after midnight, sorry.

 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-06 08:09:49.235935543 +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->wdd.min_timeout ||
+	    timeout > wdat->wdd.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
Re: [PATCH v2] watchdog: wdat_wdt: Set the min and max timeout values properly
Posted by Guenter Roeck 3 days, 11 hours ago
On 8/5/22 23:15, Jean Delvare wrote:
> 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.
> 

I dislike this part because it changes behavior and is unrelated to
the problem at hand, but I assume Mike knows the actual hardware limits
and understands the implications (ie that there is indeed no need to
enable the software-driven ping mechanism).

> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Fixes: 058dfc767008 ("ACPI / watchdog: Add support for WDAT hardware watchdog")
> Reviewed-by! Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> Changes since v1:
>   * Fix a stupid typo which broke the build. Apparently I shouldn't be
>     sending out patches after midnight, sorry.
> 
>   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-06 08:09:49.235935543 +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->wdd.min_timeout ||
> +	    timeout > wdat->wdd.max_timeout) {
>   		dev_warn(dev, "Invalid timeout %d given, using %d\n",
>   			 timeout, WDAT_DEFAULT_TIMEOUT);
>   		timeout = WDAT_DEFAULT_TIMEOUT;
> 
>