[Qemu-devel] [PATCH 00/14] usb: various usb fixes

[Qemu-devel] [PATCH 00/14] usb: various usb fixes

[Hans de Goede]

Here is a patch series with a bunch of usb fixes, mostly (linux) usb
redirection related.

Regards,

Hans

[Qemu-devel] [PATCH 01/14] usb-linux: Set usb_auto_timer to NULL after deleting it

[Hans de Goede]

We might check for it being NULL later, if the device gets unplugged.
---
 usb-linux.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 4d7a31a..ea3ab5f 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1675,6 +1675,7 @@ static void usb_host_auto_check(void *unused)
         /* nothing to watch */
         if (usb_auto_timer) {
             qemu_del_timer(usb_auto_timer);
+            usb_auto_timer = NULL;
         }
         return;
     }
-- 
1.7.5.1

[Qemu-devel] [PATCH 02/14] usb-linux: Get speed from sysfs rather then from the connectinfo ioctl

[Hans de Goede]

The connectinfo ioctl only differentiates between lo speed devices, and
all other speeds, where as we would like to know the real speed. The real
speed is available in sysfs so use that when available.
---
 usb-linux.c |   37 +++++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index ea3ab5f..db28762 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1053,10 +1053,9 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
 }
 
 static int usb_host_open(USBHostDevice *dev, int bus_num,
-                         int addr, char *port, const char *prod_name)
+                        int addr, char *port, const char *prod_name, int speed)
 {
     int fd = -1, ret;
-    struct usbdevfs_connectinfo ci;
     char buf[1024];
 
     if (dev->fd != -1) {
@@ -1111,24 +1110,29 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
         goto fail;
     }
 
-    ret = ioctl(fd, USBDEVFS_CONNECTINFO, &ci);
-    if (ret < 0) {
-        perror("usb_host_device_open: USBDEVFS_CONNECTINFO");
-        goto fail;
-    }
-
-    printf("husb: grabbed usb device %d.%d\n", bus_num, addr);
-
     ret = usb_linux_update_endp_table(dev);
     if (ret) {
         goto fail;
     }
 
-    if (ci.slow) {
-        dev->dev.speed = USB_SPEED_LOW;
-    } else {
-        dev->dev.speed = USB_SPEED_HIGH;
+    if (speed == -1) {
+        struct usbdevfs_connectinfo ci;
+
+        ret = ioctl(fd, USBDEVFS_CONNECTINFO, &ci);
+        if (ret < 0) {
+            perror("usb_host_device_open: USBDEVFS_CONNECTINFO");
+            goto fail;
+        }
+
+        if (ci.slow) {
+            speed = USB_SPEED_LOW;
+        } else {
+            speed = USB_SPEED_HIGH;
+        }
     }
+    dev->dev.speed = speed;
+
+    printf("husb: grabbed usb device %d.%d\n", bus_num, addr);
 
     if (!prod_name || prod_name[0] == '\0') {
         snprintf(dev->dev.product_desc, sizeof(dev->dev.product_desc),
@@ -1342,7 +1346,8 @@ static int usb_host_scan_dev(void *opaque, USBScanFunc *func)
     }
 
     device_count = 0;
-    bus_num = addr = speed = class_id = product_id = vendor_id = 0;
+    bus_num = addr = class_id = product_id = vendor_id = 0;
+    speed = -1; /* Can't get the speed from /[proc|dev]/bus/usb/devices */
     for(;;) {
         if (fgets(line, sizeof(line), f) == NULL) {
             break;
@@ -1652,7 +1657,7 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
         }
         DPRINTF("husb: auto open: bus_num %d addr %d\n", bus_num, addr);
 
-        usb_host_open(s, bus_num, addr, port, product_name);
+        usb_host_open(s, bus_num, addr, port, product_name, speed);
     }
 
     return 0;
-- 
1.7.5.1

[Qemu-devel] [PATCH 03/14] usb-linux: Teach about super speed

[Hans de Goede]

---
 usb-linux.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index db28762..672a589 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1375,7 +1375,9 @@ static int usb_host_scan_dev(void *opaque, USBScanFunc *func)
             if (get_tag_value(buf, sizeof(buf), line, "Spd=", " ") < 0) {
                 goto fail;
             }
-            if (!strcmp(buf, "480")) {
+            if (!strcmp(buf, "5000")) {
+                speed = USB_SPEED_SUPER;
+            } else if (!strcmp(buf, "480")) {
                 speed = USB_SPEED_HIGH;
             } else if (!strcmp(buf, "1.5")) {
                 speed = USB_SPEED_LOW;
@@ -1519,7 +1521,9 @@ static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
             if (!usb_host_read_file(line, sizeof(line), "speed", de->d_name)) {
                 goto the_end;
             }
-            if (!strcmp(line, "480\n")) {
+            if (!strcmp(line, "5000\n")) {
+                speed = USB_SPEED_SUPER;
+            } else if (!strcmp(line, "480\n")) {
                 speed = USB_SPEED_HIGH;
             } else if (!strcmp(line, "1.5\n")) {
                 speed = USB_SPEED_LOW;
@@ -1797,6 +1801,9 @@ static void usb_info_device(Monitor *mon, int bus_num, int addr, char *port,
     case USB_SPEED_HIGH:
         speed_str = "480";
         break;
+    case USB_SPEED_SUPER:
+        speed_str = "5000";
+        break;
     default:
         speed_str = "?";
         break;
-- 
1.7.5.1

[Qemu-devel] [PATCH 04/14] usb-linux: Don't do perror when errno is not set

[Hans de Goede]

Note that "op" also is not set, so before this change these error paths
would feed NULL to perror.
---
 usb-linux.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 672a589..e6c6138 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -373,7 +373,8 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
     i = 0;
     dev_descr_len = dev->descr[0];
     if (dev_descr_len > dev->descr_len) {
-        goto fail;
+        fprintf(stderr, "husb: update iface failed. descr too short\n");
+        return 0;
     }
 
     i += dev_descr_len;
@@ -401,7 +402,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
     if (i >= dev->descr_len) {
         fprintf(stderr,
                 "husb: update iface failed. no matching configuration\n");
-        goto fail;
+        return 0;
     }
     nb_interfaces = dev->descr[i + 4];
 
-- 
1.7.5.1

[Qemu-devel] [PATCH 05/14] usb-linux: Don't call usb_host_close when usb_host_open fails

[Hans de Goede]

---
 usb-linux.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index e6c6138..544aea3 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -363,13 +363,16 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
     const char *op = NULL;
     int dev_descr_len, config_descr_len;
     int interface, nb_interfaces;
-    int ret, i;
+    int ret, i, close_on_nodev;
 
     if (configuration == 0) /* address state - ignore */
         return 1;
 
     DPRINTF("husb: claiming interfaces. config %d\n", configuration);
 
+    /* Don't close on nodev when called from usb_host_open (which passes -1) */
+    close_on_nodev = (configuration == -1) ? 0 : 1;
+
     i = 0;
     dev_descr_len = dev->descr[0];
     if (dev_descr_len > dev->descr_len) {
@@ -445,7 +448,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
     return 1;
 
 fail:
-    if (errno == ENODEV) {
+    if (errno == ENODEV && close_on_nodev) {
         do_disconnect(dev);
     }
     perror(op);
-- 
1.7.5.1

[Qemu-devel] [PATCH 06/14] usb-linux: Ensure devep != 0

[Hans de Goede]

So that we don't index endp_table with a negative index.
---
 usb-linux.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 544aea3..e2f45d3 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1029,6 +1029,11 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
             }
 
             devep = descriptors[i + 2];
+            if ((devep & 0x0f) == 0) {
+                fprintf(stderr, "usb-linux: invalid ep descriptor, ep == 0\n");
+                return 1;
+            }
+
             switch (descriptors[i + 3] & 0x3) {
             case 0x00:
                 type = USBDEVFS_URB_TYPE_CONTROL;
-- 
1.7.5.1

[Qemu-devel] [PATCH 07/14] usb-linux: If opening a device fails remove it from our filter list

[Hans de Goede]

So that we don't retry to open it every 2 seconds flooding stderr with
error messages.
---
 usb-linux.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index e2f45d3..334012e 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -122,6 +122,7 @@ typedef struct USBHostDevice {
     int       configuration;
     int       ninterfaces;
     int       closing;
+    int       open_failed;
     Notifier  exit;
 
     struct endp_data endp_table[MAX_ENDPOINTS];
@@ -1208,6 +1209,10 @@ static int usb_host_initfn(USBDevice *dev)
     s->exit.notify = usb_host_exit_notifier;
     qemu_add_exit_notifier(&s->exit);
     usb_host_auto_check(NULL);
+    if (s->open_failed) {
+        usb_host_handle_destroy(dev);
+        return s->open_failed;
+    }
     return 0;
 }
 
@@ -1272,7 +1277,11 @@ USBDevice *usb_host_device_open(const char *devname)
     qdev_prop_set_uint32(&dev->qdev, "hostaddr",  filter.addr);
     qdev_prop_set_uint32(&dev->qdev, "vendorid",  filter.vendor_id);
     qdev_prop_set_uint32(&dev->qdev, "productid", filter.product_id);
-    qdev_init_nofail(&dev->qdev);
+
+    if (qdev_init(&dev->qdev) != 0) {
+        return NULL;
+    }
+
     return dev;
 
 fail:
@@ -1637,6 +1646,7 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
 {
     struct USBAutoFilter *f;
     struct USBHostDevice *s;
+    int ret;
 
     /* Ignore hubs */
     if (class_id == 9)
@@ -1670,7 +1680,15 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
         }
         DPRINTF("husb: auto open: bus_num %d addr %d\n", bus_num, addr);
 
-        usb_host_open(s, bus_num, addr, port, product_name, speed);
+        ret = usb_host_open(s, bus_num, addr, port, product_name, speed);
+        if (ret) {
+            /* If we've found a match but failed to open the device we should
+               remove it from our auto-filter list, so we don't keep trying to
+               open it every 2 seconds.  However we cannot destroy / free it
+               here, since we get called from usb_host_initfn, and destroying
+               a qdev from its initfn is not allowed. */
+            s->open_failed = ret;
+        }
     }
 
     return 0;
@@ -1678,9 +1696,16 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
 
 static void usb_host_auto_check(void *unused)
 {
-    struct USBHostDevice *s;
+    struct USBHostDevice *s, *n;
     int unconnected = 0;
 
+    /* Cleanup devices wich failed to open last time, see usb_host_auto_scan
+       for why we don't do this after usb_host_auto_scan */
+    QTAILQ_FOREACH_SAFE(s, &hostdevs, next, n) {
+        if (s->open_failed)
+            qdev_free(&s->dev.qdev);
+    }
+
     usb_host_scan(NULL, usb_host_auto_scan);
 
     QTAILQ_FOREACH(s, &hostdevs, next) {
-- 
1.7.5.1

[Qemu-devel] [PATCH 08/14] usb-linux: Don't try to open the same device twice

[Hans de Goede]

If a user wants to redirect 2 identical usb sticks, in theory this is
possible by doing:
usb_add host:1234:5678
usb_add host:1234:5678

But this will lead to us trying to open the first stick twice, since we
don't break the loop after having found a match in our filter list, so the next'
filter list entry will result in us trying to open the same device again.

Fix this by adding the missing break.
---
 usb-linux.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 334012e..eb9805b 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1689,6 +1689,7 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
                a qdev from its initfn is not allowed. */
             s->open_failed = ret;
         }
+        break;
     }
 
     return 0;
-- 
1.7.5.1

[Qemu-devel] [PATCH 09/14] usb-linux: Don't declare a usbdevice_name

[Hans de Goede]

Declaring a usbdevice_name while we still have an explicit call to
usb_host_device_open in vl.c causes usb_host_device_open to get called
twice if the initial call fails.
---
 usb-linux.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index eb9805b..3508cda 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1227,8 +1227,12 @@ static struct USBDeviceInfo usb_host_dev_info = {
     .handle_control = usb_host_handle_control,
     .handle_reset   = usb_host_handle_reset,
     .handle_destroy = usb_host_handle_destroy,
+#if 0 /* HDG: having this enabled while still having an explicit call to
+         usb_host_device_open in vl.c causes usb_host_device_open to get called
+         twice if the initial call fails */
     .usbdevice_name = "host",
     .usbdevice_init = usb_host_device_open,
+#endif
     .qdev.props     = (Property[]) {
         DEFINE_PROP_UINT32("hostbus",  USBHostDevice, match.bus_num,    0),
         DEFINE_PROP_UINT32("hostaddr", USBHostDevice, match.addr,       0),
-- 
1.7.5.1

[Qemu-devel] [PATCH 10/14] usb-linux: Enlarge buffer for descriptors to 8192 bytes

[Hans de Goede]

1024 bytes is way to small, one hd UVC webcam I have over here has so
many resolutions its descriptors take op close to 4k. Hopefully 8k will
be enough for all devices.
---
 usb-linux.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 3508cda..490c49f 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -117,7 +117,7 @@ typedef struct USBHostDevice {
     USBDevice dev;
     int       fd;
 
-    uint8_t   descr[1024];
+    uint8_t   descr[8192];
     int       descr_len;
     int       configuration;
     int       ninterfaces;
-- 
1.7.5.1

[Qemu-devel] [PATCH 11/14] usb-bus: Add knowledge of USB_SPEED_SUPER to usb_speed helper

[Hans de Goede]

---
 hw/usb-bus.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 874c253..91f2083 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -273,6 +273,7 @@ static const char *usb_speed(unsigned int speed)
         [ USB_SPEED_LOW  ] = "1.5",
         [ USB_SPEED_FULL ] = "12",
         [ USB_SPEED_HIGH ] = "480",
+        [ USB_SPEED_SUPER ] = "5000",
     };
     if (speed >= ARRAY_SIZE(txt))
         return "?";
-- 
1.7.5.1

[Qemu-devel] [PATCH 12/14] usb-bus: Don't allow attaching a device to a bus with no free ports

[Hans de Goede]

---
 hw/usb-bus.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 91f2083..bb040d2 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -179,6 +179,11 @@ static void do_attach(USBDevice *dev)
                 dev->product_desc);
         return;
     }
+    if (bus->nfree == 0) {
+        fprintf(stderr, "Warning: tried to attach usb device %s to a bus with no free ports\n",
+                dev->product_desc);
+        return;
+    }
     if (dev->port_path) {
         QTAILQ_FOREACH(port, &bus->free, next) {
             if (strcmp(port->path, dev->port_path) == 0) {
-- 
1.7.5.1

[Qemu-devel] [PATCH 13/14] usb-bus: Don't detach non attached devices on device exit

[Hans de Goede]

This causes an "Error: tried to detach unattached usb device " to be printed,
this can happen when deleting ie a usb host qdev, which did not
get attached (because a device matching the filter never got plugged in).
---
 hw/usb-bus.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index bb040d2..0a49921 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -84,7 +84,9 @@ static int usb_qdev_exit(DeviceState *qdev)
     USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
     USBBus *bus = usb_bus_from_device(dev);
 
-    usb_device_detach(dev);
+    if (dev->attached) {
+        usb_device_detach(dev);
+    }
     bus->ops->device_destroy(bus, dev);
     if (dev->info->handle_destroy) {
         dev->info->handle_destroy(dev);
-- 
1.7.5.1

[Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Hans de Goede]

---
 hw/usb-bus.c |   23 ++++++++++++-----------
 hw/usb-msd.c |    5 +++--
 usb-linux.c  |    6 +++++-
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 0a49921..2ae2678 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -75,7 +75,7 @@ static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
     QLIST_INIT(&dev->strings);
     rc = dev->info->init(dev);
     if (rc == 0 && dev->auto_attach)
-        usb_device_attach(dev);
+        rc = usb_device_attach(dev);
     return rc;
 }
 
@@ -171,20 +171,20 @@ void usb_unregister_port(USBBus *bus, USBPort *port)
     bus->nfree--;
 }
 
-static void do_attach(USBDevice *dev)
+static int do_attach(USBDevice *dev)
 {
     USBBus *bus = usb_bus_from_device(dev);
     USBPort *port;
 
     if (dev->attached) {
-        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
+        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
                 dev->product_desc);
-        return;
+        return -1;
     }
     if (bus->nfree == 0) {
-        fprintf(stderr, "Warning: tried to attach usb device %s to a bus with no free ports\n",
+        fprintf(stderr, "Error: tried to attach usb device %s to a bus with no free ports\n",
                 dev->product_desc);
-        return;
+        return -1;
     }
     if (dev->port_path) {
         QTAILQ_FOREACH(port, &bus->free, next) {
@@ -193,9 +193,9 @@ static void do_attach(USBDevice *dev)
             }
         }
         if (port == NULL) {
-            fprintf(stderr, "Warning: usb port %s (bus %s) not found\n",
+            fprintf(stderr, "Error: usb port %s (bus %s) not found\n",
                     dev->port_path, bus->qbus.name);
-            return;
+            return -1;
         }
     } else {
         port = QTAILQ_FIRST(&bus->free);
@@ -209,6 +209,8 @@ static void do_attach(USBDevice *dev)
 
     QTAILQ_INSERT_TAIL(&bus->used, port, next);
     bus->nused++;
+
+    return 0;
 }
 
 int usb_device_attach(USBDevice *dev)
@@ -220,8 +222,7 @@ int usb_device_attach(USBDevice *dev)
            (unless a physical port location is specified). */
         usb_create_simple(bus, "usb-hub");
     }
-    do_attach(dev);
-    return 0;
+    return do_attach(dev);
 }
 
 int usb_device_detach(USBDevice *dev)
@@ -230,7 +231,7 @@ int usb_device_detach(USBDevice *dev)
     USBPort *port;
 
     if (!dev->attached) {
-        fprintf(stderr, "Warning: tried to detach unattached usb device %s\n",
+        fprintf(stderr, "Error: tried to detach unattached usb device %s\n",
                 dev->product_desc);
         return -1;
     }
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index c8b7d41..68b46fa 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -481,8 +481,9 @@ static void usb_msd_password_cb(void *opaque, int err)
     MSDState *s = opaque;
 
     if (!err)
-        usb_device_attach(&s->dev);
-    else
+        err = usb_device_attach(&s->dev);
+
+    if (err)
         qdev_unplug(&s->dev.qdev);
 }
 
diff --git a/usb-linux.c b/usb-linux.c
index 490c49f..fe21da1 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1152,10 +1152,14 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
                 prod_name);
     }
 
+    ret = usb_device_attach(&dev->dev);
+    if (ret) {
+        goto fail;
+    }
+
     /* USB devio uses 'write' flag to check for async completions */
     qemu_set_fd_handler(dev->fd, NULL, async_complete, dev);
 
-    usb_device_attach(&dev->dev);
     return 0;
 
 fail:
-- 
1.7.5.1

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Michael Tokarev]

31.05.2011 13:35, Hans de Goede wrote:
> ---
>  hw/usb-bus.c |   23 ++++++++++++-----------
>  hw/usb-msd.c |    5 +++--
>  usb-linux.c  |    6 +++++-
>  3 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
> index 0a49921..2ae2678 100644
> --- a/hw/usb-bus.c
> +++ b/hw/usb-bus.c

>      if (dev->attached) {
> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>                  dev->product_desc);

qemu_error() maybe, while we're at it?
Here and in a few other places.

Thanks!

/mjt

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Hans de Goede]

Hi,

On 05/31/2011 11:42 AM, Michael Tokarev wrote:
> 31.05.2011 13:35, Hans de Goede wrote:
>> ---
>>   hw/usb-bus.c |   23 ++++++++++++-----------
>>   hw/usb-msd.c |    5 +++--
>>   usb-linux.c  |    6 +++++-
>>   3 files changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>> index 0a49921..2ae2678 100644
>> --- a/hw/usb-bus.c
>> +++ b/hw/usb-bus.c
>
>>       if (dev->attached) {
>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>                   dev->product_desc);
>
> qemu_error() maybe, while we're at it?
> Here and in a few other places.

That does not seem to exist, do you perhaps mean error_printf() ?

Regards,

Hans

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Kevin Wolf]

Am 31.05.2011 11:51, schrieb Hans de Goede:
> Hi,
> 
> On 05/31/2011 11:42 AM, Michael Tokarev wrote:
>> 31.05.2011 13:35, Hans de Goede wrote:
>>> ---
>>>   hw/usb-bus.c |   23 ++++++++++++-----------
>>>   hw/usb-msd.c |    5 +++--
>>>   usb-linux.c  |    6 +++++-
>>>   3 files changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>>> index 0a49921..2ae2678 100644
>>> --- a/hw/usb-bus.c
>>> +++ b/hw/usb-bus.c
>>
>>>       if (dev->attached) {
>>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>>                   dev->product_desc);
>>
>> qemu_error() maybe, while we're at it?
>> Here and in a few other places.
> 
> That does not seem to exist, do you perhaps mean error_printf() ?

error_report() is what you should use, so that messages go to the
monitor if the function is called from a monitor command. error_printf()
is used by it internally, but usually isn't used directly.

Kevin

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Hans de Goede]

Hi,

On 05/31/2011 11:56 AM, Kevin Wolf wrote:
> Am 31.05.2011 11:51, schrieb Hans de Goede:
>> Hi,
>>
>> On 05/31/2011 11:42 AM, Michael Tokarev wrote:
>>> 31.05.2011 13:35, Hans de Goede wrote:
>>>> ---
>>>>    hw/usb-bus.c |   23 ++++++++++++-----------
>>>>    hw/usb-msd.c |    5 +++--
>>>>    usb-linux.c  |    6 +++++-
>>>>    3 files changed, 20 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>>>> index 0a49921..2ae2678 100644
>>>> --- a/hw/usb-bus.c
>>>> +++ b/hw/usb-bus.c
>>>
>>>>        if (dev->attached) {
>>>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>>>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>>>                    dev->product_desc);
>>>
>>> qemu_error() maybe, while we're at it?
>>> Here and in a few other places.
>>
>> That does not seem to exist, do you perhaps mean error_printf() ?
>
> error_report() is what you should use, so that messages go to the
> monitor if the function is called from a monitor command. error_printf()
> is used by it internally, but usually isn't used directly.
>

I've looked at error_report, but IMHO it is made of crazy, I'm not going
to construct a json dict every time I need to log some simple error message
(and the existing ones are not suitable for many error messages).

Regards,

Hans

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Kevin Wolf]

Am 31.05.2011 12:05, schrieb Hans de Goede:
> Hi,
> 
> On 05/31/2011 11:56 AM, Kevin Wolf wrote:
>> Am 31.05.2011 11:51, schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 05/31/2011 11:42 AM, Michael Tokarev wrote:
>>>> 31.05.2011 13:35, Hans de Goede wrote:
>>>>> ---
>>>>>    hw/usb-bus.c |   23 ++++++++++++-----------
>>>>>    hw/usb-msd.c |    5 +++--
>>>>>    usb-linux.c  |    6 +++++-
>>>>>    3 files changed, 20 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>>>>> index 0a49921..2ae2678 100644
>>>>> --- a/hw/usb-bus.c
>>>>> +++ b/hw/usb-bus.c
>>>>
>>>>>        if (dev->attached) {
>>>>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>>>>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>>>>                    dev->product_desc);
>>>>
>>>> qemu_error() maybe, while we're at it?
>>>> Here and in a few other places.
>>>
>>> That does not seem to exist, do you perhaps mean error_printf() ?
>>
>> error_report() is what you should use, so that messages go to the
>> monitor if the function is called from a monitor command. error_printf()
>> is used by it internally, but usually isn't used directly.
>>
> 
> I've looked at error_report, but IMHO it is made of crazy, I'm not going
> to construct a json dict every time I need to log some simple error message
> (and the existing ones are not suitable for many error messages).

error_report() works with plain strings. Maybe you confuse it with the
QMP error reporting function?

Kevin

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Hans de Goede]

Hi,

On 05/31/2011 12:12 PM, Kevin Wolf wrote:
> Am 31.05.2011 12:05, schrieb Hans de Goede:
>> Hi,
>>
>> On 05/31/2011 11:56 AM, Kevin Wolf wrote:
>>> Am 31.05.2011 11:51, schrieb Hans de Goede:
>>>> Hi,
>>>>
>>>> On 05/31/2011 11:42 AM, Michael Tokarev wrote:
>>>>> 31.05.2011 13:35, Hans de Goede wrote:
>>>>>> ---
>>>>>>     hw/usb-bus.c |   23 ++++++++++++-----------
>>>>>>     hw/usb-msd.c |    5 +++--
>>>>>>     usb-linux.c  |    6 +++++-
>>>>>>     3 files changed, 20 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>>>>>> index 0a49921..2ae2678 100644
>>>>>> --- a/hw/usb-bus.c
>>>>>> +++ b/hw/usb-bus.c
>>>>>
>>>>>>         if (dev->attached) {
>>>>>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>>>>>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>>>>>                     dev->product_desc);
>>>>>
>>>>> qemu_error() maybe, while we're at it?
>>>>> Here and in a few other places.
>>>>
>>>> That does not seem to exist, do you perhaps mean error_printf() ?
>>>
>>> error_report() is what you should use, so that messages go to the
>>> monitor if the function is called from a monitor command. error_printf()
>>> is used by it internally, but usually isn't used directly.
>>>
>>
>> I've looked at error_report, but IMHO it is made of crazy, I'm not going
>> to construct a json dict every time I need to log some simple error message
>> (and the existing ones are not suitable for many error messages).
>
> error_report() works with plain strings. Maybe you confuse it with the
> QMP error reporting function?

Ah yes I was looking at qerror_report (who ever named that, having just
one letter difference in the function names is a bad idea). error_report
looks fine.

I'll wait a bit for more feedback and then change
[PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

To turn the fprintf(stderr, ... calls into error_report calls.

Thanks & Regards,

Hans



>
> Kevin
>

Re: [Qemu-devel] [PATCH 01/14] usb-linux: Set usb_auto_timer to NULL after deleting it

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> We might check for it being NULL later, if the device gets unplugged.
> ---
>   usb-linux.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/usb-linux.c b/usb-linux.c
> index 4d7a31a..ea3ab5f 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -1675,6 +1675,7 @@ static void usb_host_auto_check(void *unused)
>           /* nothing to watch */
>           if (usb_auto_timer) {
>               qemu_del_timer(usb_auto_timer);
> +            usb_auto_timer = NULL;

This is wrong.

qemu_del_timer just removes the scheduled timer event, not the timer 
structure itself.  qemu_free_timer does the later.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 02/14] usb-linux: Get speed from sysfs rather then from the connectinfo ioctl

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> The connectinfo ioctl only differentiates between lo speed devices, and
> all other speeds, where as we would like to know the real speed. The real
> speed is available in sysfs so use that when available.

Applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 03/14] usb-linux: Teach about super speed

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> +            if (!strcmp(buf, "5000")) {
> +                speed = USB_SPEED_SUPER;
> +            } else if (!strcmp(buf, "480")) {

Patch applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 04/14] usb-linux: Don't do perror when errno is not set

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> Note that "op" also is not set, so before this change these error paths
> would feed NULL to perror.

Patch applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 04/14] usb-linux: Don't do perror when errno is not set

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> Note that "op" also is not set, so before this change these error paths
> would feed NULL to perror.

Patch applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 05/14] usb-linux: Don't call usb_host_close when usb_host_open fails

[Gerd Hoffmann]

What bug you are trying to fix here?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 06/14] usb-linux: Ensure devep != 0

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> So that we don't index endp_table with a negative index.

