Patchwork [for-1.2,02/11] usb: Halt ep queue en cancel pending packets on a packet error

login
register
mail settings
Submitter Gerd Hoffmann
Date Aug. 31, 2012, 2:19 p.m.
Message ID <1346422762-15877-3-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/180981/
State New
Headers show

Comments

Gerd Hoffmann - Aug. 31, 2012, 2:19 p.m.
From: Hans de Goede <hdegoede@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 <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@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(-)
Blue Swirl - Sept. 1, 2012, 10:42 a.m.
On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> From: Hans de Goede <hdegoede@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 <hdegoede@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@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
>
>
Hans de Goede - Sept. 1, 2012, 1:37 p.m.
Hi,

On 09/01/2012 12:42 PM, Blue Swirl wrote:
> On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> From: Hans de Goede <hdegoede@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 <hdegoede@redhat.com>
>> Signed-off-by: Gerd Hoffmann <kraxel@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.

That talks about suffixes not prefixes.

Regards,

Hans
Michael Roth - Sept. 1, 2012, 2:12 p.m.
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 <kraxel@redhat.com> wrote:
> >>From: Hans de Goede <hdegoede@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 <hdegoede@redhat.com>
> >>Signed-off-by: Gerd Hoffmann <kraxel@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.
> 
> 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
>
Hans de Goede - Sept. 1, 2012, 6:47 p.m.
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 <kraxel@redhat.com> wrote:
>>>> From: Hans de Goede <hdegoede@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 <hdegoede@redhat.com>
>>>> Signed-off-by: Gerd Hoffmann <kraxel@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.
>>
>> 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 ?

Regards,

Hans
Peter Maydell - Sept. 1, 2012, 7:04 p.m.
On 1 September 2012 19:47, Hans de Goede <hdegoede@redhat.com> wrote:
> Ok, so lets change it to a single underscore if people prefer that.

Why does this function have any kind of starting-with-underscore
name at all? The usual reason for a leading underscore is functions
in header files or macros where you don't want them to clash with
names used by the rest of the program. This is a simple static
helper function in one C file -- there's no reason for it not to
just have a plain name like any other function. Maybe
usb_complete_one_packet() or something.

-- PMM
Michael Roth - Sept. 1, 2012, 8:07 p.m.
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 <kraxel@redhat.com> wrote:
> >>>>From: Hans de Goede <hdegoede@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 <hdegoede@redhat.com>
> >>>>Signed-off-by: Gerd Hoffmann <kraxel@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.
> >>
> >>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
>

Patch

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)
+{
+    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;