[PATCH] Fix set_timeout for big timeout values

[PATCH] Fix set_timeout for big timeout values

[Georg Hofmann]

This patch implements the documented behavior: if max_hw_heartbeat_ms is
implemented, the minimum of the set_timeout argument and
max_hw_heartbeat_ms should be used.
Previously only the first 7 bits were used and the input argument was
returned.

Signed-off-by: Georg Hofmann <georg@hofmannsweb.com>
---
 drivers/watchdog/imx2_wdt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 2b52514..3c13adc 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device *wdog,
 static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
 				unsigned int new_timeout)
 {
-	__imx2_wdt_set_timeout(wdog, new_timeout);
+	unsigned int actual;
 
-	wdog->timeout = new_timeout;
+	actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
+	__imx2_wdt_set_timeout(wdog, actual);
+	wdog->timeout = actual;
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH] Fix set_timeout for big timeout values

[Guenter Roeck]

On 4/6/19 3:17 AM, Georg Hofmann wrote:
> This patch implements the documented behavior: if max_hw_heartbeat_ms is
> implemented, the minimum of the set_timeout argument and
> max_hw_heartbeat_ms should be used.
> Previously only the first 7 bits were used and the input argument was
> returned.
> 
> Signed-off-by: Georg Hofmann <georg@hofmannsweb.com>
> ---
>   drivers/watchdog/imx2_wdt.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 2b52514..3c13adc 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device *wdog,
>   static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
>   				unsigned int new_timeout)
>   {
> -	__imx2_wdt_set_timeout(wdog, new_timeout);
> +	unsigned int actual;
>   
> -	wdog->timeout = new_timeout;
> +	actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
> +	__imx2_wdt_set_timeout(wdog, actual);
> +	wdog->timeout = actual;

That defeats the purpose of having an internal maximum. wdog->timeout
should still be set to the requested value.

Guenter

Re: [PATCH] Fix set_timeout for big timeout values

[Georg Hofmann]

----- Original Message -----
> From: "Guenter Roeck" <linux@roeck-us.net>
> To: "Georg Hofmann" <georg@hofmannsweb.com>, wim@linux-watchdog.org
> Cc: linux-watchdog@vger.kernel.org
> Sent: Saturday, April 6, 2019 3:25:44 PM
> Subject: Re: [PATCH] Fix set_timeout for big timeout values

> On 4/6/19 3:17 AM, Georg Hofmann wrote:
>> This patch implements the documented behavior: if max_hw_heartbeat_ms is
>> implemented, the minimum of the set_timeout argument and
>> max_hw_heartbeat_ms should be used.
>> Previously only the first 7 bits were used and the input argument was
>> returned.
>> 
>> Signed-off-by: Georg Hofmann <georg@hofmannsweb.com>
>> ---
>>   drivers/watchdog/imx2_wdt.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index 2b52514..3c13adc 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device
>> *wdog,
>>   static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
>>   				unsigned int new_timeout)
>>   {
>> -	__imx2_wdt_set_timeout(wdog, new_timeout);
>> +	unsigned int actual;
>>   
>> -	wdog->timeout = new_timeout;
>> +	actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
>> +	__imx2_wdt_set_timeout(wdog, actual);
>> +	wdog->timeout = actual;
> 
> That defeats the purpose of having an internal maximum. wdog->timeout
> should still be set to the requested value.
> 
> Guenter

Hi,

I don't understand, the internal maximum is max_hw_heartbeat_ms.
I have used the same code as other watchdog drivers 
(e.g. aspeed_wdt.c, loongson1_wdt.c, ...).

I have submitted this patch because I was writing a userspace
 program and I expected a different behavior on the ioctl.
The watchdog documentation says (Documentation/watchdog/watchdog-api.txt):
Setting and getting the timeout:

For some drivers it is possible to modify the watchdog timeout on the
fly with the SETTIMEOUT ioctl, those drivers have the WDIOF_SETTIMEOUT
flag set in their option field.  The argument is an integer
representing the timeout in seconds.  The driver returns the real
timeout used in the same variable, and this timeout might differ from
the requested one due to limitation of the hardware.

    int timeout = 45;
    ioctl(fd, WDIOC_SETTIMEOUT, &timeout);
    printf("The timeout was set to %d seconds\n", timeout);

This example might actually print "The timeout was set to 60 seconds"
if the device has a granularity of minutes for its timeout.

As the watchdog core driver reads the timeout just after write, I have
to set the applied value to timeout.

Initially I thought I should get a error message if the timeout can't
be applied, but the documentation describes another behavior.

Georg

Re: [PATCH] Fix set_timeout for big timeout values

[Guenter Roeck]

On 4/6/19 7:17 AM, Georg Hofmann wrote:
> ----- Original Message -----
>> From: "Guenter Roeck" <linux@roeck-us.net>
>> To: "Georg Hofmann" <georg@hofmannsweb.com>, wim@linux-watchdog.org
>> Cc: linux-watchdog@vger.kernel.org
>> Sent: Saturday, April 6, 2019 3:25:44 PM
>> Subject: Re: [PATCH] Fix set_timeout for big timeout values
> 
>> On 4/6/19 3:17 AM, Georg Hofmann wrote:
>>> This patch implements the documented behavior: if max_hw_heartbeat_ms is
>>> implemented, the minimum of the set_timeout argument and
>>> max_hw_heartbeat_ms should be used.
>>> Previously only the first 7 bits were used and the input argument was
>>> returned.
>>>
>>> Signed-off-by: Georg Hofmann <georg@hofmannsweb.com>
>>> ---
>>>    drivers/watchdog/imx2_wdt.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>>> index 2b52514..3c13adc 100644
>>> --- a/drivers/watchdog/imx2_wdt.c
>>> +++ b/drivers/watchdog/imx2_wdt.c
>>> @@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device
>>> *wdog,
>>>    static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
>>>    				unsigned int new_timeout)
>>>    {
>>> -	__imx2_wdt_set_timeout(wdog, new_timeout);
>>> +	unsigned int actual;
>>>    
>>> -	wdog->timeout = new_timeout;
>>> +	actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
>>> +	__imx2_wdt_set_timeout(wdog, actual);
>>> +	wdog->timeout = actual;
>>
>> That defeats the purpose of having an internal maximum. wdog->timeout
>> should still be set to the requested value.
>>
>> Guenter
> 
> Hi,
> 
> I don't understand, the internal maximum is max_hw_heartbeat_ms.
> I have used the same code as other watchdog drivers
> (e.g. aspeed_wdt.c, loongson1_wdt.c, ...).
> 
> I have submitted this patch because I was writing a userspace
>   program and I expected a different behavior on the ioctl.
> The watchdog documentation says (Documentation/watchdog/watchdog-api.txt):
> Setting and getting the timeout:
> 
> For some drivers it is possible to modify the watchdog timeout on the
> fly with the SETTIMEOUT ioctl, those drivers have the WDIOF_SETTIMEOUT
> flag set in their option field.  The argument is an integer
> representing the timeout in seconds.  The driver returns the real
> timeout used in the same variable, and this timeout might differ from
> the requested one due to limitation of the hardware.
> 
>      int timeout = 45;
>      ioctl(fd, WDIOC_SETTIMEOUT, &timeout);
>      printf("The timeout was set to %d seconds\n", timeout);
> 
> This example might actually print "The timeout was set to 60 seconds"
> if the device has a granularity of minutes for its timeout.
> 
> As the watchdog core driver reads the timeout just after write, I have
> to set the applied value to timeout.
> 
> Initially I thought I should get a error message if the timeout can't
> be applied, but the documentation describes another behavior.
> 

The whole point of max_hw_heartbeat_ms is to be able to specify a larger
timeout than the maximum supported by the hardware. In extreme cases,
max_hw_heartbeat_ms could be as low as 1 second (or less). Limiting
wdog->timeout to that value is identical to not having max_hw_heartbeat_ms
in the first place, and thus quite pointless.

aspeed_wdt.c does _not_ set wdd->timeout to the 'actual' value.
loongson1_wdt.c doesn't do it either. If any other driver does it,
it is wrong.

Your patch is almost correct, except it should keep
"wdog->timeout = new_timeout;".

Guenter

Re: [PATCH] Fix set_timeout for big timeout values

[Georg Hofmann]

----- Original Message -----
> From: "Guenter Roeck" <linux@roeck-us.net>
> To: "Georg Hofmann" <georg@hofmannsweb.com>
> Cc: wim@linux-watchdog.org, linux-watchdog@vger.kernel.org
> Sent: Saturday, April 6, 2019 5:46:08 PM
> Subject: Re: [PATCH] Fix set_timeout for big timeout values

> On 4/6/19 7:17 AM, Georg Hofmann wrote:
>> ----- Original Message -----
>>> From: "Guenter Roeck" <linux@roeck-us.net>
>>> To: "Georg Hofmann" <georg@hofmannsweb.com>, wim@linux-watchdog.org
>>> Cc: linux-watchdog@vger.kernel.org
>>> Sent: Saturday, April 6, 2019 3:25:44 PM
>>> Subject: Re: [PATCH] Fix set_timeout for big timeout values
>> 
>>> On 4/6/19 3:17 AM, Georg Hofmann wrote:
>>>> This patch implements the documented behavior: if max_hw_heartbeat_ms is
>>>> implemented, the minimum of the set_timeout argument and
>>>> max_hw_heartbeat_ms should be used.
>>>> Previously only the first 7 bits were used and the input argument was
>>>> returned.
>>>>
>>>> Signed-off-by: Georg Hofmann <georg@hofmannsweb.com>
>>>> ---
>>>>    drivers/watchdog/imx2_wdt.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>>>> index 2b52514..3c13adc 100644
>>>> --- a/drivers/watchdog/imx2_wdt.c
>>>> +++ b/drivers/watchdog/imx2_wdt.c
>>>> @@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device
>>>> *wdog,
>>>>    static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
>>>>    				unsigned int new_timeout)
>>>>    {
>>>> -	__imx2_wdt_set_timeout(wdog, new_timeout);
>>>> +	unsigned int actual;
>>>>    
>>>> -	wdog->timeout = new_timeout;
>>>> +	actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
>>>> +	__imx2_wdt_set_timeout(wdog, actual);
>>>> +	wdog->timeout = actual;
>>>
>>> That defeats the purpose of having an internal maximum. wdog->timeout
>>> should still be set to the requested value.
>>>
>>> Guenter
>> 
>> Hi,
>> 
>> I don't understand, the internal maximum is max_hw_heartbeat_ms.
>> I have used the same code as other watchdog drivers
>> (e.g. aspeed_wdt.c, loongson1_wdt.c, ...).
>> 
>> I have submitted this patch because I was writing a userspace
>>   program and I expected a different behavior on the ioctl.
>> The watchdog documentation says (Documentation/watchdog/watchdog-api.txt):
>> Setting and getting the timeout:
>> 
>> For some drivers it is possible to modify the watchdog timeout on the
>> fly with the SETTIMEOUT ioctl, those drivers have the WDIOF_SETTIMEOUT
>> flag set in their option field.  The argument is an integer
>> representing the timeout in seconds.  The driver returns the real
>> timeout used in the same variable, and this timeout might differ from
>> the requested one due to limitation of the hardware.
>> 
>>      int timeout = 45;
>>      ioctl(fd, WDIOC_SETTIMEOUT, &timeout);
>>      printf("The timeout was set to %d seconds\n", timeout);
>> 
>> This example might actually print "The timeout was set to 60 seconds"
>> if the device has a granularity of minutes for its timeout.
>> 
>> As the watchdog core driver reads the timeout just after write, I have
>> to set the applied value to timeout.
>> 
>> Initially I thought I should get a error message if the timeout can't
>> be applied, but the documentation describes another behavior.
>> 
> 
> The whole point of max_hw_heartbeat_ms is to be able to specify a larger
> timeout than the maximum supported by the hardware. In extreme cases,
> max_hw_heartbeat_ms could be as low as 1 second (or less). Limiting
> wdog->timeout to that value is identical to not having max_hw_heartbeat_ms
> in the first place, and thus quite pointless.
> 
> aspeed_wdt.c does _not_ set wdd->timeout to the 'actual' value.
> loongson1_wdt.c doesn't do it either. If any other driver does it,
> it is wrong.
> 
> Your patch is almost correct, except it should keep
> "wdog->timeout = new_timeout;".
> 
> Guenter
ah, ok, I missed that. I will change test it and than resend the patch.
Thanks for your time.
Georg

[PATCH] Fix set_timeout for big timeout values

[Georg Hofmann]

This patch implements the documented behavior: if max_hw_heartbeat_ms is
implemented, the minimum of the set_timeout argument and
max_hw_heartbeat_ms should be used.
Previously only the first 7 bits were used and the input argument was
returned.

Signed-off-by: Georg Hofmann <georg@hofmannsweb.com>
---
 drivers/watchdog/imx2_wdt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 2b52514..3c13adc 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device *wdog,
 static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
                                 unsigned int new_timeout)
 {
-        __imx2_wdt_set_timeout(wdog, new_timeout);
+        unsigned int actual;
 
-        wdog->timeout = new_timeout;
+        actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
+        __imx2_wdt_set_timeout(wdog, actual);

+        wdog->timeout = new_timeout;
         return 0;
 }
 
