[Qemu-devel] [PULL for-1.7 0/7] usb fixes

[Qemu-devel] [PULL for-1.7 0/7] usb fixes

[Gerd Hoffmann]

  Hi,

Here comes a collection of bugfixes for xhci and usb-host,
mostly related to usb3 streams.

please pull,
  Gerd

The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:

  Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 10:03:24 -0700)

are available in the git repository at:


  git://git.kraxel.org/qemu usb.91

for you to fetch changes up to c90daa1c109348099088c1cc954c1e9f3392ae03:

  usb-hcd-xhci: Update endpoint context dequeue pointer for streams too (2013-10-22 16:28:49 +0200)

----------------------------------------------------------------
Hans de Goede (7):
      usb-host-libusb: Fix reset handling
      usb-host-libusb: Configuration 0 may be a valid configuration
      usb-host-libusb: Detach kernel drivers earlier
      usb-hcd-xhci: Remove unused sstreamsm member from XHCIStreamContext
      usb-hcd-xhci: Remove unused cancelled member from XHCITransfer
      usb-hcd-xhci: Report completion of active transfer with CC_STOPPED on ep stop
      usb-hcd-xhci: Update endpoint context dequeue pointer for streams too

 hw/usb/hcd-xhci.c    | 50 ++++++++++++++++++++++++++------------------------
 hw/usb/host-libusb.c | 26 +++++++++++++-------------
 2 files changed, 39 insertions(+), 37 deletions(-)

[Qemu-devel] [PATCH 1/7] usb-host-libusb: Fix reset handling

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

The guest will issue an initial device reset when the device is attached, but
since the current usb-host-libusb code only actually does the reset when
udev->configuration != 0, and on attach the device is not yet configured,
the reset gets ignored. This means that the device gets passed to the guest
in an unknown state, which is not good.

The udev->configuration check is there because of the release / claim
interfaces done around the libusb_device_reset call, but these are not
necessary. If interfaces are claimed when libusb_device_reset gets called
libusb will release + reclaim them itself.

The usb_host_ep_update call also is not necessary. If the reset succeeds the
original config and interface alt settings will be restored.

Last if the reset fails, that means the device has either disconnected or
morphed into an another device and has been completely re-enumerated,
so it is treated by the host as a new device and our handle is invalid,
so on reset failure we need to call usb_host_nodev().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/host-libusb.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 128955d..428c7c5 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1256,16 +1256,14 @@ static void usb_host_flush_ep_queue(USBDevice *dev, USBEndpoint *ep)
 static void usb_host_handle_reset(USBDevice *udev)
 {
     USBHostDevice *s = USB_HOST_DEVICE(udev);
+    int rc;
 
     trace_usb_host_reset(s->bus_num, s->addr);
 
-    if (udev->configuration == 0) {
-        return;
+    rc = libusb_reset_device(s->dh);
+    if (rc != 0) {
+        usb_host_nodev(s);
     }
-    usb_host_release_interfaces(s);
-    libusb_reset_device(s->dh);
-    usb_host_claim_interfaces(s, 0);
-    usb_host_ep_update(s);
 }
 
 /*
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/7] usb-host-libusb: Configuration 0 may be a valid configuration

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

Quoting from: linux/Documentation/ABI/stable/sysfs-bus-usb:

	Note that some devices, in violation of the USB spec, have a
	configuration with a value equal to 0. Writing 0 to
	bConfigurationValue for these devices will install that
	configuration, rather then unconfigure the device.

So don't compare the configuration value against 0 to check for unconfigured
devices, instead check for a LIBUSB_ERROR_NOT_FOUND return from
libusb_get_active_config_descriptor().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/host-libusb.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 428c7c5..35bae55 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -992,15 +992,14 @@ static int usb_host_claim_interfaces(USBHostDevice *s, int configuration)
     udev->ninterfaces   = 0;
     udev->configuration = 0;
 
-    if (configuration == 0) {
-        /* address state - ignore */
-        return USB_RET_SUCCESS;
-    }
-
     usb_host_detach_kernel(s);
 
     rc = libusb_get_active_config_descriptor(s->dev, &conf);
     if (rc != 0) {
+        if (rc == LIBUSB_ERROR_NOT_FOUND) {
+            /* address state - ignore */
+            return USB_RET_SUCCESS;
+        }
         return USB_RET_STALL;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/7] usb-host-libusb: Detach kernel drivers earlier

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

