From da9fbe76a0639c529f9678aabcc052dfe4cd9cc4 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 11 Jul 2012 12:21:23 +0200 Subject: [PATCH 01/11] fix info qtree indention Without the patch bus properties are are not in line with the other properties: [ ... ] dev: fw_cfg, id "" ctl_iobase = 0x510 data_iobase = 0x511 irq 0 mmio ffffffffffffffff/0000000000000002 mmio ffffffffffffffff/0000000000000001 [ ... ] With the patch applied everything is lined up properly: [ ... ] dev: fw_cfg, id "" ctl_iobase = 0x510 data_iobase = 0x511 irq 0 mmio ffffffffffffffff/0000000000000002 mmio ffffffffffffffff/0000000000000001 [ ... ] Needed to make the autotest qtree parser happy. Signed-off-by: Gerd Hoffmann --- hw/qdev-monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index 018b386782..33b7f79a94 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -543,7 +543,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent) qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent); class = object_class_get_parent(class); } while (class != object_class_by_name(TYPE_DEVICE)); - bus_print_dev(dev->parent_bus, mon, dev, indent + 2); + bus_print_dev(dev->parent_bus, mon, dev, indent); QLIST_FOREACH(child, &dev->child_bus, sibling) { qbus_print(mon, child, indent); } From 0132b4b6595423c92f54d7e0b172b5d73aaa8375 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Fri, 17 Aug 2012 15:24:49 +0200 Subject: [PATCH 02/11] usb: Halt ep queue en cancel pending packets on a packet error For controllers which queue up more then 1 packet at a time, we must halt the ep queue, and inside the controller code cancel all pending packets on an error. There are multiple reasons for this: 1) Guests expect the controllers to halt ep queues on error, so that they get the opportunity to cancel transfers which the scheduled after the failing one, before processing continues 2) Not cancelling queued up packets after a failed transfer also messes up the controller state machine, in the case of EHCI causing the following assert to trigger: "assert(p->qtdaddr == q->qtdaddr)" at hcd-ehci.c:2075 3) For bulk endpoints with pipelining enabled (redirection to a real USB device), we must cancel all the transfers after this a failed one so that: a) If they've completed already, they are not processed further causing more stalls to be reported, originating from the same failed transfer b) If still in flight, they are cancelled before the guest does a clear stall, otherwise the guest and device can loose sync! Note this patch only touches the ehci and uhci controller changes, since AFAIK no other controllers actually queue up multiple transfer. If I'm wrong on this other controllers need to be updated too! Also note that this patch was heavily tested with the ehci code, where I had a reproducer for a device causing a transfer to fail. The uhci code is not tested with actually failing transfers and could do with a thorough review! Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb.h | 1 + hw/usb/core.c | 35 ++++++++++++++++++++++++++++------- hw/usb/hcd-ehci.c | 13 +++++++++++++ hw/usb/hcd-uhci.c | 16 ++++++++++++++++ 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/hw/usb.h b/hw/usb.h index 432ccae57f..e5744773bd 100644 --- a/hw/usb.h +++ b/hw/usb.h @@ -179,6 +179,7 @@ struct USBEndpoint { uint8_t ifnum; int max_packet_size; bool pipeline; + bool halted; USBDevice *dev; QTAILQ_HEAD(, USBPacket) queue; }; diff --git a/hw/usb/core.c b/hw/usb/core.c index c7e5bc047f..28b840e52d 100644 --- a/hw/usb/core.c +++ b/hw/usb/core.c @@ -382,12 +382,23 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p) usb_packet_check_state(p, USB_PACKET_SETUP); assert(p->ep != NULL); + /* Submitting a new packet clears halt */ + if (p->ep->halted) { + assert(QTAILQ_EMPTY(&p->ep->queue)); + p->ep->halted = false; + } + if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) { ret = usb_process_one(p); if (ret == USB_RET_ASYNC) { usb_packet_set_state(p, USB_PACKET_ASYNC); QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue); } else { + /* + * When pipelining is enabled usb-devices must always return async, + * otherwise packets can complete out of order! + */ + assert(!p->ep->pipeline); p->result = ret; usb_packet_set_state(p, USB_PACKET_COMPLETE); } @@ -399,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p) return ret; } +static void __usb_packet_complete(USBDevice *dev, USBPacket *p) +{ + USBEndpoint *ep = p->ep; + + assert(p->result != USB_RET_ASYNC && p->result != USB_RET_NAK); + + if (p->result < 0) { + ep->halted = true; + } + usb_packet_set_state(p, USB_PACKET_COMPLETE); + QTAILQ_REMOVE(&ep->queue, p, queue); + dev->port->ops->complete(dev->port, p); +} + /* Notify the controller that an async packet is complete. This should only be called for packets previously deferred by returning USB_RET_ASYNC from handle_packet. */ @@ -409,11 +434,9 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p) usb_packet_check_state(p, USB_PACKET_ASYNC); assert(QTAILQ_FIRST(&ep->queue) == p); - usb_packet_set_state(p, USB_PACKET_COMPLETE); - QTAILQ_REMOVE(&ep->queue, p, queue); - dev->port->ops->complete(dev->port, p); + __usb_packet_complete(dev, p); - while (!QTAILQ_EMPTY(&ep->queue)) { + while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) { p = QTAILQ_FIRST(&ep->queue); if (p->state == USB_PACKET_ASYNC) { break; @@ -425,9 +448,7 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p) break; } p->result = ret; - usb_packet_set_state(p, USB_PACKET_COMPLETE); - QTAILQ_REMOVE(&ep->queue, p, queue); - dev->port->ops->complete(dev->port, p); + __usb_packet_complete(ep->dev, p); } } diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 8b94b17723..8504a6a6b0 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -2138,6 +2138,19 @@ static int ehci_state_writeback(EHCIQueue *q) * bit is clear. */ if (q->qh.token & QTD_TOKEN_HALT) { + /* + * We should not do any further processing on a halted queue! + * This is esp. important for bulk endpoints with pipelining enabled + * (redirection to a real USB device), where we must cancel all the + * transfers after this one so that: + * 1) If they've completed already, they are not processed further + * causing more stalls, originating from the same failed transfer + * 2) If still in flight, they are cancelled before the guest does + * a clear stall, otherwise the guest and device can loose sync! + */ + while ((p = QTAILQ_FIRST(&q->packets)) != NULL) { + ehci_free_packet(p); + } ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH); again = 1; } else { diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index 1ace2a41da..898773414e 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -748,6 +748,22 @@ static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_ return TD_RESULT_COMPLETE; out: + /* + * We should not do any further processing on a queue with errors! + * This is esp. important for bulk endpoints with pipelining enabled + * (redirection to a real USB device), where we must cancel all the + * transfers after this one so that: + * 1) If they've completed already, they are not processed further + * causing more stalls, originating from the same failed transfer + * 2) If still in flight, they are cancelled before the guest does + * a clear stall, otherwise the guest and device can loose sync! + */ + while (!QTAILQ_EMPTY(&async->queue->asyncs)) { + UHCIAsync *as = QTAILQ_FIRST(&async->queue->asyncs); + uhci_async_unlink(as); + uhci_async_cancel(as); + } + switch(ret) { case USB_RET_STALL: td->ctrl |= TD_CTRL_STALL; From e983395d30d1d5bfa0ed3ae9c028c130f7c498cc Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 23 Aug 2012 13:30:13 +0200 Subject: [PATCH 03/11] usb: unique packet ids This patch adds IDs to usb packets. Those IDs are (a) supposed to be unique for the lifecycle of a packet (from packet setup until the packet is either completed or canceled) and (b) stable across migration. uhci, ohci, ehci and xhci use the guest physical address of the transfer descriptor for this. musb needs a different approach because there is no transfer descriptor. But musb also doesn't support pipelining, so we have never more than one packet per endpoint in flight. So we go create an ID based on endpoint and device address. Signed-off-by: Gerd Hoffmann --- hw/usb.h | 3 ++- hw/usb/core.c | 3 ++- hw/usb/hcd-ehci.c | 9 +++++---- hw/usb/hcd-musb.c | 3 ++- hw/usb/hcd-ohci.c | 4 ++-- hw/usb/hcd-uhci.c | 4 ++-- hw/usb/hcd-xhci.c | 2 +- 7 files changed, 16 insertions(+), 12 deletions(-) diff --git a/hw/usb.h b/hw/usb.h index e5744773bd..b8fceec895 100644 --- a/hw/usb.h +++ b/hw/usb.h @@ -332,6 +332,7 @@ typedef enum USBPacketState { struct USBPacket { /* Data fields for use by the driver. */ int pid; + uint64_t id; USBEndpoint *ep; QEMUIOVector iov; uint64_t parameter; /* control transfers */ @@ -344,7 +345,7 @@ struct USBPacket { void usb_packet_init(USBPacket *p); void usb_packet_set_state(USBPacket *p, USBPacketState state); void usb_packet_check_state(USBPacket *p, USBPacketState expected); -void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep); +void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id); void usb_packet_addbuf(USBPacket *p, void *ptr, size_t len); int usb_packet_map(USBPacket *p, QEMUSGList *sgl); void usb_packet_unmap(USBPacket *p, QEMUSGList *sgl); diff --git a/hw/usb/core.c b/hw/usb/core.c index 28b840e52d..2da38e7fd0 100644 --- a/hw/usb/core.c +++ b/hw/usb/core.c @@ -520,10 +520,11 @@ void usb_packet_set_state(USBPacket *p, USBPacketState state) p->state = state; } -void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep) +void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id) { assert(!usb_packet_is_inflight(p)); assert(p->iov.iov != NULL); + p->id = id; p->pid = pid; p->ep = ep; p->result = 0; diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 8504a6a6b0..f43d690eb5 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1530,7 +1530,7 @@ static int ehci_execute(EHCIPacket *p, const char *action) endp = get_field(p->queue->qh.epchar, QH_EPCHAR_EP); ep = usb_ep_get(p->queue->dev, p->pid, endp); - usb_packet_setup(&p->packet, p->pid, ep); + usb_packet_setup(&p->packet, p->pid, ep, p->qtdaddr); usb_packet_map(&p->packet, &p->sgl); trace_usb_ehci_packet_action(p->queue, p, action); @@ -1552,7 +1552,8 @@ static int ehci_execute(EHCIPacket *p, const char *action) */ static int ehci_process_itd(EHCIState *ehci, - EHCIitd *itd) + EHCIitd *itd, + uint32_t addr) { USBDevice *dev; USBEndpoint *ep; @@ -1598,7 +1599,7 @@ static int ehci_process_itd(EHCIState *ehci, dev = ehci_find_device(ehci, devaddr); ep = usb_ep_get(dev, pid, endp); if (ep->type == USB_ENDPOINT_XFER_ISOC) { - usb_packet_setup(&ehci->ipacket, pid, ep); + usb_packet_setup(&ehci->ipacket, pid, ep, addr); usb_packet_map(&ehci->ipacket, &ehci->isgl); ret = usb_handle_packet(dev, &ehci->ipacket); assert(ret != USB_RET_ASYNC); @@ -1862,7 +1863,7 @@ static int ehci_state_fetchitd(EHCIState *ehci, int async) sizeof(EHCIitd) >> 2); ehci_trace_itd(ehci, entry, &itd); - if (ehci_process_itd(ehci, &itd) != 0) { + if (ehci_process_itd(ehci, &itd, entry) != 0) { return -1; } diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c index fa9385ee49..0bb5c7b19e 100644 --- a/hw/usb/hcd-musb.c +++ b/hw/usb/hcd-musb.c @@ -626,7 +626,8 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep, /* A wild guess on the FADDR semantics... */ dev = usb_find_device(&s->port, ep->faddr[idx]); uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf); - usb_packet_setup(&ep->packey[dir].p, pid, uep); + usb_packet_setup(&ep->packey[dir].p, pid, uep, + (dev->addr << 16) | (uep->nr << 8) | pid); usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len); ep->packey[dir].ep = ep; ep->packey[dir].dir = dir; diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 844e7ed166..c36184ae4f 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -812,7 +812,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, } else { dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA)); ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN)); - usb_packet_setup(&ohci->usb_packet, pid, ep); + usb_packet_setup(&ohci->usb_packet, pid, ep, addr); usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len); ret = usb_handle_packet(dev, &ohci->usb_packet); if (ret == USB_RET_ASYNC) { @@ -1011,7 +1011,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) } dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA)); ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN)); - usb_packet_setup(&ohci->usb_packet, pid, ep); + usb_packet_setup(&ohci->usb_packet, pid, ep, addr); usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen); ret = usb_handle_packet(dev, &ohci->usb_packet); #ifdef DEBUG_PACKET diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index 898773414e..b0db92145a 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -859,14 +859,14 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td, * for initial isochronous requests */ async->queue->valid = 32; - async->isoc = td->ctrl & TD_CTRL_IOS; + async->isoc = td->ctrl & TD_CTRL_IOS; max_len = ((td->token >> 21) + 1) & 0x7ff; pid = td->token & 0xff; dev = uhci_find_device(s, (td->token >> 8) & 0x7f); ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf); - usb_packet_setup(&async->packet, pid, ep); + usb_packet_setup(&async->packet, pid, ep, addr); qemu_sglist_add(&async->sgl, td->buffer, max_len); usb_packet_map(&async->packet, &async->sgl); diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 6c2ff024e0..3eb27fadb8 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -1392,7 +1392,7 @@ static int xhci_setup_packet(XHCITransfer *xfer, USBDevice *dev) dir = xfer->in_xfer ? USB_TOKEN_IN : USB_TOKEN_OUT; ep = usb_ep_get(dev, dir, xfer->epid >> 1); - usb_packet_setup(&xfer->packet, dir, ep); + usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr); usb_packet_addbuf(&xfer->packet, xfer->data, xfer->data_length); DPRINTF("xhci: setup packet pid 0x%x addr %d ep %d\n", xfer->packet.pid, dev->addr, ep->nr); From 7ce86aa1aafaa65e7d3e572873bdf37bdb896f49 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 28 Aug 2012 11:50:26 +0200 Subject: [PATCH 04/11] ehci: Fix NULL ptr deref when unplugging an USB dev with an iso stream active Signed-off-by: Hans de Goede --- hw/usb/hcd-ehci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index f43d690eb5..a3aea6debe 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1598,7 +1598,7 @@ static int ehci_process_itd(EHCIState *ehci, dev = ehci_find_device(ehci, devaddr); ep = usb_ep_get(dev, pid, endp); - if (ep->type == USB_ENDPOINT_XFER_ISOC) { + if (ep && ep->type == USB_ENDPOINT_XFER_ISOC) { usb_packet_setup(&ehci->ipacket, pid, ep, addr); usb_packet_map(&ehci->ipacket, &ehci->isgl); ret = usb_handle_packet(dev, &ehci->ipacket); From a1c3e4b839f8e7ec7f1792b8a11c63ca845aa021 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Thu, 30 Aug 2012 09:55:19 +0200 Subject: [PATCH 05/11] ehci: Schedule async-bh when IAAD bit gets set After the "ehci: Print a warning when a queue unexpectedly contains packets on cancel" commit. Under certain reproducable conditions I was getting the following message: "EHCI: Warning queue not empty on queue reset". After aprox. 8 hours of debugging I've finally found the cause. The Linux EHCI driver has an IAAD watchdog, to work around certain EHCI hardware sometimes not acknowledging the doorbell at all. This watchdog has a timeout of 10 ms, which is less then the time between 2 runs through the async schedule when async_stepdown is at its highest value. Thus the watchdog can trigger, after which Linux clears the IAAD bit and re-uses the QH. IOW we were not properly detecting the unlink of the qh, due to us missing (ignoring for more then 10 ms) the IAAD command, which triggered the warning. Signed-off-by: Hans de Goede --- hw/usb/hcd-ehci.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index a3aea6debe..3e10977492 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1194,6 +1194,15 @@ static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val) val &= ~USBCMD_FLS; } + if (val & USBCMD_IAAD) { + /* + * Process IAAD immediately, otherwise the Linux IAAD watchdog may + * trigger and re-use a qh without us seeing the unlink. + */ + s->async_stepdown = 0; + qemu_bh_schedule(s->async_bh); + } + if (((USBCMD_RUNSTOP | USBCMD_PSE | USBCMD_ASE) & val) != ((USBCMD_RUNSTOP | USBCMD_PSE | USBCMD_ASE) & s->usbcmd)) { if (s->pstate == EST_INACTIVE) { From 53dd6f7032ec4898ca8f95356df795a92cd27e09 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Thu, 16 Aug 2012 15:47:29 +0200 Subject: [PATCH 06/11] ehci: Remove unnecessary ehci_flush_qh call ehci_qh_do_overlay() already calls ehci_flush_qh() before it returns, calling it twice is useless. Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-ehci.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 3e10977492..923a949ea5 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1966,7 +1966,6 @@ static int ehci_state_fetchqtd(EHCIQueue *q) } if (p != NULL) { ehci_qh_do_overlay(q); - ehci_flush_qh(q); if (p->async == EHCI_ASYNC_INFLIGHT) { ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH); } else { From 574ef17191f5ec5a3cc4782c1f59dc5eb8279654 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Fri, 17 Aug 2012 11:39:17 +0200 Subject: [PATCH 07/11] ehci: simplify ehci_state_executing ehci_state_executing does not need to check for p->usb_status == USB_RET_ASYNC or USB_RET_PROCERR, since ehci_execute_complete already does a similar check and will trigger an assert if either value is encountered. USB_RET_ASYNC should never be the packet status when execute_complete runs for obvious reasons, and USB_RET_PROCERR is only used by ehci_state_execute / ehci_execute not by ehci_state_executing / ehci_execute_complete. Signed-off-by: Hans de Goede Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-ehci.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 923a949ea5..b6169ce968 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -2084,19 +2084,11 @@ out: static int ehci_state_executing(EHCIQueue *q) { EHCIPacket *p = QTAILQ_FIRST(&q->packets); - int again = 0; assert(p != NULL); assert(p->qtdaddr == q->qtdaddr); ehci_execute_complete(q); - if (p->usb_status == USB_RET_ASYNC) { - goto out; - } - if (p->usb_status == USB_RET_PROCERR) { - again = -1; - goto out; - } // 4.10.3 if (!q->async) { @@ -2114,11 +2106,8 @@ static int ehci_state_executing(EHCIQueue *q) ehci_set_state(q->ehci, q->async, EST_WRITEBACK); } - again = 1; - -out: ehci_flush_qh(q); - return again; + return 1; } From c7cdca3b853eed0dd521c43098b6d07bcce24fd1 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Tue, 21 Aug 2012 13:58:40 +0200 Subject: [PATCH 08/11] ehci: add ehci_cancel_queue() Factor out function to cancel all packets of a queue. No behavior change. Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-ehci.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index b6169ce968..30e5e8fa98 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -766,15 +766,27 @@ static EHCIQueue *ehci_alloc_queue(EHCIState *ehci, uint32_t addr, int async) return q; } +static void ehci_cancel_queue(EHCIQueue *q) +{ + EHCIPacket *p; + + p = QTAILQ_FIRST(&q->packets); + if (p == NULL) { + return; + } + + trace_usb_ehci_queue_action(q, "cancel"); + do { + ehci_free_packet(p); + } while ((p = QTAILQ_FIRST(&q->packets)) != NULL); +} + static void ehci_free_queue(EHCIQueue *q) { EHCIQueueHead *head = q->async ? &q->ehci->aqueues : &q->ehci->pqueues; - EHCIPacket *p; trace_usb_ehci_queue_action(q, "free"); - while ((p = QTAILQ_FIRST(&q->packets)) != NULL) { - ehci_free_packet(p); - } + ehci_cancel_queue(q); QTAILQ_REMOVE(head, q, next); g_free(q); } @@ -1796,9 +1808,7 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async) if (q->dev != NULL && q->dev->addr != devaddr) { if (!QTAILQ_EMPTY(&q->packets)) { /* should not happen (guest bug) */ - while ((p = QTAILQ_FIRST(&q->packets)) != NULL) { - ehci_free_packet(p); - } + ehci_cancel_queue(q); } q->dev = NULL; } @@ -1959,10 +1969,10 @@ static int ehci_state_fetchqtd(EHCIQueue *q) ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd); p = QTAILQ_FIRST(&q->packets); - while (p != NULL && p->qtdaddr != q->qtdaddr) { + if (p != NULL && p->qtdaddr != q->qtdaddr) { /* should not happen (guest bug) */ - ehci_free_packet(p); - p = QTAILQ_FIRST(&q->packets); + ehci_cancel_queue(q); + p = NULL; } if (p != NULL) { ehci_qh_do_overlay(q); From 287fd3f1dd0b2abbd69e58b402e5364b334e95bd Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Tue, 21 Aug 2012 14:03:09 +0200 Subject: [PATCH 09/11] ehci: handle TD deactivation of inflight packets Check the TDs of inflight packets, cancel packets in case the guest clears the active bit. Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-ehci.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 30e5e8fa98..eca1431bff 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1816,11 +1816,6 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async) q->dev = ehci_find_device(q->ehci, devaddr); } - if (p && p->async == EHCI_ASYNC_INFLIGHT) { - /* I/O still in progress -- skip queue */ - ehci_set_state(ehci, async, EST_HORIZONTALQH); - goto out; - } if (p && p->async == EHCI_ASYNC_FINISHED) { /* I/O finished -- continue processing queue */ trace_usb_ehci_packet_action(p->queue, p, "complete"); @@ -1969,28 +1964,41 @@ static int ehci_state_fetchqtd(EHCIQueue *q) ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd); p = QTAILQ_FIRST(&q->packets); - if (p != NULL && p->qtdaddr != q->qtdaddr) { - /* should not happen (guest bug) */ - ehci_cancel_queue(q); - p = NULL; - } if (p != NULL) { - ehci_qh_do_overlay(q); + if (p->qtdaddr != q->qtdaddr || + (!NLPTR_TBIT(p->qtd.next) && (p->qtd.next != qtd.next)) || + (!NLPTR_TBIT(p->qtd.altnext) && (p->qtd.altnext != qtd.altnext)) || + p->qtd.bufptr[0] != qtd.bufptr[0]) { + /* guest bug: guest updated active QH or qTD underneath us */ + ehci_cancel_queue(q); + p = NULL; + } else { + p->qtd = qtd; + ehci_qh_do_overlay(q); + } + } + + if (!(qtd.token & QTD_TOKEN_ACTIVE)) { + if (p != NULL) { + /* transfer canceled by guest (clear active) */ + ehci_cancel_queue(q); + p = NULL; + } + ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH); + again = 1; + } else if (p != NULL) { if (p->async == EHCI_ASYNC_INFLIGHT) { ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH); } else { ehci_set_state(q->ehci, q->async, EST_EXECUTING); } again = 1; - } else if (qtd.token & QTD_TOKEN_ACTIVE) { + } else { p = ehci_alloc_packet(q); p->qtdaddr = q->qtdaddr; p->qtd = qtd; ehci_set_state(q->ehci, q->async, EST_EXECUTE); again = 1; - } else { - ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH); - again = 1; } return again; From adf478342b11cf9f540baf1f387b669210d3bea1 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Thu, 30 Aug 2012 11:20:51 +0200 Subject: [PATCH 10/11] ehci: Fix interrupt endpoints no longer working One of the recent changes (likely the addition of queuing support) has broken interrupt endpoints, this patch fixes this. Signed-off-by: Hans de Goede --- hw/usb/hcd-ehci.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index eca1431bff..017342b56a 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1987,10 +1987,19 @@ static int ehci_state_fetchqtd(EHCIQueue *q) ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH); again = 1; } else if (p != NULL) { - if (p->async == EHCI_ASYNC_INFLIGHT) { + switch (p->async) { + case EHCI_ASYNC_NONE: + /* Previously nacked packet (likely interrupt ep) */ + ehci_set_state(q->ehci, q->async, EST_EXECUTE); + break; + case EHCI_ASYNC_INFLIGHT: + /* Unfinyshed async handled packet, go horizontal */ ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH); - } else { + break; + case EHCI_ASYNC_FINISHED: + /* Should never happen, as this case is caught by fetchqh */ ehci_set_state(q->ehci, q->async, EST_EXECUTING); + break; } again = 1; } else { From 347e40ffe61b7cc8d4565be476c20acd00611669 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Fri, 31 Aug 2012 14:34:19 +0200 Subject: [PATCH 11/11] uas: move transfer kickoff Kick next scsi transfer from request release callback instead of command completion callback, otherwise we might get stuck in case scsi_req_unref() doesn't release the request instantly due to someone else holding a reference too. Signed-off-by: Gerd Hoffmann --- hw/usb/dev-uas.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c index b13eeba565..5a0057a36b 100644 --- a/hw/usb/dev-uas.c +++ b/hw/usb/dev-uas.c @@ -424,6 +424,7 @@ static void usb_uas_scsi_free_request(SCSIBus *bus, void *priv) } QTAILQ_REMOVE(&uas->requests, req, next); g_free(req); + usb_uas_start_next_transfer(uas); } static UASRequest *usb_uas_find_request(UASDevice *uas, uint16_t tag) @@ -456,7 +457,6 @@ static void usb_uas_scsi_command_complete(SCSIRequest *r, uint32_t status, size_t resid) { UASRequest *req = r->hba_private; - UASDevice *uas = req->uas; trace_usb_uas_scsi_complete(req->uas->dev.addr, req->tag, status, resid); req->complete = true; @@ -465,7 +465,6 @@ static void usb_uas_scsi_command_complete(SCSIRequest *r, } usb_uas_queue_sense(req, status); scsi_req_unref(req->req); - usb_uas_start_next_transfer(uas); } static void usb_uas_scsi_request_cancelled(SCSIRequest *r)