Patch applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 07/14] usb-linux: If opening a device fails remove it from our filter list

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> So that we don't retry to open it every 2 seconds flooding stderr with
> error messages.

The polling here is done intentionally, so the devices catched by the 
filter show up in the guest automagically as soon as they are plugged 
in.  Just zapping the filter on failure isn't the right thing to do here.

We could try to do something more clever than polling sysfs every two 
seconds though, such using inotify to watch /sys/bus/usb/devices for new 
devices poping up.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 08/14] usb-linux: Don't try to open the same device twice

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> If a user wants to redirect 2 identical usb sticks, in theory this is
> possible by doing:
> usb_add host:1234:5678
> usb_add host:1234:5678
>
> But this will lead to us trying to open the first stick twice, since we
> don't break the loop after having found a match in our filter list, so the next'
> filter list entry will result in us trying to open the same device again.
>
> Fix

Good catch.

Patch applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 09/14] usb-linux: Don't declare a usbdevice_name

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> Declaring a usbdevice_name while we still have an explicit call to
> usb_host_device_open in vl.c causes usb_host_device_open to get called
> twice if the initial call fails.

Wrong way around, the vl.c call should be zapped instead.  Just did.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 10/14] usb-linux: Enlarge buffer for descriptors to 8192 bytes

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> 1024 bytes is way to small, one hd UVC webcam I have over here has so
> many resolutions its descriptors take op close to 4k. Hopefully 8k will
> be enough for all devices.

