Date: Thu, 12 May 2005 15:48:01 +0200 From: Hans Petter Selasky <hselasky@c2i.net> To: "M. Warner Losh" <imp@bsdimp.com> Cc: dga+@cs.cmu.edu Subject: Re: Panic when removing Airprime PC5220 card (usb hub). Message-ID: <200505121548.02651.hselasky@c2i.net> In-Reply-To: <20050511.175830.58826830.imp@bsdimp.com> References: <200505112228.49253.hselasky@c2i.net> <200505120058.51834.hselasky@c2i.net> <20050511.175830.58826830.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 12 May 2005 01:58, M. Warner Losh wrote:
> In message: <200505120058.51834.hselasky@c2i.net>
>
> Hans Petter Selasky <hselasky@c2i.net> writes:
> : On Wednesday 11 May 2005 22:33, Warner Losh wrote:
> : > From: Hans Petter Selasky <hselasky@c2i.net>
> : > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200505121548.02651.hselasky>