Greg:
This is a reworked version of the earlier patch that used nonstandard
rwsem semantics. Unlike that one, this patch doesn't change any semantics
and doesn't allow writers to be starved by readers. Instead, nested calls
to usb_lock_device() are replaced with calls to down(&udev->serialize),
thereby avoiding reentrant locking. In the end it turned out that only
about four spots needed to be changed. The largest part of the patch is a
big fat comment explaining how this all works and why.
Something like this isn't all that easy to test. It works okay on my
system, even when I insert an artificial time delay and force multiple
readers and writers to be active simultaneously. I suggest you try it out
and put it in the development tree if it works okay for you.
Alan Stern
Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
===== drivers/usb/core/devices.c 1.38 vs edited =====
--- 1.38/drivers/usb/core/devices.c Thu Jun 24 12:44:16 2004
+++ edited/drivers/usb/core/devices.c Mon Jul 12 14:01:04 2004
@@ -555,10 +555,10 @@
struct usb_device *childdev = usbdev->children[chix];
if (childdev) {
- usb_lock_device(childdev);
+ down(&childdev->serialize);
ret = usb_device_dump(buffer, nbytes, skip_bytes, file_offset,
childdev,
bus, level + 1, chix, ++cnt);
- usb_unlock_device(childdev);
+ up(&childdev->serialize);
if (ret == -EFAULT)
return total_written;
total_written += ret;
===== drivers/usb/core/hub.c 1.182 vs edited =====
--- 1.182/drivers/usb/core/hub.c Thu Jul 1 10:53:20 2004
+++ edited/drivers/usb/core/hub.c Mon Jul 12 14:10:07 2004
@@ -965,9 +965,11 @@
usb_set_device_state(udev, USB_STATE_NOTATTACHED);
/* lock the bus list on behalf of HCDs unregistering their root hubs */
- if (!udev->parent)
+ if (!udev->parent) {
down(&usb_bus_list_lock);
- usb_lock_device(udev);
+ usb_lock_device(udev);
+ } else
+ down(&udev->serialize);
dev_info (&udev->dev, "USB disconnect, address %d\n", udev->devnum);
@@ -996,9 +998,11 @@
*pdev = NULL;
spin_unlock_irq(&device_state_lock);
- usb_unlock_device(udev);
- if (!udev->parent)
+ if (!udev->parent) {
+ usb_unlock_device(udev);
up(&usb_bus_list_lock);
+ } else
+ up(&udev->serialize);
device_unregister(&udev->dev);
}
@@ -1693,7 +1697,7 @@
* udev becomes globally accessible, although presumably
* no one will look at it until hdev is unlocked.
*/
- usb_lock_device (udev);
+ down(&udev->serialize);
status = 0;
/* We mustn't add new devices if the parent hub has
@@ -1717,7 +1721,7 @@
}
}
- usb_unlock_device (udev);
+ up(&udev->serialize);
if (status)
goto loop;
===== drivers/usb/core/usb.c 1.282 vs edited =====
--- 1.282/drivers/usb/core/usb.c Tue Jul 6 08:00:32 2004
+++ edited/drivers/usb/core/usb.c Mon Jul 12 14:00:12 2004
@@ -845,6 +845,43 @@
put_device(&intf->dev);
}
+
+/* USB device locking
+ *
+ * Although locking USB devices should be straightforward, it is
+ * complicated by the way the driver-model core works. When a new USB
+ * driver is registered or unregistered, the core will automatically
+ * probe or disconnect all matching interfaces on all USB devices while
+ * holding the USB subsystem writelock. There's no good way for us to
+ * tell which devices will be used or to lock them beforehand. Our only
+ * option is to effectively lock all the USB devices.
+ *
+ * We do that by using a private rw-semaphore, usb_all_devices_rwsem.
+ * When locking an individual device you must first acquire the rwsem's
+ * readlock. When a driver is registered or unregistered the writelock
+ * must be held. These actions are encapsulated in the subroutines
+ * below, so all a driver needs to do is call usb_lock_device() and
+ * usb_unlock_device().
+ *
+ * Complications arise when several devices are to be locked at the same
+ * time. Only hub-aware drivers that are part of usbcore ever have to
+ * do this; nobody else needs to worry about it. The problem is that
+ * usb_lock_device() must not be called to lock a second device since it
+ * would acquire the rwsem's readlock reentrantly, leading to deadlock if
+ * another thread was waiting for the writelock. The solution is simple:
+ *
+ * When locking more than one device, call usb_lock_device()
+ * to lock the first one. Lock the others by calling
+ * down(&udev->serialize) directly.
+ *
+ * When unlocking multiple devices, use up(&udev->serialize)
+ * to unlock all but the last one. Unlock the last one by
+ * calling usb_unlock_device().
+ *
+ * When locking more than one device, always lock the parent
+ * before locking the child.
+ */
+
/**
* usb_lock_device - acquire the lock for a usb device structure
* @udev: device that's being locked
@@ -976,10 +1013,10 @@
/* look through all of the children of this device */
for (child = 0; child < dev->maxchild; ++child) {
if (dev->children[child]) {
- usb_lock_device(dev->children[child]);
+ down(&dev->children[child]->serialize);
ret_dev = match_device(dev->children[child],
vendor_id, product_id);
- usb_unlock_device(dev->children[child]);
+ up(&dev->children[child]->serialize);
if (ret_dev)
goto exit;
}
-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=4721&alloc_id=10040&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
