[PATCH v5] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[PATCH v5] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Liu Xinpeng]

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.

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..9db01d165310 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -55,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 #define WDAT_DEFAULT_TIMEOUT	30
+#define WDAT_MIN_TIMEOUT     1
 
 static int timeout = WDAT_DEFAULT_TIMEOUT;
 module_param(timeout, int, 0);
@@ -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_MIN_TIMEOUT;
 	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

Re: [PATCH v5] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Guenter Roeck]

On 4/26/22 01:43, Liu Xinpeng wrote:
> Context: If max_hw_heartbeat_ms is provided, the configured maximum timeout

I am quite sure I asked you to drop "Context: " and to change "existed" in
the subject to "existing".

Guenter

> 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 | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index 195c8c004b69..9db01d165310 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -55,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>   		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>   
>   #define WDAT_DEFAULT_TIMEOUT	30
> +#define WDAT_MIN_TIMEOUT     1
>   
>   static int timeout = WDAT_DEFAULT_TIMEOUT;
>   module_param(timeout, int, 0);
> @@ -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_MIN_TIMEOUT;
>   	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;

Re: [PATCH v5] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Guenter Roeck]

On 4/26/22 02:00, Guenter Roeck wrote:
> On 4/26/22 01:43, Liu Xinpeng wrote:
>> Context: If max_hw_heartbeat_ms is provided, the configured maximum timeout
> 
> I am quite sure I asked you to drop "Context: " and to change "existed" in
> the subject to "existing".
> 


Also, I asked you to separate the iTCO patch from the wdat series, not this one.
Why did you send this patch not as part of the other patches for the same driver ?

Guenter

> Guenter
> 
>> 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 | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
>> index 195c8c004b69..9db01d165310 100644
>> --- a/drivers/watchdog/wdat_wdt.c
>> +++ b/drivers/watchdog/wdat_wdt.c
>> @@ -55,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>>            __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>   #define WDAT_DEFAULT_TIMEOUT    30
>> +#define WDAT_MIN_TIMEOUT     1
>>   static int timeout = WDAT_DEFAULT_TIMEOUT;
>>   module_param(timeout, int, 0);
>> @@ -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_MIN_TIMEOUT;
>>       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;
>