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

[Marek Vasut <marex@denx.de>]

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 ?

> 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.

The implementation in lib/time.c uses 32bit usec argument though, so
it's not prone to this overflow. Please correct me if I'm wrong.

> >>>> 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".

Tom, do you want a repost ?

> 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?

I think Tyler can elaborate on that, but in his test case, he still
triggers the USB issue.