Patch applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 11/14] usb-bus: Add knowledge of USB_SPEED_SUPER to usb_speed helper

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> +        [ USB_SPEED_SUPER ] = "5000",

Patch applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Gerd Hoffmann]

   Hi,

> I'll wait a bit for more feedback and then change
> [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

I'll wait for new revisions of patches 12+14 then.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 13/14] usb-bus: Don't detach non attached devices on device exit

[Gerd Hoffmann]

On 05/31/11 11:35, Hans de Goede wrote:
> This causes an "Error: tried to detach unattached usb device " to be printed,
> this can happen when deleting ie a usb host qdev, which did not
> get attached (because a device matching the filter never got plugged in).

Patch applied.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 01/14] usb-linux: Set usb_auto_timer to NULL after deleting it

[Hans de Goede]

Hi,

On 06/01/2011 12:48 PM, Gerd Hoffmann wrote:
> On 05/31/11 11:35, Hans de Goede wrote:
>> We might check for it being NULL later, if the device gets unplugged.
>> ---
>> usb-linux.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/usb-linux.c b/usb-linux.c
>> index 4d7a31a..ea3ab5f 100644
>> --- a/usb-linux.c
>> +++ b/usb-linux.c
>> @@ -1675,6 +1675,7 @@ static void usb_host_auto_check(void *unused)
>> /* nothing to watch */
>> if (usb_auto_timer) {
>> qemu_del_timer(usb_auto_timer);
>> + usb_auto_timer = NULL;
>
> This is wrong.
>
> qemu_del_timer just removes the scheduled timer event, not the timer structure itself. qemu_free_timer does the later.
>

Ok, I stand corrected :)

