A separate usbfs patch is needed to fix its driver binding; mostly just to use the right lock, but those changes were more extensive and uncovered other issues. (Like, I think, some that Duncan has been noticing ...)
And here it is. I currently don't have code using usbfs for anything interesting ... this patch should get more testing than I can do lately. (Duncan, it'd be interesting to know if the combination of these two patches affects any of any of the issues you were seeing....)
What it does:
- Locking fixes ... use the bus rwsem and dev->serialize, as needed. Removed more (inappropriate) BKL usage.
- Hmm, that strcpy() should be a strncpy(), even though it's locked better now.
- Huh, usb_probe_interface() was blatantly wrong there; use bus_rescan_devices(), even though that may rebind other devices too.
Those dev->serialize updates were places where the config could change out from usbfs. It still assumes ps->dev is going to be valid.
If the combination of these two behaves, I think they should go into the 2.6.1 queue.
- Dave
--- 1.22/drivers/usb/core/devices.c Tue Jul 29 04:28:37 2003
+++ edited/drivers/usb/core/devices.c Tue Dec 9 17:45:03 2003
@@ -238,7 +238,8 @@
if (start > end)
return start;
- lock_kernel(); /* driver might be unloaded */
+ /* lock against driver unbind */
+ down_read(&usb_bus_type.subsys.rwsem);
start += sprintf(start, format_iface,
desc->bInterfaceNumber,
desc->bAlternateSetting,
@@ -248,7 +249,7 @@
desc->bInterfaceSubClass,
desc->bInterfaceProtocol,
iface->driver ? iface->driver->name : "(none)");
- unlock_kernel();
+ up_read(&usb_bus_type.subsys.rwsem);
return start;
}
@@ -464,6 +465,7 @@
if (!(pages_start = (char*) __get_free_pages(GFP_KERNEL,1)))
return -ENOMEM;
+ down(&usbdev->serialize);
if (usbdev->parent && usbdev->parent->devnum != -1)
parent_devnum = usbdev->parent->devnum;
/*
@@ -514,6 +516,7 @@
}
data_end = usb_dump_desc(data_end, pages_start + (2 * PAGE_SIZE) - 256,
usbdev);
+ up(&usbdev->serialize);
if (data_end > (pages_start + (2 * PAGE_SIZE) - 256))
data_end += sprintf(data_end, "(truncated)\n");
--- 1.54/drivers/usb/core/devio.c Thu Oct 9 03:20:10 2003
+++ edited/drivers/usb/core/devio.c Tue Dec 9 17:49:14 2003
@@ -358,18 +358,22 @@
if (intf >= 8*sizeof(ps->ifclaimed) || !dev
|| intf >= dev->actconfig->desc.bNumInterfaces)
return -EINVAL;
- /* already claimed */
- if (test_bit(intf, &ps->ifclaimed))
- return 0;
- iface = dev->actconfig->interface[intf];
- err = -EBUSY;
- lock_kernel();
- if (!usb_interface_claimed(iface)) {
- usb_driver_claim_interface(&usbdevfs_driver, iface, ps);
- set_bit(intf, &ps->ifclaimed);
+
+ /* lock against config and driver binding changes */
+ down(&dev->serialize);
+ down_write(&usb_bus_type.subsys.rwsem);
+
+ /* already claimed? */
+ if (!test_bit(intf, &ps->ifclaimed)) {
+ iface = usb_ifnum_to_if(ps->dev, intf);
+ err = usb_driver_claim_interface(&usbdevfs_driver, iface, ps);
+ if (err == 0)
+ set_bit(intf, &ps->ifclaimed);
+ } else
err = 0;
- }
- unlock_kernel();
+
+ up_write(&usb_bus_type.subsys.rwsem);
+ up(&dev->serialize);
return err;
}
@@ -383,12 +387,16 @@
return -EINVAL;
err = -EINVAL;
dev = ps->dev;
+
+ /* lock against config and driver binding changes */
down(&dev->serialize);
+ down_write(&usb_bus_type.subsys.rwsem);
if (test_and_clear_bit(intf, &ps->ifclaimed)) {
- iface = dev->actconfig->interface[intf];
+ iface = usb_ifnum_to_if(ps->dev, intf);
usb_driver_release_interface(&usbdevfs_driver, iface);
err = 0;
}
+ up_write(&usb_bus_type.subsys.rwsem);
up(&dev->serialize);
return err;
}
@@ -688,15 +696,19 @@
return -EFAULT;
if ((ret = findintfif(ps->dev, gd.interface)) < 0)
return ret;
+
+ down(&ps->dev->serialize);
interface = usb_ifnum_to_if(ps->dev, gd.interface);
- if (!interface)
- return -EINVAL;
- if (!interface->driver)
- return -ENODATA;
- strcpy(gd.driver, interface->driver->name);
- if (copy_to_user(arg, &gd, sizeof(gd)))
- return -EFAULT;
- return 0;
+ down_read(&usb_bus_type.subsys.rwsem);
+ if (interface && interface->driver) {
+ strncpy(gd.driver, interface->driver->name, sizeof gd.driver);
+ if (copy_to_user(arg, &gd, sizeof(gd)))
+ ret = -EFAULT;
+ } else
+ ret = -ENODATA;
+ up_read(&usb_bus_type.subsys.rwsem);
+ up(&ps->dev->serialize);
+ return ret;
}
static int proc_connectinfo(struct dev_state *ps, void __user *arg)
@@ -744,16 +756,18 @@
return -EFAULT;
if ((ret = findintfif(ps->dev, setintf.interface)) < 0)
return ret;
+ ret = -EINVAL;
+
+ down(&ps->dev->serialize);
interface = usb_ifnum_to_if(ps->dev, setintf.interface);
- if (!interface)
- return -EINVAL;
- if (interface->driver) {
- if ((ret = checkintf(ps, ret)))
- return ret;
+ if (interface && interface->driver) {
+ ret = checkintf(ps, ret);
+ if (ret == 0)
+ ret = usb_set_interface(ps->dev, setintf.interface,
+ setintf.altsetting);
}
- if (usb_set_interface(ps->dev, setintf.interface, setintf.altsetting))
- return -EINVAL;
- return 0;
+ up(&ps->dev->serialize);
+ return ret;
}
static int proc_setconfig(struct dev_state *ps, void __user *arg)
@@ -1089,58 +1103,56 @@
}
}
- if (!ps->dev)
- retval = -ENODEV;
- else if (!(ifp = usb_ifnum_to_if (ps->dev, ctrl.ifno)))
- retval = -EINVAL;
- else switch (ctrl.ioctl_code) {
-
- /* disconnect kernel driver from interface, leaving it unbound. */
- /* maybe unbound - you get no guarantee it stays unbound */
- case USBDEVFS_DISCONNECT:
- /* this function is misdesigned - retained for compatibility */
- lock_kernel();
- driver = ifp->driver;
- if (driver) {
- dbg ("disconnect '%s' from dev %d interface %d",
- driver->name, ps->dev->devnum, ctrl.ifno);
- usb_unbind_interface(&ifp->dev);
+ if (!ps->dev) {
+ if (buf)
+ kfree(buf);
+ return -ENODEV;
+ }
+
+ down(&ps->dev->serialize);
+ if (!(ifp = usb_ifnum_to_if(ps->dev, ctrl.ifno)))
+ retval = -EINVAL;
+ else switch (ctrl.ioctl_code) {
+
+ /* disconnect kernel driver from interface */
+ case USBDEVFS_DISCONNECT:
+ down_write(&usb_bus_type.subsys.rwsem);
+ if (ifp->dev.driver) {
+ dev_dbg(&ifp->dev, "usbfs disconnect\n");
+ device_release_driver(&ifp->dev);
+ } else if (ifp->driver) {
+ /* for now be paranoid: don't disconnect() drivers
+ * bound using usb_driver_claim_interface().
+ */
+ dev_dbg(&ps->dev->dev,
+ "usbfs: intf %d half-bound to %s\n",
+ ctrl.ifno, ifp->driver->name);
+ retval = -EBUSY;
} else
retval = -ENODATA;
- unlock_kernel();
+ up_write(&usb_bus_type.subsys.rwsem);
+ /* any driver, even usbfs, may now (re)bind to this interface */
break;
/* let kernel drivers try to (re)bind to the interface */
case USBDEVFS_CONNECT:
- lock_kernel();
- retval = usb_probe_interface (&ifp->dev);
- unlock_kernel();
+ bus_rescan_devices(ifp->dev.bus);
break;
/* talk directly to the interface's driver */
default:
- /* BKL used here to protect against changing the binding
- * of this driver to this device, as well as unloading its
- * driver module.
- */
- lock_kernel ();
+ down_read(&usb_bus_type.subsys.rwsem);
driver = ifp->driver;
if (driver == 0 || driver->ioctl == 0) {
- unlock_kernel();
- retval = -ENOSYS;
+ retval = -ENOTTY;
} else {
- if (!try_module_get (driver->owner)) {
- unlock_kernel();
- retval = -ENOSYS;
- break;
- }
- unlock_kernel ();
retval = driver->ioctl (ifp, ctrl.ioctl_code, buf);
if (retval == -ENOIOCTLCMD)
retval = -ENOTTY;
- module_put (driver->owner);
}
+ up_read(&usb_bus_type.subsys.rwsem);
}
+ up(&ps->dev->serialize);
/* cleanup and return */
if (retval >= 0
