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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
