ChangeSet 1.1325.4.18, 2003/09/24 16:57:17-07:00, david-b@pacbell.net
[PATCH] USB: usb_set_configuration() rework (v2)
This is the latest update of the patch resolving bugs in how device
configurations were reflected in the driver model. It addresses
the last significant problems I know about in that area.
- Moves code around so that usb_set_configuration() updates sysfs
to reflect the current configuration. Previously, that only
worked right for the initial configuration chosen by khubd.
* Previous interfaces are inaccessible. The code to handle this
moved from usb_disconnect() into usb_disable_device(), which
is now called both on disconnect and set_configuration paths.
* There are new interfaces. The code to handle this moved
from usb_new_device() into usb_set_configuration().
* Resolves a double-refcount problem with USB interfaces,
by not getting the extra reference in the first place
and switching to use device_del() to disable interfaces.
* Comments a similar double-refcount problem with usb
devices (not interfaces).
Its kerneldoc is updated appropriately. The main point being
that calling usb_set_configuration() in driver probe() methods
is even more of a no-no, since it'll self-deadlock.
- Sysfs names for USB interfaces now include the configuation
number, so that user mode code can't get as easily confused.
Old style: "3-1:0" for configs 2 and 3 (interface zero).
New style: "3-1:2.0" for config 2, "3-3:3.0" for config 3.
- Moves usb_new_device() code around a bit, so that the device
is visible in sysfs before usb_set_configuration() is called.
(Before the devices for that config's interfaces appear.)
- Makes the bConfigurationValue be writable through sysfs, so
device configurations can be easily changed from user mode.
(Or devices can be de-configured, by setting config 0.)
There are devices that can benefit from this functionality;
notably, cdc-acm modems need it right now, so that they can
be told to use the non-proprietary configuration. (Since
the old "change config in probe callback" trick won't work.)
drivers/usb/core/config.c | 3 -
drivers/usb/core/driverfs.c | 26 ++++++++-
drivers/usb/core/message.c | 126 ++++++++++++++++++++++++++++++++------------
drivers/usb/core/usb.c | 80 ++++++++++-----------------
4 files changed, 148 insertions(+), 87 deletions(-)
diff -Nru a/drivers/usb/core/config.c b/drivers/usb/core/config.c
--- a/drivers/usb/core/config.c Thu Sep 25 14:31:01 2003
+++ b/drivers/usb/core/config.c Thu Sep 25 14:31:01 2003
@@ -237,9 +237,6 @@
memset(interface, 0, sizeof(struct usb_interface));
interface->dev.release = usb_release_intf;
device_initialize(&interface->dev);
-
- /* put happens in usb_destroy_configuration */
- get_device(&interface->dev);
}
/* Go through the descriptors, checking their length and counting the
diff -Nru a/drivers/usb/core/driverfs.c b/drivers/usb/core/driverfs.c
--- a/drivers/usb/core/driverfs.c Thu Sep 25 14:31:01 2003
+++ b/drivers/usb/core/driverfs.c Thu Sep 25 14:31:01 2003
@@ -23,21 +23,43 @@
#include "usb.h"
/* Active configuration fields */
-#define usb_actconfig_attr(field, format_string) \
+#define usb_actconfig_show(field, format_string) \
static ssize_t \
show_##field (struct device *dev, char *buf) \
{ \
struct usb_device *udev; \
\
udev = to_usb_device (dev); \
+ if (udev->actconfig) \
return sprintf (buf, format_string, udev->actconfig->desc.field); \
+ else return 0; \
} \
+
+#define usb_actconfig_attr(field, format_string) \
+usb_actconfig_show(field,format_string) \
static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL);
usb_actconfig_attr (bNumInterfaces, "%2d\n")
-usb_actconfig_attr (bConfigurationValue, "%2d\n")
usb_actconfig_attr (bmAttributes, "%2x\n")
usb_actconfig_attr (bMaxPower, "%3dmA\n")
+
+/* configuration value is always present, and r/w */
+usb_actconfig_show(bConfigurationValue,"%u\n");
+
+static ssize_t
+set_bConfigurationValue (struct device *dev, const char *buf, size_t count)
+{
+ struct usb_device *udev = udev = to_usb_device (dev);
+ int config, value;
+
+ if (sscanf (buf, "%u", &config) != 1 || config > 255)
+ return -EINVAL;
+ value = usb_set_configuration (udev, config);
+ return (value < 0) ? value : count;
+}
+
+static DEVICE_ATTR(bConfigurationValue, S_IRUGO | S_IWUSR,
+ show_bConfigurationValue, set_bConfigurationValue);
/* String fields */
static ssize_t show_product (struct device *dev, char *buf)
diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c
--- a/drivers/usb/core/message.c Thu Sep 25 14:31:01 2003
+++ b/drivers/usb/core/message.c Thu Sep 25 14:31:01 2003
@@ -781,18 +781,40 @@
* @skip_ep0: 0 to disable endpoint 0, 1 to skip it.
*
* Disables all the device's endpoints, potentially including endpoint 0.
- * Deallocates hcd/hardware state for the endpoints ... and nukes all
- * pending urbs.
+ * Deallocates hcd/hardware state for the endpoints (nuking all or most
+ * pending urbs) and usbcore state for the interfaces, so that usbcore
+ * must usb_set_configuration() before any interfaces could be used.
*/
void usb_disable_device(struct usb_device *dev, int skip_ep0)
{
int i;
- dbg("nuking URBs for device %s", dev->dev.bus_id);
+ dev_dbg(&dev->dev, "%s nuking %s URBs\n", __FUNCTION__,
+ skip_ep0 ? "non-ep0" : "all");
for (i = skip_ep0; i < 16; ++i) {
usb_disable_endpoint(dev, i);
usb_disable_endpoint(dev, i + USB_DIR_IN);
}
+ dev->toggle[0] = dev->toggle[1] = 0;
+ dev->halted[0] = dev->halted[1] = 0;
+
+ /* getting rid of interfaces will disconnect
+ * any drivers bound to them (a key side effect)
+ */
+ if (dev->actconfig) {
+ for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
+ struct usb_interface *interface;
+
+ /* remove this interface */
+ interface = dev->actconfig->interface[i];
+ dev_dbg (&dev->dev, "unregistering interface %s\n",
+ interface->dev.bus_id);
+ device_del(&interface->dev);
+ }
+ dev->actconfig = 0;
+ if (dev->state == USB_STATE_CONFIGURED)
+ dev->state = USB_STATE_ADDRESS;
+ }
}
@@ -979,6 +1001,9 @@
int i, retval;
struct usb_host_config *config;
+ /* dev->serialize guards all config changes */
+ down(&dev->serialize);
+
for (i = 1; i < 16; ++i) {
usb_disable_endpoint(dev, i);
usb_disable_endpoint(dev, i + USB_DIR_IN);
@@ -989,8 +1014,10 @@
USB_REQ_SET_CONFIGURATION, 0,
config->desc.bConfigurationValue, 0,
NULL, 0, HZ * USB_CTRL_SET_TIMEOUT);
- if (retval < 0)
- return retval;
+ if (retval < 0) {
+ dev->state = USB_STATE_ADDRESS;
+ goto done;
+ }
dev->toggle[0] = dev->toggle[1] = 0;
dev->halted[0] = dev->halted[1] = 0;
@@ -1002,7 +1029,9 @@
intf->act_altsetting = 0;
usb_enable_interface(dev, intf);
}
- return 0;
+done:
+ up(&dev->serialize);
+ return (retval < 0) ? retval : 0;
}
/**
@@ -1012,71 +1041,104 @@
* Context: !in_interrupt ()
*
* This is used to enable non-default device modes. Not all devices
- * support this kind of configurability. By default, configuration
- * zero is selected after enumeration; many devices only have a single
+ * use this kind of configurability; many devices only have one
* configuration.
*
- * USB devices may support one or more configurations, which affect
+ * USB device configurations may affect Linux interoperability,
* power consumption and the functionality available. For example,
* the default configuration is limited to using 100mA of bus power,
* so that when certain device functionality requires more power,
- * and the device is bus powered, that functionality will be in some
+ * and the device is bus powered, that functionality should be in some
* non-default device configuration. Other device modes may also be
* reflected as configuration options, such as whether two ISDN
- * channels are presented as independent 64Kb/s interfaces or as one
- * bonded 128Kb/s interface.
+ * channels are available independently; and choosing between open
+ * standard device protocols (like CDC) or proprietary ones.
*
* Note that USB has an additional level of device configurability,
* associated with interfaces. That configurability is accessed using
* usb_set_interface().
*
- * This call is synchronous, and may not be used in an interrupt context.
+ * This call is synchronous. The calling context must be able to sleep,
+ * and must not hold the driver model lock for USB; usb device driver
+ * probe() methods may not use this routine.
*
* Returns zero on success, or else the status code returned by the
- * underlying usb_control_msg() call.
+ * underlying call that failed. On succesful completion, each interface
+ * in the original device configuration has been destroyed, and each one
+ * in the new configuration has been probed by all relevant usb device
+ * drivers currently known to the kernel.
*/
int usb_set_configuration(struct usb_device *dev, int configuration)
{
int i, ret;
struct usb_host_config *cp = NULL;
+ /* dev->serialize guards all config changes */
+ down(&dev->serialize);
+
for (i=0; i<dev->descriptor.bNumConfigurations; i++) {
if (dev->config[i].desc.bConfigurationValue == configuration) {
cp = &dev->config[i];
break;
}
}
- if ((!cp && configuration != 0) || (cp && configuration == 0)) {
- warn("selecting invalid configuration %d", configuration);
- return -EINVAL;
+ if ((!cp && configuration != 0)) {
+ ret = -EINVAL;
+ goto out;
}
+ if (cp && configuration == 0)
+ dev_warn(&dev->dev, "config 0 descriptor??\n");
- /* if it's already configured, clear out old state first. */
+ /* 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
- dev->toggle[0] = dev->toggle[1] = 0;
- dev->halted[0] = dev->halted[1] = 0;
- dev->state = USB_STATE_ADDRESS;
if ((ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
USB_REQ_SET_CONFIGURATION, 0, configuration, 0,
NULL, 0, HZ * USB_CTRL_SET_TIMEOUT)) < 0)
- return ret;
- if (configuration)
- dev->state = USB_STATE_CONFIGURED;
- dev->actconfig = cp;
+ goto out;
- /* reset more hc/hcd interface/endpoint state */
- for (i = 0; i < cp->desc.bNumInterfaces; ++i) {
- struct usb_interface *intf = cp->interface[i];
+ dev->actconfig = cp;
+ if (!configuration)
+ dev->state = USB_STATE_ADDRESS;
+ else {
+ dev->state = USB_STATE_CONFIGURED;
- intf->act_altsetting = 0;
- usb_enable_interface(dev, intf);
+ /* re-initialize hc/hcd/usbcore interface/endpoint state.
+ * this triggers binding of drivers to interfaces; and
+ * maybe probe() calls will choose different altsettings.
+ */
+ for (i = 0; i < cp->desc.bNumInterfaces; ++i) {
+ struct usb_interface *intf = cp->interface[i];
+ struct usb_interface_descriptor *desc;
+
+ intf->act_altsetting = 0;
+ desc = &intf->altsetting [0].desc;
+ usb_enable_interface(dev, intf);
+
+ intf->dev.parent = &dev->dev;
+ intf->dev.driver = NULL;
+ intf->dev.bus = &usb_bus_type;
+ intf->dev.dma_mask = dev->dev.dma_mask;
+ sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d",
+ dev->bus->busnum, dev->devpath,
+ configuration,
+ desc->bInterfaceNumber);
+ dev_dbg (&dev->dev,
+ "registering %s (config #%d, interface %d)\n",
+ intf->dev.bus_id, configuration,
+ desc->bInterfaceNumber);
+ device_add (&intf->dev);
+ usb_create_driverfs_intf_files (intf);
+ }
}
- return 0;
+out:
+ up(&dev->serialize);
+ return ret;
}
-
/**
* usb_string - returns ISO 8859-1 version of a string descriptor
diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
--- a/drivers/usb/core/usb.c Thu Sep 25 14:31:01 2003
+++ b/drivers/usb/core/usb.c Thu Sep 25 14:31:01 2003
@@ -898,6 +898,7 @@
* this device will fail.
*/
dev->state = USB_STATE_NOTATTACHED;
+ down(&dev->serialize);
dev_info (&dev->dev, "USB disconnect, address %d\n", dev->devnum);
@@ -908,31 +909,23 @@
usb_disconnect(child);
}
- /* deallocate hcd/hardware state ... and nuke all pending urbs */
+ /* deallocate hcd/hardware state ... nuking all pending urbs and
+ * cleaning up all state associated with the current configuration
+ */
usb_disable_device(dev, 0);
- /* disconnect() drivers from interfaces (a key side effect) */
- dev_dbg (&dev->dev, "unregistering interfaces\n");
- if (dev->actconfig) {
- for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
- struct usb_interface *interface;
-
- /* remove this interface */
- interface = dev->actconfig->interface[i];
- device_unregister(&interface->dev);
- }
- }
-
dev_dbg (&dev->dev, "unregistering device\n");
/* Free the device number and remove the /proc/bus/usb entry */
if (dev->devnum > 0) {
clear_bit(dev->devnum, dev->bus->devmap.devicemap);
usbfs_remove_device(dev);
}
+ up(&dev->serialize);
device_unregister(&dev->dev);
/* Decrement the reference count, it'll auto free everything when */
/* it hits 0 which could very well be now */
+ /* FIXME the decrement in device_unregister() should suffice ... */
usb_put_dev(dev);
}
@@ -1090,27 +1083,12 @@
err = usb_get_configuration(dev);
if (err < 0) {
- dev_err(&dev->dev, "unable to get device %d configuration (error=%d)\n",
- dev->devnum, err);
- goto fail;
- }
-
- /* choose and set the configuration here */
- if (dev->descriptor.bNumConfigurations != 1) {
- dev_info(&dev->dev,
- "configuration #%d chosen from %d choices\n",
- dev->config[0].desc.bConfigurationValue,
- dev->descriptor.bNumConfigurations);
- }
- err = usb_set_configuration(dev, dev->config[0].desc.bConfigurationValue);
- if (err) {
- dev_err(&dev->dev, "failed to set device %d default configuration (error=%d)\n",
- dev->devnum, err);
+ dev_err(&dev->dev, "can't read configurations, error %d\n",
+ err);
goto fail;
}
- /* USB device state == configured ... tell the world! */
-
+ /* Tell the world! */
dev_dbg(&dev->dev, "new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
dev->descriptor.iManufacturer, dev->descriptor.iProduct, dev->descriptor.iSerialNumber);
@@ -1123,30 +1101,32 @@
usb_show_string(dev, "SerialNumber", dev->descriptor.iSerialNumber);
#endif
- /* put into sysfs, with device and config specific files */
+ /* put device-specific files into sysfs */
err = device_add (&dev->dev);
- if (err)
+ if (err) {
+ dev_err(&dev->dev, "can't device_add, error %d\n", err);
goto fail;
+ }
usb_create_driverfs_dev_files (dev);
- /* Register all of the interfaces for this device with the driver core.
- * Remember, interfaces get bound to drivers, not devices. */
- for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
- struct usb_interface *interface = dev->actconfig->interface[i];
- struct usb_interface_descriptor *desc;
-
- desc = &interface->altsetting [interface->act_altsetting].desc;
- interface->dev.parent = &dev->dev;
- interface->dev.driver = NULL;
- interface->dev.bus = &usb_bus_type;
- interface->dev.dma_mask = parent->dma_mask;
- sprintf (&interface->dev.bus_id[0], "%d-%s:%d",
- dev->bus->busnum, dev->devpath,
- desc->bInterfaceNumber);
- dev_dbg (&dev->dev, "%s - registering interface %s\n", __FUNCTION__, interface->dev.bus_id);
- device_add (&interface->dev);
- usb_create_driverfs_intf_files (interface);
+ /* choose and set the configuration. that registers the interfaces
+ * with the driver core, and lets usb device drivers bind to them.
+ */
+ if (dev->descriptor.bNumConfigurations != 1) {
+ dev_info(&dev->dev,
+ "configuration #%d chosen from %d choices\n",
+ dev->config[0].desc.bConfigurationValue,
+ dev->descriptor.bNumConfigurations);
+ }
+ err = usb_set_configuration(dev,
+ dev->config[0].desc.bConfigurationValue);
+ if (err) {
+ dev_err(&dev->dev, "can't set config #%d, error %d\n",
+ dev->config[0].desc.bConfigurationValue, err);
+ goto fail;
}
+
+ /* USB device state == configured ... usable */
/* add a /proc/bus/usb entry */
usbfs_add_device(dev);