[PATCH] usb-host: workaround libusb bug

[PATCH] usb-host: workaround libusb bug

[Gerd Hoffmann]

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);
+        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) {
-- 
2.27.0

Re: [PATCH] usb-host: workaround libusb bug

[Alex Bennée]

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....

-- 
Alex Bennée

Re: [PATCH] usb-host: workaround libusb bug

[Gerd Hoffmann]

> > +#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

Re: [PATCH] usb-host: workaround libusb bug

[Alex Bennée]

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>

-- 
Alex Bennée

Re: [PATCH] usb-host: workaround libusb bug

[Gerd Hoffmann]

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

Re: [PATCH] usb-host: workaround libusb bug

[BALATON Zoltan]

On Fri, 29 May 2020, Gerd Hoffmann wrote:
> libusb seems to no allways call the completion callback for requests

Typo: not always call.

Regards,
BALATON Zoltan

> canceled (which it is supposed to do according to the docs).  So add
> a limit to avoid qemu waiting forever.
>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/usb/host-libusb.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index e28441379d99..094010d5f849 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -951,6 +951,7 @@ fail:
> static void usb_host_abort_xfers(USBHostDevice *s)
> {
>     USBHostRequest *r, *rtmp;
> +    int limit = 100;
>
>     QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
>         usb_host_req_abort(r);
> @@ -961,6 +962,19 @@ static void usb_host_abort_xfers(USBHostDevice *s)
>         memset(&tv, 0, sizeof(tv));
>         tv.tv_usec = 2500;
>         libusb_handle_events_timeout(ctx, &tv);
> +        if (--limit == 0) {
> +            /*
> +             * Don't wait forever for libusb calling the complete
> +             * callback (which will unlink and free the request).
> +             *
> +             * Leaking memory here, to make sure libusb will not
> +             * access memory which we have released already.
> +             */
> +            QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
> +                QTAILQ_REMOVE(&s->requests, r, next);
> +            }
> +            return;
> +        }
>     }
> }
>
>

[PATCH] usb-host: workaround libusb bug

[Gerd Hoffmann]

libusb seems to no allways call the completion callback for requests
canceled (which it is supposed to do according to the docs).  So add
a limit to avoid qemu waiting forever.

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/host-libusb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index e28441379d99..094010d5f849 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -951,6 +951,7 @@ fail:
 static void usb_host_abort_xfers(USBHostDevice *s)
 {
     USBHostRequest *r, *rtmp;
+    int limit = 100;
 
     QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
         usb_host_req_abort(r);
@@ -961,6 +962,19 @@ static void usb_host_abort_xfers(USBHostDevice *s)
         memset(&tv, 0, sizeof(tv));
         tv.tv_usec = 2500;
         libusb_handle_events_timeout(ctx, &tv);
+        if (--limit == 0) {
+            /*
+             * Don't wait forever for libusb calling the complete
+             * callback (which will unlink and free the request).
+             *
+             * Leaking memory here, to make sure libusb will not
+             * access memory which we have released already.
+             */
+            QTAILQ_FOREACH_SAFE(r, &s->requests, next, rtmp) {
+                QTAILQ_REMOVE(&s->requests, r, next);
+            }
+            return;
+        }
     }
 }
 
-- 
2.18.4