From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44964) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7siD-0002BR-2u for qemu-devel@nongnu.org; Sat, 01 Sep 2012 14:46:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T7siB-0007UQ-NU for qemu-devel@nongnu.org; Sat, 01 Sep 2012 14:46:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1536) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7siB-0007UL-Eo for qemu-devel@nongnu.org; Sat, 01 Sep 2012 14:46:27 -0400 Message-ID: <50425840.5040007@redhat.com> Date: Sat, 01 Sep 2012 20:47:28 +0200 From: Hans de Goede MIME-Version: 1.0 References: <1346422762-15877-1-git-send-email-kraxel@redhat.com> <1346422762-15877-3-git-send-email-kraxel@redhat.com> <50420F7F.5000000@redhat.com> <20120901141211.GA2301@illuin> In-Reply-To: <20120901141211.GA2301@illuin> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-1.2 02/11] 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: Michael Roth Cc: Blue Swirl , Gerd Hoffmann , qemu-devel@nongnu.org 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 wrote: >>>> From: Hans de Goede >>>> >>>> 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 >>>> Signed-off-by: Gerd Hoffmann >>>> --- >>>> 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