diff mbox

[U-Boot] Fix USB keyboard polling via control endpoint

Message ID 20140406111746.25D3538099A@gemini.denx.de
State Accepted
Delegated to: Marek Vasut
Headers show

Commit Message

Wolfgang Denk April 6, 2014, 11:17 a.m. UTC 
Dear Adrian Cox,

In message <4526969.2646.1396715845133.JavaMail.adrian@Gurnard> you wrote:
> 
> USB keyboard polling failed for some keyboards on PowerPC 5020.
> This was caused by requesting only 4 bytes of data from keyboards that
> produce an 8 byte HID report.
> 
> Signed-off-by: Adrian Cox <adrian@humboldt.co.uk>
> 
> ---
>  common/usb_kbd.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 1ad67ca..0f6c579 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -341,8 +341,8 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>  	struct usb_kbd_pdata *data = dev->privptr;
>  	iface = &dev->config.if_desc[0];
>  	usb_get_report(dev, iface->desc.bInterfaceNumber,
> -			1, 0, data->new, sizeof(data->new));
> -	if (memcmp(data->old, data->new, sizeof(data->new)))
> +			1, 0, data->new, 8);
> +	if (memcmp(data->old, data->new, 8))
>  		usb_kbd_irq_worker(dev);

I agree that the code is wrong and needs fixing.  data->new is an
uint8_t pointer, so taking the size of the pointer is obviously
wrong.  But what you fix here is not the only place where
sizeof(data->new) is used, so this patch fixes part of the problem at
best.

Can you please try out if the following extended version f the patch
works and fixes your problem?  You will note that I removed all
occurrences of this magic number 8 by replacing it with
USB_KBD_PDATA_SIZE  so the could should also be easier to read.

------------------- snip -------------------
> From 315d78747a61330afe66b13747f03d74e60b8fcd Mon Sep 17 00:00:00 2001
> From: Wolfgang Denk <wd@denx.de>
Date: Sun, 6 Apr 2014 13:14:26 +0200
Subject: [PATCH] Fix USB keyboard polling via control endpoint

USB keyboard polling failed for some keyboards on PowerPC 5020.
This was caused by requesting only 4 bytes of data from keyboards that
produce an 8 byte HID report.

Signed-off-by: Adrian Cox <adrian@humboldt.co.uk>
Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Marek Vasut <marex@denx.de>
---
 common/usb_kbd.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

Comments

Adrian Cox April 7, 2014, 9:56 a.m. UTC | #1 
> From: "Wolfgang Denk" <wd@denx.de>

> I agree that the code is wrong and needs fixing.  data->new is an
> uint8_t pointer, so taking the size of the pointer is obviously
> wrong.  But what you fix here is not the only place where
> sizeof(data->new) is used, so this patch fixes part of the problem at
> best.

I can't find any other instances of sizeof in usb_kbd.c. Is this a broader problem in the USB stack?

> Can you please try out if the following extended version f the patch
> works and fixes your problem?  You will note that I removed all
> occurrences of this magic number 8 by replacing it with
> USB_KBD_PDATA_SIZE  so the could should also be easier to read.

I've tested your extended patch, and it does fix the problem for me. I agree that adding USB_KBD_PDATA_SIZE
does improve readability.

Thanks and regards,
Adrian Cox
Wolfgang Denk April 7, 2014, 4:59 p.m. UTC | #2 
Dear Adrian,

In message <3392231.3254.1396864604431.JavaMail.adrian@Gurnard> you wrote:
> 
> > From: "Wolfgang Denk" <wd@denx.de>
> 
> > I agree that the code is wrong and needs fixing.  data->new is an
> > uint8_t pointer, so taking the size of the pointer is obviously
> > wrong.  But what you fix here is not the only place where
> > sizeof(data->new) is used, so this patch fixes part of the problem at
> > best.
> 
> I can't find any other instances of sizeof in usb_kbd.c.

Yes, you are right.  I did not see that your patch fixes both
occurrences of sizeof(data->new).

> Is this a broader problem in the USB stack?

I haven't looked at the other code, so I cannot comment on that.

> > Can you please try out if the following extended version f the patch
> > works and fixes your problem?  You will note that I removed all
> > occurrences of this magic number 8 by replacing it with
> > USB_KBD_PDATA_SIZE  so the could should also be easier to read.
> 
> I've tested your extended patch, and it does fix the problem for me. I agree that adding USB_KBD_PDATA_SIZE
> does improve readability.

Thanks for testing.

Marek, can you then please use my version of the patch instead of the
original one?

Thanks.

Best regards,

Wolfgang Denk
Marek Vasut April 10, 2014, 8:12 a.m. UTC | #3 
On Sunday, April 06, 2014 at 01:17:46 PM, Wolfgang Denk wrote:
[...]

> @@ -131,7 +133,9 @@ void usb_kbd_generic_poll(void)
>  	/* Submit a interrupt transfer request */
>  	maxp = usb_maxpacket(usb_kbd_dev, pipe);
>  	usb_submit_int_msg(usb_kbd_dev, pipe, data->new,
> -			maxp > 8 ? 8 : maxp, ep->bInterval);
> +			maxp > USB_KBD_PDATA_SIZE ?
> +				USB_KBD_PDATA_SIZE : maxp,
> +			ep->bInterval);

Can we use min(8, USB_KBD_PDATA_SIZE) here instead of this multi-line monster 
call please ?

Also, this USB_KBD_PDATA_SIZE should instead be called "USB_KBD_LS_REPORT_SIZE" 
according to USB HID spec v1.11 section 5.6 :

5.6 Reports
Using USB terminology, a device may send or receive a transaction every USB
frame (1 millisecond). A transaction may be made up of multiple packets (token,
data, handshake) but is limited in size to 8 bytes for low-speed devices and 64
bytes for high-speed devices. A transfer is one or more transactions creating a 
set of data that is meaningful to the device—for example, Input, Output, and
Feature reports. In this document, a transfer is synonymous with a report.

What worries me a bit is that 64-byte high-speed report, but I never saw a 
device that would generate those. This section 5.6 is also the only place that 
mentions the high-speed HID device report size limit.

[...]

> @@ -333,7 +340,9 @@ static inline void usb_kbd_poll_for_event(struct
> usb_device *dev) /* Submit a interrupt transfer request */
>  	maxp = usb_maxpacket(dev, pipe);
>  	usb_submit_int_msg(dev, pipe, &data->new[0],
> -			maxp > 8 ? 8 : maxp, ep->bInterval);
> +			maxp > USB_KBD_PDATA_SIZE ?
> +				USB_KBD_PDATA_SIZE : maxp,

min(USB_KBD_LS_REPORT_SIZE, maxp) would be nice.

> +			ep->bInterval);
> 
>  	usb_kbd_irq_worker(dev);
>  #elif	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)

[...]

> @@ -459,7 +469,9 @@ 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, maxp > 8 ? 8 : maxp,
> +	if (usb_submit_int_msg(dev, pipe, data->new,
> +			       maxp > USB_KBD_PDATA_SIZE ?
> +					USB_KBD_PDATA_SIZE : maxp,

Here as well.

Other than that, I don't see a problem.

Thank you!
Adrian Cox April 10, 2014, 8:49 a.m. UTC | #4 
> From: "Marek Vasut" <marex@denx.de>

> Also, this USB_KBD_PDATA_SIZE should instead be called
> "USB_KBD_LS_REPORT_SIZE"
> [...]
> What worries me a bit is that 64-byte high-speed report, but I never
> saw a
> device that would generate those. This section 5.6 is also the only
> place that
> mentions the high-speed HID device report size limit.

How about renaming to USB_KBD_BOOT_REPORT_SIZE? We know that the keyboard report will
be 8 bytes because we explicitly set the keyboard into boot protocol
(Appendix B of HID 1.11: "The report may not exceed 8 bytes in length.").

Regards,
Adrian Cox
Marek Vasut April 10, 2014, 9:04 a.m. UTC | #5 
On Thursday, April 10, 2014 at 10:49:56 AM, Adrian Cox wrote:
> > From: "Marek Vasut" <marex@denx.de>
> > 
> > Also, this USB_KBD_PDATA_SIZE should instead be called
> > "USB_KBD_LS_REPORT_SIZE"
> > [...]
> > What worries me a bit is that 64-byte high-speed report, but I never
> > saw a
> > device that would generate those. This section 5.6 is also the only
> > place that
> > mentions the high-speed HID device report size limit.
> 
> How about renaming to USB_KBD_BOOT_REPORT_SIZE? We know that the keyboard
> report will be 8 bytes because we explicitly set the keyboard into boot
> protocol (Appendix B of HID 1.11: "The report may not exceed 8 bytes in
> length.").

Good point, thanks. A comment in the code about where this number '8' comes from 
would be nice.

Best regards,
Marek Vasut
Wolfgang Denk April 10, 2014, 10:15 a.m. UTC | #6 
Dear Marek,

In message <201404101012.42527.marex@denx.de> you wrote:
> On Sunday, April 06, 2014 at 01:17:46 PM, Wolfgang Denk wrote:
> [...]
>
> > @@ -131,7 +133,9 @@ void usb_kbd_generic_poll(void)
> >  	/* Submit a interrupt transfer request */
> >  	maxp = usb_maxpacket(usb_kbd_dev, pipe);
> >  	usb_submit_int_msg(usb_kbd_dev, pipe, data->new,
> > -			maxp > 8 ? 8 : maxp, ep->bInterval);
> > +			maxp > USB_KBD_PDATA_SIZE ?
> > +				USB_KBD_PDATA_SIZE : maxp,
> > +			ep->bInterval);
>
> Can we use min(8, USB_KBD_PDATA_SIZE) here instead of this multi-line monster 
> call please ?

Agreed.

> Also, this USB_KBD_PDATA_SIZE should instead be called "USB_KBD_LS_REPORT_S> IZE" 
> according to USB HID spec v1.11 section 5.6 :

Fine with me.

> What worries me a bit is that 64-byte high-speed report, but I never saw a 
> device that would generate those. This section 5.6 is also the only place that 
> mentions the high-speed HID device report size limit.

I'm not an USB expert; I cannot comment on this.

> Other than that, I don't see a problem.

So how should we proceed?  Shall I prepare an updated patch - or
Adrian, will you do that?

Best regards,

Wolfgang Denk
Marek Vasut April 10, 2014, 12:33 p.m. UTC | #7 
On Thursday, April 10, 2014 at 12:15:30 PM, Wolfgang Denk wrote:
> Dear Marek,
> 
> In message <201404101012.42527.marex@denx.de> you wrote:
> > On Sunday, April 06, 2014 at 01:17:46 PM, Wolfgang Denk wrote:
> > [...]
> > 
> > > @@ -131,7 +133,9 @@ void usb_kbd_generic_poll(void)
> > > 
> > >  	/* Submit a interrupt transfer request */
> > >  	maxp = usb_maxpacket(usb_kbd_dev, pipe);
> > >  	usb_submit_int_msg(usb_kbd_dev, pipe, data->new,
> > > 
> > > -			maxp > 8 ? 8 : maxp, ep->bInterval);
> > > +			maxp > USB_KBD_PDATA_SIZE ?
> > > +				USB_KBD_PDATA_SIZE : maxp,
> > > +			ep->bInterval);
> > 
> > Can we use min(8, USB_KBD_PDATA_SIZE) here instead of this multi-line
> > monster call please ?
> 
> Agreed.
> 
> > Also, this USB_KBD_PDATA_SIZE should instead be called
> > "USB_KBD_LS_REPORT_S> IZE"
> 
> > according to USB HID spec v1.11 section 5.6 :
> Fine with me.
> 
> > What worries me a bit is that 64-byte high-speed report, but I never saw
> > a device that would generate those. This section 5.6 is also the only
> > place that mentions the high-speed HID device report size limit.
> 
> I'm not an USB expert; I cannot comment on this.
> 
> > Other than that, I don't see a problem.
> 
> So how should we proceed?  Shall I prepare an updated patch - or
> Adrian, will you do that?

Adrian, can you please take all the discussion here and roll out a new patch 
please ?

Best regards,
Marek Vasut
diff mbox

Patch

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 1ad67ca..c6692c5 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -91,6 +91,8 @@  static const unsigned char usb_kbd_arrow[] = {
 #define USB_KBD_LEDMASK		\
 	(USB_KBD_NUMLOCK | USB_KBD_CAPSLOCK | USB_KBD_SCROLLLOCK)
 
+#define USB_KBD_PDATA_SIZE	8
+
 struct usb_kbd_pdata {
 	uint32_t	repeat_delay;
 
@@ -99,7 +101,7 @@  struct usb_kbd_pdata {
 	uint8_t		usb_kbd_buffer[USB_KBD_BUFFER_LEN];
 
 	uint8_t		*new;
-	uint8_t		old[8];
+	uint8_t		old[USB_KBD_PDATA_SIZE];
 
 	uint8_t		flags;
 };
@@ -131,7 +133,9 @@  void usb_kbd_generic_poll(void)
 	/* Submit a interrupt transfer request */
 	maxp = usb_maxpacket(usb_kbd_dev, pipe);
 	usb_submit_int_msg(usb_kbd_dev, pipe, data->new,
-			maxp > 8 ? 8 : maxp, ep->bInterval);
+			maxp > USB_KBD_PDATA_SIZE ?
+				USB_KBD_PDATA_SIZE : maxp,
+			ep->bInterval);
 }
 
 /* Puts character in the queue and sets up the in and out pointer. */
@@ -266,8 +270,10 @@  static uint32_t usb_kbd_service_key(struct usb_device *dev, int i, int up)
 		old = data->old;
 	}
 
