| Message ID | 1650874932-18407-2-git-send-email-liuxp11@chinatelecom.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Impovements about acpi hardware watchdog | expand |
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?
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
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;
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(-)