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

[Marek Vasut]

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.

Best regards,
Marek Vasut