Regards,

Hans

Re: [Qemu-devel] [PATCH 05/14] usb-linux: Don't call usb_host_close when usb_host_open fails

[Hans de Goede]

Hi,

On 06/01/2011 02:22 PM, Gerd Hoffmann wrote:
>
> What bug you are trying to fix here?

Nothing in particular, while looking at some other
stuff I noticed that we have the following sequence,
which is wrong:

usb_host_open called
  usb_host_open calls usb_host_claim_interfaces
   usb_host_claim_interfaces calls do_disconnect because of failure
    do_disconnect calls usb_host_close
     usb_host_close iterates over endpoints, but usb_linux_update_endp_table
       has not been called to initialize the endpoints at this points
     usb_host_close calls usb_device_detach, but not attached yet
     usb_host_close does an not needed ioctl(dev->fd, USBDEVFS_RESET);
     usb_host_closes the fd
  usb_host_open jumps to fail, closes the fd *again*

All of this is does not lead to any real user visible bugs,
but from a code flow pov it is wrong.

Regards,

Hans

Re: [Qemu-devel] [PATCH 07/14] usb-linux: If opening a device fails remove it from our filter list

[Hans de Goede]

Hi,

On 06/01/2011 02:32 PM, Gerd Hoffmann wrote:
> On 05/31/11 11:35, Hans de Goede wrote:
>> So that we don't retry to open it every 2 seconds flooding stderr with
>> error messages.
>
> The polling here is done intentionally, so the devices catched by the filter show up in the guest automagically as soon as they are plugged in. Just zapping the filter on failure isn't the right thing to do here.

