Fix USB keyboard polling via control endpoint

PATCH New Adrian Cox 3 participants 11 messages Apr 5, 2014 – Apr 10, 2014 1 message not in the archive

Share & tools

A message that is not in the archive (sent off-list or not yet received): 13471364.2645.1396715619854.JavaMail.adrian@Gurnard

Adrian Cox

author

2014-04-05 16:36 UTC #1 lore raw

PATCH 1/1 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 <[email protected]>

---
 common/usb_kbd.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

common/usb_kbd.c +2 -2

index 1ad67ca..0f6c579 100644
@@ -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
 }

Wolfgang Denk

2014-04-06 11:17 UTC #2 lore raw

PATCH 1/1 Fix USB keyboard polling via control endpoint

Dear Adrian Cox,

Quoted 16 lines · 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 <[email protected]>
---
 common/usb_kbd.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
common/usb_kbd.c +0 -0
index 1ad67ca..0f6c579 100644

common/usb_kbd.c static inline void usb_kbd_poll_for_event(struct usb_device *dev) · line 346

Quoted 8 lines

common/usb_kbd.c +2 -2

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

Quoted 2 lines

From 315d78747a61330afe66b13747f03d74e60b8fcd Mon Sep 17 00:00:00 2001
From: Wolfgang Denk <[email protected]>

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 <[email protected]>

Signed-off-by Wolfgang Denk <[email protected]>

Cc Marek Vasut <[email protected]>

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

common/usb_kbd.c +23 ~~-11~~

index 1ad67ca..c6692c5 100644
@@ -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);

Signature · 17 lines

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.

Adrian Cox

author

2014-04-07 09:56 UTC #3 lore raw

PATCH 1/1 Fix USB keyboard polling via control endpoint

Quoted 8 lines

From: "Wolfgang Denk" <[email protected]>
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?

Quoted 4 lines

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

2014-04-07 16:59 UTC #4 lore raw

PATCH 1/1 Fix USB keyboard polling via control endpoint

Dear Adrian,

Quoted 10 lines · In message <3392231.3254.1396864604431.JavaMail.adrian@Gurnard> you wrote:

Quoted 8 lines
From: "Wolfgang Denk" <[email protected]>
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).

Quoted 1 line

Is this a broader problem in the USB stack?

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

Quoted 7 lines

Quoted 4 lines
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

Signature · 5 lines

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_

Marek Vasut

2014-04-10 08:12 UTC #5 lore raw

PATCH 1/1 Fix USB keyboard polling via control endpoint

In reply to Wolfgang Denk

On Sunday, April 06, 2014 at 01:17:46 PM, Wolfgang Denk wrote:
[...]

common/usb_kbd.c void usb_kbd_generic_poll(void) · line 138

Quoted 8 lines

common/usb_kbd.c +3 -1

@@ -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.

[...]

common/usb_kbd.c static inline void usb_kbd_poll_for_event(struct usb_device *dev) · line 344

Quoted 7 lines

(quoted diff) +0 -0

@@ -333,7 +340,9 @@ static inline void usb_kbd_poll_for_event(struct

usb_device *dev) /* Submit a interrupt transfer request */

common/usb_kbd.c +2 -1

 	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.

Quoted 4 lines

common/usb_kbd.c +1 -0

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

[...]

common/usb_kbd.c static int usb_kbd_probe(struct usb_device *dev, · line 475

Quoted 9 lines

common/usb_kbd.c +3 -1

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

author

2014-04-10 08:49 UTC #6 lore raw

PATCH 1/1 Fix USB keyboard polling via control endpoint

Quoted 10 lines

From: "Marek Vasut" <[email protected]>
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

2014-04-10 09:04 UTC #7 lore raw

PATCH 1/1 Fix USB keyboard polling via control endpoint

Quoted 11 lines · Adrian Cox

Quoted 10 lines
From: "Marek Vasut" <[email protected]>
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.

Quoted 4 lines

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

2014-04-10 10:15 UTC #8 lore raw

PATCH 1/1 Fix USB keyboard polling via control endpoint

In reply to Marek Vasut

Dear Marek,

Quoted 14 lines · In message <[email protected]> you wrote:

On Sunday, April 06, 2014 at 01:17:46 PM, Wolfgang Denk wrote:
[...]
common/usb_kbd.c void usb_kbd_generic_poll(void) · line 138
Quoted 8 lines
common/usb_kbd.c +3 -1
@@ -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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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

Signature · 5 lines

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

Marek Vasut

2014-04-10 12:33 UTC #9 lore raw

PATCH 1/1 Fix USB keyboard polling via control endpoint

Quoted 32 lines · Wolfgang Denk

Dear Marek,
Quoted 9 lines · In message <[email protected]> you wrote:
On Sunday, April 06, 2014 at 01:17:46 PM, Wolfgang Denk wrote:
[...]
Quoted 6 lines
common/usb_kbd.c +0 -0
@@ -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,
 
Quoted 7 lines
common/usb_kbd.c void usb_kbd_generic_poll(void) · line 138
Quoted 4 lines
common/usb_kbd.c +3 -1
-			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.
Quoted 4 lines
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.
Quoted 3 lines
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.

Quoted 6 lines

I'm not an USB expert; I cannot comment on this.
Quoted 1 line
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

Adrian Cox

author

2014-04-10 13:02 UTC #10 lore raw

PATCH v3 1/1 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 <[email protected]>

Signed-off-by Wolfgang Denk <[email protected]>

Cc Marek Vasut <[email protected]>

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

common/usb_kbd.c +25 ~~-11~~

index 1ad67ca..0b77c16 100644
@@ -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);

Signature · 1 line

1.7.9.5

Marek Vasut

2014-04-10 14:14 UTC #11 lore raw

PATCH v3 1/1 Fix USB keyboard polling via control endpoint

Quoted 6 lines · 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 <[email protected]>
Signed-off-by Wolfgang Denk <[email protected]>

Applied for -next, thanks!

Best regards,
Marek Vasut