[U-Boot] [PATCH 7/7] usb: kbd: Add (optional) support for using an interrupt queue for polling

[Hans de Goede]

Hi,

On 09/20/2014 05:01 PM, Hans de Goede wrote:
> Waiting an interrupt packet to complete in usb_kbd_poll_for_event, causes
> a 40 ms latency for each call to usb_kbd_testc, which is undesirable.
> 
> Using control messages leads to lower (but still not 0) latency, but some
> devices do not work well with control messages (e.g. my kvm behaves funny
> with them).
> 
> This commit adds support for using the int_queue mechanism which at least
> the ehci-hcd driver supports. This allows polling with 0 latency, while
> using interrupt packets.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  common/usb_kbd.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 85ee1c8..278937c 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -102,6 +102,7 @@ struct usb_kbd_pdata {
>  	unsigned long	intpipe;
>  	int		intpktsize;
>  	int		intinterval;
> +	struct int_queue *intq;
>  
>  	uint32_t	repeat_delay;
>  
> @@ -324,6 +325,15 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>  		       1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE);
>  	if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE))
>  		usb_kbd_irq_worker(dev);
> +#elif	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
> +	struct usb_kbd_pdata *data = dev->privptr;
> +	if (poll_int_queue(dev, data->intq)) {
> +		usb_kbd_irq_worker(dev);
> +		/* We've consumed all queued int packets, create new */
> +		destroy_int_queue(dev, data->intq);
> +		data->intq = create_int_queue(dev, data->intpipe, 1,
> +				      USB_KBD_BOOT_REPORT_SIZE, data->new);
> +	}
>  #endif
>  }
>  
> @@ -441,8 +451,14 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
>  	usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
>  
>  	debug("USB KBD: enable interrupt pipe...\n");
> +#ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
> +	data->intq = create_int_queue(dev, data->intpipe, 1,
> +				      USB_KBD_BOOT_REPORT_SIZE, data->new);
> +	if (!data->intq) {
> +#else
>  	if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
>  			       data->intinterval) < 0) {
> +#endif
>  		printf("Failed to get keyboard state from device %04x:%04x\n",
>  		       dev->descriptor.idVendor, dev->descriptor.idProduct);
>  		/* Abort, we don't want to use that non-functional keyboard. */
> @@ -515,13 +531,28 @@ int drv_usb_kbd_init(void)
>  /* Deregister the keyboard. */
>  int usb_kbd_deregister(int force)
>  {
> -#ifdef CONFIG_SYS_STDIO_DEREGISTER
> +#if !defined(CONFIG_SYS_STDIO_DEREGISTER)
> +	return 1;
> +#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
> +	struct stdio_dev *dev;
> +	struct usb_device *usb_kbd_dev;
> +	struct usb_kbd_pdata *data;
> +
> +	dev = stdio_get_by_name(DEVNAME);
> +	if (dev) {
> +		if (stdio_deregister_dev(dev, force) != 0)
> +			return 1;
> +		usb_kbd_dev = (struct usb_device *)dev->priv;

Hmm I've just realized that this is a use after free for dev.
I've fixed this in my local tree. I'll wait for review of the
other patches in this set before sending a v2.

Note this is not really a use after free, since stdio_deregister_dev
does not free dev, but it reallu should free dev, since stdio_register_dev
does a clone of the passed in dev, so stdio_deregister_dev should free it,
so as to not leak memory. Once that memory leak gets fixed, this will be
a use after free.

Likewise usb_kbd.c should free usb_kbd_pdata and the data->new buffer on
deregister. I'll add a fix plugging the memleak in usb_kbd.c to v2 of this
set.

Regards,

Hans