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 <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.
>
> 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
>
