usb: kbd: don't use int xfers when polling via ctrl xfers

PATCH New Stephen Warren 3 participants 9 messages Nov 13, 2015 – Dec 16, 2015

lore

Share & tools

lore

Stephen Warren

author

2015-11-13 20:34 UTC #0 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

From: Stephen Warren <[email protected]>

When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.

Cc Hans de Goede <[email protected]>

Signed-off-by Stephen Warren <[email protected]>

---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.

---
 common/usb_kbd.c | 3 +++
 1 file changed, 3 insertions(+)

common/usb_kbd.c +3 -0

index 95912f99c767..81c4d62c632b 100644
@@ -475,6 +475,9 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 				      USB_KBD_BOOT_REPORT_SIZE, data->new,
 				      data->intinterval);
 	if (!data->intq) {
+#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
+	if (usb_get_report(dev, iface->desc.bInterfaceNumber,
+			   1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) {
 #else
 	if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
 			       data->intinterval) < 0) {

Signature · 1 line

1.9.1

Marek Vasut

2015-11-14 01:16 UTC #1 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

Quoted 9 lines · Stephen Warren

From: Stephen Warren <[email protected]>
When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.

Quoted 6 lines

Cc Hans de Goede <[email protected]>
Signed-off-by Stephen Warren <[email protected]>
---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.

I initially implemented the control EP polling because I had a keyboard which had issues with interrupt transfers.

Reviewed-by Marek Vasut <[email protected]>

Quoted 9 lines

---
 common/usb_kbd.c | 3 +++
 1 file changed, 3 insertions(+)

common/usb_kbd.c +0 -0

index 95912f99c767..81c4d62c632b 100644

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

Quoted 9 lines

common/usb_kbd.c +3 -0

@@ -475,6 +475,9 @@ static int usb_kbd_probe(struct usb_device *dev,
 unsigned int ifnum) USB_KBD_BOOT_REPORT_SIZE, data->new,
 				      data->intinterval);
 	if (!data->intq) {
+#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
+	if (usb_get_report(dev, iface->desc.bInterfaceNumber,
+			   1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) {
 #else
 	if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
 			       data->intinterval) < 0) {

Best regards,
Marek Vasut

Stephen Warren

author

2015-12-15 23:35 UTC #2 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

Quoted 17 lines · Marek Vasut

Quoted 9 lines · Stephen Warren
From: Stephen Warren <[email protected]>
When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.
Quoted 6 lines
Cc Hans de Goede <[email protected]>
Signed-off-by Stephen Warren <[email protected]>
---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.

Quoted 4 lines

I initially implemented the control EP polling because I had a keyboard which had issues with interrupt transfers.
Reviewed-by Marek Vasut <[email protected]>

Did you intend someone else to apply this?

Marek Vasut

2015-12-16 00:42 UTC #3 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

Quoted 18 lines · Stephen Warren

Quoted 17 lines · Marek Vasut
Quoted 15 lines · Stephen Warren
From: Stephen Warren <[email protected]>
When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.
Cc Hans de Goede <[email protected]>
Signed-off-by Stephen Warren <[email protected]>
---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.

Quoted 6 lines

Quoted 4 lines
I initially implemented the control EP polling because I had a keyboard which had issues with interrupt transfers.
Reviewed-by Marek Vasut <[email protected]>
Did you intend someone else to apply this?

Is the discussion concluded already? I was under the impression that there was no general agreement.

Otherwise I can pick this of course.

Best regards,
Marek Vasut

Stephen Warren

author

2015-12-16 02:45 UTC #4 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

Quoted 22 lines · Marek Vasut

Quoted 21 lines · Stephen Warren
Quoted 20 lines · Marek Vasut
Quoted 9 lines · Stephen Warren
From: Stephen Warren <[email protected]>
When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.
Quoted 6 lines
Cc Hans de Goede <[email protected]>
Signed-off-by Stephen Warren <[email protected]>
---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.
I initially implemented the control EP polling because I had a keyboard which had issues with interrupt transfers.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Reviewed-by Marek Vasut <[email protected]>
Did you intend someone else to apply this?
Is the discussion concluded already? I was under the impression that there was no general agreement.
Otherwise I can pick this of course.

The last comments in the thread were:

Quoted 6 lines · Hans de Goede

Quoted 2 lines · Stephen Warren
However, I think that fixing the existing "use control transfers" support so that it exclusively uses control transfers is still reasonable?
Ack, as long as we have it, we should fix it. I do believe we should get rid of it in the long run though.

Marek Vasut

2015-12-16 14:13 UTC #5 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

Quoted 27 lines · Stephen Warren

Quoted 26 lines · Marek Vasut
Quoted 24 lines · Stephen Warren
Quoted 17 lines · Marek Vasut
Quoted 9 lines · Stephen Warren
From: Stephen Warren <[email protected]>
When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.
Quoted 6 lines
Cc Hans de Goede <[email protected]>
Signed-off-by Stephen Warren <[email protected]>
---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.
Quoted 4 lines
I initially implemented the control EP polling because I had a keyboard which had issues with interrupt transfers.
Reviewed-by Marek Vasut <[email protected]>
Did you intend someone else to apply this?

Quoted 15 lines

Quoted 4 lines
Is the discussion concluded already? I was under the impression that there was no general agreement.
Otherwise I can pick this of course.
The last comments in the thread were:
Quoted 7 lines · Hans de Goede
Quoted 3 lines · Stephen Warren
However, I think that fixing the existing "use control transfers" support so that it exclusively uses control transfers is still reasonable?
Ack, as long as we have it, we should fix it. I do believe we should get rid of it in the long run though.

Oki, in that case, it makes sense to apply this in the short term. Thanks for the reminder, applied!

Best regards,
Marek Vasut

Hans de Goede

2015-11-15 19:40 UTC #6 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

In reply to Stephen Warren

Hi,

Quoted 9 lines · Stephen Warren

From: Stephen Warren <[email protected]>
When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.

Quoted 6 lines

Cc Hans de Goede <[email protected]>
Signed-off-by Stephen Warren <[email protected]>
---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.

I know of at least 2 issues with using control transfers, and IMHO we should try to just remove support for control transfers from the usb kbd drivers.

The 2 known issues are:

1) I've some cheap wireless keyboards where the usb-dongle does not support the ctrl method of polling and things simply do not work when using it.

2) My USB+DVI kvm switch will report keys as pressed to all connected machines, even those without focus when using the ctrl method (this makes sense since other control methods must work without focus to keep the os on the machine happy).

Regards,

Hans

Quoted 19 lines

---
  common/usb_kbd.c | 3 +++
  1 file changed, 3 insertions(+)

common/usb_kbd.c +3 -0

index 95912f99c767..81c4d62c632b 100644
@@ -475,6 +475,9 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
  				      USB_KBD_BOOT_REPORT_SIZE, data->new,
  				      data->intinterval);
  	if (!data->intq) {
+#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
+	if (usb_get_report(dev, iface->desc.bInterfaceNumber,
+			   1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) {
  #else
  	if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
  			       data->intinterval) < 0) {

Stephen Warren

author

2015-11-16 17:16 UTC #7 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

Quoted 25 lines · Hans de Goede

Hi,
Quoted 9 lines · Stephen Warren
From: Stephen Warren <[email protected]>
When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.
Quoted 6 lines
Cc Hans de Goede <[email protected]>
Signed-off-by Stephen Warren <[email protected]>
---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.
I know of at least 2 issues with using control transfers, and IMHO we should try to just remove support for control transfers from the usb kbd drivers.
The 2 known issues are:

Quoted 8 lines

1) I've some cheap wireless keyboards where the usb-dongle does not support the ctrl method of polling and things simply do not work when using it.
2) My USB+DVI kvm switch will report keys as pressed to all connected machines, even those without focus when using the ctrl method (this makes sense since other control methods must work without focus to keep the os on the machine happy).

