ChangeSet 1.1123.18.8, 2003/08/11 16:20:45-07:00, david-b@pacbell.net [PATCH] add usb_reset_configuration() Unfortunately, usb_set_configuration() is widely mis-used as a lightweight device reset. That's trouble because setting a configuration must sometimes involve things that don't relate at all to a light reset, and can't be done in contexts like driver probe() calls. This patch updates most usb_set_configuration() users to use a call that provides more appropriate functionality: - Adds a new usb_reset_configuration() call, which never needs to change very much usbcore state. - Uses it to replace most usb_set_configuration() calls, in many serial drivers, hisax, dvb, irda, and so on. - Modifies usb_reset_device() so it issues the control request directly. It's both more of a reset (hides a USB reset) and less of one (altsettings are unchanged). - Makes usbfs return the error code instead of discarding it. Once this goes in, then usb_set_configuration() can be made to work properly (including from sysfs). drivers/isdn/hisax/st5481_usb.c | 4 - drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c | 2 drivers/net/irda/irda-usb.c | 6 +- drivers/usb/class/cdc-acm.c | 9 +++ drivers/usb/core/devio.c | 4 - drivers/usb/core/hub.c | 5 +- drivers/usb/core/message.c | 52 ++++++++++++++++++++++ drivers/usb/media/dabusb.c | 4 - drivers/usb/media/stv680.c | 5 +- drivers/usb/misc/tiglusb.c | 8 ++- drivers/usb/serial/empeg.c | 9 ++- drivers/usb/serial/ipaq.c | 8 ++- drivers/usb/serial/visor.c | 10 +++- drivers/usb/storage/usb.c | 9 ++- include/linux/usb.h | 1 sound/usb/usbaudio.c | 8 +-- 16 files changed, 114 insertions(+), 30 deletions(-) diff -Nru a/drivers/isdn/hisax/st5481_usb.c b/drivers/isdn/hisax/st5481_usb.c --- a/drivers/isdn/hisax/st5481_usb.c Fri Aug 15 10:47:16 2003 +++ b/drivers/isdn/hisax/st5481_usb.c Fri Aug 15 10:47:16 2003 @@ -252,8 +252,8 @@ DBG(1,""); - if ((status = usb_set_configuration (dev,dev->config[0].desc.bConfigurationValue)) < 0) { - WARN("set_configuration failed,status=%d",status); + if ((status = usb_reset_configuration (dev)) < 0) { + WARN("reset_configuration failed,status=%d",status); return status; } diff -Nru a/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c --- a/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c Fri Aug 15 10:47:16 2003 +++ b/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c Fri Aug 15 10:47:16 2003 @@ -1017,7 +1017,7 @@ static int ttusb_setup_interfaces(struct ttusb *ttusb) { - usb_set_configuration(ttusb->dev, 1); + usb_reset_configuration(ttusb->dev); usb_set_interface(ttusb->dev, 1, 1); ttusb->bulk_out_pipe = usb_sndbulkpipe(ttusb->dev, 1); diff -Nru a/drivers/net/irda/irda-usb.c b/drivers/net/irda/irda-usb.c --- a/drivers/net/irda/irda-usb.c Fri Aug 15 10:47:16 2003 +++ b/drivers/net/irda/irda-usb.c Fri Aug 15 10:47:16 2003 @@ -1482,9 +1482,9 @@ goto err_out_2; } - /* Is this really necessary? */ - if (usb_set_configuration (dev, dev->config[0].desc.bConfigurationValue) < 0) { - err("set_configuration failed"); + /* Is this really necessary? (no, except maybe for broken devices) */ + if (usb_reset_configuration (dev) < 0) { + err("reset_configuration failed"); ret = -EIO; goto err_out_3; } diff -Nru a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c --- a/drivers/usb/class/cdc-acm.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/class/cdc-acm.c Fri Aug 15 10:47:16 2003 @@ -593,7 +593,14 @@ epwrite = &ifdata->endpoint[0].desc; } - usb_set_configuration(dev, cfacm->desc.bConfigurationValue); + /* FIXME don't scan every config. it's either correct + * when we probe(), or some other task must fix this. + */ + if (dev->actconfig != cfacm) { + err("need inactive config #%d", + cfacm->desc.bConfigurationValue); + return -ENODEV; + } for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++); if (acm_table[minor]) { diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c --- a/drivers/usb/core/devio.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/core/devio.c Fri Aug 15 10:47:16 2003 @@ -762,9 +762,7 @@ if (get_user(u, (unsigned int __user *)arg)) return -EFAULT; - if (usb_set_configuration(ps->dev, u) < 0) - return -EINVAL; - return 0; + return usb_set_configuration(ps->dev, u); } static int proc_submiturb(struct dev_state *ps, void __user *arg) diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c --- a/drivers/usb/core/hub.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/core/hub.c Fri Aug 15 10:47:16 2003 @@ -1335,7 +1335,10 @@ kfree(descriptor); - ret = usb_set_configuration(dev, dev->actconfig->desc.bConfigurationValue); + ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), + USB_REQ_SET_CONFIGURATION, 0, + dev->actconfig->desc.bConfigurationValue, 0, + NULL, 0, HZ * USB_CTRL_SET_TIMEOUT); if (ret < 0) { err("failed to set dev %s active configuration (error=%d)", dev->devpath, ret); diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c --- a/drivers/usb/core/message.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/core/message.c Fri Aug 15 10:47:16 2003 @@ -964,6 +964,55 @@ } /** + * usb_reset_configuration - lightweight device reset + * @dev: the device whose configuration is being reset + * + * This issues a standard SET_CONFIGURATION request to the device using + * the current configuration. The effect is to reset most USB-related + * state in the device, including interface altsettings (reset to zero), + * endpoint halts (cleared), and data toggle (only for bulk and interrupt + * endpoints). Other usbcore state is unchanged, including bindings of + * usb device drivers to interfaces. + * + * Because this affects multiple interfaces, avoid using this with composite + * (multi-interface) devices. Instead, the driver for each interface may + * use usb_set_interface() on the interfaces it claims. Resetting the whole + * configuration would affect other drivers' interfaces. + * + * Returns zero on success, else a negative error code. + */ +int usb_reset_configuration(struct usb_device *dev) +{ + int i, retval; + struct usb_host_config *config; + + for (i = 1; i < 16; ++i) { + usb_disable_endpoint(dev, i); + usb_disable_endpoint(dev, i + USB_DIR_IN); + } + + config = dev->actconfig; + retval = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), + USB_REQ_SET_CONFIGURATION, 0, + config->desc.bConfigurationValue, 0, + NULL, 0, HZ * USB_CTRL_SET_TIMEOUT); + if (retval < 0) + return retval; + + dev->toggle[0] = dev->toggle[1] = 0; + dev->halted[0] = dev->halted[1] = 0; + + /* re-init hc/hcd interface/endpoint state */ + for (i = 0; i < config->desc.bNumInterfaces; i++) { + struct usb_interface *intf = config->interface[i]; + + intf->act_altsetting = 0; + usb_enable_interface(dev, intf); + } + 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. @@ -1136,7 +1185,10 @@ EXPORT_SYMBOL(usb_get_status); EXPORT_SYMBOL(usb_get_string); EXPORT_SYMBOL(usb_string); + +// 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/media/dabusb.c b/drivers/usb/media/dabusb.c --- a/drivers/usb/media/dabusb.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/media/dabusb.c Fri Aug 15 10:47:16 2003 @@ -747,8 +747,8 @@ s->usbdev = usbdev; s->devnum = intf->minor; - if (usb_set_configuration (usbdev, usbdev->config[0].desc.bConfigurationValue) < 0) { - err("set_configuration failed"); + if (usb_reset_configuration (usbdev) < 0) { + err("reset_configuration failed"); goto reject; } if (usbdev->descriptor.idProduct == 0x2131) { diff -Nru a/drivers/usb/media/stv680.c b/drivers/usb/media/stv680.c --- a/drivers/usb/media/stv680.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/media/stv680.c Fri Aug 15 10:47:16 2003 @@ -225,8 +225,9 @@ static int stv_set_config (struct usb_stv *dev, int configuration, int interface, int alternate) { - if (usb_set_configuration (dev->udev, configuration) < 0) { - PDEBUG (1, "STV(e): FAILED to set configuration %i", configuration); + if (configuration != dev->udev->actconfig->desc.bConfigurationValue + || usb_reset_configuration (dev->udev) < 0) { + PDEBUG (1, "STV(e): FAILED to reset configuration %i", configuration); return -1; } if (usb_set_interface (dev->udev, interface, alternate) < 0) { diff -Nru a/drivers/usb/misc/tiglusb.c b/drivers/usb/misc/tiglusb.c --- a/drivers/usb/misc/tiglusb.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/misc/tiglusb.c Fri Aug 15 10:47:16 2003 @@ -57,7 +57,7 @@ static inline int clear_device (struct usb_device *dev) { - if (usb_set_configuration (dev, dev->config[0].desc.bConfigurationValue) < 0) { + if (usb_reset_configuration (dev) < 0) { err ("clear_device failed"); return -1; } @@ -343,8 +343,10 @@ && (dev->descriptor.idVendor != 0x451)) return -ENODEV; - if (usb_set_configuration (dev, dev->config[0].desc.bConfigurationValue) < 0) { - err ("tiglusb_probe: set_configuration failed"); + // NOTE: it's already in this config, this shouldn't be needed. + // is this working around some hardware bug? + if (usb_reset_configuration (dev) < 0) { + err ("tiglusb_probe: reset_configuration failed"); return -ENODEV; } diff -Nru a/drivers/usb/serial/empeg.c b/drivers/usb/serial/empeg.c --- a/drivers/usb/serial/empeg.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/serial/empeg.c Fri Aug 15 10:47:16 2003 @@ -464,8 +464,13 @@ dbg("%s", __FUNCTION__); - dbg("%s - Set config to 1", __FUNCTION__); - r = usb_set_configuration (serial->dev, 1); + if (serial->dev->actconfig->desc.bConfigurationValue != 1) { + err("active config #%d != 1 ??", + serial->dev->actconfig->desc.bConfigurationValue); + return -ENODEV; + } + dbg("%s - reset config", __FUNCTION__); + r = usb_reset_configuration (serial->dev); /* continue on with initialization */ return r; diff -Nru a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c --- a/drivers/usb/serial/ipaq.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/serial/ipaq.c Fri Aug 15 10:47:16 2003 @@ -555,8 +555,12 @@ static int ipaq_startup(struct usb_serial *serial) { dbg("%s", __FUNCTION__); - usb_set_configuration(serial->dev, 1); - return 0; + if (serial->dev->actconfig->desc.bConfigurationValue != 1) { + err("active config #%d != 1 ??", + serial->dev->actconfig->desc.bConfigurationValue); + return -ENODEV; + } + return usb_reset_configuration (serial->dev); } static void ipaq_shutdown(struct usb_serial *serial) diff -Nru a/drivers/usb/serial/visor.c b/drivers/usb/serial/visor.c --- a/drivers/usb/serial/visor.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/serial/visor.c Fri Aug 15 10:47:16 2003 @@ -763,8 +763,14 @@ dbg("%s", __FUNCTION__); - dbg("%s - Set config to 1", __FUNCTION__); - usb_set_configuration (serial->dev, 1); + if (serial->dev->actconfig->desc.bConfigurationValue != 1) { + err("active config #%d != 1 ??", + serial->dev->actconfig->desc.bConfigurationValue); + return -ENODEV; + } + dbg("%s - reset config", __FUNCTION__); + retval = usb_reset_configuration (serial->dev); + if (id->driver_info) { startup = (void *)id->driver_info; diff -Nru a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c --- a/drivers/usb/storage/usb.c Fri Aug 15 10:47:16 2003 +++ b/drivers/usb/storage/usb.c Fri Aug 15 10:47:16 2003 @@ -921,9 +921,14 @@ if (us->protocol == US_PR_EUSB_SDDR09 || us->protocol == US_PR_DPCM_USB) { /* set the configuration -- STALL is an acceptable response here */ - result = usb_set_configuration(us->pusb_dev, 1); + if (us->pusb_dev->actconfig->desc.bConfigurationValue != 1) { + US_DEBUGP("active config #%d != 1 ??\n", us->pusb_dev + ->actconfig->desc.bConfigurationValue); + goto BadDevice; + } + result = usb_reset_configuration(us->pusb_dev); - US_DEBUGP("Result from usb_set_configuration is %d\n", result); + US_DEBUGP("Result of usb_reset_configuration is %d\n", result); if (result == -EPIPE) { US_DEBUGP("-- stall on control interface\n"); } else if (result != 0) { diff -Nru a/include/linux/usb.h b/include/linux/usb.h --- a/include/linux/usb.h Fri Aug 15 10:47:16 2003 +++ b/include/linux/usb.h Fri Aug 15 10:47:16 2003 @@ -869,6 +869,7 @@ /* 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); diff -Nru a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c --- a/sound/usb/usbaudio.c Fri Aug 15 10:47:16 2003 +++ b/sound/usb/usbaudio.c Fri Aug 15 10:47:16 2003 @@ -2560,8 +2560,8 @@ err = usb_get_device_descriptor(dev); config = dev->actconfig; if (err < 0) snd_printdd("error usb_get_device_descriptor: %d\n", err); - err = usb_set_configuration(dev, get_cfg_desc(config)->bConfigurationValue); - if (err < 0) snd_printdd("error usb_set_configuration: %d\n", err); + err = usb_reset_configuration(dev); + if (err < 0) snd_printdd("error usb_reset_configuration: %d\n", err); snd_printdd("extigy_boot: new boot length = %d\n", get_cfg_desc(config)->wTotalLength); return -ENODEV; /* quit this anyway */ } @@ -2761,8 +2761,8 @@ * now look for an empty slot and create a new card instance */ /* first, set the current configuration for this device */ - if (usb_set_configuration(dev, get_cfg_desc(config)->bConfigurationValue) < 0) { - snd_printk(KERN_ERR "cannot set configuration (value 0x%x)\n", get_cfg_desc(config)->bConfigurationValue); + if (usb_reset_configuration(dev) < 0) { + snd_printk(KERN_ERR "cannot reset configuration (value 0x%x)\n", get_cfg_desc(config)->bConfigurationValue); goto __error; } for (i = 0; i < SNDRV_CARDS; i++)