ChangeSet 1.1673.8.49, 2004/03/30 09:25:17-08:00, david-b@pacbell.net [PATCH] USB: set_configuration locking cleanups I've posted all these before, the only notable change is treating that one gphoto2 case as warn-and-continue rather than return-with-failure. usb_set_configuration() cleanup * Remove it from the USB kernel driver API. No drivers need it now, and the sysadmin can change bConfigurationValue using sysfs (say, when hotplugging an otherwise problematic device). * Simpler/cleaner locking: caller must own dev->serialize. * Access from usbfs now uses usb_reset_configuration() sometimes, preventing sysfs thrash, and warns about some dangerous usage (which gphoto2 and other programs may be relying on). (This is from Alan Stern, but I morphed an error return into a warning.) * Prevent a couple potential "no configuration" oopses. (Alan's?) * Remove one broken call from usbcore, in the "device morphed" path of usb_reset_device(). This should be more polite now, hanging that one device rather than khubd. drivers/usb/core/devio.c | 47 +++++++++++++++++++++++++++++++++++++++++++- drivers/usb/core/driverfs.c | 2 + drivers/usb/core/hub.c | 34 +++++++------------------------ drivers/usb/core/message.c | 7 +----- drivers/usb/core/usb.c | 4 +++ drivers/usb/core/usb.h | 1 include/linux/usb.h | 1 7 files changed, 63 insertions(+), 33 deletions(-) diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c --- a/drivers/usb/core/devio.c Wed Apr 14 14:36:02 2004 +++ b/drivers/usb/core/devio.c Wed Apr 14 14:36:02 2004 @@ -430,6 +430,8 @@ if (ep & ~(USB_DIR_IN|0xf)) return -EINVAL; + if (!dev->actconfig) + return -ESRCH; for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) { iface = dev->actconfig->interface[i]; for (j = 0; j < iface->num_altsetting; j++) { @@ -450,6 +452,8 @@ if (ifn & ~0xff) return -EINVAL; + if (!dev->actconfig) + return -ESRCH; for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) { if (dev->actconfig->interface[i]-> altsetting[0].desc.bInterfaceNumber == ifn) @@ -766,10 +770,51 @@ static int proc_setconfig(struct dev_state *ps, void __user *arg) { unsigned int u; + int status = 0; + struct usb_host_config *actconfig; if (get_user(u, (unsigned int __user *)arg)) return -EFAULT; - return usb_set_configuration(ps->dev, u); + + down(&ps->dev->serialize); + actconfig = ps->dev->actconfig; + + /* Don't touch the device if any interfaces are claimed. + * It could interfere with other drivers' operations, and if + * an interface is claimed by usbfs it could easily deadlock. + */ + if (actconfig) { + int i; + + for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) { + if (usb_interface_claimed(actconfig->interface[i])) { + dev_warn (&ps->dev->dev, + "usbfs: interface %d claimed " + "while '%s' sets config #%d\n", + actconfig->interface[i] + ->cur_altsetting + ->desc.bInterfaceNumber, + current->comm, u); +#if 0 /* FIXME: enable in 2.6.10 or so */ + status = -EBUSY; + break; +#endif + } + } + } + + /* SET_CONFIGURATION is often abused as a "cheap" driver reset, + * so avoid usb_set_configuration()'s kick to sysfs + */ + if (status == 0) { + if (actconfig && actconfig->desc.bConfigurationValue == u) + status = usb_reset_configuration(ps->dev); + else + status = usb_set_configuration(ps->dev, u); + } + up(&ps->dev->serialize); + + return status; } static int proc_submiturb(struct dev_state *ps, void __user *arg) diff -Nru a/drivers/usb/core/driverfs.c b/drivers/usb/core/driverfs.c --- a/drivers/usb/core/driverfs.c Wed Apr 14 14:36:02 2004 +++ b/drivers/usb/core/driverfs.c Wed Apr 14 14:36:02 2004 @@ -55,7 +55,9 @@ if (sscanf (buf, "%u", &config) != 1 || config > 255) return -EINVAL; + down(&udev->serialize); value = usb_set_configuration (udev, config); + up(&udev->serialize); return (value < 0) ? value : count; } diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c --- a/drivers/usb/core/hub.c Wed Apr 14 14:36:02 2004 +++ b/drivers/usb/core/hub.c Wed Apr 14 14:36:02 2004 @@ -1299,33 +1299,15 @@ kfree(descriptor); usb_destroy_configuration(dev); - ret = usb_get_device_descriptor(dev, sizeof(dev->descriptor)); - if (ret != sizeof(dev->descriptor)) { - if (ret < 0) - err("unable to get device %s descriptor " - "(error=%d)", dev->devpath, ret); - else - err("USB device %s descriptor short read " - "(expected %Zi, got %i)", - dev->devpath, - sizeof(dev->descriptor), ret); + /* FIXME Linux doesn't yet handle these "device morphed" + * paths. DFU variants need this to work ... and they + * include the "config descriptors changed" case this + * doesn't yet detect! + */ + dev->state = USB_STATE_NOTATTACHED; + dev_err(&dev->dev, "device morphed (DFU?), nyet supported\n"); - clear_bit(dev->devnum, dev->bus->devmap.devicemap); - dev->devnum = -1; - return -EIO; - } - - ret = usb_get_configuration(dev); - if (ret < 0) { - err("unable to get configuration (error=%d)", ret); - usb_destroy_configuration(dev); - clear_bit(dev->devnum, dev->bus->devmap.devicemap); - dev->devnum = -1; - return 1; - } - - usb_set_configuration(dev, dev->config[0].desc.bConfigurationValue); - return 1; + return -ENODEV; } kfree(descriptor); diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c --- a/drivers/usb/core/message.c Wed Apr 14 14:36:02 2004 +++ b/drivers/usb/core/message.c Wed Apr 14 14:36:02 2004 @@ -1075,11 +1075,11 @@ return 0; } -/** +/* * usb_set_configuration - Makes a particular device setting be current * @dev: the device whose configuration is being updated * @configuration: the configuration being chosen. - * Context: !in_interrupt () + * Context: !in_interrupt(), caller holds dev->serialize * * This is used to enable non-default device modes. Not all devices * use this kind of configurability; many devices only have one @@ -1115,7 +1115,6 @@ struct usb_host_config *cp = NULL; /* dev->serialize guards all config changes */ - down(&dev->serialize); for (i=0; idescriptor.bNumConfigurations; i++) { if (dev->config[i].desc.bConfigurationValue == configuration) { @@ -1191,7 +1190,6 @@ } out: - up(&dev->serialize); return ret; } @@ -1300,6 +1298,5 @@ // synchronous calls that also maintain usbcore state EXPORT_SYMBOL(usb_clear_halt); EXPORT_SYMBOL(usb_reset_configuration); -EXPORT_SYMBOL(usb_set_configuration); EXPORT_SYMBOL(usb_set_interface); diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c --- a/drivers/usb/core/usb.c Wed Apr 14 14:36:02 2004 +++ b/drivers/usb/core/usb.c Wed Apr 14 14:36:02 2004 @@ -1173,10 +1173,13 @@ usb_show_string(dev, "SerialNumber", dev->descriptor.iSerialNumber); #endif + down(&dev->serialize); + /* put device-specific files into sysfs */ err = device_add (&dev->dev); if (err) { dev_err(&dev->dev, "can't device_add, error %d\n", err); + up(&dev->serialize); goto fail; } usb_create_driverfs_dev_files (dev); @@ -1211,6 +1214,7 @@ dev->descriptor.bNumConfigurations); } err = usb_set_configuration(dev, config); + up(&dev->serialize); if (err) { dev_err(&dev->dev, "can't set config #%d, error %d\n", config, err); diff -Nru a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h --- a/drivers/usb/core/usb.h Wed Apr 14 14:36:02 2004 +++ b/drivers/usb/core/usb.h Wed Apr 14 14:36:02 2004 @@ -17,6 +17,7 @@ extern int usb_get_device_descriptor(struct usb_device *dev, unsigned int size); +extern int usb_set_configuration(struct usb_device *dev, int configuration); /* for labeling diagnostics */ extern const char *usbcore_name; diff -Nru a/include/linux/usb.h b/include/linux/usb.h --- a/include/linux/usb.h Wed Apr 14 14:36:02 2004 +++ b/include/linux/usb.h Wed Apr 14 14:36:02 2004 @@ -904,7 +904,6 @@ /* wrappers that also update important state inside usbcore */ extern int usb_clear_halt(struct usb_device *dev, int pipe); extern int usb_reset_configuration(struct usb_device *dev); -extern int usb_set_configuration(struct usb_device *dev, int configuration); extern int usb_set_interface(struct usb_device *dev, int ifnum, int alternate); /*