All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PULL for-1.3 0/7] usb patch queue
@ 2012-11-21 13:59 Gerd Hoffmann
  2012-11-21 13:59 ` [Qemu-devel] [PATCH 1/7] uhci: Add a completions_only flag for async completions Gerd Hoffmann
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2012-11-21 13:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann

  Hi,

Here comes the usb patch queue with a set od bugfixes from Hans.

please pull,
  Gerd

The following changes since commit 1ccbc2851282564308f790753d7158487b6af8e2:

  qemu-sockets: Fix parsing of the inet option 'to'. (2012-11-21 12:07:59 +0400)

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

Hans de Goede (7):
      uhci: Add a completions_only flag for async completions
      uhci: Don't allow the guest to set port-enabled when there is no dev connected
      uhci: Fix double unlink
      usb-bt: Return NAK instead of STALL when interrupt ep has no data
      usb-smartcard-reader: Properly NAK interrupt eps when we've no events
      usb-redir: Split usb_handle_interrupt_data into separate in/out functions
      usb-redir: Don't handle interrupt output packets async

 hw/usb/dev-bluetooth.c        |   25 ++++++-
 hw/usb/dev-smartcard-reader.c |    2 +
 hw/usb/hcd-uhci.c             |   19 ++++--
 hw/usb/redirect.c             |  150 +++++++++++++++++++++-------------------
 4 files changed, 116 insertions(+), 80 deletions(-)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/7] uhci: Add a completions_only flag for async completions
  2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
@ 2012-11-21 13:59 ` Gerd Hoffmann
  2012-11-21 13:59 ` [Qemu-devel] [PATCH 2/7] uhci: Don't allow the guest to set port-enabled when there is no dev connected Gerd Hoffmann
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2012-11-21 13:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: Hans de Goede, Gerd Hoffmann

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

Add a completions_only flag, and set this when running process_frame for async
completion handling, this fixes 2 issues in a single patch:

1) It makes sure async completed packets get written to guest mem immediately,
even if all the bandwidth for the frame was consumed from the timer run
process_frame. This is necessary as delaying their writeback to the next frame
can cause the completion to get lost on migration.

2) The calling of process_frame from a bh on async completion causes iso
tds to get server more often they should, messing up usb sound class device
timing. By only processing completed packets, the iso tds get skipped fixing
this.

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 2838d21..ef32633 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -152,6 +152,7 @@ struct UHCIState {
     QEMUBH *bh;
     uint32_t frame_bytes;
     uint32_t frame_bandwidth;
+    bool completions_only;
     UHCIPort ports[NB_PORTS];
 
     /* Interrupts that should be raised at the end of the current frame.  */
@@ -891,6 +892,10 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
         goto done;
     }
 
+    if (s->completions_only) {
+        return TD_RESULT_ASYNC_CONT;
+    }
+
     /* Allocate new packet */
     if (q == NULL) {
         USBDevice *dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
@@ -960,9 +965,9 @@ static void uhci_async_complete(USBPort *port, USBPacket *packet)
     }
 
     async->done = 1;
-    if (s->frame_bytes < s->frame_bandwidth) {
-        qemu_bh_schedule(s->bh);
-    }
+    /* Force processing of this packet *now*, needed for migration */
+    s->completions_only = true;
+    qemu_bh_schedule(s->bh);
 }
 
 static int is_valid(uint32_t link)
@@ -1054,7 +1059,7 @@ static void uhci_process_frame(UHCIState *s)
     qhdb_reset(&qhdb);
 
     for (cnt = FRAME_MAX_LOOPS; is_valid(link) && cnt; cnt--) {
-        if (s->frame_bytes >= s->frame_bandwidth) {
+        if (!s->completions_only && s->frame_bytes >= s->frame_bandwidth) {
             /* We've reached the usb 1.1 bandwidth, which is
                1280 bytes/frame, stop processing */
             trace_usb_uhci_frame_stop_bandwidth();
@@ -1170,6 +1175,7 @@ static void uhci_frame_timer(void *opaque)
     /* prepare the timer for the next frame */
     s->expire_time += (get_ticks_per_sec() / FRAME_TIMER_FREQ);
     s->frame_bytes = 0;
+    s->completions_only = false;
     qemu_bh_cancel(s->bh);
 
     if (!(s->cmd & UHCI_CMD_RS)) {
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/7] uhci: Don't allow the guest to set port-enabled when there is no dev connected
  2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
  2012-11-21 13:59 ` [Qemu-devel] [PATCH 1/7] uhci: Add a completions_only flag for async completions Gerd Hoffmann
