From patchwork Fri Aug 17 13:24:49 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 178203 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 6B2A72C0092 for ; Fri, 17 Aug 2012 23:24:04 +1000 (EST) Received: from localhost ([::1]:44501 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2MWw-0001Gf-A2 for incoming@patchwork.ozlabs.org; Fri, 17 Aug 2012 09:24:02 -0400 Received: from eggs.gnu.org ([208.118.235.92]:60063) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2MWk-00019k-5M for qemu-devel@nongnu.org; Fri, 17 Aug 2012 09:23:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T2MWh-0000Hl-2x for qemu-devel@nongnu.org; Fri, 17 Aug 2012 09:23:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36946) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2MWg-0000Hb-RG for qemu-devel@nongnu.org; Fri, 17 Aug 2012 09:23:47 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7HDNkA0022095 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 17 Aug 2012 09:23:46 -0400 Received: from shalem.localdomain.com (vpn1-6-250.ams2.redhat.com [10.36.6.250]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q7HDNivS002952; Fri, 17 Aug 2012 09:23:45 -0400 From: Hans de Goede To: Gerd Hoffmann Date: Fri, 17 Aug 2012 15:24:49 +0200 Message-Id: <1345209889-8585-1-git-send-email-hdegoede@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: Hans de Goede , qemu-devel@nongnu.org Subject: [Qemu-devel] [PATCH] usb: Halt ep queue en cancel pending packets on a packet error X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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 432ccae..e574477 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 c7e5bc0..28b840e 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 623b3e6..378b42b 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -2122,6 +2122,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 0b55924..4e43d20 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -743,6 +743,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;