[PATCH v3 0/4] Some Impovements about watchdog

[PATCH v3 0/4] Some Impovements about 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.

Liu Xinpeng (4):
  watchdog: wdat_wdg: Using the existed function to check parameter
    timeout
  watchdog: wdat_wdg: Stop watchdog when rebooting the system
  watchdog: wdat_wdg: Stop watchdog when uninstalling module
  watchdog: iTCO_wdg: Make code more clearly with macro definition

 drivers/watchdog/iTCO_wdt.c | 12 +++---------
 drivers/watchdog/wdat_wdt.c |  7 +++++--
 2 files changed, 8 insertions(+), 11 deletions(-)

-- 
2.23.0

[PATCH v3 1/4] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Liu Xinpeng]

The module arguement timeout is a configured timeout value.
“separate minimum and maximum HW timeouts and configured timeout value.”
(patch v1 is explained by Guenter Roeck)

So using watchdog_timeout_invalid to check timeout invalid is more justified.

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

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 195c8c004b69..3040a0554055 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -14,6 +14,7 @@
 #include <linux/watchdog.h>
 
 #define MAX_WDAT_ACTIONS ACPI_WDAT_ACTION_RESERVED
+#define WDAT_TIMEOUT_MIN     1
 
 /**
  * struct wdat_instruction - Single ACPI WDAT instruction
@@ -344,6 +345,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 = WDAT_TIMEOUT_MIN;
 	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
 	wdat->wdd.info = &wdat_wdt_info;
 	wdat->wdd.ops = &wdat_wdt_ops;
@@ -450,8 +452,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 v3 2/4] watchdog: wdat_wdg: 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 3040a0554055..609922ed4744 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)
 		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 v3 3/4] watchdog: wdat_wdg: 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 609922ed4744..502da41ac0b2 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -464,6 +464,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

[PATCH v3 4/4] watchdog: iTCO_wdg: Make code more clearly with macro definition

[Liu Xinpeng]

Using SET_NOIRQ_SYSTEM_SLEEP_PM_OPS reduces redundant code.

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

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3f2f4343644f..f870cf08f143 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -596,7 +596,6 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 /*
  * Suspend-to-idle requires this, because it stops the ticks and timekeeping, so
  * the watchdog cannot be pinged while in that state.  In ACPI sleep states the
@@ -637,20 +636,15 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
 }
 
 static const struct dev_pm_ops iTCO_wdt_pm = {
-	.suspend_noirq = iTCO_wdt_suspend_noirq,
-	.resume_noirq = iTCO_wdt_resume_noirq,
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(iTCO_wdt_suspend_noirq,
+				      iTCO_wdt_resume_noirq)
 };
 
-#define ITCO_WDT_PM_OPS	(&iTCO_wdt_pm)
-#else
-#define ITCO_WDT_PM_OPS	NULL
-#endif /* CONFIG_PM_SLEEP */
-
 static struct platform_driver iTCO_wdt_driver = {
 	.probe          = iTCO_wdt_probe,
 	.driver         = {
 		.name   = DRV_NAME,
-		.pm     = ITCO_WDT_PM_OPS,
+		.pm     = &iTCO_wdt_pm,
 	},
 };
 
-- 
2.23.0

Re: [PATCH v3 1/4] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Tzung-Bi Shih]

On Tue, Apr 26, 2022 at 11:35:17AM +0800, Liu Xinpeng wrote:
> The module arguement timeout is a configured timeout value.
> “separate minimum and maximum HW timeouts and configured timeout value.”
> (patch v1 is explained by Guenter Roeck)
> 
> So using watchdog_timeout_invalid to check timeout invalid is more justified.

The v3 commit message doesn't help too much for understanding the patch.  You
could see [1] for some reference sentences.  See also [2].

[1]: https://patchwork.kernel.org/project/linux-watchdog/patch/1650874932-18407-2-git-send-email-liuxp11@chinatelecom.cn/#24831418
[2]: https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/watchdog/watchdog-kernel-api.rst#L95

> @@ -14,6 +14,7 @@
>  #include <linux/watchdog.h>
>  
>  #define MAX_WDAT_ACTIONS ACPI_WDAT_ACTION_RESERVED
> +#define WDAT_TIMEOUT_MIN     1

To be consistent, would MIN_WDAT_TIMEOUT be a better name?

> @@ -344,6 +345,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 = WDAT_TIMEOUT_MIN;

Does it really need to configure the `min_timeout`?  What if leave it as is
(i.e. 0)?

Re: [PATCH v3 4/4] watchdog: iTCO_wdg: Make code more clearly with macro definition

[Tzung-Bi Shih]

On Tue, Apr 26, 2022 at 11:35:20AM +0800, Liu Xinpeng wrote:
> @@ -596,7 +596,6 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  /*
>   * Suspend-to-idle requires this, because it stops the ticks and timekeeping, so
>   * the watchdog cannot be pinged while in that state.  In ACPI sleep states the
> @@ -637,20 +636,15 @@ static int iTCO_wdt_resume_noirq(struct device *dev)
>  }
>  
>  static const struct dev_pm_ops iTCO_wdt_pm = {
> -	.suspend_noirq = iTCO_wdt_suspend_noirq,
> -	.resume_noirq = iTCO_wdt_resume_noirq,
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(iTCO_wdt_suspend_noirq,
> +				      iTCO_wdt_resume_noirq)

Thus, iTCO_wdt_suspend_noirq and iTCO_wdt_resume_noirq are possible unused.
Put __maybe_unused attribute to them.

Re: [PATCH v3 1/4] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Guenter Roeck]

On 4/25/22 23:10, Tzung-Bi Shih wrote:
> On Tue, Apr 26, 2022 at 11:35:17AM +0800, Liu Xinpeng wrote:
>> The module arguement timeout is a configured timeout value.
>> “separate minimum and maximum HW timeouts and configured timeout value.”
>> (patch v1 is explained by Guenter Roeck)
>>
>> So using watchdog_timeout_invalid to check timeout invalid is more justified.
> 
> The v3 commit message doesn't help too much for understanding the patch.  You
> could see [1] for some reference sentences.  See also [2].
> 
> [1]: https://patchwork.kernel.org/project/linux-watchdog/patch/1650874932-18407-2-git-send-email-liuxp11@chinatelecom.cn/#24831418
> [2]: https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/watchdog/watchdog-kernel-api.rst#L95
> 
>> @@ -14,6 +14,7 @@
>>   #include <linux/watchdog.h>
>>   
>>   #define MAX_WDAT_ACTIONS ACPI_WDAT_ACTION_RESERVED
>> +#define WDAT_TIMEOUT_MIN     1
> 
> To be consistent, would MIN_WDAT_TIMEOUT be a better name?
> 

Should just have set it to 1 below without using a define.

>> @@ -344,6 +345,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 = WDAT_TIMEOUT_MIN;
> 
> Does it really need to configure the `min_timeout`?  What if leave it as is
> (i.e. 0)?

It is better to set it to 1. Otherwise "0" is considered a valid timeout,
which doesn't make much sense.

Guenter