@ 2012-11-21 13:59 ` Gerd Hoffmann
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 3/7] uhci: Fix double unlink Gerd Hoffmann
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2012-11-21 13:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: Hans de Goede, Gerd Hoffmann

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

It is possible for device disconnect and the guest trying to reset the port
(because of USB xact errors prior to the disconnect getting signaled) to race,
when we hit this race, the guest will write the port-control register with its
pre-disconnect value + the reset bit set, after which we have a disconnected
device with its port-enabled bit set in its port-control register, which
is no good :)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-uhci.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index ef32633..078be2a 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -556,6 +556,10 @@ static void uhci_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
                 }
             }
             port->ctrl &= UHCI_PORT_READ_ONLY;
+            /* enabled may only be set if a device is connected */
+            if (!(port->ctrl & UHCI_PORT_CCS)) {
+                val &= ~UHCI_PORT_EN;
+            }
             port->ctrl |= (val & ~UHCI_PORT_READ_ONLY);
             /* some bits are reset when a '1' is written to them */
             port->ctrl &= ~(val & UHCI_PORT_WRITE_CLEAR);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 3/7] uhci: Fix double unlink
  2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
  2012-11-21 13:59 ` [Qemu-devel] [PATCH 1/7] uhci: Add a completions_only flag for async completions Gerd Hoffmann
  2012-11-21 13:59 ` [Qemu-devel] [PATCH 2/7] uhci: Don't allow the guest to set port-enabled when there is no dev connected Gerd Hoffmann
@ 2012-11-21 14:00 ` Gerd Hoffmann
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 4/7] usb-bt: Return NAK instead of STALL when interrupt ep has no data Gerd Hoffmann
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2012-11-21 14:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Hans de Goede, Gerd Hoffmann

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

uhci_async_cancel() already does a uhci_async_unlink().

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

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 078be2a..8e47803 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -963,7 +963,6 @@ static void uhci_async_complete(USBPort *port, USBPacket *packet)
     UHCIState *s = async->queue->uhci;
 
     if (packet->status == USB_RET_REMOVE_FROM_QUEUE) {
-        uhci_async_unlink(async);
         uhci_async_cancel(async);
         return;
     }
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 4/7] usb-bt: Return NAK instead of STALL when interrupt ep has no data
  2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
                   ` (2 preceding siblings ...)
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 3/7] uhci: Fix double unlink Gerd Hoffmann
@ 2012-11-21 14:00 ` Gerd Hoffmann
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 5/7] usb-smartcard-reader: Properly NAK interrupt eps when we've no events Gerd Hoffmann
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2012-11-21 14:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Hans de Goede, Gerd Hoffmann

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

I noticed this while making all devices with interrupt endpoints properly
do wakeup. While at it also add wakeup support.

