Panic when removing Airprime PC5220 card (usb hub).

[Hans Petter Selasky]

On Thursday 12 May 2005 01:58, M. Warner Losh wrote:
> 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.

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