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

[Matt DeVillier]

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;
-
     // 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++) {
+            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 (ret)
         goto fail;

[Kevin O'Connor]

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?
> // 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().
>        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.)

Thanks.
-Kevin

[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

[Kevin O'Connor]

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

[Kevin O'Connor]

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)

if (!num_iface-- || (void*)iface + iface->bLength > config_end)

-Kevin

[Kevin O'Connor]

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

[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;

[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;

[Matt DeVillier]

On Mon, Sep 7, 2020 at 4:36 PM Matt DeVillier  wrote:
> 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

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;