[U-Boot] [PATCH v2 0/9] Add a better USB keyboard polling method

[U-Boot] [PATCH v2 0/9] Add a better USB keyboard polling method

[Hans de Goede]

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

[U-Boot] [PATCH v2 1/9] usb: ehci: Do not disable an already disabled periodic schedule

[Hans de Goede]

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 */
-- 
2.1.0

[U-Boot] [PATCH v2 2/9] usb: ehci: Move interrupt packet length check to create_int_queue

[Hans de Goede]

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

[U-Boot] [PATCH v2 3/9] usb: ehci: Move cache invalidation to poll_int_queue

[Hans de Goede]

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

[U-Boot] [PATCH v2 4/9] usb: Make pollable int support available outside of ehci-hcd.c

[Hans de Goede]

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

[U-Boot] [PATCH v2 5/9] usb: kbd: Remove unused usb_kbd_generic_poll function

[Hans de Goede]

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)
 {
-- 
2.1.0

[U-Boot] [PATCH v2 6/9] usb: kbd: Fix memleak on usb_kbd_deregister()

[Hans de Goede]

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

[U-Boot] [PATCH v2 7/9] stdio: Fix memleak on stdio_deregister

[Hans de Goede]

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)) {
-- 
2.1.0

[U-Boot] [PATCH v2 8/9] usb: kbd: Cache pipe, interval and packetsize

[Hans de Goede]

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. */
-- 
2.1.0

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

[Hans de Goede]

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);
 	}
-- 
2.1.0

[U-Boot] [PATCH v2 0/9] Add a better USB keyboard polling method

[Marek Vasut]

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

[U-Boot] [PATCH v2 0/9] Add a better USB keyboard polling method

[Hans de Goede]

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

[U-Boot] [PATCH v2 0/9] Add a better USB keyboard polling method

[Marek Vasut]

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