diff mbox series

usb-host: workaround libusb bug

Message ID 20200824110057.32089-1-kraxel@redhat.com
State New
Headers show
Series usb-host: workaround libusb bug | expand

Commit Message

Gerd Hoffmann Aug. 24, 2020, 11 a.m. UTC
libusb_get_device_speed() does not work for
libusb_wrap_sys_device() devices in v1.0.23.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1871090
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/host-libusb.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

Comments

Alex Bennée Sept. 1, 2020, 2:05 p.m. UTC | #1
Gerd Hoffmann <kraxel@redhat.com> writes:

> libusb_get_device_speed() does not work for
> libusb_wrap_sys_device() devices in v1.0.23.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1871090
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/host-libusb.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index c474551d8456..77f1eaa5fe9e 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -39,6 +39,11 @@
>  #endif
>  #include <libusb.h>
>  
> +#ifdef CONFIG_LINUX
> +#include <sys/ioctl.h>
> +#include <linux/usbdevice_fs.h>
> +#endif
> +
>  #include "qapi/error.h"
>  #include "migration/vmstate.h"
>  #include "monitor/monitor.h"
> @@ -885,6 +890,7 @@ static void usb_host_ep_update(USBHostDevice *s)
>  static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>  {
>      USBDevice *udev = USB_DEVICE(s);
> +    int libusb_speed;
>      int bus_num = 0;
>      int addr = 0;
>      int rc;
> @@ -935,7 +941,36 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>      usb_ep_init(udev);
>      usb_host_ep_update(s);
>  
> -    udev->speed     = speed_map[libusb_get_device_speed(dev)];
> +    libusb_speed = libusb_get_device_speed(dev);
> +#ifdef CONFIG_LINUX
> +    if (hostfd && libusb_speed == 0) {
> +        /*
> +         * Workaround libusb bug: libusb_get_device_speed() does not
> +         * work for libusb_wrap_sys_device() devices in v1.0.23.
> +         *
> +         * Speeds are defined in linux/usb/ch9.h, file not included
> +         * due to name conflicts.
> +         */
> +        int rc = ioctl(hostfd, USBDEVFS_GET_SPEED, NULL);

This (further) breaks a bunch of the Travis jobs - I assume because libusb doesn't
always have this symbol:

  Compiling C object libcommon.fa.p/hw_input_vhost-user-input.c.o

  ../hw/usb/host-libusb.c: In function ‘usb_host_open’:

  ../hw/usb/host-libusb.c:954:32: error: ‘USBDEVFS_GET_SPEED’ undeclared (first use in this function)

  int rc = ioctl(hostfd, USBDEVFS_GET_SPEED, NULL);

  ^

  ../hw/usb/host-libusb.c:954:32: note: each undeclared identifier is reported only once for each function it appears in

  Makefile.ninja:961: recipe for target 'libcommon.fa.p/hw_usb_host-libusb.c.o' failed

  make: *** [libcommon.fa.p/hw_usb_host-libusb.c.o] Error 1

  make: *** Waiting for unfinished jobs....
Gerd Hoffmann Sept. 2, 2020, 8:09 a.m. UTC | #2
> > +#include <linux/usbdevice_fs.h>

> > +        int rc = ioctl(hostfd, USBDEVFS_GET_SPEED, NULL);
> 
> This (further) breaks a bunch of the Travis jobs - I assume because libusb doesn't
> always have this symbol:
> 
>   ../hw/usb/host-libusb.c:954:32: error: ‘USBDEVFS_GET_SPEED’ undeclared (first use in this function)

It isn't libusb, it is the kernel (linux/usbdevice_fs.h).

/me checks git ...
Added in 4.13, so available for quite a while.
I guess that is the prehistoric ubuntu version travis has?

Given that we only need that workaround with rather new libusb versions
(which have libusb_wrap_sys_device() support) which most likely isn't
present in that old ubuntu version we can probably just do this:

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 08604f787fdf..c25102f3aca1 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -942,7 +942,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
     usb_host_ep_update(s);
 
     libusb_speed = libusb_get_device_speed(dev);
-#ifdef CONFIG_LINUX
+#if LIBUSB_API_VERSION >= 0x01000107 && defined(CONFIG_LINUX)
     if (hostfd && libusb_speed == 0) {
         /*
          * Workaround libusb bug: libusb_get_device_speed() does not
Alex Bennée Sept. 2, 2020, 2:31 p.m. UTC | #3
Gerd Hoffmann <kraxel@redhat.com> writes:

>> > +#include <linux/usbdevice_fs.h>
>
>> > +        int rc = ioctl(hostfd, USBDEVFS_GET_SPEED, NULL);
>> 
>> This (further) breaks a bunch of the Travis jobs - I assume because libusb doesn't
>> always have this symbol:
>> 
>>   ../hw/usb/host-libusb.c:954:32: error: ‘USBDEVFS_GET_SPEED’ undeclared (first use in this function)
>
> It isn't libusb, it is the kernel (linux/usbdevice_fs.h).
>
> /me checks git ...
> Added in 4.13, so available for quite a while.
> I guess that is the prehistoric ubuntu version travis has?
>
> Given that we only need that workaround with rather new libusb versions
> (which have libusb_wrap_sys_device() support) which most likely isn't
> present in that old ubuntu version we can probably just do this:
>
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 08604f787fdf..c25102f3aca1 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -942,7 +942,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>      usb_host_ep_update(s);
>  
>      libusb_speed = libusb_get_device_speed(dev);
> -#ifdef CONFIG_LINUX
> +#if LIBUSB_API_VERSION >= 0x01000107 && defined(CONFIG_LINUX)
>      if (hostfd && libusb_speed == 0) {
>          /*
>           * Workaround libusb bug: libusb_get_device_speed() does not

That works. Do you want to include that in your next PR or send a patch
for me to include in my testing/next PR?

In the meantime:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Gerd Hoffmann Sept. 3, 2020, 5:33 a.m. UTC | #4
On Wed, Sep 02, 2020 at 03:31:46PM +0100, Alex Bennée wrote:
> 
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> >> > +#include <linux/usbdevice_fs.h>
> >
> >> > +        int rc = ioctl(hostfd, USBDEVFS_GET_SPEED, NULL);
> >> 
> >> This (further) breaks a bunch of the Travis jobs - I assume because libusb doesn't
> >> always have this symbol:
> >> 
> >>   ../hw/usb/host-libusb.c:954:32: error: ‘USBDEVFS_GET_SPEED’ undeclared (first use in this function)
> >
> > It isn't libusb, it is the kernel (linux/usbdevice_fs.h).
> >
> > /me checks git ...
> > Added in 4.13, so available for quite a while.
> > I guess that is the prehistoric ubuntu version travis has?
> >
> > Given that we only need that workaround with rather new libusb versions
> > (which have libusb_wrap_sys_device() support) which most likely isn't
> > present in that old ubuntu version we can probably just do this:
> >
> > diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> > index 08604f787fdf..c25102f3aca1 100644
> > --- a/hw/usb/host-libusb.c
> > +++ b/hw/usb/host-libusb.c
> > @@ -942,7 +942,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
> >      usb_host_ep_update(s);
> >  
> >      libusb_speed = libusb_get_device_speed(dev);
> > -#ifdef CONFIG_LINUX
> > +#if LIBUSB_API_VERSION >= 0x01000107 && defined(CONFIG_LINUX)
> >      if (hostfd && libusb_speed == 0) {
> >          /*
> >           * Workaround libusb bug: libusb_get_device_speed() does not
> 
> That works. Do you want to include that in your next PR or send a patch
> for me to include in my testing/next PR?

Patch is on the list already:
https://patchwork.ozlabs.org/project/qemu-devel/patch/20200902081445.3291-1-kraxel@redhat.com/

Feel free to include it in testing pull.

take care,
  Gerd
diff mbox series

Patch

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index c474551d8456..77f1eaa5fe9e 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -39,6 +39,11 @@ 
 #endif
 #include <libusb.h>
 
+#ifdef CONFIG_LINUX
+#include <sys/ioctl.h>
+#include <linux/usbdevice_fs.h>
+#endif
+
 #include "qapi/error.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
@@ -885,6 +890,7 @@  static void usb_host_ep_update(USBHostDevice *s)
 static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
 {
     USBDevice *udev = USB_DEVICE(s);
+    int libusb_speed;
     int bus_num = 0;
     int addr = 0;
     int rc;
@@ -935,7 +941,36 @@  static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
     usb_ep_init(udev);
     usb_host_ep_update(s);
 
-    udev->speed     = speed_map[libusb_get_device_speed(dev)];
+    libusb_speed = libusb_get_device_speed(dev);
+#ifdef CONFIG_LINUX
+    if (hostfd && libusb_speed == 0) {
+        /*
+         * Workaround libusb bug: libusb_get_device_speed() does not
+         * work for libusb_wrap_sys_device() devices in v1.0.23.
+         *
+         * Speeds are defined in linux/usb/ch9.h, file not included
+         * due to name conflicts.
+         */
+        int rc = ioctl(hostfd, USBDEVFS_GET_SPEED, NULL);
+        switch(rc) {
+        case 1: /* low */
+            libusb_speed = LIBUSB_SPEED_LOW;
+            break;
+        case 2: /* full */
+            libusb_speed = LIBUSB_SPEED_FULL;
+            break;
+        case 3: /* high */
+        case 4: /* wireless */
+            libusb_speed = LIBUSB_SPEED_HIGH;
+            break;
+        case 5: /* super */
+        case 6: /* super plus */
+            libusb_speed = LIBUSB_SPEED_SUPER;
+            break;
+        }
+    }
+#endif
+    udev->speed = speed_map[libusb_speed];
     usb_host_speed_compat(s);
 
     if (s->ddesc.iProduct) {