From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2BUe-00081q-1p for qemu-devel@nongnu.org; Mon, 27 Feb 2012 20:04:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S2BUb-0002WK-So for qemu-devel@nongnu.org; Mon, 27 Feb 2012 20:04:39 -0500 Received: from mail-bk0-f45.google.com ([209.85.214.45]:49741) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2BUb-0002Vy-Ln for qemu-devel@nongnu.org; Mon, 27 Feb 2012 20:04:37 -0500 Received: by bkcjg9 with SMTP id jg9so1553492bkc.4 for ; Mon, 27 Feb 2012 17:04:34 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <201202271545.08365.paul@codesourcery.com> References: <201202211304.18584.paul@codesourcery.com> <201202271545.08365.paul@codesourcery.com> Date: Tue, 28 Feb 2012 11:04:34 +1000 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paul Brook Cc: Peter Maydell , monstr@monstr.eu, qemu-devel@nongnu.org, John Linn , duyl@xilinx.com, linnj@xilinx.com, edgar.iglesias@gmail.com, afaerber@suse.de, john.williams@petalogix.com On Tue, Feb 28, 2012 at 1:45 AM, Paul Brook wrote: >> On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook wro= te: >> >> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b) >> >> > +{ >> >> > + =A0 =A0if (a < b) { >> >> > + =A0 =A0 =A0 =A0return x > a && x <=3D b; >> >> > + =A0 =A0} >> >> > + =A0 =A0return x < a && x >=3D b; >> >> > +} >> >> >> >> This looks slightly odd -- should the boundary condition for whether >> >> a value equal to the max/min really change depending on :whether a >> >> or b is greater? >> >> The function determines whether x is in-between a and b exclusive of >> a, inclusive of b, so it is consistent with itself in that regard. >> >> > This is a ugly hack. =A0Instead of figuring out whether we have a coun= t-up >> > or count-down timer the code checks for both, and have the "in_between= " >> > function magically DTRT. =A0I haven't followed the paths through in en= ough >> > detail to figure out whether it gets all the corner cases right. >> >> Is it really a "hack"?? For count up b will always be greater than a, >> and for count down the reverse. I suppose I could assert these >> conditions at the call site for peace of mind? The invocation from >> cadence_timer_run doesn't care whether it is count up of count down, >> it really does just only care if the match value is in-between the >> current timer value and the next timer value, which is exactly what >> this function determines. > > When you explain it like this, it makes a more sense. =A0But this isn't > immediately obvious from the code. =A0It took me at least a couple of rea= dings > to figure out what was going on. This is exactly the sort of thing that s= hould > be described in comments. Ok, ill be a little more descriptive :) A function with a very generic name Perhaps clarify the whole inclusive a exclusive b in comment? is used in a > way that has fairly subtle implications. =A0There's a good chance someone= [1] > will come along in a few months/years, reuse this function and "fix" the > wierdness at the same time. > > Annother non-obvious detail is the way you handle overflow. =A0Specifical= ly you > check a range both plus and minus the wrap value before wrapping the fina= l > count. =A0This is certainly confusing/surprising when you first encounter= it. > Very large steps result in overlapping ranges, which triggers [in this ca= se > harmless] warning bells. > > Thinking about that, I realised why I don't like the following line: > >> + =A0 =A0s->reg_value =3D (uint32_t)((x + interval) % interval); > > This assumes x > -interval, which is not always true. This would mean you have wrapped twice or more in one time step, which I am assuming is a fatal error condition, as It means your software has missed interrupts and all sort of race conditions would occur. I would personally prefer to assert !(x < -interval) and have qemu hw_error or something, as in these cases QEMU can just not handle your super quick timer wrap around. > > Paul > > [1] "someone" includes me. =A0After I've forgotten this obscure detail.