[Qemu-devel] [PATCH 0/7] usb-host: bugfixes

[Qemu-devel] [PATCH 0/7] usb-host: bugfixes

[Gerd Hoffmann]

  Hi,

Here comes a bunch of bugfixes for the usb host (aka usb pass-thru)
driver.  Gets some of my devices working which didn't work before, so
anyone who had trouble with it recently is encouraged to re-test with
these patches applied.

cheers,
  Gerd

Gerd Hoffmann (7):
  usb-host: start tracing support
  usb-host: reapurb error report fix
  usb-host: fix halted endpoints
  usb-host: limit open retries
  usb-host: fix configuration tracking.
  usb-host: claim port
  usb: fix use after free

 hw/usb.c     |    2 +-
 trace-events |   19 +++++
 usb-linux.c  |  236 +++++++++++++++++++++++++++++++++++-----------------------
 3 files changed, 163 insertions(+), 94 deletions(-)

[Qemu-devel] [PATCH 1/7] usb-host: start tracing support

[Gerd Hoffmann]

Add a bunch of trace points to usb-linux.c  Drop a bunch of DPRINTK's in
favor of the trace points.  Also cleanup error reporting a bit while being
at it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 trace-events |   18 ++++++++++++++
 usb-linux.c  |   73 ++++++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 66 insertions(+), 25 deletions(-)

diff --git a/trace-events b/trace-events
index 14e6f8b..1c7a624 100644
--- a/trace-events
+++ b/trace-events
@@ -243,6 +243,24 @@ disable usb_set_config(int addr, int config, int ret) "dev %d, config %d, ret %d
 disable usb_clear_device_feature(int addr, int feature, int ret) "dev %d, feature %d, ret %d"
 disable usb_set_device_feature(int addr, int feature, int ret) "dev %d, feature %d, ret %d"
 
+# usb-linux.c
+usb_host_open(int bus, int addr, const char *state) "dev %d:%d, %s"
+usb_host_disconnect(int bus, int addr) "dev %d:%d"
+usb_host_close(int bus, int addr) "dev %d:%d"
+usb_host_set_address(int bus, int addr, int config) "dev %d:%d, address %d"
+usb_host_set_config(int bus, int addr, int config) "dev %d:%d, config %d"
+usb_host_set_interface(int bus, int addr, int interface, int alt) "dev %d:%d, interface %d, alt %d"
+usb_host_claim_interfaces(int bus, int addr, int config, int nif) "dev %d:%d, config %d, nif %d"
+usb_host_release_interfaces(int bus, int addr) "dev %d:%d"
+usb_host_req_control(int bus, int addr, int req, int value, int index) "dev %d:%d, req 0x%x, value %d, index %d"
+usb_host_req_data(int bus, int addr, const char *dir, int ep, int size) "dev %d:%d, %s, ep %d, size %d"
+usb_host_req_complete(int bus, int addr, int status) "dev %d:%d, status %d"
+usb_host_urb_submit(int bus, int addr, void *aurb, int length, int more) "dev %d:%d, aurb %p, length %d, more %d"
+usb_host_urb_complete(int bus, int addr, void *aurb, int status, int length, int more) "dev %d:%d, aurb %p, status %d, length %d, more %d"
+usb_host_ep_op(int bus, int addr, int ep, const char *op) "dev %d:%d, ep %d, %s"
+usb_host_reset(int bus, int addr) "dev %d:%d"
+usb_host_auto_scan(const char *state) "%s"
+
 # hw/scsi-bus.c
 disable scsi_req_alloc(int target, int lun, int tag) "target %d lun %d tag %d"
 disable scsi_req_data(int target, int lun, int tag, int len) "target %d lun %d tag %d len %d"
diff --git a/usb-linux.c b/usb-linux.c
index 2e20f8e..d813a85 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -34,6 +34,7 @@
 #include "qemu-timer.h"
 #include "monitor.h"
 #include "sysemu.h"
+#include "trace.h"
 
 #include <dirent.h>
 #include <sys/ioctl.h>
@@ -165,11 +166,13 @@ static int is_halted(USBHostDevice *s, int ep)
 
 static void clear_halt(USBHostDevice *s, int ep)
 {
+    trace_usb_host_ep_op(s->bus_num, s->addr, ep, "clear halt");
     get_endp(s, ep)->halted = 0;
 }
 
 static void set_halt(USBHostDevice *s, int ep)
 {
+    trace_usb_host_ep_op(s->bus_num, s->addr, ep, "set halt");
     get_endp(s, ep)->halted = 1;
 }
 
