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

[Hans de Goede <hdegoede@redhat.com>]

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