From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48449) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7oQu-0002Nx-ON for qemu-devel@nongnu.org; Sat, 01 Sep 2012 10:12:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T7oQt-00009v-84 for qemu-devel@nongnu.org; Sat, 01 Sep 2012 10:12:20 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:52612) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7oQt-00009e-1D for qemu-devel@nongnu.org; Sat, 01 Sep 2012 10:12:19 -0400 Received: by obbta14 with SMTP id ta14so6631183obb.4 for ; Sat, 01 Sep 2012 07:12:18 -0700 (PDT) Sender: fluxion Date: Sat, 1 Sep 2012 09:12:11 -0500 From: Michael Roth Message-ID: <20120901141211.GA2301@illuin> References: <1346422762-15877-1-git-send-email-kraxel@redhat.com> <1346422762-15877-3-git-send-email-kraxel@redhat.com> <50420F7F.5000000@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50420F7F.5000000@redhat.com> Subject: Re: [Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hans de Goede Cc: Blue Swirl , Gerd Hoffmann , qemu-devel@nongnu.org On Sat, Sep 01, 2012 at 03:37:03PM +0200, Hans de Goede wrote: > Hi, > > On 09/01/2012 12:42 PM, Blue Swirl wrote: > >On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann wrote: > >>From: Hans de Goede > >> > >>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 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) > > > >Please check reserved namespaces in HACKING. > > That talks about suffixes not prefixes. I think it's just poorly wordly. At least, recent discussions on the list assume it's referencing __ prefixes: http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04781.html > > Regards, > > Hans >