From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60067) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2Jp6-0002n3-Jk for qemu-devel@nongnu.org; Fri, 17 Aug 2012 06:30:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T2Jp5-0000MD-Dc for qemu-devel@nongnu.org; Fri, 17 Aug 2012 06:30:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19361) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2Jp5-0000J3-4g for qemu-devel@nongnu.org; Fri, 17 Aug 2012 06:30:35 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7HAUXlg032078 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 17 Aug 2012 06:30:33 -0400 Message-ID: <502E1D47.9090801@redhat.com> Date: Fri, 17 Aug 2012 12:30:31 +0200 From: Gerd Hoffmann MIME-Version: 1.0 References: <1345196357-6076-1-git-send-email-hdegoede@redhat.com> In-Reply-To: <1345196357-6076-1-git-send-email-hdegoede@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] usb: Halt ep queue en cancel pending packets on a packet error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hans de Goede Cc: qemu-devel@nongnu.org Hi, > 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! xhci does it too (although it is hard to test as xhci can happily submit 256k transfers where ehci and uhci have to use a bunch of smaller packets instead). Some minor nits below (the other two patches look good): > + /* Submitting a new packet clears halt */ > + p->ep->halted = false; check that the queue is empty when halted is set (i.e. enforce cancel on error) ? > + > 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); Strictly speaking returning something != async is fine for the first package in the queue, but I guess in practice this doesn't matter as enabling pipelining doesn't make sense unless you actually go async. > p->result = ret; > usb_packet_set_state(p, USB_PACKET_COMPLETE); > } > @@ -402,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *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. */ That comments should be ... > +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); > +} > + ... here, where the function for the external users is. > void usb_packet_complete(USBDevice *dev, USBPacket *p) > { cheers, Gerd