* [v1,1/1 1/2] Check the timeout module parameter is in the min-max range
@ 2020-05-27 1:18 yuechao.zhao
2020-05-27 1:18 ` [v1,1/1 2/2] Set the default timeout yuechao.zhao
2020-05-27 1:42 ` [v1,1/1 1/2] Check the timeout module parameter is in the min-max range Guenter Roeck
0 siblings, 2 replies; 4+ messages in thread
From: yuechao.zhao @ 2020-05-27 1:18 UTC (permalink / raw)
To: 345351830
Cc: amy.shih, oakley.ding, jia.sui, Yuechao Zhao, Jean Delvare,
Guenter Roeck, linux-hwmon, linux-kernel
From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
Check the return value of 'watchdog_init_timeout()' for checking the
timeout module parameter is in the min-max range.
Signed-off-by: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
---
drivers/hwmon/nct7904.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/nct7904.c b/drivers/hwmon/nct7904.c
index 18c95be..d069d59 100644
--- a/drivers/hwmon/nct7904.c
+++ b/drivers/hwmon/nct7904.c
@@ -1152,7 +1152,10 @@ static int nct7904_probe(struct i2c_client *client,
data->wdt.max_timeout = MAX_TIMEOUT;
data->wdt.parent = &client->dev;
- watchdog_init_timeout(&data->wdt, timeout * 60, &client->dev);
+ ret = watchdog_init_timeout(&data->wdt, timeout * 60, &client->dev);
+ if (ret < 0)
+ return ret;
+
watchdog_set_nowayout(&data->wdt, nowayout);
watchdog_set_drvdata(&data->wdt, data);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [v1,1/1 2/2] Set the default timeout
2020-05-27 1:18 [v1,1/1 1/2] Check the timeout module parameter is in the min-max range yuechao.zhao
@ 2020-05-27 1:18 ` yuechao.zhao
2020-05-27 1:46 ` Guenter Roeck
2020-05-27 1:42 ` [v1,1/1 1/2] Check the timeout module parameter is in the min-max range Guenter Roeck
1 sibling, 1 reply; 4+ messages in thread
From: yuechao.zhao @ 2020-05-27 1:18 UTC (permalink / raw)
To: 345351830
Cc: amy.shih, oakley.ding, jia.sui, Yuechao Zhao, Jean Delvare,
Guenter Roeck, linux-hwmon, linux-kernel
From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
The timeout module parameter should not be used for setting the default
timeout. Because, if you set the timeout = 0, the default timeout will
be meaningless. And the wahtchdog_init_timeout() can not detect this
error because the timeout module parameter of 0 means "no timeout
module paraameter specified".
Signed-off-by: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
---
drivers/hwmon/nct7904.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/nct7904.c b/drivers/hwmon/nct7904.c
index d069d59..ddbe47e 100644
--- a/drivers/hwmon/nct7904.c
+++ b/drivers/hwmon/nct7904.c
@@ -1147,7 +1147,7 @@ static int nct7904_probe(struct i2c_client *client,
data->wdt.ops = &nct7904_wdt_ops;
data->wdt.info = &nct7904_wdt_info;
- data->wdt.timeout = timeout * 60; /* in seconds */
+ data->wdt.timeout = WATCHDOG_TIMEOUT * 60; /* Set default timeout */
data->wdt.min_timeout = MIN_TIMEOUT;
data->wdt.max_timeout = MAX_TIMEOUT;
data->wdt.parent = &client->dev;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [v1,1/1 1/2] Check the timeout module parameter is in the min-max range
2020-05-27 1:18 [v1,1/1 1/2] Check the timeout module parameter is in the min-max range yuechao.zhao
2020-05-27 1:18 ` [v1,1/1 2/2] Set the default timeout yuechao.zhao
@ 2020-05-27 1:42 ` Guenter Roeck
1 sibling, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2020-05-27 1:42 UTC (permalink / raw)
To: yuechao.zhao, 345351830
Cc: amy.shih, oakley.ding, jia.sui, Jean Delvare, linux-hwmon, linux-kernel
On 5/26/20 6:18 PM, yuechao.zhao@advantech.com.cn wrote:
> From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
>
> Check the return value of 'watchdog_init_timeout()' for checking the
> timeout module parameter is in the min-max range.
>
> Signed-off-by: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
Please fix the subject. It should start with "hwmon: (nct7904)"
> ---
> drivers/hwmon/nct7904.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/nct7904.c b/drivers/hwmon/nct7904.c
> index 18c95be..d069d59 100644
> --- a/drivers/hwmon/nct7904.c
> +++ b/drivers/hwmon/nct7904.c
> @@ -1152,7 +1152,10 @@ static int nct7904_probe(struct i2c_client *client,
> data->wdt.max_timeout = MAX_TIMEOUT;
> data->wdt.parent = &client->dev;
>
> - watchdog_init_timeout(&data->wdt, timeout * 60, &client->dev);
> + ret = watchdog_init_timeout(&data->wdt, timeout * 60, &client->dev);
> + if (ret < 0)
> + return ret;
> +
Why ? The idea of returning an error from watchdog_init_timeout
is to give the driver a chance to select a default, not to refuse
loading the driver.
> watchdog_set_nowayout(&data->wdt, nowayout);
> watchdog_set_drvdata(&data->wdt, data);
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v1,1/1 2/2] Set the default timeout
2020-05-27 1:18 ` [v1,1/1 2/2] Set the default timeout yuechao.zhao
@ 2020-05-27 1:46 ` Guenter Roeck
0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2020-05-27 1:46 UTC (permalink / raw)
To: yuechao.zhao, 345351830
Cc: amy.shih, oakley.ding, jia.sui, Jean Delvare, linux-hwmon, linux-kernel
On 5/26/20 6:18 PM, yuechao.zhao@advantech.com.cn wrote:
> From: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
>
> The timeout module parameter should not be used for setting the default
> timeout. Because, if you set the timeout = 0, the default timeout will
> be meaningless. And the wahtchdog_init_timeout() can not detect this
> error because the timeout module parameter of 0 means "no timeout
> module paraameter specified".
>
> Signed-off-by: Yuechao Zhao <yuechao.zhao@advantech.com.cn>
Please fix the subject to something like "hmwmon: (nct7904) Set default timeout".
Also, the change only makes sense if you also change "static int timeout"
to default to 0. Then you can just drop patch 1/2.
Guenter
> ---
> drivers/hwmon/nct7904.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/nct7904.c b/drivers/hwmon/nct7904.c
> index d069d59..ddbe47e 100644
> --- a/drivers/hwmon/nct7904.c
> +++ b/drivers/hwmon/nct7904.c
> @@ -1147,7 +1147,7 @@ static int nct7904_probe(struct i2c_client *client,
> data->wdt.ops = &nct7904_wdt_ops;
> data->wdt.info = &nct7904_wdt_info;
>
> - data->wdt.timeout = timeout * 60; /* in seconds */
> + data->wdt.timeout = WATCHDOG_TIMEOUT * 60; /* Set default timeout */
> data->wdt.min_timeout = MIN_TIMEOUT;
> data->wdt.max_timeout = MAX_TIMEOUT;
> data->wdt.parent = &client->dev;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-27 1:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 1:18 [v1,1/1 1/2] Check the timeout module parameter is in the min-max range yuechao.zhao
2020-05-27 1:18 ` [v1,1/1 2/2] Set the default timeout yuechao.zhao
2020-05-27 1:46 ` Guenter Roeck
2020-05-27 1:42 ` [v1,1/1 1/2] Check the timeout module parameter is in the min-max range Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.