[Qemu-devel] Re: [PATCH v2] add event queueing to USB HID

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/12/2011 01:34 PM, Ian Jackson wrote:
> Paolo Bonzini writes ("[PATCH v2] add event queueing to USB HID"):
>>          For v2 I changed the head/tail implementation of the FIFO buffer
>>          (which was buggy when the queue became full) to head/count.
>>          I then removed "have_data", which is the same as count>0
>>          for pointer devices and the same as "changed" for keyboards.
>>          This made event merging more correct than in Ian's code and
>>          easier to understand than my v1.
>
> Thanks.  Was my code buggy then ?  It's possible; the event merging
> case is not easy to trigger in testing.

It's all pretty academic as in practice it worked well.  The queue-full 
code would never trigger in usb_pointer_event, and instead the queue 
would be instantly emptied when a 17th event arrived.  This is lucky 
actually, because the device would also stop unqueuing from a full queue 
if it had SET_IDLE 0.  Both bugs happened because head==tail could mean 
both "empty queue" and "full queue".  I'm not even sure it's possible to 
fill the queue in practice; even with a very high latency you'd have to 
do 8 clicks in say half a second.  I didn't try with a shorter queue, 
which of course would have made the problems evident.

>>          I left the "changed" member in USBHIDState, rather than moving it
>>          to the keyboard, because it is useful to handle the idle period
>>          (in USB_TOKEN_IN) in a device-independent way.  Without it the
>>          code became more messy.
>
> This leaves the same information recorded in the driver in two places
> and is therefore IMO a bad idea.  I still think the way I did this is
> best: have a common helper function used by the keyboard and pointer
> code to deal with the idle handling.

I don't disagree, but I think this is better left for a separate patch. 
  I see three reasons for this:

1) I would like to understand better the relation between GET_REPORT and 
TOKEN_IN.  If an event occurs between two TOKEN_IN messages, and the OS 
sends a GET_REPORT in between, should the second TOKEN_IN return 
USB_RET_NAK or not?  With your patch it will, with current QEMU and my 
patch it won't.  In this sense, the "changed" flag is recording slightly 
different information at least in my version of the code.

2) if buffering is implemented for the keyboard device (and it probably 
should) most of the differences would go away again.

3) Secondarily, this is the only part of your patch that would need 
adjustments, since finite idle delays are now implemented.

Paolo