[Qemu-devel] [PULL 1.0] usb fixes

[Qemu-devel] [PULL 1.0] usb fixes

[Gerd Hoffmann]

  Hi,

A few more usb bugfixes.  Two are pretty serious error handling issues
(patches 1+2) which can lead to use-after-free.  The other ones are
minor but still nice to have and simple enougth that the risk to break
something is low.

please pull,
  Gerd

The following changes since commit 40897c9c160393df922dfdb59cfa210048d3071d:

  Update version for 1.0-rc3 release (2011-11-21 15:05:59 -0600)

are available in the git repository at:
  git://git.kraxel.org/qemu usb.32

Gerd Hoffmann (7):
      usb: make usb_create_simple catch and pass up errors.
      usb: fix usb_qdev_init error handling.
      usb-hub: wakeup on detach too.
      usb-hub: implement reset
      usb-ehci: codestyle fixups
      usb-ehci: add register names
      usb-host: add usb_host_do_reset function.

 hw/usb-bt.c   |    3 ++
 hw/usb-bus.c  |   29 +++++++++++++++++++++++----
 hw/usb-ehci.c |   58 +++++++++++++++++++++++++++++---------------------------
 hw/usb-hub.c  |   22 ++++++++++++++++++--
 usb-linux.c   |   25 +++++++++++++++++++++--
 5 files changed, 98 insertions(+), 39 deletions(-)

[Qemu-devel] [PATCH 1/7] usb: make usb_create_simple catch and pass up errors.

[Gerd Hoffmann]

Use qdev_init() instead of qdev_init_nofail(), usb device initialization
can fail, most common case being port and device speed mismatch.  Handle
failures correctly and pass up NULL pointers then.

Also fixup usb_create_simple() callers (only one was buggy) to properly
check for NULL pointers before referncing the usb_create_simple() return
value.

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

diff --git a/hw/usb-bt.c b/hw/usb-bt.c
index 529fa33..f30eec1 100644
--- a/hw/usb-bt.c
+++ b/hw/usb-bt.c
@@ -528,6 +528,9 @@ USBDevice *usb_bt_init(HCIInfo *hci)
     if (!hci)
         return NULL;
     dev = usb_create_simple(NULL /* FIXME */, "usb-bt-dongle");
+    if (!dev) {
+        return NULL;
+    }
     s = DO_UPCAST(struct USBBtState, dev, dev);
     s->dev.opaque = s;
 
diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 93f640d..f8b9807 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -139,10 +139,17 @@ USBDevice *usb_create(USBBus *bus, const char *name)
 USBDevice *usb_create_simple(USBBus *bus, const char *name)
 {
     USBDevice *dev = usb_create(bus, name);
+    int rc;
+
     if (!dev) {
-        hw_error("Failed to create USB device '%s'\n", name);
+        error_report("Failed to create USB device '%s'\n", name);
+        return NULL;
+    }
+    rc = qdev_init(&dev->qdev);
+    if (rc < 0) {
+        error_report("Failed to initialize USB device '%s'\n", name);
+        return NULL;
     }
-    qdev_init_nofail(&dev->qdev);
     return dev;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 2/7] usb: fix usb_qdev_init error handling.

[Gerd Hoffmann]

qdev doesn't call the ->exit callback on ->init failures, so we have to
take care ourself that we cleanup property on errors.

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

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index f8b9807..8cafb76 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -9,6 +9,7 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
 
 static char *usb_get_dev_path(DeviceState *dev);
 static char *usb_get_fw_dev_path(DeviceState *qdev);
