[U-Boot] [PATCH v2 0/9] Add a better USB keyboard polling method
Hi Marek,
Here is v2 of my "Add a better USB keyboard polling method" patch-set.
Changes in v2: -Added 2 new patches: usb: kbd: Fix memleak on usb_kbd_deregister() stdio: Fix memleak on stdio_deregister -Fixed use after free in: usb: kbd: Add (optional) support for using an interrupt queue for polling
Regards,
Hans
When periodic_schedules == 0, the schedule is disabled and there is no reason to disable it again.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/usb/host/ehci-hcd.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 6323c50..20830d7 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1258,9 +1258,11 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize, ALIGN_END_ADDR(struct qTD, result->tds, queuesize));
- if (disable_periodic(ctrl) < 0) { - debug("FATAL: periodic should never fail, but did"); - goto fail3; + if (ctrl->periodic_schedules > 0) { + if (disable_periodic(ctrl) < 0) { + debug("FATAL: periodic should never fail, but did"); + goto fail3; + } }
/* hook up to periodic list */
Preperation patch to use create_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/usb/host/ehci-hcd.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 20830d7..cf3e3c0 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize, struct int_queue *result = NULL; int i;
+ /* + * Interrupt transfers requiring several transactions are not supported + * because bInterval is ignored. + * + * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2 + * <= PKT_ALIGN if several qTDs are required, while the USB + * specification does not constrain this for interrupt transfers. That + * means that ehci_submit_async() would support interrupt transfers + * requiring several transactions only as long as the transfer size does + * not require more than a single qTD. + */ + if (elementsize > usb_maxpacket(dev, pipe)) { + printf("%s: xfers requiring several transactions are not supported.\n", + __func__); + return NULL; + } + debug("Enter create_int_queue\n"); if (usb_pipetype(pipe) != PIPE_INTERRUPT) { debug("non-interrupt pipe (type=%lu)", usb_pipetype(pipe)); @@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, debug("dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d", dev, pipe, buffer, length, interval);
- /* - * Interrupt transfers requiring several transactions are not supported - * because bInterval is ignored. - * - * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2 - * <= PKT_ALIGN if several qTDs are required, while the USB - * specification does not constrain this for interrupt transfers. That - * means that ehci_submit_async() would support interrupt transfers - * requiring several transactions only as long as the transfer size does - * not require more than a single qTD. - */ - if (length > usb_maxpacket(dev, pipe)) { - printf("%s: Interrupt transfers requiring several " - "transactions are not supported.\n", __func__); - return -1; - } - queue = create_int_queue(dev, pipe, 1, length, buffer); + if (!queue) + return -1;
timeout = get_timer(0) + USB_TIMEOUT_MS(pipe); while ((backbuffer = poll_int_queue(dev, queue)) == NULL)
Preperation patch to use poll_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/usb/host/ehci-hcd.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index cf3e3c0..e527a7a 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1106,6 +1106,7 @@ submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer, }
struct int_queue { + int elementsize; struct QH *first; struct QH *current; struct QH *last; @@ -1200,6 +1201,7 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize, debug("ehci intr queue: out of memory\n"); goto fail1; } + result->elementsize = elementsize; result->first = memalign(USB_DMA_MINALIGN, sizeof(struct QH) * queuesize); if (!result->first) { @@ -1336,6 +1338,11 @@ void *poll_int_queue(struct usb_device *dev, struct int_queue *queue) queue->current++; else queue->current = NULL; + + invalidate_dcache_range((uint32_t)cur->buffer, + ALIGN_END_ADDR(char, cur->buffer, + queue->elementsize)); + debug("Exit poll_int_queue with completed intr transfer. token is %x at %p (first at %p)\n", hc32_to_cpu(cur_td->qt_token), cur, queue->first); return cur->buffer; @@ -1419,9 +1426,6 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, return -EINVAL; }
- invalidate_dcache_range((uint32_t)buffer, - ALIGN_END_ADDR(char, buffer, length)); - ret = destroy_int_queue(dev, queue); if (ret < 0) return ret;
Signed-off-by: Hans de Goede hdegoede@redhat.com --- include/usb.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/include/usb.h b/include/usb.h index c355fbe..6b29aed 100644 --- a/include/usb.h +++ b/include/usb.h @@ -129,6 +129,8 @@ struct usb_device { unsigned int slot_id; };
+struct int_queue; + /* * You can initialize platform's USB host or device * ports by passing this enum as an argument to @@ -162,6 +164,13 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer, int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, int transfer_len, int interval);
+#ifdef CONFIG_USB_EHCI /* Only the ehci code has pollable int support */ +struct int_queue *create_int_queue(struct usb_device *dev, unsigned long pipe, + int queuesize, int elementsize, void *buffer); +int destroy_int_queue(struct usb_device *dev, struct int_queue *queue); +void *poll_int_queue(struct usb_device *dev, struct int_queue *queue); +#endif + /* Defines */ #define USB_UHCI_VEND_ID 0x8086 #define USB_UHCI_DEV_ID 0x7112
This is not used anywhere, so lets remove it.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- common/usb_kbd.c | 26 -------------------------- 1 file changed, 26 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index fdc083c..cb869ac 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -116,32 +116,6 @@ extern int __maybe_unused net_busy_flag; /* The period of time between two calls of usb_kbd_testc(). */ static unsigned long __maybe_unused kbd_testc_tms;
-/* Generic keyboard event polling. */ -void usb_kbd_generic_poll(void) -{ - struct stdio_dev *dev; - struct usb_device *usb_kbd_dev; - struct usb_kbd_pdata *data; - struct usb_interface *iface; - struct usb_endpoint_descriptor *ep; - int pipe; - int maxp; - - /* Get the pointer to USB Keyboard device pointer */ - dev = stdio_get_by_name(DEVNAME); - usb_kbd_dev = (struct usb_device *)dev->priv; - data = usb_kbd_dev->privptr; - iface = &usb_kbd_dev->config.if_desc[0]; - ep = &iface->ep_desc[0]; - pipe = usb_rcvintpipe(usb_kbd_dev, ep->bEndpointAddress); - - /* Submit a interrupt transfer request */ - maxp = usb_maxpacket(usb_kbd_dev, pipe); - usb_submit_int_msg(usb_kbd_dev, pipe, data->new, - min(maxp, USB_KBD_BOOT_REPORT_SIZE), - ep->bInterval); -} - /* Puts character in the queue and sets up the in and out pointer. */ static void usb_kbd_put_queue(struct usb_kbd_pdata *data, char c) {
Free the keyboard hid-report buffer and private data on deregister.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- common/usb_kbd.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index cb869ac..253530a 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -524,9 +524,19 @@ int drv_usb_kbd_init(void) int usb_kbd_deregister(int force) { #ifdef CONFIG_SYS_STDIO_DEREGISTER - int ret = stdio_deregister(DEVNAME, force); - if (ret && ret != -ENODEV) - return ret; + struct stdio_dev *dev; + struct usb_device *usb_kbd_dev; + struct usb_kbd_pdata *data; + + dev = stdio_get_by_name(DEVNAME); + if (dev) { + usb_kbd_dev = (struct usb_device *)dev->priv; + data = usb_kbd_dev->privptr; + if (stdio_deregister_dev(dev, force) != 0) + return 1; + free(data->new); + free(data); + }
return 0; #else
stdio_register makes a malloc-ed copy of struct stdio_dev through stdio_clone, free the malloc-ed memory on stdio_deregister.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- common/stdio.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/common/stdio.c b/common/stdio.c index 8232815..c3ccbf5 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -197,6 +197,7 @@ int stdio_deregister_dev(struct stdio_dev *dev, int force) }
list_del(&(dev->list)); + free(dev);
/* reassign Device list */ list_for_each(pos, &(devs.list)) {
Instead of looking them up every time we need them.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- common/usb_kbd.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 253530a..a303d95 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -99,6 +99,10 @@ static const unsigned char usb_kbd_arrow[] = { #define USB_KBD_BOOT_REPORT_SIZE 8
struct usb_kbd_pdata { + unsigned long intpipe; + int intpktsize; + int intinterval; + uint32_t repeat_delay;
uint32_t usb_in_pointer; @@ -305,23 +309,11 @@ static int usb_kbd_irq(struct usb_device *dev) static inline void usb_kbd_poll_for_event(struct usb_device *dev) { #if defined(CONFIG_SYS_USB_EVENT_POLL) - struct usb_interface *iface; - struct usb_endpoint_descriptor *ep; - struct usb_kbd_pdata *data; - int pipe; - int maxp; - - /* Get the pointer to USB Keyboard device pointer */ - data = dev->privptr; - iface = &dev->config.if_desc[0]; - ep = &iface->ep_desc[0]; - pipe = usb_rcvintpipe(dev, ep->bEndpointAddress); + struct usb_kbd_pdata *data = dev->privptr;
/* Submit a interrupt transfer request */ - maxp = usb_maxpacket(dev, pipe); - usb_submit_int_msg(dev, pipe, &data->new[0], - min(maxp, USB_KBD_BOOT_REPORT_SIZE), - ep->bInterval); + usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize, + data->intinterval);
usb_kbd_irq_worker(dev); #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) @@ -389,7 +381,6 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) struct usb_interface *iface; struct usb_endpoint_descriptor *ep; struct usb_kbd_pdata *data; - int pipe, maxp;
if (dev->descriptor.bNumConfigurations != 1) return 0; @@ -438,8 +429,10 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) /* Set IRQ handler */ dev->irq_handle = usb_kbd_irq;
- pipe = usb_rcvintpipe(dev, ep->bEndpointAddress); - maxp = usb_maxpacket(dev, pipe); + data->intpipe = usb_rcvintpipe(dev, ep->bEndpointAddress); + data->intpktsize = min(usb_maxpacket(dev, data->intpipe), + USB_KBD_BOOT_REPORT_SIZE); + data->intinterval = ep->bInterval;
/* We found a USB Keyboard, install it. */ usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0); @@ -448,9 +441,8 @@ 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"); - if (usb_submit_int_msg(dev, pipe, data->new, - min(maxp, USB_KBD_BOOT_REPORT_SIZE), - ep->bInterval) < 0) { + if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize, + data->intinterval) < 0) { 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. */
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@redhat.com --- common/usb_kbd.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index a303d95..bc7145e 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. */ @@ -526,6 +542,9 @@ int usb_kbd_deregister(int force) data = usb_kbd_dev->privptr; if (stdio_deregister_dev(dev, force) != 0) return 1; +#ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE + destroy_int_queue(usb_kbd_dev, data->intq); +#endif free(data->new); free(data); }
On Wednesday, September 24, 2014 at 02:06:02 PM, Hans de Goede wrote:
Hi Marek,
Here is v2 of my "Add a better USB keyboard polling method" patch-set.
Changes in v2: -Added 2 new patches: usb: kbd: Fix memleak on usb_kbd_deregister() stdio: Fix memleak on stdio_deregister -Fixed use after free in: usb: kbd: Add (optional) support for using an interrupt queue for polling
Applied all to u-boot-usb/next . Please verify if all is in place.
Best regards, Marek Vasut
Hi,
On 09/24/2014 09:04 PM, Marek Vasut wrote:
On Wednesday, September 24, 2014 at 02:06:02 PM, Hans de Goede wrote:
Hi Marek,
Here is v2 of my "Add a better USB keyboard polling method" patch-set.
Changes in v2: -Added 2 new patches: usb: kbd: Fix memleak on usb_kbd_deregister() stdio: Fix memleak on stdio_deregister -Fixed use after free in: usb: kbd: Add (optional) support for using an interrupt queue for polling
Applied all to u-boot-usb/next . Please verify if all is in place.
Verified, everything is in place.
Thanks and Regards,
Hans
On Thursday, September 25, 2014 at 03:27:44 PM, Hans de Goede wrote:
Hi,
On 09/24/2014 09:04 PM, Marek Vasut wrote:
On Wednesday, September 24, 2014 at 02:06:02 PM, Hans de Goede wrote:
Hi Marek,
Here is v2 of my "Add a better USB keyboard polling method" patch-set.
Changes in v2:
-Added 2 new patches: usb: kbd: Fix memleak on usb_kbd_deregister() stdio: Fix memleak on stdio_deregister
-Fixed use after free in: usb: kbd: Add (optional) support for using an interrupt queue for polling
Applied all to u-boot-usb/next . Please verify if all is in place.
Verified, everything is in place.
Thanks and Regards,
Thank you!
Best regards, Marek Vasut
participants (2)
-
Hans de Goede -
Marek Vasut