From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36600) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7tz4-00062R-F8 for qemu-devel@nongnu.org; Sat, 01 Sep 2012 16:07:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T7tz3-0005Fq-5p for qemu-devel@nongnu.org; Sat, 01 Sep 2012 16:07:58 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:57395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7tz2-0005Fl-RY for qemu-devel@nongnu.org; Sat, 01 Sep 2012 16:07:57 -0400 Received: by obbta14 with SMTP id ta14so6891240obb.4 for ; Sat, 01 Sep 2012 13:07:55 -0700 (PDT) Sender: fluxion Date: Sat, 1 Sep 2012 15:07:47 -0500 From: Michael Roth Message-ID: <20120901200747.GB2301@illuin> References: <1346422762-15877-1-git-send-email-kraxel@redhat.com> <1346422762-15877-3-git-send-email-kraxel@redhat.com> <50420F7F.5000000@redhat.com> <20120901141211.GA2301@illuin> <50425840.5040007@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50425840.5040007@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 08:47:28PM +0200, Hans de Goede wrote: > Hi, > > On 09/01/2012 04:12 PM, Michael Roth wrote: > >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 > > Ok, so lets change it to a single underscore if people prefer that. > > Gerd can you make that change in your tree, or do you want me to > resend the (corrected) patch ? Looks like it's in master already. Can probably fix it up in a seperate patch for 1.3 if it's not causing any issues currently. > > Regards, > > Hans >