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

[Matt DeVillier]

On Thu, Sep 3, 2020 at 7:28 PM Kevin O'Connor  wrote:
> 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?

if we're already checking the subclass before the call to
usb_hid_setup(), then it's competely unnecessary as it will always
pass. If I drop the subclass check as per revised loop below, then
would still be needed and could drop this hunk
> > // 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().

will do.
> >        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.)

how about something like:

int i;
ret = -1;
for (i=0; i<config->bNumInterfaces; i++) {
    if (iface->bInterfaceClass == USB_CLASS_HUB)
        ret = usb_hub_setup(usbdev);
    else if (iface->bInterfaceClass == USB_CLASS_MASS_STORAGE) {
        if (iface->bInterfaceProtocol == US_PR_BULK)
            ret = usb_msc_setup(usbdev);
        else if (iface->bInterfaceProtocol == US_PR_UAS)
            ret = usb_uas_setup(usbdev);
    } else
        ret = usb_hid_setup(usbdev);

if (ret == 0)
        break;

iface = (void*)iface + iface->bLength;
    usb_set_interface(iface);  //need to create this
    usbdev->iface = iface;
}
> Thanks.
> -Kevin