Re: broken src/sys/arch/x86/isa/clock.c (i8254_get_timecount)

["Frank Kardel" <kardel%kardel.name@localhost>]

Hi !

> hi-
>
>     while debugging my 4.0/i386 auich_calibrate() problem, I've
> come to the conclusion that i8254_get_timecount() has been broken
> since rev 1.98 of the old src/sys/arch/i386/isa/clock.c (2006/09/03)
> when "clkintr_pending" was removed.
>
> ...
>
> the problem with that is that i8254_ticked is 0 to start, so the very
> first time you hit the "if" statement it is going to set i8254_ticked to
> 1 and advance the i8254_offset irrespective of if a wrap is detected
> (e.g. count < i8254_lastcount).
>
That is a very good catch - it doesn't make sense. That part of the code
is meant to detect wraps.

>
> The earlier version of the if statement was:
>
>   if (rtclock_tval &&
>        (count < i8254_lastcount || (!i8254_ticked && clkintr_pending))) {
> ... }
>
> where "clkintr_pending" was an int that was always zero.   So this if
> only fired when the wrap detection (count < i8254_lastcount) was true.
>
>
> I looked at the FreeBSD version of this code and it looks like this:
>
>         if (count < i8254_lastcount ||
>             (!i8254_ticked && (clkintr_pending ||
>             ((count < 20 || (!(eflags & PSL_I) && count < timer0_max_count
> / 2u)
> ) &&
>             i8254_pending != NULL && i8254_pending(i8254_intsrc))))) { ...
> }
>
>
> not easy to understand (4 line "if" with 4 levels of parens: YUCK!), but
> I believe the idea of the extra stuff in the if statement is to only fire
> the !i8254_ticked case if !i8254_ticked AND an interrupt is pending.   the
> NetBSD version clearly doesn't have the "AND an interrupt is pending"
> part.
Right - we have not really the interrupt-pending infrastructure. And I
don't see how it would help much when we are able to catch the wraps.

> That is what is causing auich_calibrate() to fail!
>
>
>
>
> To fix the problem, I was thinking one could either ignore i8254_ticked
> like we used to:
>
>         if (rtclock_tval && count < i8254_lastcount)
>
>
> or alternately, maybe change the "||" to an "&&" (assuming that the
> interrupt will run before the counter wraps more than once?).
>
>         if (rtclock_tval && (count < i8254_lastcount && !i8254_ticked))
>
>
> either way makes auich_calibrate() work properly once again!  any
> suggestions on what to go with?
>
I'd go for the second one as it is needed to suppress the ..._offset update
from the clock interrupt code. If it would not be there, the wrap would
be detected and ..._offset would be incremented in ...get_timecount AND
in the clock interrupt code - this would result in a clock running
sometimes fast.

>
> chuck
>

Frank