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.