Re: [PATCH] USB: additional regression fix for device removal

From: Éric Piel
Date: Thu Jul 07 2011 - 04:36:16 EST


Op 06-07-11 23:03, Alan Stern schreef:
Commit e534c5b831c8b8e9f5edee5c8a37753c808b80dc (USB: fix regression
occurring during device removal) didn't go far enough. It failed to
take into account that when a driver claims multiple interfaces, it may
release them all at the same time. As a result, some interfaces can
get released before they are unregistered, and we deadlock trying to
acquire the bandwidth_mutex that we already own.

This patch (asl478) handles this case by setting the "unregistering"
flag on all the interfaces before removing any of them.

Signed-off-by: Alan Stern<stern@xxxxxxxxxxxxxxxxxxx>
CC:<stable@xxxxxxxxxx>

Great, it works here :-)
Tested-by: Éric Piel <eric.piel@xxxxxxxxxxxxxxxx>

Thanks,
Éric

---

This should take care of Eric's problem as well as Arkadiusz, since
they seemed to be hitting the same thing (cdc_ether claiming multiple
interfaces and hanging while releasing them).

Still, we need to rewrite this stuff. A possible race remains, because
a driver may try to change an altsetting at the same time as the device
is removed. Either the driver's disconnect routine would hang waiting
for the altsetting change (which is waiting to acquire the
bandwidth_mutex) or else the altsetting change would go through after
the driver was unbound from the device. Neither alternative is good.

Alan Stern



drivers/usb/core/message.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

Index: usb-3.0/drivers/usb/core/message.c
===================================================================
--- usb-3.0.orig/drivers/usb/core/message.c
+++ usb-3.0/drivers/usb/core/message.c
@@ -1147,6 +1147,14 @@ void usb_disable_device(struct usb_devic
* any drivers bound to them (a key side effect)
*/
if (dev->actconfig) {
+ /*
+ * FIXME: In order to avoid self-deadlock involving the
+ * bandwidth_mutex, we have to mark all the interfaces
+ * before unregistering any of them.
+ */
+ for (i = 0; i< dev->actconfig->desc.bNumInterfaces; i++)
+ dev->actconfig->interface[i]->unregistering = 1;
+
for (i = 0; i< dev->actconfig->desc.bNumInterfaces; i++) {
struct usb_interface *interface;

@@ -1156,7 +1164,6 @@ void usb_disable_device(struct usb_devic
continue;
dev_dbg(&dev->dev, "unregistering interface %s\n",
dev_name(&interface->dev));
- interface->unregistering = 1;
remove_intf_ep_devs(interface);
device_del(&interface->dev);
}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/