[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Stephen Warren]

On 05/05/2015 04:42 PM, Marek Vasut wrote:
> On Wednesday, May 06, 2015 at 12:37:38 AM, Stephen Warren wrote:
>> On 05/05/2015 04:17 PM, Marek Vasut wrote:
>>> On Tuesday, May 05, 2015 at 11:46:56 PM, Stephen Warren wrote:
>>>> On 05/04/2015 02:54 PM, Marek Vasut wrote:
>>>>> Switch to generic timer implementation from lib/time.c .
>>>>> This also fixes a signed overflow which was in __udelay()
>>>>> implementation.
>>>>
>>>> Can you explain that a bit more?
>>>>
>>>>> -void __udelay(unsigned long usec)
>>>>> -{
>>>>> -	ulong endtime;
>>>>> -	signed long diff;
>>>>> -
>>>>> -	endtime = get_timer_us(0) + usec;
>>>>> -
>>>>> -	do {
>>>>> -		ulong now = get_timer_us(0);
>>>>> -		diff = endtime - now;
>>>>> -	} while (diff >= 0);
>>>>> -}
>>>>
>>>> I believe since endtime and now hold micro seconds, there shouldn't be
>>>> any overflow so long as the microsecond difference fits into 31 bits,
>>>> i.e. so long as usec is less than ~36 minutes. I doubt anything is
>>>> calling __udelay() with that large of a value. Perhaps the issue this
>>>> patch fixes is in get_timer_us(0) instead, or something else changed as
>>>> a side-effect?
>>>
>>> The generic implementation caters for full 32-bit range, that's all.
>>> Since the argument of this function is unsigned, it can overflow if
>>> you use argument which is bigger than 31 bits. OK like that ?
>>
>> Sorry, I still don't understand. Both the __udelay() here and in
>> lib/time.c take an unsigned long argument. I don't see how switching one
>> out for the other can affect anything if the argument type is the issue.
>
> So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish,
> diff will become negative and this udelay() will not perform the correct delay,
> right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than 
well-researched) understanding of C math promotion rules means that 
"endtime - now" will be calculated as an unsigned value, then converted 
into a signed value to be stored in the signed diff. As such, I would 
expect the value of diff to be a small value in this case. I wrote a 
test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, 
yields diff=4 as expected.

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 
0xffffffff? This doesn't cause issues either. All that's relevant is the 
difference between endtime and now, not their absolute values, and not 
whether endtime has wrapped but now has or hasn't. For example, endtime 
= 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

>> Besides, what's passing a value >~36 minutes to udelay()?
>
> Nothing, but that doesn't mean we can have a possibly broken implementation,
> right ?

True. However, I'd expect that any specification for udelay would 
disallow such large parameter values, and hence its behaviour wouldn't 
be relevant if such values were passed.