If we detach the kernel drivers on the first set_config, then they will
be still attached when the device gets its initial reset. Causing the drivers
to re-initialize the device after the reset, dirtying the device state.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/host-libusb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 35bae55..fd320cd 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -137,6 +137,7 @@ static QTAILQ_HEAD(, USBHostDevice) hostdevs =
 static void usb_host_auto_check(void *unused);
 static void usb_host_release_interfaces(USBHostDevice *s);
 static void usb_host_nodev(USBHostDevice *s);
+static void usb_host_detach_kernel(USBHostDevice *s);
 static void usb_host_attach_kernel(USBHostDevice *s);
 
 /* ------------------------------------------------------------------------ */
@@ -787,10 +788,13 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev)
         goto fail;
     }
 
-    libusb_get_device_descriptor(dev, &s->ddesc);
     s->dev     = dev;
     s->bus_num = bus_num;
     s->addr    = addr;
+
+    usb_host_detach_kernel(s);
+
+    libusb_get_device_descriptor(dev, &s->ddesc);
     usb_host_get_port(s->dev, s->port, sizeof(s->port));
 
     usb_ep_init(udev);
@@ -1051,7 +1055,6 @@ static void usb_host_set_config(USBHostDevice *s, int config, USBPacket *p)
     trace_usb_host_set_config(s->bus_num, s->addr, config);
 
     usb_host_release_interfaces(s);
-    usb_host_detach_kernel(s);
     rc = libusb_set_configuration(s->dh, config);
     if (rc != 0) {
         usb_host_libusb_error("libusb_set_configuration", rc);
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/7] usb-hcd-xhci: Remove unused sstreamsm member from XHCIStreamContext

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 469c24d..e078c50 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -374,7 +374,6 @@ struct XHCIStreamContext {
     dma_addr_t pctx;
     unsigned int sct;
     XHCIRing ring;
-    XHCIStreamContext *sstreams;
 };
 
 struct XHCIEPContext {
@@ -1133,7 +1132,6 @@ static void xhci_reset_streams(XHCIEPContext *epctx)
 
     for (i = 0; i < epctx->nr_pstreams; i++) {
         epctx->pstreams[i].sct = -1;
-        g_free(epctx->pstreams[i].sstreams);
     }
 }
 
@@ -1146,15 +1144,8 @@ static void xhci_alloc_streams(XHCIEPContext *epctx, dma_addr_t base)
 
 static void xhci_free_streams(XHCIEPContext *epctx)
 {
-    int i;
-
     assert(epctx->pstreams != NULL);
 
-    if (!epctx->lsa) {
-        for (i = 0; i < epctx->nr_pstreams; i++) {
-            g_free(epctx->pstreams[i].sstreams);
-        }
-    }
     g_free(epctx->pstreams);
     epctx->pstreams = NULL;
     epctx->nr_pstreams = 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] usb-hcd-xhci: Remove unused cancelled member from XHCITransfer

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