+static int usb_qdev_exit(DeviceState *qdev);
 
 static struct BusInfo usb_bus_info = {
     .name      = "USB",
@@ -75,12 +76,23 @@ static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
     dev->auto_attach = 1;
     QLIST_INIT(&dev->strings);
     rc = usb_claim_port(dev);
-    if (rc == 0) {
-        rc = dev->info->init(dev);
+    if (rc != 0) {
+        goto err;
     }
-    if (rc == 0 && dev->auto_attach) {
+    rc = dev->info->init(dev);
+    if (rc != 0) {
+        goto err;
+    }
+    if (dev->auto_attach) {
         rc = usb_device_attach(dev);
+        if (rc != 0) {
+            goto err;
+        }
     }
+    return 0;
+
+err:
+    usb_qdev_exit(qdev);
     return rc;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 3/7] usb-hub: wakeup on detach too.

[Gerd Hoffmann]

When detaching devices from the usb hub we must wakeup too,
otherwise the host misses the detach event.

Commit 4a33a9ea06f6fbb08d8311a7cfed72975344f9ab does the
same for device attach.

Found by hkran@linux.vnet.ibm.com

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

diff --git a/hw/usb-hub.c b/hw/usb-hub.c
index 3eb0f1a..5b48763 100644
--- a/hw/usb-hub.c
+++ b/hw/usb-hub.c
@@ -171,6 +171,8 @@ static void usb_hub_detach(USBPort *port1)
     USBHubState *s = port1->opaque;
     USBHubPort *port = &s->ports[port1->index];
 
+    usb_wakeup(&s->dev);
+
     /* Let upstream know the device on this port is gone */
     s->dev.port->ops->child_detach(s->dev.port, port1->dev);
 
-- 
1.7.1

[Qemu-devel] [PATCH 4/7] usb-hub: implement reset

[Gerd Hoffmann]

based on a patch from hkran@linux.vnet.ibm.com

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

diff --git a/hw/usb-hub.c b/hw/usb-hub.c
index 5b48763..e195937 100644
--- a/hw/usb-hub.c
+++ b/hw/usb-hub.c
@@ -222,7 +222,22 @@ static void usb_hub_complete(USBPort *port, USBPacket *packet)
 
 static void usb_hub_handle_reset(USBDevice *dev)
 {
-    /* XXX: do it */
+    USBHubState *s = DO_UPCAST(USBHubState, dev, dev);
+    USBHubPort *port;
+    int i;
+
+    for (i = 0; i < NUM_PORTS; i++) {
+        port = s->ports + i;
+        port->wPortStatus = PORT_STAT_POWER;
+        port->wPortChange = 0;
+        if (port->port.dev && port->port.dev->attached) {
+            port->wPortStatus |= PORT_STAT_CONNECTION;
+            port->wPortChange |= PORT_STAT_C_CONNECTION;
+            if (port->port.dev->speed == USB_SPEED_LOW) {
+                port->wPortStatus |= PORT_STAT_LOW_SPEED;
+            }
+        }
+    }
 }
 
 static int usb_hub_handle_control(USBDevice *dev, USBPacket *p,
@@ -497,9 +512,8 @@ static int usb_hub_initfn(USBDevice *dev)
                           &port->port, s, i, &usb_hub_port_ops,
                           USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
         usb_port_location(&port->port, dev->port, i+1);
-        port->wPortStatus = PORT_STAT_POWER;
-        port->wPortChange = 0;
     }
+    usb_hub_handle_reset(dev);
     return 0;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 5/7] usb-ehci: codestyle fixups

[Gerd Hoffmann]

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 3eea94d..2609aba 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -437,37 +437,37 @@ struct EHCIState {
     } while(0)
 
 static const char *ehci_state_names[] = {
-    [ EST_INACTIVE ]     = "INACTIVE",
-    [ EST_ACTIVE ]       = "ACTIVE",
-    [ EST_EXECUTING ]    = "EXECUTING",
-    [ EST_SLEEPING ]     = "SLEEPING",
-    [ EST_WAITLISTHEAD ] = "WAITLISTHEAD",
-    [ EST_FETCHENTRY ]   = "FETCH ENTRY",
-    [ EST_FETCHQH ]      = "FETCH QH",
-    [ EST_FETCHITD ]     = "FETCH ITD",
-    [ EST_ADVANCEQUEUE ] = "ADVANCEQUEUE",
-    [ EST_FETCHQTD ]     = "FETCH QTD",
-    [ EST_EXECUTE ]      = "EXECUTE",
-    [ EST_WRITEBACK ]    = "WRITEBACK",
-    [ EST_HORIZONTALQH ] = "HORIZONTALQH",
+    [EST_INACTIVE]     = "INACTIVE",
+    [EST_ACTIVE]       = "ACTIVE",
+    [EST_EXECUTING]    = "EXECUTING",
+    [EST_SLEEPING]     = "SLEEPING",
+    [EST_WAITLISTHEAD] = "WAITLISTHEAD",
+    [EST_FETCHENTRY]   = "FETCH ENTRY",
+    [EST_FETCHQH]      = "FETCH QH",
+    [EST_FETCHITD]     = "FETCH ITD",
+    [EST_ADVANCEQUEUE] = "ADVANCEQUEUE",
+    [EST_FETCHQTD]     = "FETCH QTD",
+    [EST_EXECUTE]      = "EXECUTE",
+    [EST_WRITEBACK]    = "WRITEBACK",
+    [EST_HORIZONTALQH] = "HORIZONTALQH",
 };
 
 static const char *ehci_mmio_names[] = {
-    [ CAPLENGTH ]        = "CAPLENGTH",
-    [ HCIVERSION ]       = "HCIVERSION",
-    [ HCSPARAMS ]        = "HCSPARAMS",
-    [ HCCPARAMS ]        = "HCCPARAMS",
-    [ USBCMD ]           = "USBCMD",
-    [ USBSTS ]           = "USBSTS",
-    [ USBINTR ]          = "USBINTR",
-    [ FRINDEX ]          = "FRINDEX",
-    [ PERIODICLISTBASE ] = "P-LIST BASE",
-    [ ASYNCLISTADDR ]    = "A-LIST ADDR",
-    [ PORTSC_BEGIN ]     = "PORTSC #0",
-    [ PORTSC_BEGIN + 4]  = "PORTSC #1",
-    [ PORTSC_BEGIN + 8]  = "PORTSC #2",
-    [ PORTSC_BEGIN + 12] = "PORTSC #3",
-    [ CONFIGFLAG ]       = "CONFIGFLAG",
+    [CAPLENGTH]         = "CAPLENGTH",
+    [HCIVERSION]        = "HCIVERSION",
+    [HCSPARAMS]         = "HCSPARAMS",
+    [HCCPARAMS]         = "HCCPARAMS",
+    [USBCMD]            = "USBCMD",
+    [USBSTS]            = "USBSTS",
+    [USBINTR]           = "USBINTR",
+    [FRINDEX]           = "FRINDEX",
+    [PERIODICLISTBASE]  = "P-LIST BASE",
+    [ASYNCLISTADDR]     = "A-LIST ADDR",
+    [PORTSC_BEGIN]      = "PORTSC #0",
+    [PORTSC_BEGIN + 4]  = "PORTSC #1",
+    [PORTSC_BEGIN + 8]  = "PORTSC #2",
+    [PORTSC_BEGIN + 12] = "PORTSC #3",
+    [CONFIGFLAG]        = "CONFIGFLAG",
 };
 
 static const char *nr2str(const char **n, size_t len, uint32_t nr)
-- 
1.7.1

[Qemu-devel] [PATCH 6/7] usb-ehci: add register names

[Gerd Hoffmann]

The mmio register name list only had the names for four port status
registers.  We emulate a EHCI adapter with six ports though, the last
two ones are listed as "unknown" in traces.  Fix it.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 2609aba..a946e1d 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -467,6 +467,8 @@ static const char *ehci_mmio_names[] = {
     [PORTSC_BEGIN + 4]  = "PORTSC #1",
     [PORTSC_BEGIN + 8]  = "PORTSC #2",
     [PORTSC_BEGIN + 12] = "PORTSC #3",
+    [PORTSC_BEGIN + 16] = "PORTSC #4",
+    [PORTSC_BEGIN + 20] = "PORTSC #5",
     [CONFIGFLAG]        = "CONFIGFLAG",
 };
 
-- 
1.7.1

[Qemu-devel] [PATCH 7/7] usb-host: add usb_host_do_reset function.

[Gerd Hoffmann]

Add a special function to reset the host usb device.  It tracks the time
needed by the USBDEVFS_RESET ioctl and prints a warning in case it needs
too long.  Usually it should be finished in 200 - 300 miliseconds.
Warning threshold is one second.

Intention is to help troubleshooting by indicating that the usb device
stopped responding even to a reset request and is possibly broken.

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

diff --git a/usb-linux.c b/usb-linux.c
index d4426ea..ab4c693 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -148,6 +148,25 @@ 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 int usb_host_do_reset(USBHostDevice *dev)
+{
+    struct timeval s, e;
+    uint32_t usecs;
+    int ret;
+
+    gettimeofday(&s, NULL);
+    ret = ioctl(dev->fd, USBDEVFS_RESET);
+    gettimeofday(&e, NULL);
+    usecs = (e.tv_sec  - s.tv_sec) * 1000000;
+    usecs += e.tv_usec - s.tv_usec;
+    if (usecs > 1000000) {
+        /* more than a second, something is fishy, broken usb device? */
+        fprintf(stderr, "husb: device %d:%d reset took %d.%06d seconds\n",
+                dev->bus_num, dev->addr, usecs / 1000000, usecs % 1000000);
+    }
+    return ret;
+}
+
 static struct endp_data *get_endp(USBHostDevice *s, int pid, int ep)
 {
     struct endp_data *eps = pid == USB_TOKEN_IN ? s->ep_in : s->ep_out;
@@ -606,7 +625,7 @@ static void usb_host_handle_reset(USBDevice *dev)
 
     trace_usb_host_reset(s->bus_num, s->addr);
 
-    ioctl(s->fd, USBDEVFS_RESET);
+    usb_host_do_reset(s);;
 
     usb_host_claim_interfaces(s, 0);
     usb_linux_update_endp_table(s);
@@ -1370,7 +1389,7 @@ static int usb_host_close(USBHostDevice *dev)
     if (dev->dev.attached) {
         usb_device_detach(&dev->dev);
     }
-    ioctl(dev->fd, USBDEVFS_RESET);
+    usb_host_do_reset(dev);
     close(dev->fd);
     dev->fd = -1;
     return 0;
@@ -1381,7 +1400,7 @@ static void usb_host_exit_notifier(struct Notifier *n, void *data)
     USBHostDevice *s = container_of(n, USBHostDevice, exit);
 
     if (s->fd != -1) {
-        ioctl(s->fd, USBDEVFS_RESET);
+        usb_host_do_reset(s);;
     }
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PULL 1.0] usb fixes

[Anthony Liguori]

On 11/23/2011 11:25 AM, Gerd Hoffmann wrote:
>    Hi,
>
> A few more usb bugfixes.  Two are pretty serious error handling issues
> (patches 1+2) which can lead to use-after-free.  The other ones are
> minor but still nice to have and simple enougth that the risk to break
> something is low.
>
> please pull,
>    Gerd
>
> The following changes since commit 40897c9c160393df922dfdb59cfa210048d3071d:
>
>    Update version for 1.0-rc3 release (2011-11-21 15:05:59 -0600)
>
> are available in the git repository at:
>    git://git.kraxel.org/qemu usb.32

Pulled.  Thanks.

Regards,

Anthony Liguori

>
> Gerd Hoffmann (7):
>        usb: make usb_create_simple catch and pass up errors.
>        usb: fix usb_qdev_init error handling.
>        usb-hub: wakeup on detach too.
>        usb-hub: implement reset
>        usb-ehci: codestyle fixups
>        usb-ehci: add register names
>        usb-host: add usb_host_do_reset function.
>
>   hw/usb-bt.c   |    3 ++
>   hw/usb-bus.c  |   29 +++++++++++++++++++++++----
>   hw/usb-ehci.c |   58 +++++++++++++++++++++++++++++---------------------------
>   hw/usb-hub.c  |   22 ++++++++++++++++++--
>   usb-linux.c   |   25 +++++++++++++++++++++--
>   5 files changed, 98 insertions(+), 39 deletions(-)
>
>