Re: the PS/2 mouse problem

On Sat, 8 Nov 2003, Morten Johansen wrote:

> Scott Long wrote:
> > Bruce Evans wrote:
> >>[... possibly too much trimmed]
> > The problem here is that the keyboard controller driver tries to be too
> > smart. If it detects that the hardware FIFO is full, it'll drain it into
> > a per-softc, per-function ring buffer.  So having psm(4) just directly
> > read the hardware is insufficient in this scheme.

What is the per-function part?  (I'm not very familar with psm, but once
understood simpler versions of the keyboard driver.)  Several layers of
buffering might not be too bad for slow devices.  The i/o times tend to
dominate unless you do silly things like a context switch to move each
character from one buffer to other, and even that can be fast enough
(I believe it is normal for interactive input on ptys; then there's often
a remote IPC or two per character as well).

> >> - it sometimes calls the DELAY() function, which is not permitted in fast
> >>   interrupt handlers since apart from locking issues, fast interrupt
> >> handlers
> >>   are not permitted to busy-wait.
> >
> > Again, the keyboard controller driver is too smart for its own good.  To
> > summarize:
> >
> > read_aux_data_no_wait()
> > {
> >     Does softc->aux ring buffer contain data?
> >         return ring buffer data
> >
> >     Check the status register
> >     Is the keyboard fifo full?
> >         DELAY(7us)
> >         read keyboard fifo into softc->kbd ring buffer
> >         Check the status register
> >
> >     Is the aux fifo full?
> >         DELAY(7us)
> >         return aux fifo data
> > }
> >
> > So you can wind up stalling for 14us in there, presumably because you
> > cannot read the status and data registers back-to-back without a delay.
> > I don't have the atkbd spec handy so I'm not sure how to optimize this.
> > Do you really need to check the status register before reading the data
> > register?

At least it's a bounded delay.  I believe such delays are required for
some layers of the keyboard.  Perhaps only for the keyboard (old hardware
only?) and not for the keyboard controller or the mouse.

> >> Many of the complications for fast interrupt handlers shouldn't be needed
> >> in psm.  Just make psmintr() INTR_MPSAFE.
> >
> > I believe that the previous poster actually tried making it INTR_MPSAFE,
> > but didn't see a measurable benefit because the latency of scheduling
> > the ithread is still unacceptable.
>
> That is 100% correct.
> In the meantime I have taken your's and Bruce's advice and rearranged
> the interrupt handler to look like this:
>
> mtx_lock(&sc->input_mtx);

Er, this is reasonable for INTR_MPSAFE but not for INTR_FAST.
mtx_lock() is a "sleep" lock so it cannot be used in fast interrupt
handlers.  mtx_lock_spin() must be used.  (My version doesn't permit
use of mtx_lock_spin() either; more primitive locking must be used.)

> while((c = read_aux_data_no_wait(sc->kbdc)) != -1) {

This is probably INTR_FAST-safe enough in practice.

>      sc->input_queue.buf[sc->input_queue.tail] = c;
>      if ((++ sc->input_queue.tail) >= PSM_BUFSIZE)
>          sc->input_queue.tail = 0;
>      count = (++ sc->input_queue.count);
> }
> mtx_unlock(&sc->input_mtx);

The locking for the queue seems to be correct except this should operate
on a spinlock too.

> if (count >= sc->mode.packetsize)
>      taskqueue_enqueue(taskqueue_swi_giant, &sc->psm_task);

taskqueue_enqueue() can only be used in non-fast interrupt handlers.
taskqueue_enqueue_fast() must be used in fast interrupt handlers (except
in my version, it is not permitted so it shouldn't exist).  Note that
the spinlock/fast versions can be used for normal interrupt handlers
too, so not much more code is needed to support handlers whose fastness
is dynamically configured.

> And it works, but having it INTR_MPSAFE still does NOT help my problem.
> It looks to me like data is getting lost because the interrupt handler
> is unable to read it before it's gone, and the driver gets out of sync,
> and has to reset itself.
> However it now takes a few more tries to provoke the problem, so
> something seems to have improved somewhere.

This is a bit surprising.  There are still so few INTR_MPSAFE handlers
that there aren't many system activities that get in the way of running
the INTR_MPSAFE ones.  Shared interrupts prevent running of a handler
while other handlers on the same interrupt are running, and the mouse
interrupt is often shared, but if it is shared then it couldn't be fast
until recently and still can't be fast unless all the other handlers on
it are fast.

Bruce