On Fri, 10 Dec 2010, Sarah Sharp wrote:
> On Fri, Dec 10, 2010 at 02:50:14PM -0500, Alan Stern wrote:
> > Okay, very good. This shows exactly what the "Safely remove drive"
> > button does:
> >
> > The drive is unmounted;
> >
> > A "spin-down" command is sent (obviously it doesn't do anything
> > for a flash drive);
> >
> > A SYNCHRONIZE CACHE comand is sent, perhaps as part of the
> > unmount procedure;
> >
> > The USB port is suspended and then immediately resumed (no
> > idea why);
> >
> > The device is set to config 0, i.e., unconfigured;
>
> usb_set_configuration() resumes the device. So if the device had
> autosuspend enabled, and the filesystem was unmounted, the USB storage
> driver would suspend it, and then usb_set_configuration() would resume
> it.
_If_ the device was enabled for autosuspend. By default USB drives
aren't, but maybe Maddog's environment does enable it.
> > The USB port is suspended again (no idea why);
>
> In usb_set_configuration(), when it's called with configuration = -1,
> configuration gets set to 0, and cp remains NULL after the first if
> statement.
>
> Later, usb_set_configuration() calls into the xHCI to remove all the
> endpoints (except control ep 0) by calling usb_hcd_alloc_bandwidth(dev,
> NULL, NULL, NULL). It sends the control message to set config 0, and
> then there's this interesting bit of code:
>
> ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> USB_REQ_SET_CONFIGURATION, 0, configuration, 0,
> NULL, 0, USB_CTRL_SET_TIMEOUT);
> if (ret < 0) {
> /* All the old state is gone, so what else can we do?
> * The device is probably useless now anyway.
> */
> cp = NULL;
> }
>
> dev->actconfig = cp;
> if (!cp) {
> usb_set_device_state(dev, USB_STATE_ADDRESS);
> usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
> mutex_unlock(hcd->bandwidth_mutex);
> usb_autosuspend_device(dev);
> goto free_interfaces;
> }
>
> Since cp was never set in this function, the second if statement always
> runs if the function was called with configuration = -1. It sort of
> looks like the second if statement was supposed to only run if the
> control message failed, but it does seem to have the effect of skipping
> the new interface installation and returning 0, so I guess it's
> intentional?
You've got it wrong. The _first_ "if" statement runs if the control
message fails, and the second runs if the device got unconfigured (cp
== NULL means there is no new configuration). Yes, it is all
intentional.
> In any case, that second call to usb_autosuspend_device() is where the
> second suspend came from.
>
> If this code is intentional (and not a bug) I think I should move the
> call to usb_hcd_alloc_bandwidth() in the second if statement into the
> first if statement. That way the xHCI driver doesn't get called twice
> to remove bandwidth for all the endpoints.
Actually, it looks like the usb_hcd_alloc_bandwidth() in the second
"if" statement is completely redundant. It should simply be removed.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