Note that I've not tested this, but returning STALL for an interrupt ep
which has no data is cleary the wrong thing to do.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-bluetooth.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
index bfb96bf..39984f5 100644
--- a/hw/usb/dev-bluetooth.c
+++ b/hw/usb/dev-bluetooth.c
@@ -27,6 +27,7 @@
 struct USBBtState {
     USBDevice dev;
     struct HCIInfo *hci;
+    USBEndpoint *intr;
 
     int config;
 
@@ -290,10 +291,7 @@ static inline void usb_bt_fifo_dequeue(struct usb_hci_in_fifo_s *fifo,
 {
     int len;
 
-    if (likely(!fifo->len)) {
-        p->status = USB_RET_STALL;
-        return;
-    }
+    assert(fifo->len != 0);
 
     len = MIN(p->iov.size, fifo->fifo[fifo->start].len);
     usb_packet_copy(p, fifo->fifo[fifo->start].data, len);
@@ -422,14 +420,26 @@ static void usb_bt_handle_data(USBDevice *dev, USBPacket *p)
     case USB_TOKEN_IN:
         switch (p->ep->nr) {
         case USB_EVT_EP:
+            if (s->evt.len == 0) {
+                p->status = USB_RET_NAK;
+                break;
+            }
             usb_bt_fifo_dequeue(&s->evt, p);
             break;
 
         case USB_ACL_EP:
+            if (s->evt.len == 0) {
+                p->status = USB_RET_STALL;
+                break;
+            }
             usb_bt_fifo_dequeue(&s->acl, p);
             break;
 
         case USB_SCO_EP:
+            if (s->evt.len == 0) {
+                p->status = USB_RET_STALL;
+                break;
+            }
             usb_bt_fifo_dequeue(&s->sco, p);
             break;
 
@@ -467,6 +477,9 @@ static void usb_bt_out_hci_packet_event(void *opaque,
 {
     struct USBBtState *s = (struct USBBtState *) opaque;
 
+    if (s->evt.len == 0) {
+        usb_wakeup(s->intr);
+    }
     usb_bt_fifo_enqueue(&s->evt, data, len);
 }
 
@@ -489,8 +502,12 @@ static void usb_bt_handle_destroy(USBDevice *dev)
 
 static int usb_bt_initfn(USBDevice *dev)
 {
+    struct USBBtState *s = DO_UPCAST(struct USBBtState, dev, dev);
+
     usb_desc_create_serial(dev);
     usb_desc_init(dev);
+    s->intr = usb_ep_get(dev, USB_TOKEN_IN, USB_EVT_EP);
+
     return 0;
 }
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 5/7] usb-smartcard-reader: Properly NAK interrupt eps when we've no events
  2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
                   ` (3 preceding siblings ...)
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 4/7] usb-bt: Return NAK instead of STALL when interrupt ep has no data Gerd Hoffmann
@ 2012-11-21 14:00 ` Gerd Hoffmann
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 6/7] usb-redir: Split usb_handle_interrupt_data into separate in/out functions Gerd Hoffmann
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2012-11-21 14:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Hans de Goede, Alon Levy, Gerd Hoffmann

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

When we've no data to return from the interrupt endpoint, return NAK rather
then a 0 length packet.

CC: Alon Levy <alevy@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-smartcard-reader.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 190fcd6..de955b7 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1002,6 +1002,8 @@ static void ccid_handle_data(USBDevice *dev, USBPacket *p)
                         "handle_data: int_in: notify_slot_change %X, "
                         "requested len %zd\n",
                         s->bmSlotICCState, p->iov.size);
+            } else {
+                p->status = USB_RET_NAK;
             }
             break;
         default:
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 6/7] usb-redir: Split usb_handle_interrupt_data into separate in/out functions
  2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
                   ` (4 preceding siblings ...)
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 5/7] usb-smartcard-reader: Properly NAK interrupt eps when we've no events Gerd Hoffmann
@ 2012-11-21 14:00 ` Gerd Hoffmann
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 7/7] usb-redir: Don't handle interrupt output packets async Gerd Hoffmann
  2012-11-26 15:34 ` [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Anthony Liguori
  7 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2012-11-21 14:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Hans de Goede, Gerd Hoffmann

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

No functional changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/redirect.c |  138 +++++++++++++++++++++++++++-------------------------
 1 files changed, 72 insertions(+), 66 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 0c95e6b..66637a8 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -610,80 +610,82 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
     p->status = USB_RET_ASYNC;
 }
 
-static void usbredir_handle_interrupt_data(USBRedirDevice *dev,
-                                           USBPacket *p, uint8_t ep)
+static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
+                                              USBPacket *p, uint8_t ep)
 {
-    if (ep & USB_DIR_IN) {
-        /* Input interrupt endpoint, buffered packet input */
-        struct buf_packet *intp;
-        int status, len;
-
-        if (!dev->endpoint[EP2I(ep)].interrupt_started &&
-                !dev->endpoint[EP2I(ep)].interrupt_error) {
-            struct usb_redir_start_interrupt_receiving_header start_int = {
-                .endpoint = ep,
-            };
-            /* No id, we look at the ep when receiving a status back */
-            usbredirparser_send_start_interrupt_receiving(dev->parser, 0,
-                                                          &start_int);
-            usbredirparser_do_write(dev->parser);
-            DPRINTF("interrupt recv started ep %02X\n", ep);
-            dev->endpoint[EP2I(ep)].interrupt_started = 1;
-            /* We don't really want to drop interrupt packets ever, but
-               having some upper limit to how much we buffer is good. */
-            dev->endpoint[EP2I(ep)].bufpq_target_size = 1000;
-            dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
-        }
+    /* Input interrupt endpoint, buffered packet input */
+    struct buf_packet *intp;
+    int status, len;
 
-        intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
-        if (intp == NULL) {
-            DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
-            /* Check interrupt_error for stream errors */
-            status = dev->endpoint[EP2I(ep)].interrupt_error;
-            dev->endpoint[EP2I(ep)].interrupt_error = 0;
-            if (status) {
-                usbredir_handle_status(dev, p, status);
-            } else {
-                p->status = USB_RET_NAK;
-            }
-            return;
-        }
-        DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
-                intp->status, intp->len);
+    if (!dev->endpoint[EP2I(ep)].interrupt_started &&
+            !dev->endpoint[EP2I(ep)].interrupt_error) {
+        struct usb_redir_start_interrupt_receiving_header start_int = {
+            .endpoint = ep,
+        };
+        /* No id, we look at the ep when receiving a status back */
+        usbredirparser_send_start_interrupt_receiving(dev->parser, 0,
+                                                      &start_int);
+        usbredirparser_do_write(dev->parser);
+        DPRINTF("interrupt recv started ep %02X\n", ep);
+        dev->endpoint[EP2I(ep)].interrupt_started = 1;
+        /* We don't really want to drop interrupt packets ever, but
+           having some upper limit to how much we buffer is good. */
+        dev->endpoint[EP2I(ep)].bufpq_target_size = 1000;
+        dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
+    }
 
-        status = intp->status;
-        len = intp->len;
-        if (len > p->iov.size) {
-            ERROR("received int data is larger then packet ep %02X\n", ep);
-            len = p->iov.size;
-            status = usb_redir_babble;
+    intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
+    if (intp == NULL) {
+        DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
+        /* Check interrupt_error for stream errors */
+        status = dev->endpoint[EP2I(ep)].interrupt_error;
+        dev->endpoint[EP2I(ep)].interrupt_error = 0;
+        if (status) {
+            usbredir_handle_status(dev, p, status);
+        } else {
+            p->status = USB_RET_NAK;
         }
-        usb_packet_copy(p, intp->data, len);
-        bufp_free(dev, intp, ep);
-        usbredir_handle_status(dev, p, status);
-    } else {
-        /* Output interrupt endpoint, normal async operation */
-        struct usb_redir_interrupt_packet_header interrupt_packet;
-        uint8_t buf[p->iov.size];
+        return;
+    }
+    DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
+            intp->status, intp->len);
 
-        DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
-                p->iov.size, p->id);
+    status = intp->status;
+    len = intp->len;
+    if (len > p->iov.size) {
+        ERROR("received int data is larger then packet ep %02X\n", ep);
+        len = p->iov.size;
+        status = usb_redir_babble;
+    }
+    usb_packet_copy(p, intp->data, len);
+    bufp_free(dev, intp, ep);
+    usbredir_handle_status(dev, p, status);
+}
 
-        if (usbredir_already_in_flight(dev, p->id)) {
-            p->status = USB_RET_ASYNC;
-            return;
-        }
+static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
+                                               USBPacket *p, uint8_t ep)
+{
+    /* Output interrupt endpoint, normal async operation */
+    struct usb_redir_interrupt_packet_header interrupt_packet;
+    uint8_t buf[p->iov.size];
 
-        interrupt_packet.endpoint  = ep;
-        interrupt_packet.length    = p->iov.size;
+    DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
+            p->iov.size, p->id);
 
-        usb_packet_copy(p, buf, p->iov.size);
-        usbredir_log_data(dev, "interrupt data out:", buf, p->iov.size);
-        usbredirparser_send_interrupt_packet(dev->parser, p->id,
-                                        &interrupt_packet, buf, p->iov.size);
-        usbredirparser_do_write(dev->parser);
+    if (usbredir_already_in_flight(dev, p->id)) {
         p->status = USB_RET_ASYNC;
+        return;
     }
+
+    interrupt_packet.endpoint  = ep;
+    interrupt_packet.length    = p->iov.size;
+
+    usb_packet_copy(p, buf, p->iov.size);
+    usbredir_log_data(dev, "interrupt data out:", buf, p->iov.size);
+    usbredirparser_send_interrupt_packet(dev->parser, p->id,
+                                    &interrupt_packet, buf, p->iov.size);
+    usbredirparser_do_write(dev->parser);
+    p->status = USB_RET_ASYNC;
 }
 
 static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
@@ -729,7 +731,11 @@ static void usbredir_handle_data(USBDevice *udev, USBPacket *p)
         usbredir_handle_bulk_data(dev, p, ep);
         break;
     case USB_ENDPOINT_XFER_INT:
-        usbredir_handle_interrupt_data(dev, p, ep);
+        if (ep & USB_DIR_IN) {
+            usbredir_handle_interrupt_in_data(dev, p, ep);
+        } else {
+            usbredir_handle_interrupt_out_data(dev, p, ep);
+        }
         break;
     default:
         ERROR("handle_data ep %02X has unknown type %d\n", ep,
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 7/7] usb-redir: Don't handle interrupt output packets async
  2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
                   ` (5 preceding siblings ...)
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 6/7] usb-redir: Split usb_handle_interrupt_data into separate in/out functions Gerd Hoffmann
@ 2012-11-21 14:00 ` Gerd Hoffmann
  2012-11-26 15:34 ` [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Anthony Liguori
  7 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2012-11-21 14:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Hans de Goede, Gerd Hoffmann

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

Instead report them as successfully completed directly on submission, this
has 2 advantages:

1) This matches the timing of interrupt output packets on real hardware,
with the previous async handling, if an ep has an interval of say 500 ms,
then there would be 500+ ms between the submission and the guest seeing the
completion, as we wont do the write back until the qh gets polled again. And
in the mean time the guest may very well have timed out, as the guest can
reasonable expect a much quicker completion.

