[PATCH v8 0/3] Some impovements about acpi hardware watchdog

[PATCH v8 0/3] Some impovements about acpi hardware watchdog

[Liu Xinpeng]

Changelog:
v1->v2 Update the commit messages

v2->v3 - Add the context about why using watchdog_timeout_invalid.
       - Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code for
       iTCO watchdog.

v3->v4 - For patch 1, update commit message, rename WDAT_TIMEOUT_MIN
       to WDAT_MIN_TIMEOUT, keeps consistent with WDAT_DEFAULT_TIMEOUT.
       - For patch 4, iTCO_wdt_suspend_noirq and iTCO_wdt_resume_noirq
       are possible unused, so keep "ifdef CONFIG_PM_SLEEP ... #endif".

v4->v5 Split iTCO_wdt's commit from this patchset.

v5->v6 For patch 1, drop "Context: " and to change "existed" in the
       subject to "existing".

v6->v7 Rename "watchdog: wdat_wdg" in subject to "watchdog: wdat_wdt".

v7->v8 Set min_timeout to 1 without using a define.

Liu Xinpeng (3):
  watchdog: wdat_wdt: Using the existing function to check parameter
    timeout
  watchdog: wdat_wdt: Stop watchdog when rebooting the system
  watchdog: wdat_wdt: Stop watchdog when uninstalling module

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

-- 
2.23.0

[PATCH v8 1/3] watchdog: wdat_wdt: Using the existing function to check parameter timeout

[Liu Xinpeng]

If max_hw_heartbeat_ms is provided, the configured maximum timeout is not
limited by it. The limit check in this driver therefore doesn't make much
sense. Similar, the watchdog core ensures that minimum timeout limits are
met if min_hw_heartbeat_ms is set. Using watchdog_timeout_invalid() makes
more sense because it takes this into account.

Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
---
 drivers/watchdog/wdat_wdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 195c8c004b69..df0865a61a70 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -344,6 +344,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	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 = 1;
 	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
 	wdat->wdd.info = &wdat_wdt_info;
 	wdat->wdd.ops = &wdat_wdt_ops;
@@ -450,8 +451,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	 * 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 (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
 		dev_warn(dev, "Invalid timeout %d given, using %d\n",
 			 timeout, WDAT_DEFAULT_TIMEOUT);
 		timeout = WDAT_DEFAULT_TIMEOUT;
-- 
2.23.0

Re: [PATCH v8 1/3] watchdog: wdat_wdt: Using the existing function to check parameter timeout

[Guenter Roeck]

On Tue, Apr 26, 2022 at 10:53:28PM +0800, Liu Xinpeng wrote:
> If max_hw_heartbeat_ms is provided, the configured maximum timeout is not
> limited by it. The limit check in this driver therefore doesn't make much
> sense. Similar, the watchdog core ensures that minimum timeout limits are
> met if min_hw_heartbeat_ms is set. Using watchdog_timeout_invalid() makes
> more sense because it takes this into account.
> 
> Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>

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

> ---
>  drivers/watchdog/wdat_wdt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index 195c8c004b69..df0865a61a70 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -344,6 +344,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  	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 = 1;
>  	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
>  	wdat->wdd.info = &wdat_wdt_info;
>  	wdat->wdd.ops = &wdat_wdt_ops;
> @@ -450,8 +451,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  	 * 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 (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
>  		dev_warn(dev, "Invalid timeout %d given, using %d\n",
>  			 timeout, WDAT_DEFAULT_TIMEOUT);
>  		timeout = WDAT_DEFAULT_TIMEOUT;
> -- 
> 2.23.0
> 

[PATCH v8 2/3] watchdog: wdat_wdt: Stop watchdog when rebooting the system

[Liu Xinpeng]

Executing reboot command several times on the machine "Dell
PowerEdge R740", UEFI security detection stopped machine
with the following prompt:

UEFI0082: The system was reset due to a timeout from the watchdog
timer. Check the System Event Log (SEL) or crash dumps from
Operating Sysstem to identify the source that triggered the
watchdog timer reset. Update the firmware or driver for the
identified device.

iDRAC has warning event: "The watchdog timer reset the system".

This patch fixes this issue by adding the reboot notifier.

Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
---
 drivers/watchdog/wdat_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index df0865a61a70..6f36a653767b 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -462,6 +462,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 		return ret;
 
 	watchdog_set_nowayout(&wdat->wdd, nowayout);
+	watchdog_stop_on_reboot(&wdat->wdd);
 	return devm_watchdog_register_device(dev, &wdat->wdd);
 }
 
-- 
2.23.0

Re: [PATCH v8 2/3] watchdog: wdat_wdt: Stop watchdog when rebooting the system

[Guenter Roeck]

On Tue, Apr 26, 2022 at 10:53:29PM +0800, Liu Xinpeng wrote:
> Executing reboot command several times on the machine "Dell
> PowerEdge R740", UEFI security detection stopped machine
> with the following prompt:
> 
> UEFI0082: The system was reset due to a timeout from the watchdog
> timer. Check the System Event Log (SEL) or crash dumps from
> Operating Sysstem to identify the source that triggered the
> watchdog timer reset. Update the firmware or driver for the
> identified device.
> 
> iDRAC has warning event: "The watchdog timer reset the system".
> 
> This patch fixes this issue by adding the reboot notifier.
> 
> Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>

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

> ---
>  drivers/watchdog/wdat_wdt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index df0865a61a70..6f36a653767b 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -462,6 +462,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	watchdog_set_nowayout(&wdat->wdd, nowayout);
> +	watchdog_stop_on_reboot(&wdat->wdd);
>  	return devm_watchdog_register_device(dev, &wdat->wdd);
>  }
>  
> -- 
> 2.23.0
> 

[PATCH v8 3/3] watchdog: wdat_wdt: Stop watchdog when uninstalling module

[Liu Xinpeng]

Test shows that wachdog still reboots machine after the module
is removed. Use watchdog_stop_on_unregister to stop the watchdog
on removing.

Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>
---
 drivers/watchdog/wdat_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 6f36a653767b..e6f95e99156d 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -463,6 +463,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_nowayout(&wdat->wdd, nowayout);
 	watchdog_stop_on_reboot(&wdat->wdd);
+	watchdog_stop_on_unregister(&wdat->wdd);
 	return devm_watchdog_register_device(dev, &wdat->wdd);
 }
 
-- 
2.23.0

Re: [PATCH v8 3/3] watchdog: wdat_wdt: Stop watchdog when uninstalling module

[Guenter Roeck]

On Tue, Apr 26, 2022 at 10:53:30PM +0800, Liu Xinpeng wrote:
> Test shows that wachdog still reboots machine after the module
> is removed. Use watchdog_stop_on_unregister to stop the watchdog
> on removing.
> 
> Signed-off-by: Liu Xinpeng <liuxp11@chinatelecom.cn>

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

> ---
>  drivers/watchdog/wdat_wdt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index 6f36a653767b..e6f95e99156d 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -463,6 +463,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  
>  	watchdog_set_nowayout(&wdat->wdd, nowayout);
>  	watchdog_stop_on_reboot(&wdat->wdd);
> +	watchdog_stop_on_unregister(&wdat->wdd);
>  	return devm_watchdog_register_device(dev, &wdat->wdd);
>  }
>  
> -- 
> 2.23.0
>