Since qemu's USB model is geared towards emulated devices cancellation
is instanteneous, so no need to wait for cancellation to complete, as
such there is no wait for cancellation code, and the cancelled bool
as well as the bogus comment about it can be removed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index e078c50..7cf89ce 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -346,7 +346,6 @@ typedef struct XHCITransfer {
     QEMUSGList sgl;
     bool running_async;
     bool running_retry;
-    bool cancelled;
     bool complete;
     bool int_req;
     unsigned int iso_pkts;
@@ -1310,8 +1309,6 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
     if (t->running_async) {
         usb_cancel_packet(&t->packet);
         t->running_async = 0;
-        t->cancelled = 1;
-        DPRINTF("xhci: cancelling transfer, waiting for it to complete\n");
         killed = 1;
     }
     if (t->running_retry) {
@@ -1728,14 +1725,12 @@ static int xhci_complete_packet(XHCITransfer *xfer)
         xfer->running_async = 1;
         xfer->running_retry = 0;
         xfer->complete = 0;
-        xfer->cancelled = 0;
         return 0;
     } else if (xfer->packet.status == USB_RET_NAK) {
         trace_usb_xhci_xfer_nak(xfer);
         xfer->running_async = 0;
         xfer->running_retry = 1;
         xfer->complete = 0;
-        xfer->cancelled = 0;
         return 0;
     } else {
         xfer->running_async = 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/7] usb-hcd-xhci: Report completion of active transfer with CC_STOPPED on ep stop

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

As we should per the XHCI spec "4.6.9 Stop Endpoint".

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 7cf89ce..0131151 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -505,6 +505,7 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
                          unsigned int epid, unsigned int streamid);
 static TRBCCode xhci_disable_ep(XHCIState *xhci, unsigned int slotid,
                                 unsigned int epid);
