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
