From owner-freebsd-usb@FreeBSD.ORG Thu May 12 13:47:21 2005 Return-Path: Delivered-To: freebsd-usb@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5261F16A4CE for ; Thu, 12 May 2005 13:47:21 +0000 (GMT) Received: from swip.net (mailfe06.swip.net [212.247.154.161]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5EB7243D75 for ; Thu, 12 May 2005 13:47:20 +0000 (GMT) (envelope-from hselasky@c2i.net) X-T2-Posting-ID: Y1QAsIk9O44SO+J/q9KNyQ== Received: from mp-216-46-97.daxnet.no ([193.216.46.97] verified) by mailfe06.swip.net (CommuniGate Pro SMTP 4.3c5) with ESMTP id 353385979; Thu, 12 May 2005 15:47:17 +0200 From: Hans Petter Selasky To: "M. Warner Losh" Date: Thu, 12 May 2005 15:48:01 +0200 User-Agent: KMail/1.7 References: <200505112228.49253.hselasky@c2i.net> <200505120058.51834.hselasky@c2i.net> <20050511.175830.58826830.imp@bsdimp.com> In-Reply-To: <20050511.175830.58826830.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200505121548.02651.hselasky@c2i.net> cc: usb@freebsd.org cc: dga+@cs.cmu.edu Subject: Re: Panic when removing Airprime PC5220 card (usb hub). X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: hselasky@c2i.net List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 May 2005 13:47:21 -0000 On Thursday 12 May 2005 01:58, M. Warner Losh wrote: > In message: <200505120058.51834.hselasky@c2i.net> > > Hans Petter Selasky writes: > : On Wednesday 11 May 2005 22:33, Warner Losh wrote: > : > From: Hans Petter Selasky > : > Subject: Re: Panic when removing Airprime PC5220 card (usb hub). > : > Date: Wed, 11 May 2005 22:28:48 +0200 > : > > : > > The patch for the free it twice problem, in -current, is just pushing > : > > the problem ahead. You need to implement that "free_subdev" argument > : > > passed to "usbd_free_device" like I suggested too. Because there is > : > > no guarantee that a parent device will call "device_detach()" before > : > > "device_delete_child()" on a device with USB-subdevices somewhere, > : > > which is the problem! > : > > : > Actually, there is. newbus requires that device_detach be called > : > before device_delete_child(). If something isn't playing by those > : > rules, then it will cause problems to other parts of the system that > : > rely on that behavior. Children must be in the detache state before > : > they can be deleted from the newbus tree. > : > : If that is so, then "device_delete_child()" must be consequent and post a > : warning if there are any children to detach ? Because > : "device_delete_child()" will call "device_detach()" too, detaching > : children before detaching parents. > > I believe that's a bug. Certainly all drivers I'm aware of assume > that they are still in the tree when they are detached, and they > assume that detach will be called to free up resources. We should add > a warning to catch these sorts of things. > > : But I think the current USB-code depends on the old behaviour. I just did > : a "cat" and found the following in "if_aue.c": > : > : /* > : * Do MII setup. > : * NOTE: Doing this causes child devices to be attached to us, > : * which we would normally disconnect at in the detach routine > : * using device_delete_child(). However the USB code is set up > : * such that when this driver is removed, all children devices > : * are removed as well. In effect, the USB code ends up detaching > : * all of our children for us, so we don't have to do is > : ourselves * in aue_detach(). It's important to point this out since if * > : we *do* try to detach the child devices ourselves, we will * end up > : getting the children deleted twice, which will crash * the system. > : */ > : > : It is better that the ones writing USB drivers gets used to that > : subdevices created are detached by "uhub". That saves code. Now there is > : a race condition where the child of "if_aue" can access the softc before > : it is being detached. > > This comment is wrong in some ways. The old usb code deletes the > children, so that any dangling references to them will cause a crash. > Detaching a detached device is a nop. However, referencing a cached > pointer to a child is a crash waiting to happen. Once upon a time, > drivers would cache these. It is still done, but less frequently than > before. One needs a mechanism to invalidate the cache. > > : > I agree that it does push the problem ahead a little. That's why I've > : > been working on a more general cleanup that doesn't duplicate > : > information in multiple places. The NetBSD code ill fit the newbus > : > abstraction and many of the kludges to try to make it fit need to > : > die. The whole subdev structure is duplicative and shouldn't be used > : > on FreeBSD at all, imho. > : > : The subdev structure shouldn't be used like it is, but it should be > : allowed to cache device pointers. Sure you can put some information into > : the "uaa" structure, but that is not going to save memory. > > Actually, the subdev structure exists because we have a many to one > relationship between the usbd_device entries and the device_t entries. > Usually, in newbus land, when this happens, an intermediate class is > inserted into the mix (see pccard for an example of multi-function > devices, which is what the interfaces are, kinda, in usbland). The "pccard" driver will store device_t's in a separate structure (struct pccard_function) too. For example I found "pf->dev = child;". But the difference is that the "pccard" driver will pre-allocate devices for all "interfaces", and then probe those interfaces when a new driver is added. > The > usb code is special in that drivers are allowed to eat the entire > device *OR* individual interfaces (or even a subset of interfaces), > while in other parts of the tree the option of eating the entire > device is not given. It isn't so much an issue of saving memory, but > instead of properly organzing the information. You were correct that > this issue dogs attempts to support kldload/unload of drivers since if > another device grabs an interface (say because it is a generic modem), > you can't then load a device driver that grabs the entire device. > ugen is the tip of the iceburg here because other devices may grab > things generically too, and that makes it harder to load complex > devices... I think this problem can be solved. Here are the three driver types listed: 1. special drivers 2. standard interface drivers 3. the generic driver If a device using "standard interface drivers"(2) or "the generic driver"(3) is present, and a driver of type "special drivers"(1) is loaded, then at the time "usbd_set_config_index()" is called all devices of type "standard interface drivers" or "the generic driver"(3) are deleted. Maybe the device_t that is calling "usbd_set_config_index()" has to be passed as an argument, so that "usbd_set_config_index()" doesn't delete the calling device_t. The reason this will work is that the configuration index has to be set before any pipes can be used. If a device using "the generic driver"(3) is present, and a "standard interface driver"(2) is loaded, this is somewhat more difficult. But one could make some assumptions. For example: The configuration value must be the same as the one currently selected. Then one could loop over the ifaces again, and probe/attach for new devices. What is the problem allowing ugen attached at the same time as "standard interface drivers"(2). This leads to some problems with "usbd_pipe_abort"/"usbd_pipe_close". When two devices attach at the same "usbd_device" they might setup transfers on the same pipe. So the last device calling usbd_pipe_xxxx is going to close all setup transfers. Therefore I suggest, and it is not the only reason, that all "usbd_pipe_xxx" functions be removed. And instead one "setup"/"unsetup" transfers. Then a device will only unsetup its setup transfers. This is going to work fine, because all transfer types ISOC/BULK/INTR/CTRL allow more than one transfer queued at a time. This will also require that the clear-stall mechanism be changed. There is no problem having "the generic driver"(3) and "standard interface drivers"(2) attached at the same time as long as ugen doesn't set a new configuration value during attach unless necessary. Then one can access pipes that have no driver? > > I'm not entirely sure the best way to proceed. > > : Maybe I mixed up the function names, because I replaced > : "usb_disconnect_port()" with "usbd_free_device()" in my USB-driver, but I > : counted "usb_disconnect_port()" three times in the existing USB-driver, > : and if you add three characters to each call, it is going to be less > : patching than if you add "device_detach()", not just one place, but > : several places, and not to mention break the existing detach behavior. > > I'm having trouble understanding the point you are trying to make > here. Can you restate it please? change the existing code so that it looks like this: uhub_explore(usbd_device_handle dev) { ... usb_disconnect_port(up, USBDEV(sc->sc_dev), 1); ... } USB_DETACH(uhub) { ... usb_disconnect_port(rup, self, 0); ... } USB_DETACH(usb) { ... usb_disconnect_port(&sc->sc_port, self, 0); ... } usb_disconnect_port(struct usbd_port *up, device_ptr_t parent, u_int8_t free_subdev) { ... #ifdef __FreeBSD__ if (free_subdev) #endif config_detach(dev->subdevs[i], DETACH_FORCE); ... } #define config_detach(dev, flag) \ do { \ free(device_get_ivars(dev), M_USB); \ device_delete_child(device_get_parent(dev), dev); \ } while (0); device_delete_child(device_t dev, device_t child) { ... if(dev->flags & DF_PRE_DETACH) { if ((error = device_detach(child)) != 0) return (error); } /* remove children first */ while ( (grandchild = TAILQ_FIRST(&child->children)) ) { error = device_delete_child(child, grandchild); if (error) return (error); } if(!(dev->flags & DF_PRE_DETACH)) /* default */ { if ((error = device_detach(child)) != 0) return (error); } ... } device_probe_and_attach(device_t dev) { ... dev->flags &= ~DF_PRE_DETACH; error = device_attach(dev); ... } device_set_pre_detach(device_t dev) { dev->flags |= DF_PRE_DETACH; } Then [USB-]devices that will detach its children, call "device_set_pre_detach()" during attach, and the problem is solved? --HPS