What I think I'll do is wrap up Alan's more-correct patch with a minor locking update that's been in my queue for some time, and ask you to test the combined patch.
And here it is.
I don't have particular interest in gphoto2 now, the more pressing issue that was starting this thread /for me/ is that the SuSE 8.2 default hotplug was able to screw the scanner so much it would bump its sled into the stopper with *loud* noise so it had to be unplugged to prevent permanent damage.
As I recall, there were two bugs there. One was a "usbfs broken" bug, which this patch should resolve. The other was a "khubd broken" problem, to be resolved by reverting a patch. You'd be needing both patches to have gphoto2 behave, but I think this one alone might be enough to shut the scanner up.
Of course, every drop in the ocean towards a better USB system is still appreciated, and I'll be happy to test your patch.
This should resolve the some of the issues reported on this thread, and I think it includes one of the changes you said did improve behavior with your scanner. I've been running with almost the same code for some months now, except for the usbfs changes that make it (finally!) reasonable to issue SET_CONFIGURATION.
- Dave
usb_set_configuration() cleanup
* Simplified locking rules to work better with upcoming usbcore
changes: caller must own dev->serialize.
* Remove it from the USB driver API. No drivers need it now, and
the sysadmin can change bConfigurationValue using sysfs.
* Fix access from usbfs, to prevent system thrash by user mode code.
(a) use usb_reset_configuration() where that's the real intent;
(b) insist the device have no drivers bound to it, avoiding some
deadlocks and similar driver confusion; and (c) prevent a couple
potential "no configuration" oopses.
* Remove one broken call from usbcore, in the "device morphed" path
of usb_reset_device(). Some DFU-ish devices previously deadlocked
here during probe, after the firmware was uploaded. Now they'll just
fail and disconnect. (Eventually they need to re-enumerate instead
of disconnecting.)
This is another in the series of cleanups that leads to more robust
device reset and enumeration processing. Those usbfs problems have
recently been reported through "gphoto2".
--- 1.100/include/linux/usb.h Tue Mar 16 10:14:00 2004
+++ edited/include/linux/usb.h Thu Mar 25 12:53:21 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);
/*
--- 1.4/drivers/usb/core/usb.h Mon Jan 26 05:46:24 2004
+++ edited/drivers/usb/core/usb.h Thu Mar 25 12:53:21 2004
@@ -17,3 +17,4 @@
extern int usb_get_device_descriptor(struct usb_device *dev,
unsigned int size);
+extern int usb_set_configuration(struct usb_device *dev, int configuration);
--- 1.44/drivers/usb/core/message.c Wed Mar 3 04:48:13 2004
+++ edited/drivers/usb/core/message.c Thu Mar 25 13:25:09 2004
@@ -1072,11 +1072,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
@@ -1112,7 +1112,6 @@
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) {
@@ -1188,7 +1187,6 @@
}
out:
- up(&dev->serialize);
return ret;
}
@@ -1295,6 +1293,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);
--- 1.56/drivers/usb/core/devio.c Mon Mar 1 03:40:11 2004
+++ edited/drivers/usb/core/devio.c Thu Mar 25 13:03:41 2004
@@ -414,6 +414,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++) {
@@ -434,6 +436,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)
@@ -750,10 +754,44 @@
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])) {
+ status = -EBUSY;
+ break;
+ }
+ }
+ }
+
+ /* Many drivers use SET_CONFIGURATION to force devices into a
+ * known state. SET_INTERFACE would generally make more sense,
+ * especially for composite devices, but maybe we can avoid
+ * thrashing the host side driver model code.
+ */
+ 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)
--- 1.9/drivers/usb/core/driverfs.c Mon Mar 1 03:40:11 2004
+++ edited/drivers/usb/core/driverfs.c Thu Mar 25 12:53:21 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;
}
--- 1.154/drivers/usb/core/usb.c Tue Mar 16 10:14:00 2004
+++ edited/drivers/usb/core/usb.c Thu Mar 25 12:53:21 2004
@@ -1164,10 +1164,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);
@@ -1202,6 +1205,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);
--- 1.88/drivers/usb/core/hub.c Mon Mar 1 03:40:11 2004
+++ edited/drivers/usb/core/hub.c Thu Mar 25 12:53:21 2004
@@ -1300,33 +1300,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);
vim: syntax=diff
