From owner-p4-projects@FreeBSD.ORG Tue May 12 15:59:51 2009 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 1E5E11065673; Tue, 12 May 2009 15:59:51 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D23861065670 for ; Tue, 12 May 2009 15:59:50 +0000 (UTC) (envelope-from hselasky@freebsd.org) Received: from swip.net (mailfe01.swip.net [212.247.154.1]) by mx1.freebsd.org (Postfix) with ESMTP id 40C558FC12 for ; Tue, 12 May 2009 15:59:50 +0000 (UTC) (envelope-from hselasky@freebsd.org) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.0 c=1 a=PNk-2GfCM84A:10 a=j+k/Ze5hWUCaCztCgEjzDQ==:17 a=6I5d2MoRAAAA:8 a=pGLkceISAAAA:8 a=DZS4klXWeaC5DWzOUSIA:9 a=65qNvJ2X3gPX_eTnpDkA:7 a=1C9gX6FvuTa7vNFQ0gM5pnMqSNEA:4 a=MSl-tDqOz04A:10 Received: from [81.191.55.181] (account mc467741@c2i.net HELO laptop) by mailfe01.swip.net (CommuniGate Pro SMTP 5.2.13) with ESMTPA id 188851970; Tue, 12 May 2009 16:59:47 +0200 From: Hans Petter Selasky To: Andrew Thompson Date: Tue, 12 May 2009 17:02:21 +0200 User-Agent: KMail/1.9.7 References: <200905120639.n4C6drRs026284@repoman.freebsd.org> <20090512142718.GI45375@citylink.fud.org.nz> In-Reply-To: <20090512142718.GI45375@citylink.fud.org.nz> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200905121702.22017.hselasky@freebsd.org> Cc: Perforce Change Reviews Subject: Re: PERFORCE change 161961 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 May 2009 15:59:51 -0000 On Tuesday 12 May 2009, Andrew Thompson wrote: > On Tue, May 12, 2009 at 06:39:53AM +0000, Hans Petter Selasky wrote: > > http://perforce.freebsd.org/chv.cgi?CH=161961 > > > > Change 161961 by hselasky@hselasky_laptop001 on 2009/05/12 06:38:57 > > > > > > USB CORE: > > - Cancelled error code must be returned in special > > case, else device drivers might end up in infinite loop. > > - The special case code was introduced by a recent commit. > > > > Bug report by: Andrey > > > > Affected files ... > > > > .. //depot/projects/usb/src/sys/dev/usb/usb_transfer.c#144 edit > > > > Differences ... > > > > ==== //depot/projects/usb/src/sys/dev/usb/usb_transfer.c#144 (text+ko) > > ==== > > > > @@ -1439,7 +1439,11 @@ > > /* Check if the device is still alive */ > > if (info->udev->state < USB_STATE_POWERED) { > > USB_BUS_LOCK(bus); > > - usb2_transfer_done(xfer, USB_ERR_NOT_CONFIGURED); > > + /* > > + * Must return cancelled error code else > > + * device drivers can hang. > > + */ > > + usb2_transfer_done(xfer, USB_ERR_CANCELLED); > > USB_BUS_UNLOCK(bus); > > return; > > } > > I rather think this is a bug in the drivers, they blindly resubmit the > transfer without knowing if the error is restartable. No, this is no bug. And they do not blindly re-submit the transfer. When a transfer goes through a failure, then it will do a clear stall. After the clear stall is finished the transfer is re-started. The clear stall is niced at 50ms intervals. Because you added that check, the driver will spin 100% CPU instead of every 50ms interval at detach. All drivers check for CANCELLED. Adding another check for a similar error code is just waste in my opinion. Nicing a transfer is a feature, but the timeout is started after the not configured check, so that is where the bug is. --HPS