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

[Stephen Warren]

On 05/08/2015 10:31 AM, Marek Vasut wrote:
> On Friday, May 08, 2015 at 06:03:34 PM, Stephen Warren wrote:
>> On 05/06/2015 12:13 PM, Marek Vasut wrote:
>>> On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
>>> [...]
>>>
>>>>>>> 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.
>>>
>>> So what you say is that it's OK to have a function which is buggy in
>>> corner cases ?
>>
>> A corner case (something that's within spec but perhaps hard/unusual)
>> should not be buggy.
>>
>> The behaviour of something outside spec isn't relevant; it's actively
>> not specified.
>>
>> I suppose there is no specification of what range of values this
>> function is supposed to accept. I'd argue we should create one, and that
>> spec should likely limit the range to much less than the 32-bit
>> parameter can actually hold, since some HW timer implementations may
>> have well less than 32-bits of range.
>
> Maybe we should just accept this patch and be done with it? It's clearly
> and improvement which migrates away from old timer code to generic timer.

The code change is fine. I have no issues with that.

I just don't think the patch description is appropriate, since the 
version in lib/time.c has exactly the same overflow issue (albeit with a 
64-bit type rather than a 32-bit type).