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