+static void xhci_xfer_report(XHCITransfer *xfer);
 static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v);
 static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v);
 static USBEndpoint *xhci_epid_to_usbep(XHCIState *xhci,
@@ -1302,10 +1303,15 @@ static TRBCCode xhci_enable_ep(XHCIState *xhci, unsigned int slotid,
     return CC_SUCCESS;
 }
 
-static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
+static int xhci_ep_nuke_one_xfer(XHCITransfer *t, TRBCCode report)
 {
     int killed = 0;
 
+    if (report && (t->running_async || t->running_retry)) {
+        t->status = report;
+        xhci_xfer_report(t);
+    }
+
     if (t->running_async) {
         usb_cancel_packet(&t->packet);
         t->running_async = 0;
@@ -1318,6 +1324,7 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
             timer_del(epctx->kick_timer);
         }
         t->running_retry = 0;
+        killed = 1;
     }
     if (t->trbs) {
         g_free(t->trbs);
@@ -1330,7 +1337,7 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
 }
 
 static int xhci_ep_nuke_xfers(XHCIState *xhci, unsigned int slotid,
-                               unsigned int epid)
+                               unsigned int epid, TRBCCode report)
 {
     XHCISlot *slot;
     XHCIEPContext *epctx;
@@ -1351,7 +1358,10 @@ static int xhci_ep_nuke_xfers(XHCIState *xhci, unsigned int slotid,
 
     xferi = epctx->next_xfer;
     for (i = 0; i < TD_QUEUE; i++) {
-        killed += xhci_ep_nuke_one_xfer(&epctx->transfers[xferi]);
+        killed += xhci_ep_nuke_one_xfer(&epctx->transfers[xferi], report);
+        if (killed) {
+            report = 0; /* Only report once */
+        }
         epctx->transfers[xferi].packet.ep = NULL;
         xferi = (xferi + 1) % TD_QUEUE;
     }
@@ -1381,7 +1391,7 @@ static TRBCCode xhci_disable_ep(XHCIState *xhci, unsigned int slotid,
         return CC_SUCCESS;
     }
 
-    xhci_ep_nuke_xfers(xhci, slotid, epid);
+    xhci_ep_nuke_xfers(xhci, slotid, epid, 0);
 
     epctx = slot->eps[epid-1];
 
@@ -1423,7 +1433,7 @@ static TRBCCode xhci_stop_ep(XHCIState *xhci, unsigned int slotid,
         return CC_EP_NOT_ENABLED_ERROR;
     }
 
-    if (xhci_ep_nuke_xfers(xhci, slotid, epid) > 0) {
+    if (xhci_ep_nuke_xfers(xhci, slotid, epid, CC_STOPPED) > 0) {
         fprintf(stderr, "xhci: FIXME: endpoint stopped w/ xfers running, "
                 "data might be lost\n");
     }
@@ -1468,7 +1478,7 @@ static TRBCCode xhci_reset_ep(XHCIState *xhci, unsigned int slotid,
         return CC_CONTEXT_STATE_ERROR;
     }
 
-    if (xhci_ep_nuke_xfers(xhci, slotid, epid) > 0) {
+    if (xhci_ep_nuke_xfers(xhci, slotid, epid, 0) > 0) {
         fprintf(stderr, "xhci: FIXME: endpoint reset w/ xfers running, "
                 "data might be lost\n");
     }
@@ -2461,7 +2471,7 @@ static void xhci_detach_slot(XHCIState *xhci, USBPort *uport)
 
     for (ep = 0; ep < 31; ep++) {
         if (xhci->slots[slot].eps[ep]) {
-            xhci_ep_nuke_xfers(xhci, slot+1, ep+1);
+            xhci_ep_nuke_xfers(xhci, slot + 1, ep + 1, 0);
         }
     }
     xhci->slots[slot].uport = NULL;
@@ -3276,7 +3286,7 @@ static void xhci_complete(USBPort *port, USBPacket *packet)
     XHCITransfer *xfer = container_of(packet, XHCITransfer, packet);
 
     if (packet->status == USB_RET_REMOVE_FROM_QUEUE) {
-        xhci_ep_nuke_one_xfer(xfer);
+        xhci_ep_nuke_one_xfer(xfer, 0);
         return;
     }
     xhci_complete_packet(xfer);
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/7] usb-hcd-xhci: Update endpoint context dequeue pointer for streams too

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

With streams the endpoint context dequeue pointer should point to the
dequeue value for the currently active stream.

At least Linux guests expect it to point to value set by an set_ep_dequeue
upon completion of the set_ep_dequeue (before kicking the ep).

Otherwise the Linux kernel will complain (and things won't work):

xhci_hcd 0000:00:05.0: Mismatch between completed Set TR Deq Ptr command & xHCI internal state.
xhci_hcd 0000:00:05.0: ep deq seg = ffff8800366f0880, deq ptr = ffff8800366ec010

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 0131151..fa27299 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1187,6 +1187,7 @@ static XHCIStreamContext *xhci_find_stream(XHCIEPContext *epctx,
 static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
                               XHCIStreamContext *sctx, uint32_t state)
 {
+    XHCIRing *ring = NULL;
     uint32_t ctx[5];
     uint32_t ctx2[2];
 
@@ -1197,6 +1198,7 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
     /* update ring dequeue ptr */
     if (epctx->nr_pstreams) {
         if (sctx != NULL) {
+            ring = &sctx->ring;
             xhci_dma_read_u32s(xhci, sctx->pctx, ctx2, sizeof(ctx2));
             ctx2[0] &= 0xe;
             ctx2[0] |= sctx->ring.dequeue | sctx->ring.ccs;
@@ -1204,8 +1206,12 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
             xhci_dma_write_u32s(xhci, sctx->pctx, ctx2, sizeof(ctx2));
         }
     } else {
-        ctx[2] = epctx->ring.dequeue | epctx->ring.ccs;
-        ctx[3] = (epctx->ring.dequeue >> 16) >> 16;
+        ring = &epctx->ring;
+    }
+    if (ring) {
+        ctx[2] = ring->dequeue | ring->ccs;
+        ctx[3] = (ring->dequeue >> 16) >> 16;
+
         DPRINTF("xhci: set epctx: " DMA_ADDR_FMT " state=%d dequeue=%08x%08x\n",
                 epctx->pctx, state, ctx[3], ctx[2]);
     }
-- 
1.8.3.1