From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Fri, 08 May 2015 10:40:22 -0600 Subject: [U-Boot] [PATCH 4/4] ARM: bcm283x: Switch to generic timer In-Reply-To: <201505081831.41339.marex@denx.de> References: <1430772877-7301-1-git-send-email-marex@denx.de> <201505062013.57612.marex@denx.de> <554CDE56.3090207@wwwdotorg.org> <201505081831.41339.marex@denx.de> Message-ID: <554CE6F6.1010108@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/08/2015 10:31 AM, Marek Vasut wrote: > On Friday, May 08, 2015 at 06:03:34 PM, Stephen Warren wrote: >> On 05/06/2015 12:13 PM, Marek Vasut wrote: >>> On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote: >>> [...] >>> >>>>>>> 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. >>> >>> So what you say is that it's OK to have a function which is buggy in >>> corner cases ? >> >> A corner case (something that's within spec but perhaps hard/unusual) >> should not be buggy. >> >> The behaviour of something outside spec isn't relevant; it's actively >> not specified. >> >> I suppose there is no specification of what range of values this >> function is supposed to accept. I'd argue we should create one, and that >> spec should likely limit the range to much less than the 32-bit >> parameter can actually hold, since some HW timer implementations may >> have well less than 32-bits of range. > > Maybe we should just accept this patch and be done with it? It's clearly > and improvement which migrates away from old timer code to generic timer. The code change is fine. I have no issues with that. I just don't think the patch description is appropriate, since the version in lib/time.c has exactly the same overflow issue (albeit with a 64-bit type rather than a 32-bit type).