Interesting; Marek observed the opposite i.e. some keyboards that only worked with control transfers.

However, I think that fixing the existing "use control transfers" support so that it exclusively uses control transfers is still reasonable? Perhaps the issues with control transfers affect my decision not to implement interrupt transfers at all in my USB driver, but probably not this patch.

Perhaps a useful future extension would be to register two keyboard drivers; one explicitly using interrupt transfers and one using control transfers, so that the user can set stdin depending on which mode of operation they want.

Hans de Goede

2015-11-16 17:20 UTC #8 lore raw

PATCH 1/1 usb: kbd: don't use int xfers when polling via ctrl xfers

Hi,

Quoted 30 lines · Stephen Warren

Quoted 29 lines · Hans de Goede
Hi,
Quoted 9 lines · Stephen Warren
From: Stephen Warren <[email protected]>
When CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled, use a GET_REPORT control transfer to retrieve the initial state of the keyboard. This matches the technique used to poll the keyboard state. This is useful since it eliminates the remaining use of interrupt transfers from the USB keyboard driver, which allows it to work with USB HCD that don't support interrupt transfers.
Quoted 6 lines
Cc Hans de Goede <[email protected]>
Signed-off-by Stephen Warren <[email protected]>
---
Are there any disadvantages to using control transfers over interrupt transfers? I'm not aware of any, but I assume there must be a reason that U-Boot typically uses interrupt transfers.
I know of at least 2 issues with using control transfers, and IMHO we should try to just remove support for control transfers from the usb kbd drivers.
The 2 known issues are:
1) I've some cheap wireless keyboards where the usb-dongle does not support the ctrl method of polling and things simply do not work when using it.

Quoted 6 lines

Quoted 4 lines
2) My USB+DVI kvm switch will report keys as pressed to all connected machines, even those without focus when using the ctrl method (this makes sense since other control methods must work without focus to keep the os on the machine happy).
Interesting; Marek observed the opposite i.e. some keyboards that only worked with control transfers.

Right, that is likely due to a broken host-driver implementation for irq endpoints in u-boot, rather then due to broken hardware though. All significant operating systems by default use interrupt endpoints for keyboards, so a keyboard where those are broken will not sell well.

Quoted 1 line

However, I think that fixing the existing "use control transfers" support so that it exclusively uses control transfers is still reasonable?

Ack, as long as we have it, we should fix it. I do believe we should get rid of it in the long run though.

Quoted 3 lines

Perhaps the issues with control transfers affect my decision not to implement interrupt transfers at all in my USB driver, but probably not this patch.
Perhaps a useful future extension would be to register two keyboard drivers; one explicitly using interrupt transfers and one using control transfers, so that the user can set stdin depending on which mode of operation they want.

I do not think that that is necessary. As said I believe that any problems with interrupt transfers are host-driver issues, not hardware issues so they can be fixed. Where as the issues with ctrl transfers are hardware issues which cannot be worked-around.

Regards,

Hans