Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model

[Peter Crosthwaite]

On Tue, Feb 28, 2012 at 1:45 AM, Paul Brook <address@hidden> wrote:
>> On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook <address@hidden> wrote:
>> >> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
>> >> > +{
>> >> > +    if (a < b) {
>> >> > +        return x > a && x <= b;
>> >> > +    }
>> >> > +    return x < a && x >= 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.  Instead of figuring out whether we have a count-up
>> > or count-down timer the code checks for both, and have the "in_between"
>> > function magically DTRT.  I haven't followed the paths through in enough
>> > 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.  But this isn't
> immediately obvious from the code.  It took me at least a couple of readings
> to figure out what was going on. This is exactly the sort of thing that should
> 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.  There'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.  Specifically you
> check a range both plus and minus the wrap value before wrapping the final
> count.  This is certainly confusing/surprising when you first encounter it.
> Very large steps result in overlapping ranges, which triggers [in this case
> harmless] warning bells.
>
> Thinking about that, I realised why I don't like the following line:
>
>> +    s->reg_value = (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.  After I've forgotten this obscure detail.