| Message ID | 1346422762-15877-3-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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 >
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
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
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 >
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;