linux-kernel - Re: [PATCH v4 1/4] watchdog: wdat_wdg: Using the existed function to check parameter timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f55c7bba-2096-c392-d81f-b04d44ea1175@roeck-us.net>
Date:   Tue, 26 Apr 2022 01:05:58 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Liu Xinpeng <liuxp11@...natelecom.cn>, wim@...ux-watchdog.org,
        tzungbi@...nel.org
Cc:     linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/4] watchdog: wdat_wdg: Using the existed function to
 check parameter timeout

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

Drop "Context:".

Also, in the subject, it should be "existing".

> 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@...natelecom.cn>
> Reviewed-by: Guenter Roeck <linux@...ck-us.net>
> Reviewed-by: Tzung-Bi Shih <tzungbi@...nel.org>

And you really should not make up Reviewed-by: tags. This is completely
inappropriate.

Guenter

> ---
>   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;