Am Dienstag, 15. Dezember 2009 16:42:54 schrieb Alan Stern:
> On Tue, 15 Dec 2009, Oliver Neukum wrote:
>
> > Am Montag, 14. Dezember 2009 23:12:06 schrieb Alan Stern:
> > Yes, I have some code to fall back to usb_reset_configuration().
> > Should we do so also for storage?
>
> I don't think that's such a good idea. For instance, I doubt
> usb_reset_configuration() will be powerful enough to recover from the
> errors usb-storage encounters. In fact, even a port reset isn't always
> powerful enough.
True, but we cannot do a port reset for such devices.
> Also it doesn't notify drivers about the reset. Really it was meant
> for use only with non-composite devices, where there's never more than
> one driver involved.
Yes, we'd need an extended version that calls pre/post_reset.
I am attaching the stuff I wrote before I looked closely at the
reset logic.
> > > Well, if we don't have any choice then it has to be done. Obviously a
> > > sysfs attribute is the best way. However I can't help thinking that
> > > this is only a symptom of something deeper, and it ought to be
> > > generalized. But it's not clear how...
> >
> > The obvious alternative is to put mode switching into the kernel.
> > If you don't want to do that, there'll be other drawbacks, we are seeing
> > here.
>
> Even if mode switching were in the kernel (and in a few cases it is),
> there still would be problems. Mode switching often involves changing
> descriptors. The core interprets these changes as meaning that the
> device has disconnected and another device was plugged into the port.
We'd have to save copies of descriptors in both modes, compare,
initiate a reswitch and wait for it. Extremely ugly.
> Face it, mode switching is a bad idea. These devices should have been
> designed to use multiple configurations instead. Since they weren't,
> their functionality is severely compromised: It's very difficult to
> reset them for error recovery.
I do face it. I am asking whether the unease you feel is due to sucking
hardware or have I overlooked something?
Regards
Oliver
commit d7253d1e33bd46c8237635763a65b5c2ef27486e
Author: Oliver Neukum <oliver@xxxxxxxxxx>
Date: Fri Nov 6 07:59:32 2009 +0100
usb:first draft of new quirk
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 03bd703..7d74b22 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -30,6 +30,7 @@
#include <linux/workqueue.h>
#include <linux/usb.h>
+#include <linux/usb/quirks.h>
#include <linux/hid.h>
#include <linux/hiddev.h>
@@ -123,10 +124,14 @@ static void hid_reset(struct work_struct *work)
}
else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
+
dev_dbg(&usbhid->intf->dev, "resetting device\n");
rc = usb_lock_device_for_reset(hid_to_usb_dev(hid), usbhid->intf);
if (rc == 0) {
- rc = usb_reset_device(hid_to_usb_dev(hid));
+ if (hid_to_usb_dev(hid)->quirks & USB_QUIRK_RESET_MORPHS)
+ rc = usb_multiple_reset_configuration(hid_to_usb_dev(hid));
+ else
+ rc = usb_reset_device(hid_to_usb_dev(hid));
usb_unlock_device(hid_to_usb_dev(hid));
}
clear_bit(HID_RESET_PENDING, &usbhid->iofl);
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index da718e8..06a856f 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1398,6 +1398,71 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate)
EXPORT_SYMBOL_GPL(usb_set_interface);
/**
+ * usb_multiple_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 endpoint state (only for bulk and interrupt
+ * endpoints). Other usbcore state is unchanged, including bindings of
+ * usb device drivers to interfaces.
+ *
+ * This version notifies other drivers via pre/post_reset if they
+ * support it or else unbinds and rebinds the drivers
+ *
+ * The caller must own the device lock.
+ *
+ * Returns zero on success, else a negative error code.
+ */
+int usb_multiple_reset_configuration(struct usb_device *dev)
+{
+ int result, i;
+ struct usb_host_config *config = dev->actconfig;
+
+ /* Prevent autosuspend during the reset */
+ usb_autoresume_device(dev);
+
+ for (i = 0; i < config->desc.bNumInterfaces; ++i) {
+ struct usb_interface *cintf = config->interface[i];
+ struct usb_driver *drv;
+ int unbind = 0;
+
+ if (cintf->dev.driver) {
+ drv = to_usb_driver(cintf->dev.driver);
+ if (drv->pre_reset && drv->post_reset)
+ unbind = (drv->pre_reset)(cintf);
+ else if (cintf->condition ==
+ USB_INTERFACE_BOUND)
+ unbind = 1;
+ if (unbind)
+ usb_forced_unbind_intf(cintf);
+ }
+ }
+
+ result = usb_reset_configuration(dev);
+
+ for (i = config->desc.bNumInterfaces - 1; i >= 0; --i) {
+ struct usb_interface *cintf = config->interface[i];
+ struct usb_driver *drv;
+ int rebind = cintf->needs_binding;
+
+ if (!rebind && cintf->dev.driver) {
+ drv = to_usb_driver(cintf->dev.driver);
+ if (drv->post_reset)
+ rebind = (drv->post_reset)(cintf);
+ else if (cintf->condition ==
+ USB_INTERFACE_BOUND)
+ rebind = 1;
+ }
+ if (result == 0 && rebind)
+ usb_rebind_intf(cintf);
+ }
+ return result;
+}
+EXPORT_SYMBOL_GPL(usb_multiple_reset_configuration);
+
+/**
* usb_reset_configuration - lightweight device reset
* @dev: the device whose configuration is being reset
*
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index ab93918..0b68922 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -120,6 +120,7 @@ void usb_detect_quirks(struct usb_device *udev)
* for all devices. It will affect things like hub resets
* and EMF-related port disables.
*/
- udev->persist_enabled = 1;
+ if (!(udev->quirks & USB_QUIRK_RESET_MORPHS))
+ udev->persist_enabled = 1;
#endif /* CONFIG_PM */
}
diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 589f6b4..cb12486 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -47,6 +47,8 @@
#include <linux/errno.h>
#include <linux/slab.h>
+#include <linux/usb/quirks.h>
+
#include <scsi/scsi.h>
#include <scsi/scsi_eh.h>
#include <scsi/scsi_device.h>
@@ -1288,6 +1290,10 @@ int usb_stor_port_reset(struct us_data *us)
{
int result;
+ /*for these devices we must use the class specific method */
+ if (us->pusb_dev->quirks & USB_QUIRK_RESET_MORPHS)
+ return -EPERM;
+
result = usb_lock_device_for_reset(us->pusb_dev, us->pusb_intf);
if (result < 0)
US_DEBUGP("unable to lock device for reset: %d\n", result);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index a34fa89..ea0eb6f 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1421,6 +1421,7 @@ extern int usb_string(struct usb_device *dev, int index,
/* 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_multiple_reset_configuration(struct usb_device *dev);
extern int usb_set_interface(struct usb_device *dev, int ifnum, int alternate);
extern void usb_reset_endpoint(struct usb_device *dev, unsigned int epaddr);
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index 2526f3b..0a555dd 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -19,4 +19,7 @@
/* device can't handle its Configuration or Interface strings */
#define USB_QUIRK_CONFIG_INTF_STRINGS 0x00000008
+/*device will morph if reset, don't use reset for handling errors */
+#define USB_QUIRK_RESET_MORPHS 0x00000010
+
#endif /* __LINUX_USB_QUIRKS_H */
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
