Panic when removing Airprime PC5220 card (usb hub).

[M. Warner Losh]

In message: <200505120058.51834.hselasky at c2i.net>
            Hans Petter Selasky <hselasky at c2i.net> writes:
: On Wednesday 11 May 2005 22:33, Warner Losh wrote:
: > From: Hans Petter Selasky <hselasky at 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.

: > 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
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'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?

: So how is it going to be?

I'm not entirely sure.  I've been playing around with some changes and
have stumbled into understanding why things are the way they are
better than I did before.  I think that my understanding of the
problem is still incomplete, so I'll need to do some more studying and
experimenting before I'll have a good idea of how to proceed.

# This was a private reply, but I think it is useful to have it on the
# list and HPS said it was OK to post.

Warner