[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
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; ibNumInterfaces; 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, , 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*)([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
> >
[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
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; ibNumInterfaces; 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, , 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*)([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
[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
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; ibNumInterfaces; 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, , 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*)([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-le...@seabios.org
[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
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; ibNumInterfaces; 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, , 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*)([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-le...@seabios.org
[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
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; ibNumInterfaces; 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, , 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*)([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-le...@seabios.org
[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
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; ibNumInterfaces; 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, , 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*)([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-le...@seabios.org
[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
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; ibNumInterfaces; 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; ibNumInterfaces; 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-le...@seabios.org
[SeaBIOS] Re: [SEABIOS] [PATCH] usb-hid: Fix keyboards using non-primary interface descriptor
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; ibNumInterfaces; 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-le...@seabios.org
