* [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to check parameter timeout
2022-04-25 8:22 [PATCH v2 0/3] Impovements about acpi hardware watchdog Liu Xinpeng
@ 2022-04-25 8:22 ` Liu Xinpeng
2022-04-26 3:04 ` Tzung-Bi Shih
2022-04-25 8:22 ` [PATCH v2 2/3] watchdog: wdat_wdg: Stop watchdog when rebooting the system Liu Xinpeng
2022-04-25 8:22 ` [PATCH v2 3/3] watchdog: wdat_wdg: Stop watchdog when uninstalling module Liu Xinpeng
2 siblings, 1 reply; 6+ messages in thread
From: Liu Xinpeng @ 2022-04-25 8:22 UTC (permalink / raw)
To: wim, linux; +Cc: linux-watchdog, linux-kernel, Liu Xinpeng
Setting up min_timeout, and determine if a timeout value is
invalid using watchdog_timeout_invalid.
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to check parameter timeout
2022-04-25 8:22 ` [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to check parameter timeout Liu Xinpeng
@ 2022-04-26 3:04 ` Tzung-Bi Shih
2022-04-26 3:35 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Tzung-Bi Shih @ 2022-04-26 3:04 UTC (permalink / raw)
To: Liu Xinpeng; +Cc: wim, linux, linux-watchdog, linux-kernel
On Mon, Apr 25, 2022 at 04:22:10PM +0800, Liu Xinpeng wrote:
> #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)) {
Probably lacking of the context, I failed to see why the checks are
equivalent. Could you provide more information?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to check parameter timeout
2022-04-26 3:04 ` Tzung-Bi Shih
@ 2022-04-26 3:35 ` Guenter Roeck
0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2022-04-26 3:35 UTC (permalink / raw)
To: Tzung-Bi Shih, Liu Xinpeng; +Cc: wim, linux-watchdog, linux-kernel
On 4/25/22 20:04, Tzung-Bi Shih wrote:
> On Mon, Apr 25, 2022 at 04:22:10PM +0800, Liu Xinpeng wrote:
>> #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)) {
>
> Probably lacking of the context, I failed to see why the checks are
> equivalent. Could you provide more information?
Not exactly equivalent; this is a fix.
Context: 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.
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/3] watchdog: wdat_wdg: Stop watchdog when rebooting the system
2022-04-25 8:22 [PATCH v2 0/3] Impovements about acpi hardware watchdog Liu Xinpeng
2022-04-25 8:22 ` [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to check parameter timeout Liu Xinpeng
@ 2022-04-25 8:22 ` Liu Xinpeng
2022-04-25 8:22 ` [PATCH v2 3/3] watchdog: wdat_wdg: Stop watchdog when uninstalling module Liu Xinpeng
2 siblings, 0 replies; 6+ messages in thread
From: Liu Xinpeng @ 2022-04-25 8:22 UTC (permalink / raw)
To: wim, linux; +Cc: linux-watchdog, linux-kernel, Liu Xinpeng
Executing reboot command serveral 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] watchdog: wdat_wdg: Stop watchdog when uninstalling module
2022-04-25 8:22 [PATCH v2 0/3] Impovements about acpi hardware watchdog Liu Xinpeng
2022-04-25 8:22 ` [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to check parameter timeout Liu Xinpeng
2022-04-25 8:22 ` [PATCH v2 2/3] watchdog: wdat_wdg: Stop watchdog when rebooting the system Liu Xinpeng
@ 2022-04-25 8:22 ` Liu Xinpeng
2 siblings, 0 replies; 6+ messages in thread
From: Liu Xinpeng @ 2022-04-25 8:22 UTC (permalink / raw)
To: wim, linux; +Cc: linux-watchdog, linux-kernel, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread