[PATCH] watchdog: ath79: fix maximum timeout

[PATCH] watchdog: ath79: fix maximum timeout

[John Crispin]

If the userland tries to set a timeout higher than the max_timeout,
then we should fallback to max_timeout.

Signed-off-by: John Crispin <john@phrozen.org>
---
 drivers/watchdog/ath79_wdt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/ath79_wdt.c b/drivers/watchdog/ath79_wdt.c
index e2209bf5fa8a..c2fc6c3d0092 100644
--- a/drivers/watchdog/ath79_wdt.c
+++ b/drivers/watchdog/ath79_wdt.c
@@ -115,10 +115,14 @@ static inline void ath79_wdt_disable(void)
 
 static int ath79_wdt_set_timeout(int val)
 {
-	if (val < 1 || val > max_timeout)
+	if (val < 1)
 		return -EINVAL;
 
-	timeout = val;
+	if (val > max_timeout)
+		timeout = max_timeout;
+	else
+		timeout = val;
+
 	ath79_wdt_keepalive();
 
 	return 0;
-- 
2.11.0

Re: [PATCH] watchdog: ath79: fix maximum timeout

[Guenter Roeck]

On 05/07/2018 06:16 AM, John Crispin wrote:
> If the userland tries to set a timeout higher than the max_timeout,
> then we should fallback to max_timeout.
> 

We don't do that for drivers using the watchdog core, so we should not
do it here either for consistency.

Guenter

> Signed-off-by: John Crispin <john@phrozen.org> > ---
>   drivers/watchdog/ath79_wdt.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/ath79_wdt.c b/drivers/watchdog/ath79_wdt.c
> index e2209bf5fa8a..c2fc6c3d0092 100644
> --- a/drivers/watchdog/ath79_wdt.c
> +++ b/drivers/watchdog/ath79_wdt.c
> @@ -115,10 +115,14 @@ static inline void ath79_wdt_disable(void)
>   
>   static int ath79_wdt_set_timeout(int val)
>   {
> -	if (val < 1 || val > max_timeout)
> +	if (val < 1)
>   		return -EINVAL;
>   
> -	timeout = val;
> +	if (val > max_timeout)
> +		timeout = max_timeout;
> +	else
> +		timeout = val;
> +
>   	ath79_wdt_keepalive();
>   
>   	return 0;
> 

Re: [PATCH] watchdog: ath79: fix maximum timeout

[John Crispin]

On 07/05/18 15:34, Guenter Roeck wrote:
> On 05/07/2018 06:16 AM, John Crispin wrote:
>> If the userland tries to set a timeout higher than the max_timeout,
>> then we should fallback to max_timeout.
>>
>
> We don't do that for drivers using the watchdog core, so we should not
> do it here either for consistency.
>
> Guenter

Hi,
thanks for the quick feedback. I'll mark the patch as "rejected by 
upstream due to subsystem consistency" inside OpenWrt.
     John
>
>> Signed-off-by: John Crispin <john@phrozen.org> > ---
>>   drivers/watchdog/ath79_wdt.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/ath79_wdt.c b/drivers/watchdog/ath79_wdt.c
>> index e2209bf5fa8a..c2fc6c3d0092 100644
>> --- a/drivers/watchdog/ath79_wdt.c
>> +++ b/drivers/watchdog/ath79_wdt.c
>> @@ -115,10 +115,14 @@ static inline void ath79_wdt_disable(void)
>>     static int ath79_wdt_set_timeout(int val)
>>   {
>> -    if (val < 1 || val > max_timeout)
>> +    if (val < 1)
>>           return -EINVAL;
>>   -    timeout = val;
>> +    if (val > max_timeout)
>> +        timeout = max_timeout;
>> +    else
>> +        timeout = val;
>> +
>>       ath79_wdt_keepalive();
>>         return 0;
>>
>
>

Re: [PATCH] watchdog: ath79: fix maximum timeout

[Guenter Roeck]

On Mon, May 07, 2018 at 03:40:57PM +0200, John Crispin wrote:
> 
> 
> On 07/05/18 15:34, Guenter Roeck wrote:
> >On 05/07/2018 06:16 AM, John Crispin wrote:
> >>If the userland tries to set a timeout higher than the max_timeout,
> >>then we should fallback to max_timeout.
> >>
> >
> >We don't do that for drivers using the watchdog core, so we should not
> >do it here either for consistency.
> >
> >Guenter
> 
> Hi,
> thanks for the quick feedback. I'll mark the patch as "rejected by upstream
> due to subsystem consistency" inside OpenWrt.

Note that I am not completely opposed to the idea, but it should go through
the watchdog core. You could convert the driver to use it, and then make
the case that the subsystem should report <min, max> to userspace, relax
set_timeout to adjust the requested timeout to [min,max] instead of letting
userspace guess it, or both. Plus, of course, submit patches to actually
implement and document the necessary changes.

Thanks,
Guenter