2) This fixes interrupt output packets potentially getting send twice
surrounding a migration. As we delay the writeback to guest memory until
the qh gets polled again, there is a window between completion and writeback
where migration can happen, in this case the destination will not know
about the completion, and it will execute the packet *again*

But it does also come with a disadvantage:

1) If the actual interrupt out to the real usb device fails, there is no
way to report this back to the guest.

This patch assumes however that interrupt outs in practice never fail, as
they are only used by specialized drivers, which are unlikely to issue illegal
requests (unlike general class drivers which often issue requests which some
devices don't implement). And that thus the advantages outway the disadvantage.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/redirect.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 66637a8..490c90f 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -662,21 +662,22 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
     usbredir_handle_status(dev, p, status);
 }
 
+/*
+ * Handle interrupt out data, the usbredir protocol expects us to do this
+ * async, so that it can report back a completion status. But guests will
+ * expect immediate completion for an interrupt endpoint, and handling this
+ * async causes migration issues. So we report success directly, counting
+ * on the fact that output interrupt packets normally always succeed.
+ */
 static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
                                                USBPacket *p, uint8_t ep)
 {
-    /* Output interrupt endpoint, normal async operation */
     struct usb_redir_interrupt_packet_header interrupt_packet;
     uint8_t buf[p->iov.size];
 
     DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
             p->iov.size, p->id);
 
