From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Wed, 06 May 2015 09:52:37 -0600 Subject: [U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer In-Reply-To: <201505060137.45642.marex@denx.de> References: <1430772877-7301-1-git-send-email-marex@denx.de> <201505060042.55206.marex@denx.de> <55494AF2.6080108@wwwdotorg.org> <201505060137.45642.marex@denx.de> Message-ID: <554A38C5.9070206@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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@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?