When using the remove sysfs file, the device configuration is set to -1
(unconfigured). This eventually unbind drivers with the bandwidth_mutex
held. Some drivers may call functions that hold said mutex, like
usb_reset_device. This is the case for rtl8187, for example. This will
lead to the same process holding the mutex twice, which deadlocks.
Unbinding the driver before holding the bandwidth_mutex solves the
problem. If any operation after that fails, drivers are not bound again.
But that would be a problem anyway that the user may solve resetting the
device configuration to one that works, just like he would need to do in
most other failure cases.
---
NOTE: Not signed-off yet, because I'm waiting for some review. Thanks!
---
drivers/usb/core/message.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index fd4c36e..fe6b9e8 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1724,6 +1724,12 @@ free_interfaces:
if (ret)
goto free_interfaces;
+ /* if it's already configured, clear out old state first.
+ * getting rid of old interfaces means unbinding their drivers.
+ */
+ if (dev->state != USB_STATE_ADDRESS)
+ usb_disable_device(dev, 1); /* Skip ep0 */
+
/* Make sure we have bandwidth (and available HCD resources) for this
* configuration. Remove endpoints from the schedule if we're dropping
* this configuration to set configuration 0. After this point, the
@@ -1738,12 +1744,6 @@ free_interfaces:
goto free_interfaces;
}
- /* if it's already configured, clear out old state first.
- * getting rid of old interfaces means unbinding their drivers.
- */
- if (dev->state != USB_STATE_ADDRESS)
- usb_disable_device(dev, 1); /* Skip ep0 */
-
/* Get rid of pending async Set-Config requests for this device */
cancel_async_set_config(dev);
--
1.7.1
On Tue, 17 Aug 2010, Thadeu Lima de Souza Cascardo wrote:
> When using the remove sysfs file, the device configuration is set to -1
> (unconfigured). This eventually unbind drivers with the bandwidth_mutex
> held. Some drivers may call functions that hold said mutex, like
> usb_reset_device. This is the case for rtl8187, for example. This will
> lead to the same process holding the mutex twice, which deadlocks.
The deadlock problem probably could be handled somehow, but there's a
separate issue: Until the usb_disable_device call finishes unbinding
the drivers, the drivers are free to continue using their allocated
bandwidth. We musn't change the bandwidth allocations until after the
unbinding is done. So this patch is indeed necessary.
> Unbinding the driver before holding the bandwidth_mutex solves the
> problem. If any operation after that fails, drivers are not bound again.
> But that would be a problem anyway that the user may solve resetting the
> device configuration to one that works, just like he would need to do in
> most other failure cases.
That's right; we don't have to worry about this case.
Incidentally, you might also want to change the following code at the
same time:
mutex_lock(&hcd->bandwidth_mutex);
ret = usb_hcd_alloc_bandwidth(dev, cp, NULL, NULL);
if (ret < 0) {
usb_autosuspend_device(dev);
mutex_unlock(&hcd->bandwidth_mutex);
goto free_interfaces;
}
In the failure case, it would be safer to drop the mutex _before_
calling usb_autosuspend_device.
Alan Stern