-    if (usbredir_already_in_flight(dev, p->id)) {
-        p->status = USB_RET_ASYNC;
-        return;
-    }
-
     interrupt_packet.endpoint  = ep;
     interrupt_packet.length    = p->iov.size;
 
@@ -685,7 +686,6 @@ static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
     usbredirparser_send_interrupt_packet(dev->parser, p->id,
                                     &interrupt_packet, buf, p->iov.size);
     usbredirparser_do_write(dev->parser);
-    p->status = USB_RET_ASYNC;
 }
 
 static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
@@ -1647,11 +1647,13 @@ static void usbredir_interrupt_packet(void *priv, uint64_t id,
         /* bufp_alloc also adds the packet to the ep queue */
         bufp_alloc(dev, data, data_len, interrupt_packet->status, ep);
     } else {
-        USBPacket *p = usbredir_find_packet_by_id(dev, ep, id);
-        if (p) {
-            usbredir_handle_status(dev, p, interrupt_packet->status);
-            p->actual_length = interrupt_packet->length;
-            usb_packet_complete(&dev->dev, p);
+        /*
+         * We report output interrupt packets as completed directly upon
+         * submission, so all we can do here if one failed is warn.
+         */
+        if (interrupt_packet->status) {
+            WARNING("interrupt output failed status %d ep %02X id %"PRIu64"\n",
+                    interrupt_packet->status, ep, id);
         }
     }
 }
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PULL for-1.3 0/7] usb patch queue
  2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
                   ` (6 preceding siblings ...)
  2012-11-21 14:00 ` [Qemu-devel] [PATCH 7/7] usb-redir: Don't handle interrupt output packets async Gerd Hoffmann
