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

[Matt DeVillier]

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
> > // Configure driver.
> >    usbdev->config = config;
> >    usbdev->iface = iface;