On Wednesday, May 06, 2015 at 12:57:54 AM, Stephen Warren wrote: > On 05/05/2015 04:42 PM, Marek Vasut wrote: > > On Wednesday, May 06, 2015 at 12:37:38 AM, Stephen Warren wrote: > >> On 05/05/2015 04:17 PM, Marek Vasut wrote: > >>> On Tuesday, May 05, 2015 at 11:46:56 PM, Stephen Warren wrote: > >>>> On 05/04/2015 02:54 PM, Marek Vasut wrote: > >>>>> Switch to generic timer implementation from lib/time.c . > >>>>> This also fixes a signed overflow which was in __udelay() > >>>>> implementation. > >>>> > >>>> Can you explain that a bit more? > >>>> > >>>>> -void __udelay(unsigned long usec) > >>>>> -{ > >>>>> - ulong endtime; > >>>>> - signed long diff; > >>>>> - > >>>>> - endtime = get_timer_us(0) + usec; > >>>>> - > >>>>> - do { > >>>>> - ulong now = get_timer_us(0); > >>>>> - diff = endtime - now; > >>>>> - } while (diff >= 0); > >>>>> -} > >>>> > >>>> I believe since endtime and now hold micro seconds, there shouldn't be > >>>> any overflow so long as the microsecond difference fits into 31 bits, > >>>> i.e. so long as usec is less than ~36 minutes. I doubt anything is > >>>> calling __udelay() with that large of a value. Perhaps the issue this > >>>> patch fixes is in get_timer_us(0) instead, or something else changed > >>>> as a side-effect? > >>> > >>> The generic implementation caters for full 32-bit range, that's all. > >>> Since the argument of this function is unsigned, it can overflow if > >>> you use argument which is bigger than 31 bits. OK like that ? > >> > >> Sorry, I still don't understand. Both the __udelay() here and in > >> lib/time.c take an unsigned long argument. I don't see how switching one > >> out for the other can affect anything if the argument type is the issue. > > > > 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 ? > >> 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 ? Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot