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

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

[Adrian Cox]

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);
 #endif
 }

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

[Wolfgang Denk]

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

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);
-- 
1.9.0

------------------- snip -------------------

Thanks!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The day-to-day travails of the IBM programmer are so amusing to  most
of us who are fortunate enough never to have been one - like watching
Charlie Chaplin trying to cook a shoe.

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

[Adrian Cox]

> 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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In accord with UNIX philosophy, Perl gives you enough  rope  to  hang
yourself.              - L. Wall & R. L. Schwartz, _Programming Perl_

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

[Marek Vasut]

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!

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

[Adrian Cox]

> 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

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

[Marek Vasut]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Remember that the best relationship is one in  which  your  love  for
each other exceeds your need for each other.             - Dalai Lama

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

[Marek Vasut]

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

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

[Adrian Cox]

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>
---
Changes in v3:
   - Fixed checkstyle warnings
   - Renamed constant to USB_KBD_BOOT_REPORT_SIZE
   - Use min() for readability

 common/usb_kbd.c |   36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 1ad67ca..0b77c16 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -91,6 +91,12 @@ static const unsigned char usb_kbd_arrow[] = {
 #define USB_KBD_LEDMASK		\
 	(USB_KBD_NUMLOCK | USB_KBD_CAPSLOCK | USB_KBD_SCROLLLOCK)
 
+/*
+ * USB Keyboard reports are 8 bytes in boot protocol.
+ * Appendix B of HID Device Class Definition 1.11
+ */
+#define USB_KBD_BOOT_REPORT_SIZE 8
+
 struct usb_kbd_pdata {
 	uint32_t	repeat_delay;
 
@@ -99,7 +105,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_BOOT_REPORT_SIZE];
 
 	uint8_t		flags;
 };
@@ -131,7 +137,8 @@ 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);
+		min(maxp, USB_KBD_BOOT_REPORT_SIZE),
+		ep->bInterval);
 }
 
 /* Puts character in the queue and sets up the in and out pointer. */
@@ -266,8 +273,11 @@ 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], USB_KBD_BOOT_REPORT_SIZE - 2) ==
+			new + USB_KBD_BOOT_REPORT_SIZE)) {
 		res |= usb_kbd_translate(data, old[i], data->new[0], up);
+	}
 
 	return res;
 }
@@ -285,7 +295,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_BOOT_REPORT_SIZE; i++) {
 		res |= usb_kbd_service_key(dev, i, 0);
 		res |= usb_kbd_service_key(dev, i, 1);
 	}
@@ -297,7 +307,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_BOOT_REPORT_SIZE);
 
 	return 1;
 }
@@ -305,7 +315,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_BOOT_REPORT_SIZE)) {
 		debug("USB KBD: Error %lX, len %d\n",
 		      dev->irq_status, dev->irq_act_len);
 		return 1;
@@ -333,7 +344,8 @@ 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);
+		min(maxp, USB_KBD_BOOT_REPORT_SIZE),
+		ep->bInterval);
 
 	usb_kbd_irq_worker(dev);
 #elif	defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
@@ -341,8 +353,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_BOOT_REPORT_SIZE);
+	if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE))
 		usb_kbd_irq_worker(dev);
 #endif
 }
@@ -441,7 +453,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_BOOT_REPORT_SIZE, USB_DMA_MINALIGN));
 
 	/* Insert private data into USB device structure */
 	dev->privptr = data;
@@ -459,7 +472,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, maxp > 8 ? 8 : maxp,
+	if (usb_submit_int_msg(dev, pipe, data->new,
+			       min(maxp, USB_KBD_BOOT_REPORT_SIZE),
 			       ep->bInterval) < 0) {
 		printf("Failed to get keyboard state from device %04x:%04x\n",
 		       dev->descriptor.idVendor, dev->descriptor.idProduct);
-- 
1.7.9.5

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

[Marek Vasut]

On Thursday, April 10, 2014 at 03:02:44 PM, Adrian Cox 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>
> Signed-off-by: Wolfgang Denk <wd@denx.de>

Applied for -next, thanks!

Best regards,
Marek Vasut