src/hw/usb-hid.c | 9 ++------- src/hw/usb.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 9 deletions(-)
From 9e408a5441330b120a477324c017c0525cb5b365 Mon Sep 17 00:00:00 2001
From: Matt DeVillier <matt.devillier@puri.sm>
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 <matt.devillier@puri.sm>
---
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;
--
2.20.1
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
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 <matt.devillier@puri.sm> > 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 <matt.devillier@puri.sm> > --- > 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 _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
On Thu, Sep 3, 2020 at 7:28 PM Kevin O'Connor <kevin@koconnor.net> 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 <matt.devillier@puri.sm> > > 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 <matt.devillier@puri.sm> > > --- > > 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 -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
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 <kevin@koconnor.net> 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; _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
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 <kevin@koconnor.net> 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; _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
On Thu, Sep 3, 2020 at 10:53 PM Kevin O'Connor <kevin@koconnor.net> 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 <kevin@koconnor.net> 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; _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
On Mon, Sep 7, 2020 at 4:10 PM Matt DeVillier <matt.devillier@gmail.com> wrote: > > On Thu, Sep 3, 2020 at 10:53 PM Kevin O'Connor <kevin@koconnor.net> 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 <kevin@koconnor.net> 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; _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
On Mon, Sep 7, 2020 at 4:36 PM Matt DeVillier <matt.devillier@gmail.com> wrote: > > On Mon, Sep 7, 2020 at 4:10 PM Matt DeVillier <matt.devillier@gmail.com> wrote: > > > > On Thu, Sep 3, 2020 at 10:53 PM Kevin O'Connor <kevin@koconnor.net> 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 <kevin@koconnor.net> 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 -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
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 <kevin@koconnor.net> 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 _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
© 2016 - 2024 Red Hat, Inc.