diff mbox

[U-Boot] usb: kbd: don't use int xfers when polling via ctrl xfers

Message ID 1447446849-10438-1-git-send-email-swarren@wwwdotorg.org
State Accepted
Delegated to: Marek Vasut
Headers show

Commit Message

Stephen Warren Nov. 13, 2015, 8:34 p.m. UTC 
From: Stephen Warren <swarren@nvidia.com>

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 <hdegoede@redhat.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
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(+)

Comments

Marek Vasut Nov. 14, 2015, 1:16 a.m. UTC | #1 
On Friday, November 13, 2015 at 09:34:09 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> 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 <hdegoede@redhat.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> 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 <marex@denx.de>

> ---
>  common/usb_kbd.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 95912f99c767..81c4d62c632b 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -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
Hans de Goede Nov. 15, 2015, 7:40 p.m. UTC | #2 
Hi,

On 11/13/2015 09:34 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> 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 <hdegoede@redhat.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> 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


> ---
>   common/usb_kbd.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 95912f99c767..81c4d62c632b 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -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 Nov. 16, 2015, 5:16 p.m. UTC | #3 
On 11/15/2015 12:40 PM, Hans de Goede wrote:
> Hi,
>
> On 11/13/2015 09:34 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> 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 <hdegoede@redhat.com>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> 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).

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 Nov. 16, 2015, 5:20 p.m. UTC | #4 
Hi,

On 16-11-15 18:16, Stephen Warren wrote:
> On 11/15/2015 12:40 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 11/13/2015 09:34 PM, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> 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 <hdegoede@redhat.com>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>> ---
>>> 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).
>
> 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.

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

> 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
Stephen Warren Dec. 15, 2015, 11:35 p.m. UTC | #5 
On 11/13/2015 06:16 PM, Marek Vasut wrote:
> On Friday, November 13, 2015 at 09:34:09 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> 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 <hdegoede@redhat.com>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> 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 <marex@denx.de>

Did you intend someone else to apply this?
Marek Vasut Dec. 16, 2015, 12:42 a.m. UTC | #6 
On Wednesday, December 16, 2015 at 12:35:23 AM, Stephen Warren wrote:
> On 11/13/2015 06:16 PM, Marek Vasut wrote:
> > On Friday, November 13, 2015 at 09:34:09 PM, Stephen Warren wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >> 
> >> 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 <hdegoede@redhat.com>
> >> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> >> ---
> >> 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 <marex@denx.de>
> 
> 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 Dec. 16, 2015, 2:45 a.m. UTC | #7 
On 12/15/2015 05:42 PM, Marek Vasut wrote:
> On Wednesday, December 16, 2015 at 12:35:23 AM, Stephen Warren wrote:
>> On 11/13/2015 06:16 PM, Marek Vasut wrote:
>>> On Friday, November 13, 2015 at 09:34:09 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> 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 <hdegoede@redhat.com>
>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>>> ---
>>>> 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 <marex@denx.de>
>>
>> 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:

Hans de Goede wrote:
> Stephen Warren wrote:
>> 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 Dec. 16, 2015, 2:13 p.m. UTC | #8 
On Wednesday, December 16, 2015 at 03:45:48 AM, Stephen Warren wrote:
> On 12/15/2015 05:42 PM, Marek Vasut wrote:
> > On Wednesday, December 16, 2015 at 12:35:23 AM, Stephen Warren wrote:
> >> On 11/13/2015 06:16 PM, Marek Vasut wrote:
> >>> On Friday, November 13, 2015 at 09:34:09 PM, Stephen Warren wrote:
> >>>> From: Stephen Warren <swarren@nvidia.com>
> >>>> 
> >>>> 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 <hdegoede@redhat.com>
> >>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> >>>> ---
> >>>> 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 <marex@denx.de>
> >> 
> >> 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:
> 
> Hans de Goede wrote:
> > Stephen Warren wrote:
> >> 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
diff mbox

Patch

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 95912f99c767..81c4d62c632b 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -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) {