[U-Boot] [PATCH] timer: Fix possible underflow in udelay

[U-Boot] [PATCH] timer: Fix possible underflow in udelay

[Andrew F. Davis]

When the inputed usec is too large we process it in chunks of
CONFIG_WD_PERIOD size. Subtracting this from usec until usec is
zero. If usec is not an integer multiple of CONFIG_WD_PERIOD it
will underflow and the condition will not become false when it
should. Fix this logic.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 lib/time.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/time.c b/lib/time.c
index f37150f..4ec3cb9 100644
--- a/lib/time.c
+++ b/lib/time.c
@@ -145,14 +145,14 @@ void __weak __udelay(unsigned long usec)
 
 void udelay(unsigned long usec)
 {
-	ulong kv;
+	ulong kv = usec > CONFIG_WD_PERIOD ? CONFIG_WD_PERIOD : usec;
+	ulong elapsed = 0;
 
 	do {
 		WATCHDOG_RESET();
-		kv = usec > CONFIG_WD_PERIOD ? CONFIG_WD_PERIOD : usec;
-		__udelay (kv);
-		usec -= kv;
-	} while(usec);
+		__udelay(kv);
+		elapsed += kv;
+	} while (elapsed < usec);
 }
 
 void mdelay(unsigned long msec)
-- 
2.9.3

[U-Boot] [PATCH] timer: Fix possible underflow in udelay

[Troy Kisky]

On 8/25/2016 11:40 AM, Andrew F. Davis wrote:
> When the inputed usec is too large we process it in chunks of
> CONFIG_WD_PERIOD size. Subtracting this from usec until usec is
> zero. If usec is not an integer multiple of CONFIG_WD_PERIOD it
> will underflow and the condition will not become false when it
> should. Fix this logic.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  lib/time.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/time.c b/lib/time.c
> index f37150f..4ec3cb9 100644
> --- a/lib/time.c
> +++ b/lib/time.c
> @@ -145,14 +145,14 @@ void __weak __udelay(unsigned long usec)
>  
>  void udelay(unsigned long usec)
>  {
> -	ulong kv;
> +	ulong kv = usec > CONFIG_WD_PERIOD ? CONFIG_WD_PERIOD : usec;
> +	ulong elapsed = 0;
>  
>  	do {
>  		WATCHDOG_RESET();
> -		kv = usec > CONFIG_WD_PERIOD ? CONFIG_WD_PERIOD : usec;
> -		__udelay (kv);
> -		usec -= kv;
> -	} while(usec);
> +		__udelay(kv);
> +		elapsed += kv;
> +	} while (elapsed < usec);
>  }
>  
>  void mdelay(unsigned long msec)
> 

The original code looks fine to me. Can you give an example of failure ?
ie.
If udelay is passed value of CONFIG_WD_PERIOD+1, the udelay sequence will be

udelay(CONFIG_WD_PERIOD)
udelay(1)

whereas the need code does
udelay(CONFIG_WD_PERIOD)
udelay(CONFIG_WD_PERIOD)

[U-Boot] [PATCH] timer: Fix possible underflow in udelay

[Andrew F. Davis]

On 08/25/2016 02:02 PM, Troy Kisky wrote:
> On 8/25/2016 11:40 AM, Andrew F. Davis wrote:
>> When the inputed usec is too large we process it in chunks of
>> CONFIG_WD_PERIOD size. Subtracting this from usec until usec is
>> zero. If usec is not an integer multiple of CONFIG_WD_PERIOD it
>> will underflow and the condition will not become false when it
>> should. Fix this logic.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>  lib/time.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/time.c b/lib/time.c
>> index f37150f..4ec3cb9 100644
>> --- a/lib/time.c
>> +++ b/lib/time.c
>> @@ -145,14 +145,14 @@ void __weak __udelay(unsigned long usec)
>>  
>>  void udelay(unsigned long usec)
>>  {
>> -	ulong kv;
>> +	ulong kv = usec > CONFIG_WD_PERIOD ? CONFIG_WD_PERIOD : usec;
>> +	ulong elapsed = 0;
>>  
>>  	do {
>>  		WATCHDOG_RESET();
>> -		kv = usec > CONFIG_WD_PERIOD ? CONFIG_WD_PERIOD : usec;
>> -		__udelay (kv);
>> -		usec -= kv;
>> -	} while(usec);
>> +		__udelay(kv);
>> +		elapsed += kv;
>> +	} while (elapsed < usec);
>>  }
>>  
>>  void mdelay(unsigned long msec)
>>
> 
> The original code looks fine to me. Can you give an example of failure ?
> ie.
> If udelay is passed value of CONFIG_WD_PERIOD+1, the udelay sequence will be
> 
> udelay(CONFIG_WD_PERIOD)
> udelay(1)
> 
> whereas the need code does
> udelay(CONFIG_WD_PERIOD)
> udelay(CONFIG_WD_PERIOD)
> 

Hmm, I'm not sure where I saw the problem before, I think I may have
tried to optimize it and broke it myself, oh well, this patch can safely
be ignored.

Thanks,
Andrew