Merge remote-tracking branch 'kraxel/usb.72' into staging

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

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

4 files changed, 116 insertions, 80 deletions

diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
index bfb96bf9f0..39984f53eb 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;
 }
 
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 190fcd62d4..de955b709f 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:
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 2838d21644..8e478030ad 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.  */
@@ -555,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);
@@ -891,6 +896,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);
@@ -954,15 +963,14 @@ 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;
     }
 
     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 +1062,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 +1178,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)) {
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 0c95e6b05e..490c90fae1 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;
+    /* 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;
-        }
+    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;
+    }
 
-        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;
+    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;
         }
-        DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
-                intp->status, intp->len);
+        return;
+    }
+    DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
+            intp->status, intp->len);
 
-        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);
-    } else {
-        /* Output interrupt endpoint, normal async operation */
-        struct usb_redir_interrupt_packet_header interrupt_packet;
-        uint8_t buf[p->iov.size];
+    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);
+}
 
-        DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
-                p->iov.size, p->id);
+/*
+ * 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)
+{
+    struct usb_redir_interrupt_packet_header interrupt_packet;
+    uint8_t buf[p->iov.size];
 
-        if (usbredir_already_in_flight(dev, p->id)) {
-            p->status = USB_RET_ASYNC;
-            return;
-        }
+    DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
+            p->iov.size, p->id);
 
-        interrupt_packet.endpoint  = ep;
-        interrupt_packet.length    = p->iov.size;
+    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;
-    }
+    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);
 }
 
 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,
@@ -1641,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);
         }
     }
 }