@@ -180,12 +183,15 @@ static int is_iso_started(USBHostDevice *s, int ep)
 
 static void clear_iso_started(USBHostDevice *s, int ep)
 {
+    trace_usb_host_ep_op(s->bus_num, s->addr, ep, "stop iso");
     get_endp(s, ep)->iso_started = 0;
 }
 
 static void set_iso_started(USBHostDevice *s, int ep)
 {
     struct endp_data *e = get_endp(s, ep);
+
+    trace_usb_host_ep_op(s->bus_num, s->addr, ep, "start iso");
     if (!e->iso_started) {
         e->iso_started = 1;
         e->inflight = 0;
@@ -285,8 +291,6 @@ static void async_free(AsyncURB *aurb)
 
 static void do_disconnect(USBHostDevice *s)
 {
-    printf("husb: device %d.%d disconnected\n",
-           s->bus_num, s->addr);
     usb_host_close(s);
     usb_host_auto_check(NULL);
 }
@@ -309,11 +313,12 @@ static void async_complete(void *opaque)
                 return;
             }
             if (errno == ENODEV && !s->closing) {
+                trace_usb_host_disconnect(s->bus_num, s->addr);
                 do_disconnect(s);
                 return;
             }
 
-            DPRINTF("husb: async. reap urb failed errno %d\n", errno);
+            perror("USBDEVFS_REAPURBNDELAY");
             return;
         }
 
@@ -337,6 +342,8 @@ static void async_complete(void *opaque)
         }
 
         p = aurb->packet;
+        trace_usb_host_urb_complete(s->bus_num, s->addr, aurb, aurb->urb.status,
+                                    aurb->urb.actual_length, aurb->more);
 
         if (p) {
             switch (aurb->urb.status) {
@@ -355,8 +362,10 @@ static void async_complete(void *opaque)
             }
 
             if (aurb->urb.type == USBDEVFS_URB_TYPE_CONTROL) {
+                trace_usb_host_req_complete(s->bus_num, s->addr, p->result);
                 usb_generic_async_ctrl_complete(&s->dev, p);
             } else if (!aurb->more) {
+                trace_usb_host_req_complete(s->bus_num, s->addr, p->result);
                 usb_packet_complete(&s->dev, p);
             }
         }
@@ -418,7 +427,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
         }
         config_descr_len = dev->descr[i];
 
