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

[Marek Vasut <marex@denx.de>]

On Friday, May 08, 2015 at 06:40:22 PM, Stephen Warren wrote:
> 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).

Feel free to tweak the commit message.

Best regards,
Marek Vasut