On Mon, Sep 7, 2020 at 4:36 PM Matt DeVillier <[email protected]> wrote: > > On Mon, Sep 7, 2020 at 4:10 PM Matt DeVillier <[email protected]> > wrote: > > > > On Thu, Sep 3, 2020 at 10:53 PM Kevin O'Connor <[email protected]> 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 <[email protected]> > > > > > 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
scratch that, I had a logitech kb/mouse dongle also plugged in and that was getting prioritized. I'll resubmit as patch v2 > > > > > > + > > > > // Configure driver. > > > > usbdev->config = config; > > > > usbdev->iface = iface; _______________________________________________ SeaBIOS mailing list -- [email protected] To unsubscribe send an email to [email protected]