@ 2012-11-26 15:34 ` Anthony Liguori
  7 siblings, 0 replies; 9+ messages in thread
From: Anthony Liguori @ 2012-11-26 15:34 UTC (permalink / raw)
  To: Gerd Hoffmann, qemu-devel

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
> Here comes the usb patch queue with a set od bugfixes from Hans.
>
> please pull,
>   Gerd
>
> The following changes since commit 1ccbc2851282564308f790753d7158487b6af8e2:
>
>   qemu-sockets: Fix parsing of the inet option 'to'. (2012-11-21 12:07:59 +0400)

Pulled. Thanks.

Regards,

Anthony Liguori

>
> are available in the git repository at:
>   git://git.kraxel.org/qemu usb.72
>
> Hans de Goede (7):
>       uhci: Add a completions_only flag for async completions
>       uhci: Don't allow the guest to set port-enabled when there is no dev connected
>       uhci: Fix double unlink
>       usb-bt: Return NAK instead of STALL when interrupt ep has no data
>       usb-smartcard-reader: Properly NAK interrupt eps when we've no events
>       usb-redir: Split usb_handle_interrupt_data into separate in/out functions
>       usb-redir: Don't handle interrupt output packets async
>
>  hw/usb/dev-bluetooth.c        |   25 ++++++-
>  hw/usb/dev-smartcard-reader.c |    2 +
>  hw/usb/hcd-uhci.c             |   19 ++++--
>  hw/usb/redirect.c             |  150 +++++++++++++++++++++-------------------
>  4 files changed, 116 insertions(+), 80 deletions(-)

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-11-26 15:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-21 13:59 [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Gerd Hoffmann
2012-11-21 13:59 ` [Qemu-devel] [PATCH 1/7] uhci: Add a completions_only flag for async completions Gerd Hoffmann
2012-11-21 13:59 ` [Qemu-devel] [PATCH 2/7] uhci: Don't allow the guest to set port-enabled when there is no dev connected Gerd Hoffmann
2012-11-21 14:00 ` [Qemu-devel] [PATCH 3/7] uhci: Fix double unlink Gerd Hoffmann
2012-11-21 14:00 ` [Qemu-devel] [PATCH 4/7] usb-bt: Return NAK instead of STALL when interrupt ep has no data Gerd Hoffmann
2012-11-21 14:00 ` [Qemu-devel] [PATCH 5/7] usb-smartcard-reader: Properly NAK interrupt eps when we've no events Gerd Hoffmann
2012-11-21 14:00 ` [Qemu-devel] [PATCH 6/7] usb-redir: Split usb_handle_interrupt_data into separate in/out functions Gerd Hoffmann
2012-11-21 14:00 ` [Qemu-devel] [PATCH 7/7] usb-redir: Don't handle interrupt output packets async Gerd Hoffmann
2012-11-26 15:34 ` [Qemu-devel] [PULL for-1.3 0/7] usb patch queue Anthony Liguori

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.