From owner-freebsd-hackers Sun Dec 1 3:21:16 2002 Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 12C7337B401 for ; Sun, 1 Dec 2002 03:21:14 -0800 (PST) Received: from comp.chem.msu.su (comp-ext.chem.msu.su [158.250.32.157]) by mx1.FreeBSD.org (Postfix) with ESMTP id E267B43EAF for ; Sun, 1 Dec 2002 03:21:12 -0800 (PST) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (localhost [127.0.0.1]) by comp.chem.msu.su (8.12.3/8.12.3) with ESMTP id gB1BLh9K098639; Sun, 1 Dec 2002 14:21:43 +0300 (MSK) (envelope-from yar@comp.chem.msu.su) Received: (from yar@localhost) by comp.chem.msu.su (8.12.3/8.12.3/Submit) id gB1BLUo5098638; Sun, 1 Dec 2002 14:21:30 +0300 (MSK) Date: Sun, 1 Dec 2002 14:21:30 +0300 From: Yar Tikhiy To: Nate Lawson Cc: freebsd-hackers@freebsd.org Subject: Re: Review by USB wizard wanted Message-ID: <20021201142129.B94232@comp.chem.msu.su> References: <20021125164551.A26800@comp.chem.msu.su> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: ; from nate@root.org on Tue, Nov 26, 2002 at 03:26:19PM -0800 Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Tue, Nov 26, 2002 at 03:26:19PM -0800, Nate Lawson wrote: > I'm not a usb expert but I think your patch deserves some comments. Thank you! > On Mon, 25 Nov 2002, Yar Tikhiy wrote: > > First, sometimes (especially, if twitching a memory stick out of > > the reader while the device is being detected) a transfer to the > > umass device is initiated *after* the device is already gone. System > > panic follows. The transfer is initiated when destroying the default > > pipe to the device. Indeed, the current usb_subr.c code will detach > > child devices first and destroy the default pipe then. Reverting > > this order eliminates the panic. > > > > Second, twitching a memory stick can cause CAM jam. That happens > > because the umass detach routine won't wake up the upper layer when > > processing a device with a pending transfer on it. > > > > Patches addressing the above problems are attached below. > > [...] > > --- usb_subr.c.orig Sat Nov 16 12:07:50 2002 > > +++ usb_subr.c Fri Nov 22 15:45:35 2002 > > @@ -1292,8 +1292,6 @@ > > { > > int ifcidx, nifc; > > > > - if (dev->default_pipe != NULL) > > - usbd_kill_pipe(dev->default_pipe); > > if (dev->ifaces != NULL) { > > nifc = dev->cdesc->bNumInterface; > > for (ifcidx = 0; ifcidx < nifc; ifcidx++) > > @@ -1340,6 +1338,9 @@ > > return; > > } > > #endif > > + > > + if (dev->default_pipe != NULL) > > + usbd_kill_pipe(dev->default_pipe); > > > > if (dev->subdevs != NULL) { > > DPRINTFN(3,("usb_disconnect_port: disconnect subdevs\n")); > > > > This change looks good to me. With your patch, we need to be careful if > anyone ever calls usb_free_device() directly since the default pipe won't > be killed. But since I don't see it called by anyone else, it seems ok. Of course, I've made sure it's the only point where usb_free_device() would be called from. > Why not make usb_free_device() static too? Good point! > > --- umass.c.orig Sat Nov 16 12:07:50 2002 > > +++ umass.c Fri Nov 22 21:42:10 2002 > > @@ -1033,6 +1033,13 @@ > > /* detach the device from the SCSI host controller (SIM) */ > > err = umass_cam_detach(sc); > > > > + /* if upper layer is waiting for a transfer to finish, wake it up */ > > + if (sc->transfer_state != TSTATE_IDLE) { > > + sc->transfer_state = TSTATE_IDLE; > > + sc->transfer_cb(sc, sc->transfer_priv, > > + sc->transfer_datalen, STATUS_WIRE_FAILED); > > + } > > + > > for (i = 0; i < XFER_NR; i++) > > if (sc->transfer_xfer[i]) > > usbd_free_xfer(sc->transfer_xfer[i]); > > This looks good except you're passing the full datalen as the > residue. Does this address the situation where the data has been > partially transferred successfully? Or should you subtract > transfer_actlen in that case? I think your code is correct, I just am not > familiar enough with usb to be sure. After examining the case, I think transfer_actlen should be subtracted from transfer_datalen there for consistency. However, Nick Hibma has told me the residue argument value didn't matter in most cases of error. -- Yar To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message