On 05/05/2015 05:37 PM, Marek Vasut wrote:
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 ?
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.
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.
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".
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?
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