Note I'm zapping the filter when we fail to open the device, not when it
is not present. This can happen for example when the qemu user does not
have rights on the usbfs device node.

It seems better to me to print the relevant error once, and then require
the user to redo the usb_add / device_add if necessary, then to flood
the monitor with repeating the same error every 2 seconds. Note that
something like a permission problem (which is the most likely case
for opening a device failing once we've found it) won't go away by
itself.

> We could try to do something more clever than polling sysfs every two seconds though, such using inotify to watch /sys/bus/usb/devices for new devices poping up.

That would also result in trying to open the device ones, and if that
fails give up, I don't see the difference. Actually the user experience
would be worse, because the proper sequence in case of a permission
problem would go from:

usb_add
see error
fix permission
usb_add

to:

usb_add
see error
fix permission
usb_del
usb_add

Regards,

Hans

Re: [Qemu-devel] [PATCH 09/14] usb-linux: Don't declare a usbdevice_name

[Hans de Goede]

Hi,

On 06/01/2011 02:44 PM, Gerd Hoffmann wrote:
> On 05/31/11 11:35, Hans de Goede wrote:
>> Declaring a usbdevice_name while we still have an explicit call to
>> usb_host_device_open in vl.c causes usb_host_device_open to get called
>> twice if the initial call fails.
>
> Wrong way around, the vl.c call should be zapped instead. Just did.

Sounds good to me, but you will also need to fix usb-bsd to
declare a usbdevice_name and initfn then.

Regards,

Hans

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Hans de Goede]