-        printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
+        DPRINTF("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
 
         if (configuration < 0 || configuration == dev->descr[i + 5]) {
             configuration = dev->descr[i + 5];
@@ -457,17 +466,12 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
         op = "USBDEVFS_CLAIMINTERFACE";
         ret = ioctl(dev->fd, USBDEVFS_CLAIMINTERFACE, &interface);
         if (ret < 0) {
-            if (errno == EBUSY) {
-                printf("husb: update iface. device already grabbed\n");
-            } else {
-                perror("husb: failed to claim interface");
-            }
             goto fail;
         }
     }
 
-    printf("husb: %d interfaces claimed for configuration %d\n",
-           nb_interfaces, configuration);
+    trace_usb_host_claim_interfaces(dev->bus_num, dev->addr,
+                                    nb_interfaces, configuration);
 
     dev->ninterfaces   = nb_interfaces;
     dev->configuration = configuration;
@@ -485,16 +489,15 @@ static int usb_host_release_interfaces(USBHostDevice *s)
 {
     int ret, i;
 
-    DPRINTF("husb: releasing interfaces\n");
+    trace_usb_host_release_interfaces(s->bus_num, s->addr);
 
     for (i = 0; i < s->ninterfaces; i++) {
         ret = ioctl(s->fd, USBDEVFS_RELEASEINTERFACE, &i);
         if (ret < 0) {
-            perror("husb: failed to release interface");
+            perror("USBDEVFS_RELEASEINTERFACE");
             return 0;
         }
     }
-
     return 1;
 }
 
@@ -502,7 +505,7 @@ static void usb_host_handle_reset(USBDevice *dev)
 {
     USBHostDevice *s = DO_UPCAST(USBHostDevice, dev, dev);
 
-    DPRINTF("husb: reset device %u.%u\n", s->bus_num, s->addr);
+    trace_usb_host_reset(s->bus_num, s->addr);
 
     ioctl(s->fd, USBDEVFS_RESET);
 
@@ -564,7 +567,7 @@ static void usb_host_stop_n_free_iso(USBHostDevice *s, uint8_t ep)
         if (aurb[i].iso_frame_idx == -1) {
             ret = ioctl(s->fd, USBDEVFS_DISCARDURB, &aurb[i]);
             if (ret < 0) {
-                printf("husb: discard isoc in urb failed errno %d\n", errno);
+                perror("USBDEVFS_DISCARDURB");
                 free = 0;
                 continue;
             }
@@ -680,7 +683,7 @@ static int usb_host_handle_iso_data(USBHostDevice *s, USBPacket *p, int in)
             if (aurb[i].iso_frame_idx == ISO_FRAME_DESC_PER_URB) {
                 ret = ioctl(s->fd, USBDEVFS_SUBMITURB, &aurb[i]);
                 if (ret < 0) {
-                    printf("husb error submitting iso urb %d: %d\n", i, errno);
+                    perror("USBDEVFS_SUBMITURB");
                     if (!in || len == 0) {
                         switch(errno) {
                         case ETIMEDOUT:
@@ -711,7 +714,12 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
     uint8_t *pbuf;
     uint8_t ep;
 
+    trace_usb_host_req_data(s->bus_num, s->addr,
+                            p->pid == USB_TOKEN_IN ? "IN" : "OUT",
+                            p->devep, p->iov.size);
+
     if (!is_valid(s, p->devep)) {
+        trace_usb_host_req_complete(s->bus_num, s->addr, USB_RET_NAK);
         return USB_RET_NAK;
     }
 
@@ -724,8 +732,8 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
     if (is_halted(s, p->devep)) {
         ret = ioctl(s->fd, USBDEVFS_CLEAR_HALT, &ep);
         if (ret < 0) {
-            DPRINTF("husb: failed to clear halt. ep 0x%x errno %d\n",
-                   ep, errno);
+            perror("USBDEVFS_CLEAR_HALT");
+            trace_usb_host_req_complete(s->bus_num, s->addr, USB_RET_NAK);
             return USB_RET_NAK;
         }
         clear_halt(s, p->devep);
@@ -767,20 +775,24 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
             aurb->more         = 1;
         }
 
+        trace_usb_host_urb_submit(s->bus_num, s->addr, aurb,
+                                  urb->buffer_length, aurb->more);
         ret = ioctl(s->fd, USBDEVFS_SUBMITURB, urb);
 
         DPRINTF("husb: data submit: ep 0x%x, len %u, more %d, packet %p, aurb %p\n",
                 urb->endpoint, urb->buffer_length, aurb->more, p, aurb);
 
         if (ret < 0) {
-            DPRINTF("husb: submit failed. errno %d\n", errno);
+            perror("USBDEVFS_SUBMITURB");
             async_free(aurb);
 
             switch(errno) {
             case ETIMEDOUT:
+                trace_usb_host_req_complete(s->bus_num, s->addr, USB_RET_NAK);
                 return USB_RET_NAK;
             case EPIPE:
             default:
+                trace_usb_host_req_complete(s->bus_num, s->addr, USB_RET_STALL);
                 return USB_RET_STALL;
             }
         }
@@ -800,13 +812,15 @@ static int ctrl_error(void)
 
 static int usb_host_set_address(USBHostDevice *s, int addr)
 {
-    DPRINTF("husb: ctrl set addr %u\n", addr);
+    trace_usb_host_set_address(s->bus_num, s->addr, addr);
     s->dev.addr = addr;
     return 0;
 }
 
 static int usb_host_set_config(USBHostDevice *s, int config)
 {
+    trace_usb_host_set_config(s->bus_num, s->addr, config);
+
     usb_host_release_interfaces(s);
 
     int ret = ioctl(s->fd, USBDEVFS_SETCONFIGURATION, &config);
@@ -825,6 +839,8 @@ static int usb_host_set_interface(USBHostDevice *s, int iface, int alt)
     struct usbdevfs_setinterface si;
     int i, ret;
 
+    trace_usb_host_set_interface(s->bus_num, s->addr, iface, alt);
+
     for (i = 1; i <= MAX_ENDPOINTS; i++) {
         if (is_isoc(s, i)) {
             usb_host_stop_n_free_iso(s, i);
@@ -859,8 +875,7 @@ static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
      */
 
     /* Note request is (bRequestType << 8) | bRequest */
-    DPRINTF("husb: ctrl type 0x%x req 0x%x val 0x%x index %u len %u\n",
-            request >> 8, request & 0xff, value, index, length);
+    trace_usb_host_req_control(s->bus_num, s->addr, request, value, index);
 
     switch (request) {
     case DeviceOutRequest | USB_REQ_SET_ADDRESS:
@@ -900,6 +915,8 @@ static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
 
     urb->usercontext = s;
 
+    trace_usb_host_urb_submit(s->bus_num, s->addr, aurb,
+                              urb->buffer_length, aurb->more);
     ret = ioctl(s->fd, USBDEVFS_SUBMITURB, urb);
 
     DPRINTF("husb: submit ctrl. len %u aurb %p\n", urb->buffer_length, aurb);
@@ -1140,10 +1157,11 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
     int fd = -1, ret;
     char buf[1024];
 
+    trace_usb_host_open(bus_num, addr, "started");
+
     if (dev->fd != -1) {
         goto fail;
     }
-    printf("husb: open device %d.%d\n", bus_num, addr);
 
     if (!usb_host_device_path) {
         perror("husb: USB Host Device Path not set");
@@ -1218,7 +1236,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
         dev->dev.speedmask |= USB_SPEED_MASK_FULL;
     }
 
-    printf("husb: grabbed usb device %d.%d\n", bus_num, addr);
+    trace_usb_host_open(bus_num, addr, "success");
 
     if (!prod_name || prod_name[0] == '\0') {
         snprintf(dev->dev.product_desc, sizeof(dev->dev.product_desc),
@@ -1239,6 +1257,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
     return 0;
 
 fail:
+    trace_usb_host_open(bus_num, addr, "failure");
     if (dev->fd != -1) {
         close(dev->fd);
         dev->fd = -1;
@@ -1254,6 +1273,8 @@ static int usb_host_close(USBHostDevice *dev)
         return -1;
     }
 
+    trace_usb_host_close(dev->bus_num, dev->addr);
+
     qemu_set_fd_handler(dev->fd, NULL, NULL, NULL);
     dev->closing = 1;
     for (i = 1; i <= MAX_ENDPOINTS; i++) {
@@ -1776,6 +1797,7 @@ static void usb_host_auto_check(void *unused)
         /* nothing to watch */
         if (usb_auto_timer) {
             qemu_del_timer(usb_auto_timer);
+            trace_usb_host_auto_scan("disabled");
         }
         return;
     }
@@ -1785,6 +1807,7 @@ static void usb_host_auto_check(void *unused)
         if (!usb_auto_timer) {
             return;
         }
+        trace_usb_host_auto_scan("enabled");
     }
     qemu_mod_timer(usb_auto_timer, qemu_get_clock_ms(rt_clock) + 2000);
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/7] usb-host: start tracing support

[Stefan Hajnoczi]

On Thu, Aug 25, 2011 at 4:06 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Add a bunch of trace points to usb-linux.c  Drop a bunch of DPRINTK's in
> favor of the trace points.  Also cleanup error reporting a bit while being
> at it.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  trace-events |   18 ++++++++++++++
>  usb-linux.c  |   73 ++++++++++++++++++++++++++++++++++++++--------------------
>  2 files changed, 66 insertions(+), 25 deletions(-)
>
> diff --git a/trace-events b/trace-events
> index 14e6f8b..1c7a624 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -243,6 +243,24 @@ disable usb_set_config(int addr, int config, int ret) "dev %d, config %d, ret %d
>  disable usb_clear_device_feature(int addr, int feature, int ret) "dev %d, feature %d, ret %d"
>  disable usb_set_device_feature(int addr, int feature, int ret) "dev %d, feature %d, ret %d"
>
> +# usb-linux.c
> +usb_host_open(int bus, int addr, const char *state) "dev %d:%d, %s"

Please do not use strings.  It works for stderr but may not be
supported by other backends.  The simple backend only logs 64-bit
arguments and does not dereference into strings/arrays.  I can't
remember the status of dtrace or ust, but I'd ask you to avoid using a
string, if possible.

> +usb_host_disconnect(int bus, int addr) "dev %d:%d"
> +usb_host_close(int bus, int addr) "dev %d:%d"
> +usb_host_set_address(int bus, int addr, int config) "dev %d:%d, address %d"
> +usb_host_set_config(int bus, int addr, int config) "dev %d:%d, config %d"
> +usb_host_set_interface(int bus, int addr, int interface, int alt) "dev %d:%d, interface %d, alt %d"
> +usb_host_claim_interfaces(int bus, int addr, int config, int nif) "dev %d:%d, config %d, nif %d"
> +usb_host_release_interfaces(int bus, int addr) "dev %d:%d"
> +usb_host_req_control(int bus, int addr, int req, int value, int index) "dev %d:%d, req 0x%x, value %d, index %d"
> +usb_host_req_data(int bus, int addr, const char *dir, int ep, int size) "dev %d:%d, %s, ep %d, size %d"

String

> +usb_host_req_complete(int bus, int addr, int status) "dev %d:%d, status %d"
> +usb_host_urb_submit(int bus, int addr, void *aurb, int length, int more) "dev %d:%d, aurb %p, length %d, more %d"
> +usb_host_urb_complete(int bus, int addr, void *aurb, int status, int length, int more) "dev %d:%d, aurb %p, status %d, length %d, more %d"
> +usb_host_ep_op(int bus, int addr, int ep, const char *op) "dev %d:%d, ep %d, %s"

String

> +usb_host_reset(int bus, int addr) "dev %d:%d"
> +usb_host_auto_scan(const char *state) "%s"

String

The rest looks fine.  In some cases you can simply split the trace
event into multiple independent events which contains the
status/operation/etc value that you're trying to pretty-print.

Stefan

[Qemu-devel] [PATCH 2/7] usb-host: reapurb error report fix

[Gerd Hoffmann]

Don't report errors on devices which are in disconnected
and closing state.
---
 usb-linux.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index d813a85..ddfa50a 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -312,9 +312,11 @@ static void async_complete(void *opaque)
                 }
                 return;
             }
-            if (errno == ENODEV && !s->closing) {
-                trace_usb_host_disconnect(s->bus_num, s->addr);
-                do_disconnect(s);
+            if (errno == ENODEV) {
+                if (!s->closing) {
+                    trace_usb_host_disconnect(s->bus_num, s->addr);
+                    do_disconnect(s);
+                }
                 return;
             }
 
-- 
1.7.1

[Qemu-devel] [PATCH 3/7] usb-host: fix halted endpoints

[Gerd Hoffmann]

Two fixes for the price of one ;)

First, reinitialize the endpoint table after device reset.
This is needed anyway as the reset might have switched interfaces.
It also clears the endpoint halted state.

Second the CLEAR_HALT ioctl wants a unsigned int passed in as
argument, not uint8_t.

This gets my usb sd card reader (sandisk micromate) going.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 usb-linux.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index ddfa50a..fe7b9d9 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -143,6 +143,7 @@ static int parse_filter(const char *spec, struct USBAutoFilter *f);
 static void usb_host_auto_check(void *unused);
 static int usb_host_read_file(char *line, size_t line_size,
                             const char *device_file, const char *device_name);
+static int usb_linux_update_endp_table(USBHostDevice *s);
 
 static struct endp_data *get_endp(USBHostDevice *s, int ep)
 {
@@ -512,6 +513,7 @@ static void usb_host_handle_reset(USBDevice *dev)
     ioctl(s->fd, USBDEVFS_RESET);
 
     usb_host_claim_interfaces(s, s->configuration);
+    usb_linux_update_endp_table(s);
 }
 
 static void usb_host_handle_destroy(USBDevice *dev)
@@ -523,8 +525,6 @@ static void usb_host_handle_destroy(USBDevice *dev)
     qemu_remove_exit_notifier(&s->exit);
 }
 
-static int usb_linux_update_endp_table(USBHostDevice *s);
-
 /* iso data is special, we need to keep enough urbs in flight to make sure
    that the controller never runs out of them, otherwise the device will
    likely suffer a buffer underrun / overrun. */
@@ -732,7 +732,8 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
     }
 
     if (is_halted(s, p->devep)) {
-        ret = ioctl(s->fd, USBDEVFS_CLEAR_HALT, &ep);
+        unsigned int arg = ep;
+        ret = ioctl(s->fd, USBDEVFS_CLEAR_HALT, &arg);
         if (ret < 0) {
             perror("USBDEVFS_CLEAR_HALT");
             trace_usb_host_req_complete(s->bus_num, s->addr, USB_RET_NAK);
-- 
1.7.1

[Qemu-devel] [PATCH 4/7] usb-host: limit open retries

[Gerd Hoffmann]

Limit the number of times qemu tries to open host devices to three.
Reset error counter when the device goes away, after un-plugging and
re-plugging the device qemu will try again three times.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 usb-linux.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index fe7b9d9..b939cfc 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -132,6 +132,7 @@ typedef struct USBHostDevice {
     int addr;
     char port[MAX_PORTLEN];
     struct USBAutoFilter match;
+    int seen, errcount;
 
     QTAILQ_ENTRY(USBHostDevice) next;
 } USBHostDevice;
@@ -1769,6 +1770,10 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
             continue;
         }
         /* We got a match */
+        s->seen++;
+        if (s->errcount >= 3) {
+            return 0;
+        }
 
         /* Already attached ? */
         if (s->fd != -1) {
@@ -1776,7 +1781,9 @@ 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);
+        if (usb_host_open(s, bus_num, addr, port, product_name, speed) < 0) {
+            s->errcount++;
+        }
         break;
     }
 
@@ -1794,6 +1801,10 @@ static void usb_host_auto_check(void *unused)
         if (s->fd == -1) {
             unconnected++;
         }
+        if (s->seen == 0) {
+            s->errcount = 0;
+        }
+        s->seen = 0;
     }
 
     if (unconnected == 0) {
-- 
1.7.1

[Qemu-devel] [PATCH 5/7] usb-host: fix configuration tracking.

[Gerd Hoffmann]

It is perfectly fine to leave the usb device in unconfigured state
(USBHostDevice->configuration == 0).  Just do that and wait for the
guest to explicitly set a configuration.  This is closer to what real
hardware does and it also simplifies the device initialization.  There
is no need to figure how the device is configured on the host.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 usb-linux.c |   80 +++++++++++++---------------------------------------------
 1 files changed, 18 insertions(+), 62 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index b939cfc..3b0f53f 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -407,8 +407,11 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
     int interface, nb_interfaces;
     int ret, i;
 
-    if (configuration == 0) /* address state - ignore */
+    if (configuration == 0) { /* address state - ignore */
+        dev->ninterfaces   = 0;
+        dev->configuration = 0;
         return 1;
+    }
 
     DPRINTF("husb: claiming interfaces. config %d\n", configuration);
 
@@ -433,7 +436,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
 
         DPRINTF("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
 
-        if (configuration < 0 || configuration == dev->descr[i + 5]) {
+        if (configuration == dev->descr[i + 5]) {
             configuration = dev->descr[i + 5];
             break;
         }
@@ -835,6 +838,7 @@ static int usb_host_set_config(USBHostDevice *s, int config)
         return ctrl_error();
     }
     usb_host_claim_interfaces(s, config);
+    usb_linux_update_endp_table(s);
     return 0;
 }
 
@@ -941,51 +945,6 @@ static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
     return USB_RET_ASYNC;
 }
 
-static int usb_linux_get_configuration(USBHostDevice *s)
-{
-    uint8_t configuration;
-    struct usb_ctrltransfer ct;
-    int ret;
-
-    if (usb_fs_type == USB_FS_SYS) {
-        char device_name[32], line[1024];
-        int configuration;
-
-        sprintf(device_name, "%d-%s", s->bus_num, s->port);
-
-        if (!usb_host_read_file(line, sizeof(line), "bConfigurationValue",
-                                device_name)) {
-            goto usbdevfs;
-        }
-        if (sscanf(line, "%d", &configuration) != 1) {
-            goto usbdevfs;
-        }
-        return configuration;
-    }
-
-usbdevfs:
-    ct.bRequestType = USB_DIR_IN;
-    ct.bRequest = USB_REQ_GET_CONFIGURATION;
-    ct.wValue = 0;
-    ct.wIndex = 0;
-    ct.wLength = 1;
-    ct.data = &configuration;
-    ct.timeout = 50;
-
-    ret = ioctl(s->fd, USBDEVFS_CONTROL, &ct);
-    if (ret < 0) {
-        perror("usb_linux_get_configuration");
-        return -1;
-    }
-
-    /* in address state */
-    if (configuration == 0) {
-        return -1;
-    }
-
-    return configuration;
-}
-
 static uint8_t usb_linux_get_alt_setting(USBHostDevice *s,
     uint8_t configuration, uint8_t interface)
 {
@@ -1031,16 +990,16 @@ usbdevfs:
 static int usb_linux_update_endp_table(USBHostDevice *s)
 {
     uint8_t *descriptors;
-    uint8_t devep, type, configuration, alt_interface;
+    uint8_t devep, type, alt_interface;
     int interface, length, i;
 
     for (i = 0; i < MAX_ENDPOINTS; i++)
         s->endp_table[i].type = INVALID_EP_TYPE;
 
-    i = usb_linux_get_configuration(s);
-    if (i < 0)
-        return 1;
-    configuration = i;
+    if (s->configuration == 0) {
+        /* not configured yet -- leave all endpoints disabled */
+        return 0;
+    }
 
     /* get the desired configuration, interface, and endpoint descriptors
      * from device description */
@@ -1049,8 +1008,9 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
     i = 0;
 
     if (descriptors[i + 1] != USB_DT_CONFIG ||
-        descriptors[i + 5] != configuration) {
-        DPRINTF("invalid descriptor data - configuration\n");
+        descriptors[i + 5] != s->configuration) {
+        fprintf(stderr, "invalid descriptor data - configuration %d\n",
+                s->configuration);
         return 1;
     }
     i += descriptors[i];
@@ -1064,7 +1024,8 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
         }
 
         interface = descriptors[i + 2];
-        alt_interface = usb_linux_get_alt_setting(s, configuration, interface);
+        alt_interface = usb_linux_get_alt_setting(s, s->configuration,
+                                                  interface);
 
         /* the current interface descriptor is the active interface
          * and has endpoints */
@@ -1204,13 +1165,8 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
 #endif
 
 
-    /*
-     * Initial configuration is -1 which makes us claim first
-     * available config. We used to start with 1, which does not
-     * always work. I've seen devices where first config starts
-     * with 2.
-     */
-    if (!usb_host_claim_interfaces(dev, -1)) {
+    /* start unconfigured -- we'll wait for the guest to set a configuration */
+    if (!usb_host_claim_interfaces(dev, 0)) {
         goto fail;
     }
 
-- 
1.7.1

[Qemu-devel] [PATCH 6/7] usb-host: claim port

[Gerd Hoffmann]

When configured to pass through a specific host port (using hostbus and
hostport properties), try to claim the port if supported by the kernel.
That will avoid any kernel drivers binding to devices plugged into that
port.  It will not stop any userspace apps (such as usb_modeswitch)
access the device via usbfs though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 trace-events |    1 +
 usb-linux.c  |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/trace-events b/trace-events
index 1c7a624..6fe8f62 100644
--- a/trace-events
+++ b/trace-events
@@ -260,6 +260,7 @@ usb_host_urb_complete(int bus, int addr, void *aurb, int status, int length, int
 usb_host_ep_op(int bus, int addr, int ep, const char *op) "dev %d:%d, ep %d, %s"
 usb_host_reset(int bus, int addr) "dev %d:%d"
 usb_host_auto_scan(const char *state) "%s"
+usb_host_claim_port(int bus, int hub, int port) "bus %d, hub addr %d, port %d"
 
 # hw/scsi-bus.c
 disable scsi_req_alloc(int target, int lun, int tag) "target %d lun %d tag %d"
diff --git a/usb-linux.c b/usb-linux.c
index 3b0f53f..de6cb70 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -115,6 +115,7 @@ struct USBAutoFilter {
 typedef struct USBHostDevice {
     USBDevice dev;
     int       fd;
+    int       hub_fd;
 
     uint8_t   descr[8192];
     int       descr_len;
@@ -525,6 +526,9 @@ static void usb_host_handle_destroy(USBDevice *dev)
     USBHostDevice *s = (USBHostDevice *)dev;
 
     usb_host_close(s);
+    if (s->hub_fd != -1) {
+        close(s->hub_fd);
+    }
     QTAILQ_REMOVE(&hostdevs, s, next);
     qemu_remove_exit_notifier(&s->exit);
 }
@@ -1266,10 +1270,63 @@ static int usb_host_initfn(USBDevice *dev)
 
     dev->auto_attach = 0;
     s->fd = -1;
+    s->hub_fd = -1;
+
     QTAILQ_INSERT_TAIL(&hostdevs, s, next);
     s->exit.notify = usb_host_exit_notifier;
     qemu_add_exit_notifier(&s->exit);
     usb_host_auto_check(NULL);
+
+#ifdef USBDEVFS_CLAIM_PORT
+    if (s->match.bus_num != 0 && s->match.port != NULL) {
+        char *h, hub_name[64], line[1024];
+        int hub_addr, portnr, ret;
+
+        snprintf(hub_name, sizeof(hub_name), "%d-%s",
+                 s->match.bus_num, s->match.port);
+
+        /* try strip off last ".$portnr" to get hub */
+        h = strrchr(hub_name, '.');
+        if (h != NULL) {
+            portnr = atoi(h+1);
+            *h = '\0';
+        } else {
+            /* no dot in there -> it is the root hub */
+            snprintf(hub_name, sizeof(hub_name), "usb%d",
+                     s->match.bus_num);
+            portnr = atoi(s->match.port);
+        }
+
+        if (!usb_host_read_file(line, sizeof(line), "devnum",
+                                hub_name)) {
+            goto out;
+        }
+        if (sscanf(line, "%d", &hub_addr) != 1) {
+            goto out;
+        }
+
+        if (!usb_host_device_path) {
+            goto out;
+        }
+        snprintf(line, sizeof(line), "%s/%03d/%03d",
+                 usb_host_device_path, s->match.bus_num, hub_addr);
+        s->hub_fd = open(line, O_RDWR | O_NONBLOCK);
+        if (s->hub_fd < 0) {
+            goto out;
+        }
+
+        ret = ioctl(s->hub_fd, USBDEVFS_CLAIM_PORT, &portnr);
+        if (ret < 0) {
+            close(s->hub_fd);
+            s->hub_fd = -1;
+            goto out;
+        }
+
+        trace_usb_host_claim_port(s->match.bus_num, hub_addr, portnr);
+    }
+out:
+#endif
+
     return 0;
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 6/7] usb-host: claim port

[Erik Rull]

Hi Gerd,

is this related / the fix to my question regarding the recurring claimed 
messages? (See my mail dated 2011-08-21)

Best regards,

Erik



Gerd Hoffmann wrote:
> When configured to pass through a specific host port (using hostbus and
> hostport properties), try to claim the port if supported by the kernel.
> That will avoid any kernel drivers binding to devices plugged into that
> port.  It will not stop any userspace apps (such as usb_modeswitch)
> access the device via usbfs though.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   trace-events |    1 +
>   usb-linux.c  |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 58 insertions(+), 0 deletions(-)
>
> diff --git a/trace-events b/trace-events
> index 1c7a624..6fe8f62 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -260,6 +260,7 @@ usb_host_urb_complete(int bus, int addr, void *aurb, int status, int length, int
>   usb_host_ep_op(int bus, int addr, int ep, const char *op) "dev %d:%d, ep %d, %s"
>   usb_host_reset(int bus, int addr) "dev %d:%d"
>   usb_host_auto_scan(const char *state) "%s"
> +usb_host_claim_port(int bus, int hub, int port) "bus %d, hub addr %d, port %d"
>
>   # hw/scsi-bus.c
>   disable scsi_req_alloc(int target, int lun, int tag) "target %d lun %d tag %d"
> diff --git a/usb-linux.c b/usb-linux.c
> index 3b0f53f..de6cb70 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -115,6 +115,7 @@ struct USBAutoFilter {
>   typedef struct USBHostDevice {
>       USBDevice dev;
>       int       fd;
> +    int       hub_fd;
>
>       uint8_t   descr[8192];
>       int       descr_len;
> @@ -525,6 +526,9 @@ static void usb_host_handle_destroy(USBDevice *dev)
>       USBHostDevice *s = (USBHostDevice *)dev;
>
>       usb_host_close(s);
> +    if (s->hub_fd != -1) {
> +        close(s->hub_fd);
> +    }
>       QTAILQ_REMOVE(&hostdevs, s, next);
>       qemu_remove_exit_notifier(&s->exit);
>   }
> @@ -1266,10 +1270,63 @@ static int usb_host_initfn(USBDevice *dev)
>
>       dev->auto_attach = 0;
>       s->fd = -1;
> +    s->hub_fd = -1;
> +
>       QTAILQ_INSERT_TAIL(&hostdevs, s, next);
>       s->exit.notify = usb_host_exit_notifier;
>       qemu_add_exit_notifier(&s->exit);
>       usb_host_auto_check(NULL);
> +
> +#ifdef USBDEVFS_CLAIM_PORT
> +    if (s->match.bus_num != 0&&  s->match.port != NULL) {
> +        char *h, hub_name[64], line[1024];
> +        int hub_addr, portnr, ret;
> +
> +        snprintf(hub_name, sizeof(hub_name), "%d-%s",
> +                 s->match.bus_num, s->match.port);
> +
> +        /* try strip off last ".$portnr" to get hub */
> +        h = strrchr(hub_name, '.');
> +        if (h != NULL) {
> +            portnr = atoi(h+1);
> +            *h = '\0';
> +        } else {
> +            /* no dot in there ->  it is the root hub */
> +            snprintf(hub_name, sizeof(hub_name), "usb%d",
> +                     s->match.bus_num);
> +            portnr = atoi(s->match.port);
> +        }
> +
> +        if (!usb_host_read_file(line, sizeof(line), "devnum",
> +                                hub_name)) {
> +            goto out;
> +        }
> +        if (sscanf(line, "%d",&hub_addr) != 1) {
> +            goto out;
> +        }
> +
> +        if (!usb_host_device_path) {
> +            goto out;
> +        }
> +        snprintf(line, sizeof(line), "%s/%03d/%03d",
> +                 usb_host_device_path, s->match.bus_num, hub_addr);
> +        s->hub_fd = open(line, O_RDWR | O_NONBLOCK);
> +        if (s->hub_fd<  0) {
> +            goto out;
> +        }
> +
> +        ret = ioctl(s->hub_fd, USBDEVFS_CLAIM_PORT,&portnr);
> +        if (ret<  0) {
> +            close(s->hub_fd);
> +            s->hub_fd = -1;
> +            goto out;
> +        }
> +
> +        trace_usb_host_claim_port(s->match.bus_num, hub_addr, portnr);
> +    }
> +out:
> +#endif
> +
>       return 0;
>   }
>

Re: [Qemu-devel] [PATCH 6/7] usb-host: claim port

[Gerd Hoffmann]

On 08/25/11 18:03, Erik Rull wrote:
> Hi Gerd,
>
> is this related / the fix to my question regarding the recurring claimed
> messages? (See my mail dated 2011-08-21)

No, but patches #3 + #5 hopefully help there.

cheers,
   Gerd

[Qemu-devel] [PATCH 7/7] usb: fix use after free

[Gerd Hoffmann]

The ->complete() callback might have released the USBPacket (uhci
actually does), so we must not touch it after the callback returns.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/usb.c b/hw/usb.c
index 685e775..a091e4e 100644
--- a/hw/usb.c
+++ b/hw/usb.c
@@ -338,8 +338,8 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
 {
     /* Note: p->owner != dev is possible in case dev is a hub */
     assert(p->owner != NULL);
-    dev->port->ops->complete(dev->port, p);
     p->owner = NULL;
+    dev->port->ops->complete(dev->port, p);
 }
 
 /* Cancel an active packet.  The packed must have been deferred by
-- 
1.7.1