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

[Blue Swirl]

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.

> +{
> +    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 8b94b17..8504a6a 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 1ace2a4..8987734 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;
> --
> 1.7.1
>
>