-- 
2.7.4

Re: [PATCH] Fix set_timeout for big timeout values

[Guenter Roeck]

On Mon, Apr 08, 2019 at 10:05:36AM +0200, Georg Hofmann wrote:
> This patch implements the documented behavior: if max_hw_heartbeat_ms is
> implemented, the minimum of the set_timeout argument and
> max_hw_heartbeat_ms should be used.
> Previously only the first 7 bits were used and the input argument was
> returned.
> 
> Signed-off-by: Georg Hofmann <georg@hofmannsweb.com>

Reviewed-by: Guenetr Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/imx2_wdt.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 2b52514..3c13adc 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device *wdog,
>  static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
>                                  unsigned int new_timeout)
>  {
> -        __imx2_wdt_set_timeout(wdog, new_timeout);
> +        unsigned int actual;
>  
> -        wdog->timeout = new_timeout;
> +        actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
> +        __imx2_wdt_set_timeout(wdog, actual);
> 
> +        wdog->timeout = new_timeout;
>          return 0;
>  }
>  
> -- 
> 2.7.4

Re: [PATCH] Fix set_timeout for big timeout values

[Guenter Roeck]

On Mon, Apr 08, 2019 at 10:05:36AM +0200, Georg Hofmann wrote:
> This patch implements the documented behavior: if max_hw_heartbeat_ms is
> implemented, the minimum of the set_timeout argument and
> max_hw_heartbeat_ms should be used.
> Previously only the first 7 bits were used and the input argument was
> returned.
> 
> Signed-off-by: Georg Hofmann <georg@hofmannsweb.com>

The patch below is corrupted (tabs replaced with spaces). Please resend.
Also, when doing so, please add "watchdog: imx2_wdt:" to the subject line.

Thanks,
Guenter

> ---
>  drivers/watchdog/imx2_wdt.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 2b52514..3c13adc 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device *wdog,
>  static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
>                                  unsigned int new_timeout)
>  {
> -        __imx2_wdt_set_timeout(wdog, new_timeout);
> +        unsigned int actual;
>  
> -        wdog->timeout = new_timeout;
> +        actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
> +        __imx2_wdt_set_timeout(wdog, actual);
> 
> +        wdog->timeout = new_timeout;
>          return 0;
>  }
>  
> -- 
> 2.7.4