-	if ((old[i] > 3) && (memscan(new + 2, old[i], 6) == new + 8))
+	if ((old[i] > 3) &&
+	    (memscan(new + 2, old[i], 6) == new + USB_KBD_PDATA_SIZE)) {
 		res |= usb_kbd_translate(data, old[i], data->new[0], up);
+	}
 
 	return res;
 }
@@ -285,7 +291,7 @@  static int usb_kbd_irq_worker(struct usb_device *dev)
 	else if ((data->new[0] == LEFT_CNTR) || (data->new[0] == RIGHT_CNTR))
 		data->flags |= USB_KBD_CTRL;
 
-	for (i = 2; i < 8; i++) {
+	for (i = 2; i < USB_KBD_PDATA_SIZE; i++) {
 		res |= usb_kbd_service_key(dev, i, 0);
 		res |= usb_kbd_service_key(dev, i, 1);
 	}
@@ -297,7 +303,7 @@  static int usb_kbd_irq_worker(struct usb_device *dev)
 	if (res == 1)
 		usb_kbd_setled(dev);
 
-	memcpy(data->old, data->new, 8);
+	memcpy(data->old, data->new, USB_KBD_PDATA_SIZE);
 
 	return 1;
 }
@@ -305,7 +311,8 @@  static int usb_kbd_irq_worker(struct usb_device *dev)
 /* Keyboard interrupt handler */
 static int usb_kbd_irq(struct usb_device *dev)
 {
-	if ((dev->irq_status != 0) || (dev->irq_act_len != 8)) {
+	if ((dev->irq_status != 0) ||
+	    (dev->irq_act_len != USB_KBD_PDATA_SIZE)) {
 		debug("USB KBD: Error %lX, len %d\n",
 		      dev->irq_status, dev->irq_act_len);
 		return 1;
@@ -333,7 +340,9 @@  static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 	/* Submit a interrupt transfer request */
 	maxp = usb_maxpacket(dev, pipe);
 	usb_submit_int_msg(dev, pipe, &data->new[0],
-			maxp > 8 ? 8 : maxp, ep->bInterval);
+			maxp > USB_KBD_PDATA_SIZE ?
+				USB_KBD_PDATA_SIZE : maxp,
+			ep->bInterval);
 
 	usb_kbd_irq_worker(dev);
 #elif	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
@@ -341,8 +350,8 @@  static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 	struct usb_kbd_pdata *data = dev->privptr;
 	iface = &dev->config.if_desc[0];
 	usb_get_report(dev, iface->desc.bInterfaceNumber,
-			1, 0, data->new, sizeof(data->new));
-	if (memcmp(data->old, data->new, sizeof(data->new)))
+			1, 0, data->new, USB_KBD_PDATA_SIZE);
+	if (memcmp(data->old, data->new, USB_KBD_PDATA_SIZE))
 		usb_kbd_irq_worker(dev);
 #endif
 }
@@ -441,7 +450,8 @@  static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 	memset(data, 0, sizeof(struct usb_kbd_pdata));
 
 	/* allocate input buffer aligned and sized to USB DMA alignment */
-	data->new = memalign(USB_DMA_MINALIGN, roundup(8, USB_DMA_MINALIGN));
+	data->new = memalign(USB_DMA_MINALIGN,
+				roundup(USB_KBD_PDATA_SIZE, USB_DMA_MINALIGN));
 
 	/* Insert private data into USB device structure */
 	dev->privptr = data;
@@ -459,7 +469,9 @@  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, maxp > 8 ? 8 : maxp,
+	if (usb_submit_int_msg(dev, pipe, data->new,
+			       maxp > USB_KBD_PDATA_SIZE ?
+					USB_KBD_PDATA_SIZE : maxp,
 			       ep->bInterval) < 0) {
 		printf("Failed to get keyboard state from device %04x:%04x\n",
 		       dev->descriptor.idVendor, dev->descriptor.idProduct);