On Sat, Apr 14, 2012 at 05:07:25PM -0400, Alan Stern wrote:
> On Fri, 13 Apr 2012, Alan Stern wrote:
>
> > Sarah:
> >
> > I just tracked this down. When you do
> >
> > echo 0 >/sys/.../bConfigurationValue
> >
> > for a hub that has children, usb_disconnect() deadlocks when it tries
> > to acquire the bandwidth_mutex. This is because the task already holds
> > the mutex -- it was acquired in usb_set_configuration() just before the
> > call to usb_disable_device().
> >
> > That is, usb_disable_device() unbinds the hub driver, since its
> > interface is going away. The hub driver, in hub_quiesce(), disconnects
> > all the child devices.
> >
> > Any ideas on how to fix this? Maybe usb_disable_device() should be
> > responsible for acquiring the bandwidth_mutex on its own, after it
> > unbinds the interfaces, instead of making the caller acquire the mutex.
>
> In other words, something like this patch. However I'm not sure how
> well it would interact with your
> fccf4e86200b8f5edd9a65da26f150e32ba79808 commit; it undoes several of
> the changes you introduced.
I'm not sure what you mean by "undoes". This patch just pushes the
bandwidth mutex acquirement into usb_disable_device() and shrinks the
mutex code coverage a bit, correct? Assuming there are no unwanted race
conditions for bandwidth allocation/deallocation, the behavior should
still be basically the same, right?
The only issue I can see with pushing the mutex lock acquire into
usb_disable_device() is the following scenario:
1. Userspace (via the bConfiguration file) or a driver wants to switch a
configured device (device A) to a new configuration.
2. usb_disable_device() locks the mutex and removes the old
configuration from the xHCI internal structures. It drops the mutex and
returns to usb_set_configuration().
3. A new USB device (device B) is connected, and the hub thread takes
the bandwidth mutex before usb_set_configuration() can re-acquire it.
It installs a configuration for device B that takes up the rest of the
bandwidth.
4. usb_set_configuration() re-acquires the bandwidth mutex and attempts
to install the requested new configuration for device A. That fails
because there isn't enough bandwidth.
Now device A is in an odd state, because the old configuration is still
installed in the device. We can't re-install the old configuration in
the xHCI host internal structures because there isn't enough bandwidth.
If the bandwidth mutex coverage stayed the same, device B would fail to
be enumerated, and device A would have either the new configuration or
the old configuration (depending on if there was enough room for the new
configuration), but it would still be usable.
I guess from a user's perspective, in either case, one of the devices is
just not going to work. I think it would be better if the new device
was the one that stops working, but since there's race conditions in the
current code, I think it's OK that the old device could become unusable
and the user will need to unplug/replug it. It's going to be very
confusing to users to have inconsistent behavior, but I think it's a
corner case that not many people will run into.
So my initial reaction to the patch is that it looks fine. Did you have
any other specific concerns/scenarios that you wanted to run past me?
> Index: usb-3.4/drivers/usb/core/hub.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/core/hub.c
> +++ usb-3.4/drivers/usb/core/hub.c
> @@ -1667,7 +1667,6 @@ void usb_disconnect(struct usb_device **
> {
> struct usb_device *udev = *pdev;
> int i;
> - struct usb_hcd *hcd = bus_to_hcd(udev->bus);
>
> /* mark the device as inactive, so any further urb submissions for
> * this device (and any of its children) will fail immediately.
> @@ -1690,9 +1689,7 @@ void usb_disconnect(struct usb_device **
> * so that the hardware is now fully quiesced.
> */
> dev_dbg (&udev->dev, "unregistering device\n");
> - mutex_lock(hcd->bandwidth_mutex);
> usb_disable_device(udev, 0);
> - mutex_unlock(hcd->bandwidth_mutex);
> usb_hcd_synchronize_unlinks(udev);
>
> usb_remove_ep_devs(&udev->ep0);
> Index: usb-3.4/drivers/usb/core/message.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/core/message.c
> +++ usb-3.4/drivers/usb/core/message.c
You should also get rid of this comment above usb_disable_device():
/*
...
* Must be called with hcd->bandwidth_mutex held.
*/
void usb_disable_device(struct usb_device *dev, int skip_ep0)
{
..
}
> @@ -1189,7 +1189,9 @@ void usb_disable_device(struct usb_devic
> usb_disable_endpoint(dev, i + USB_DIR_IN, false);
> }
> /* Remove endpoints from the host controller internal state */
> + mutex_lock(hcd->bandwidth_mutex);
> usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
> + mutex_unlock(hcd->bandwidth_mutex);
> /* Second pass: remove endpoint pointers */
> }
> for (i = skip_ep0; i < 16; ++i) {
> @@ -1749,7 +1751,6 @@ free_interfaces:
> /* if it's already configured, clear out old state first.
> * getting rid of old interfaces means unbinding their drivers.
> */
> - mutex_lock(hcd->bandwidth_mutex);
> if (dev->state != USB_STATE_ADDRESS)
> usb_disable_device(dev, 1); /* Skip ep0 */
>
> @@ -1762,6 +1763,7 @@ free_interfaces:
> * host controller will not allow submissions to dropped endpoints. If
> * this call fails, the device state is unchanged.
> */
> + mutex_lock(hcd->bandwidth_mutex);
> ret = usb_hcd_alloc_bandwidth(dev, cp, NULL, NULL);
> if (ret < 0) {
> mutex_unlock(hcd->bandwidth_mutex);
>
Sarah Sharp
--
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