Hi,

On 06/01/2011 02:50 PM, Gerd Hoffmann wrote:
> Hi,
>
>> I'll wait a bit for more feedback and then change
>> [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors
>
> I'll wait for new revisions of patches 12+14 then.

I already fixed this yeterday, I opted to leave patch 12 unmodified and
replace all fprintf(stderr, ... calls with error_report calls in one go
in a new patch 14.

You can find the new version here:
http://cgit.freedesktop.org/~jwrdegoede/qemu/commit/?h=usb-patches&id=3c0be7730ff74a5cdac6aa100179b15c9392ab5f

Let me know if you'd rather have me resend 12 + 14 to the list.

Regards,

Hans

Re: [Qemu-devel] [PATCH 07/14] usb-linux: If opening a device fails remove it from our filter list

[Gerd Hoffmann]

On 06/01/11 16:37, Hans de Goede wrote:
> Hi,
>
> On 06/01/2011 02:32 PM, Gerd Hoffmann wrote:
>> On 05/31/11 11:35, Hans de Goede wrote:
>>> So that we don't retry to open it every 2 seconds flooding stderr with
>>> error messages.
>>
>> The polling here is done intentionally, so the devices catched by the
>> filter show up in the guest automagically as soon as they are plugged
>> in. Just zapping the filter on failure isn't the right thing to do here.
>
> Note I'm zapping the filter when we fail to open the device, not when it
> is not present. This can happen for example when the qemu user does not
> have rights on the usbfs device node.
>
> It seems better to me to print the relevant error once, and then require
> the user to redo the usb_add / device_add if necessary, then to flood
> the monitor with repeating the same error every 2 seconds.

Devices magically disappearing is a problem for the management stack 
(such as libvirt) though.  qemu does stuff like that in too many places 
already and we are trying to get rid of it.

Flooding the monitor (or stderr) indeed isn't nice though.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

[Gerd Hoffmann]

   Hi,

> Let me know if you'd rather have me resend 12 + 14 to the list.

I'd suggest to look at the leftover stuff after the next usb merge.

cheers,
   Gerd

PS: /me is busy doing tests, preparing the next usb pull which hopefully 
goes out later today.