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

[Matt DeVillier]

On Mon, Sep 7, 2020 at 4:10 PM Matt DeVillier  wrote:
> On Thu, Sep 3, 2020 at 10:53 PM Kevin O'Connor  wrote:
> > On Thu, Sep 03, 2020 at 10:32:24PM -0400, Kevin O'Connor wrote:
> > > On Thu, Sep 03, 2020 at 09:03:50PM -0500, Matt DeVillier wrote:
> > > > On Thu, Sep 3, 2020 at 7:28 PM Kevin O'Connor  wrote:
> > > > > 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;
> > > >
> > > > }
> > >
> > > Wouldn't it be better to run the check before calling set_configure()?
> > >
> > > Perhaps something like the below (totally untested).
> > >
> > > -Kevin
> > >
> > > diff --git a/src/hw/usb.c b/src/hw/usb.c
> > > index 4f9a852..732d4cd 100644
> > > --- a/src/hw/usb.c
> > > +++ b/src/hw/usb.c
> > > @@ -248,14 +248,14 @@ get_device_config(struct usb_pipe *pipe)
> > >      if (ret)
> > >          return NULL;
> > > void *config = malloc_tmphigh(cfg.wTotalLength);
> > > struct usb_config_descriptor *config = malloc_tmphigh(cfg.wTotalLength);
> > > if (!config) {
> > >     warn_noalloc();
> > >     return NULL;
> > > }
> > > req.wLength = cfg.wTotalLength;
> > > ret = usb_send_default_control(pipe, &req, config);
> > > if (ret) {
> > > if (ret || config->wTotalLength != cfg.wTotalLength) {
> > >     free(config);
> > >     return NULL;
> > > }
> > >
> > > @@ -367,19 +367,33 @@ configure_usb_device(struct usbdevice_s *usbdev)
> > >          return 0;
> > >  // Determine if a driver exists for this device - only look at the
> > > // first interface of the first configuration.
> > > // interfaces of the first configuration.
> > > int num_iface = config->bNumInterfaces;
> > > void *config_end = (void*)config + config->wTotalLength;
> > > struct usb_interface_descriptor *iface = (void*)(&config[1]);
> > > if (iface->bInterfaceClass != USB_CLASS_HID
> > >    && iface->bInterfaceClass != USB_CLASS_MASS_STORAGE
> > >    && iface->bInterfaceClass != USB_CLASS_HUB)
> > >    // Not a supported device.
> > >    goto fail;
> > > for (;;) {
> > >    if (!num_iface-- || (void*)iface + iface->bLength >= config_end)
> > >        // Not a supported device.
> > >        goto fail;
> > >    if (iface->bInterfaceClass == USB_CLASS_HUB
> >
> > Looking a little closer at this, it's also necessary to verify that
> > the descriptor is an interface - so something like:
> >
> > if (iface->bDescriptorType == USB_DT_INTERFACE
> >     && ...)
> >     break;
> >
> > -Kevin
> > >        || (iface->bInterfaceClass == USB_CLASS_MASS_STORAGE
> > >            && (iface->bInterfaceProtocol == US_PR_BULK
> > >                || iface->bInterfaceProtocol == US_PR_UAS))
> > >        || (iface->bInterfaceClass == USB_CLASS_HID
> > >            && iface->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT))
> > >        break;
> > >    iface = (void*)iface + iface->bLength;
> > >
> > > }
> > >
> > > // Set the configuration.
> > > ret = set_configuration(usbdev->defpipe, config->bConfigurationValue);
> > > if (ret)
> > >     goto fail;
> > >
> > > if (iface->bAlternateSetting)
> > >    // XXX
> > >    set_interface(iface);
>
> From reading up on interfaces and Alternate Modes, I'm unclear on this
> part. This would seem to indicate we want to use the alternative mode
> of the selected interface, not that we want to use a non-primary
> interface. I'm not seeing anything that requires use of the
> SET_INTERFACE command to use a non-primary interface unless that
> interface is the alternate of the primary (ie,
> iface[0]->bAlternateSetting would need to equal
> iface[x]->bInterfaceNumber) .
>
> Or am I completely misunderstanding?
>
> -Matt

also, unlike my original patch, the above patch (w/o set interface)
did not work with an UHK 60 keyboard
> > > // Configure driver.
> > >    usbdev->config = config;
> > >    usbdev->iface = iface;