On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann <kra...@redhat.com> wrote:
> From: Hans de Goede <hdego...@redhat.com>
>
> 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 <hdego...@redhat.com>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> ---
> 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
>
>
