Re: [Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error

[Michael Roth]

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 <address@hidden> wrote:
> >>>>From: Hans de Goede <address@hidden>
> >>>>
> >>>>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 <address@hidden>
> >>>>Signed-off-by: Gerd Hoffmann <address@hidden>
> >>>>---
> >>>>  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
>