[Qemu-devel] [USB Fixes for 1.3] Various uhci fixes

[Qemu-devel] [USB Fixes for 1.3] Various uhci fixes

[Hans de Goede]

Here are 3 uhci bugfix patches for 1.3

Regards,

Hans

[Qemu-devel] [PATCH 1/3] uhci: Add a completions_only flag for async completions

[Hans de Goede]

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>
---
 hw/usb/hcd-uhci.c | 14 ++++++++++----
 1 file 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.8.0

[Qemu-devel] [PATCH 2/3] uhci: Don't allow the guest to set port-enabled when there is no dev connected

[Hans de Goede]

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>
---
 hw/usb/hcd-uhci.c | 4 ++++
 1 file changed, 4 insertions(+)

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.8.0

[Qemu-devel] [PATCH 3/3] uhci: Fix double unlink

[Hans de Goede]

uhci_async_cancel() already does a uhci_async_unlink().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-uhci.c | 1 -
 1 file changed, 1 deletion(-)

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.8.0