On Thu, Sep 3, 2020 at 7:28 PM Kevin O'Connor <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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
_______________________________________________
SeaBIOS mailing list -- [email protected]
To unsubscribe send an email to [email protected]
