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