From owner-svn-src-all@FreeBSD.ORG Tue Mar 22 15:01:07 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0BC671065673; Tue, 22 Mar 2011 15:01:07 +0000 (UTC) (envelope-from hselasky@freebsd.org) Received: from swip.net (mailfe07.c2i.net [212.247.154.194]) by mx1.freebsd.org (Postfix) with ESMTP id 189008FC16; Tue, 22 Mar 2011 15:01:05 +0000 (UTC) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.1 cv=118a5bn0tGnRZbUa9ClBty6BTuM1bA5mUZqmf2RHYp4= c=1 sm=1 a=IU0TiZmyZPMA:10 a=yTxM5DjhyGsA:10 a=N659UExz7-8A:10 a=CL8lFSKtTFcA:10 a=i9M/sDlu2rpZ9XS819oYzg==:17 a=6I5d2MoRAAAA:8 a=ryKf44oYO0ZEnny9_hcA:9 a=gqaOcW-znfXyNU4YBYtoExXC9koA:4 a=pILNOxqGKmIA:10 a=kFvEmTdc9h0Gig2r:21 a=-i7RyuqCfTGhr2yl:21 a=i9M/sDlu2rpZ9XS819oYzg==:117 Received: from [188.126.198.129] (account mc467741@c2i.net HELO laptop002.hselasky.homeunix.org) by mailfe07.swip.net (CommuniGate Pro SMTP 5.2.19) with ESMTPA id 103754247; Tue, 22 Mar 2011 15:50:58 +0100 Received-SPF: softfail receiver=mailfe07.swip.net; client-ip=188.126.198.129; envelope-from=hselasky@freebsd.org From: Hans Petter Selasky To: Gavin Atkinson Date: Tue, 22 Mar 2011 15:50:15 +0100 User-Agent: KMail/1.13.5 (FreeBSD/8.2-PRERELEASE; KDE/4.4.5; amd64; ; ) References: <201103212116.p2LLGPGF021033@svn.freebsd.org> <1300804810.20711.12.camel@buffy.york.ac.uk> In-Reply-To: <1300804810.20711.12.camel@buffy.york.ac.uk> X-Face: *nPdTl_}RuAI6^PVpA02T?$%Xa^>@hE0uyUIoiha$pC:9TVgl.Oq, NwSZ4V"|LR.+tj}g5 %V,x^qOs~mnU3]Gn; cQLv&.N>TrxmSFf+p6(30a/{)KUU!s}w\IhQBj}[g}bj0I3^glmC( :AuzV9:.hESm-x4h240C`9=w MIME-Version: 1.0 Content-Type: Text/Plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Message-Id: <201103221550.15901.hselasky@freebsd.org> Cc: "svn-src-head@FreeBSD.org" , "svn-src-all@FreeBSD.org" , "src-committers@FreeBSD.org" , "thompsa@FreeBSD.org" Subject: Re: svn commit: r219845 - head/sys/dev/usb/controller X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Mar 2011 15:01:07 -0000 On Tuesday 22 March 2011 15:40:10 Gavin Atkinson wrote: > Hi, > > On Mon, 2011-03-21 at 21:16 +0000, Hans Petter Selasky wrote: > > Date: Mon Mar 21 21:16:25 2011 > > New Revision: 219845 > > URL: http://svn.freebsd.org/changeset/base/219845 > > > > Log: > > - Bugfix: Fix a EHCI hardware race, where the hardware computed data > > toggle value is updated after that we read it in the queue-head. This > > patch can fix problems with BULK timeouts. The issue was found on a > > Nvidia chipset. > > > > Modified: > > head/sys/dev/usb/controller/ehci.c > > > > Modified: head/sys/dev/usb/controller/ehci.c > > ========================================================================= > > ===== --- head/sys/dev/usb/controller/ehci.c Mon Mar 21 21:16:12 > > 2011 (r219844) +++ head/sys/dev/usb/controller/ehci.c Mon Mar 21 > > 21:16:25 2011 (r219845) @@ -1180,6 +1180,26 @@ _ehci_remove_qh(ehci_qh_t > > *sqh, ehci_qh_ > > > > return (last); > > > > } > > > > +static void > > +ehci_data_toggle_update(struct usb_xfer *xfer, uint16_t actlen, uint16_t > > xlen) +{ > > + uint8_t full = (actlen == xlen); Hi Gavin, > > style(9) strongly advises against initialising variables in the > declaration. Thanks for pointing out. > > > + uint8_t dt; > > + > > + /* count number of full packets */ > > + dt = (actlen / xfer->max_packet_size) & 1; > > + > > + /* cumpute remainder */ > > + actlen = actlen % xfer->max_packet_size; > > + > > + if (actlen > 0) > > + dt ^= 1; /* short packet at the end */ > > + else if (!full) > > Especially in this case - this would seem to be better written as Should be "oldactlen != xlen". I will try to fix the style issues, probably not before tomorrow, because I'm busy tonight. > else if (actlen != xlen) > > > + dt ^= 1; /* zero length packet at the end */ > > + > > + xfer->endpoint->toggle_next ^= dt; > > +} > > As an aside, over the years there are several PRs about bulk timeouts on > EHCI on nVidia - is it possible that this fixes any of them? Yes, I believe so. I'm not sure about 6.x and 7.x. What I caught red-handed was that the final DATA toggle value was not properly written back to the queue head at the time the last TD was not active. The data toggle only has two values 0 or 1, so there is a 50% chance it will cause a timeout when the race happens. > Some of > the PRs date back to 6.x and 7.x - if I'm understanding the change > properly, it seems that it is possible that this could be the cause? Do > you have any opinions on whether this could fix any of those PRs? --HPS