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

[Stephen Warren]

On 05/05/2015 05:37 PM, Marek Vasut wrote:
> On Wednesday, May 06, 2015 at 12:57:54 AM, Stephen Warren wrote:
>> 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.
>
> So what if the difference is bigger than 1 << 31 ?

As I said, I don't believe that case is relevant; it can only happen if 
passing ridiculously large delay values into __udelay() (i.e. greater 
than the 1<<31value you mention), and I don't believe there's any need 
to support that.

The implementation in lib/time.c probably has exactly the same problem, 
except that since it uses 64-bit math rather than 32-bit math, so the 
issue happens at 1<<63 rather than 1<<31. It's probably equally 
problematic for delay values as large as 1<<63:-) In practice, given 
1<<31 us is so large, I don't think there's any practical difference.

>>>> 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.
>
> Do you think you can pick this patch and drop the "fixes overflow" part
> or do you need resubmission ?

Tom Rini (or in the past Albert Aribaud) actually apply the patches.

Re: the patch description: I'd certainly be happy if it was re-written 
to say something more like "replace bcm2835-specific timer logic with 
common code to reduce the number of different implementations for the 
same thing".

I think you'd mentioned on IRC that this change fixed something 
USB-related for you, and I still don't understand how that could be 
possible. Perhaps there's some intermittent problem, and it just 
happened not to show up when you tested after this patch?