Re: the PS/2 mouse problem

Bruce Evans wrote:
> 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.
> 


Yes, I have submitted a PR (kern/59067), with an INTR_FAST version that 
uses spinlocks and taskqueue_fast.
You can find it here if you have time to look at it:
http://dsm.oslonett.no/patch-psm-fast
I would appreciate comments on it's correctness.


> 
>>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


It seems odd that it should be necessary to have it INTR_FAST.
But somehow it is, on my system.


   Morten