[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor

[Kevin O'Connor]

On Tue, Sep 01, 2020 at 01:31:18AM -0500, Matt DeVillier wrote:
> From 9e408a5441330b120a477324c017c0525cb5b365 Mon Sep 17 00:00:00 2001
> From: Matt DeVillier 
> Date: Tue, 1 Sep 2020 01:21:23 -0500
> Subject: [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
>
> A fair number of USB keyboards use an interface descriptor other than
> the first available, making them non-functional currently.
> To correct this, iterate through all available interface descriptors
> until one with the correct class/subclass is found, then call usb_hid_setup().
>
> Tested on an ultimate hacking keyboard (UHK 60)
> Signed-off-by: Matt DeVillier
>
> src/hw/usb-hid.c |  9 ++-------
>  src/hw/usb.c     | 17 +++++++++++++++--
>  2 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/src/hw/usb-hid.c b/src/hw/usb-hid.c
> index a22765b..c1ceaba 100644
> --- a/src/hw/usb-hid.c
> +++ b/src/hw/usb-hid.c
> @@ -138,11 +138,6 @@ usb_hid_setup(struct usbdevice_s *usbdev)
>          return -1;
>      dprintf(2, "usb_hid_setup %p\n", usbdev->defpipe);
> struct usb_interface_descriptor *iface = usbdev->iface;
> if (iface->bInterfaceSubClass != USB_INTERFACE_SUBCLASS_BOOT)
>    // Doesn't support boot protocol.
>    return -1;

Is this change needed?
> // Find intr in endpoint.
>    struct usb_endpoint_descriptor *epdesc = usb_find_desc(
>        usbdev, USB_ENDPOINT_XFER_INT, USB_DIR_IN);
>
> @@ -151,9 +146,9 @@ usb_hid_setup(struct usbdevice_s *usbdev)
>          return -1;
>      }
> if (iface->bInterfaceProtocol == USB_INTERFACE_PROTOCOL_KEYBOARD)
> if (usbdev->iface->bInterfaceProtocol == USB_INTERFACE_PROTOCOL_KEYBOARD)
>     return usb_kbd_setup(usbdev, epdesc);
> if (iface->bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
> if (usbdev->iface->bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
>     return usb_mouse_setup(usbdev, epdesc);
> return -1;
>
> }
> diff --git a/src/hw/usb.c b/src/hw/usb.c
> index 4f9a852..aea70a5 100644
> --- a/src/hw/usb.c
> +++ b/src/hw/usb.c
> @@ -391,8 +391,21 @@ configure_usb_device(struct usbdevice_s *usbdev)
>              ret = usb_msc_setup(usbdev);
>          if (iface->bInterfaceProtocol == US_PR_UAS)
>              ret = usb_uas_setup(usbdev);
> } else
>    ret = usb_hid_setup(usbdev);
> } else {
>    // Some keyboards use an interface other than the first one
>    // so iterate through all available
>    for (int i=0; i<config->bNumInterfaces; i++) {

FYI, some older versions of gcc will complain with the above.  Declare
the "int i;" outside the for().
>        if (iface->bInterfaceClass == USB_CLASS_HID &&
>                    iface->bInterfaceSubClass ==
>
> USB_INTERFACE_SUBCLASS_BOOT) {
>            usbdev->iface = iface;
>            ret = usb_hid_setup(usbdev);
>            break;
>        } else {
>            iface = (void*)iface + iface->bLength;
>            ret = -1;
>        }
>    }
> }

If we're going to support multiple interfaces, I think it would be
preferable to expand the loop so that it also works for MASS_STORAGE
and HUB devices.

Also, if an alternate interface is used, I think the usb SET_INTERFACE
command needs to be sent.  (That particular keyboard may work without
SET_INTERFACE, but it may not work on another.)

Thanks.
-Kevin