[U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer

[Marek Vasut]

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