On Fri, 19 Jan 2007, Oliver Neukum wrote:
> Hi,
>
> I did the dirty deed. Could you please tell me which quirky devices
> you know of? I am afraid this list needs to be populated and there
> are outstanding regressions in Adrian's list.
I'll get back to you on the devices.
I think this patch could be generalized. We may have other quirks to
worry about besides PM/autosuspend. This should become a general USB
blacklist mechanism.
One example that comes up immediately is the HP scanner which crashes
when we try to read the string descriptors. Others may crop up in the
future.
> --- a/include/linux/usb.h 2007-01-18 13:15:25.000000000 +0100
> +++ b/include/linux/usb.h 2007-01-19 12:12:07.000000000 +0100
> @@ -387,6 +387,7 @@
> struct usb_device *children[USB_MAXCHILDREN];
>
> int pm_usage_cnt; /* usage counter for autosuspend */
> + int pm_quirks; /* quirks of power management */
So this could simply be "quirks", not "pm_quirks", with a corresponding
change to the comment.
> +/* Power management quirks */
> +#define USB_PM_DISCONNECT 1 /* devices which disconnect when resuming */
Make the name USB_QUIRK_NO_AUTOSUSPEND. We don't really care that the
device disconnects when resuming; the only important part is that we
should never try to autosuspend it.
The list of quirk flag definitions doesn't belong in include/linux/usb.h.
We could put it in drivers/usb/core/usb.h or drivers/usb/core/quirks.h.
Or if you think that some USB drivers might eventually want to use a
quirk, you could put the definitions in include/linux/usb/quirks.h.
> --- a/drivers/usb/core/hub.c 2007-01-17 14:26:17.000000000 +0100
> +++ b/drivers/usb/core/hub.c 2007-01-19 12:31:08.000000000 +0100
> @@ -31,6 +31,7 @@
> #include "usb.h"
> #include "hcd.h"
> #include "hub.h"
> +#include "autosuspend_quirks.h"
And this, of course, should be the name you decide on above.
> @@ -1263,6 +1264,33 @@
> static int __usb_port_suspend(struct usb_device *, int port1);
> #endif
>
> +#ifdef CONFIG_USB_SUSPEND
> +
> +static int usb_autosuspend_search_quirk(u16 vendor, u16 product, u16 version)
> +{
> + const struct pm_blacklist *blentry = pm_blacklist;
> +
> + while (blentry->idVendor) {
> + if (blentry->idVendor == vendor &&
> + blentry->idProduct == product &&
> + blentry->bcdVersionMin <= version &&
> + blentry->bcdVersionMax >= version)
> + return blentry->quirks;
> + blentry++;
> + }
> +
> + return 0;
> +}
I dislike seeing this in hub.c. That file is already too big, and anyway
managing a blacklist has nothing to do with being a hub driver. You
should create a new source file: drivers/usb/core/quirks.c. Put both the
search routine and the initializer for the quirks array in the new file.
Also the prototype is awkward. It seems pretty unlikely that anyone would
want to search the blacklist for a device that isn't currently attached.
So you can simply accept a pointer to struct usb_device instead of the
vendor, product, and version numbers.
> static int __usb_new_device(void *void_data)
> {
> struct usb_device *udev = void_data;
> @@ -1359,6 +1387,12 @@
> }
> #endif
>
> + /* Determine power management quirks */
> + udev->pm_quirks = usb_autosuspend_search_quirk(
> + le16_to_cpu(udev->descriptor.idVendor),
> + le16_to_cpu(udev->descriptor.idProduct),
> + le16_to_cpu(udev->descriptor.bcdDevice));
> + dev_dbg(&udev->dev, "USB power management quirks: %d\n",
> udev->pm_quirks);
> /* Register the device. The device driver is responsible
> * for adding the device files to usbfs and sysfs and for
> * configuring the device.
After the name and prototype changes this will be okay. You can also
implement a USB_QUIRK_NO_STRINGS quirk; execute the calls to
usb_cache_string() only when this quirk isn't set.
> --- /dev/null 2006-05-02 08:46:16.000000000 +0200
> +++ b/drivers/usb/core/autosuspend_quirks.h 2007-01-19 12:29:44.000000000
> +0100
> @@ -0,0 +1,20 @@
> +/*
> + * This is a list of USB devices which are quirky with respect
> + * to power management. Keep this list ordered by
> + * 1. Vendor
> + * 2. Product
> + * 3. Version, the most specific first
> + */
> +
> +
> +static const struct pm_blacklist {
> + u16 idVendor; /* vendor id of the quirky device */
> + u16 idProduct; /* product id of the quirky device */
> + u16 bcdVersionMin; /* minimum version id of the afflicted devices */
> + u16 bcdVersionMax; /* maximum version id of the afflicted devices */
> + int quirks; /* the actual qirks, several to be joined with binary
> or */
> +} pm_blacklist[] = {
> + /* Elsa MicroLink 56k (V.250) */
> + {0x05cc, 0x2267, 0x0100, 0x0100, USB_PM_DISCONNECT},
> + {0, 0, 0, 0, 0} /* keep this last */
> +};
Like I said above, this should be a .c file, not a .h.
> --- a/drivers/usb/core/driver.c 2007-01-18 13:13:22.000000000 +0100
> +++ b/drivers/usb/core/driver.c 2007-01-19 12:04:46.000000000 +0100
> @@ -952,7 +952,7 @@
> * Also fail if any interfaces require remote wakeup but it
> * isn't available. */
> udev->do_remote_wakeup = device_may_wakeup(&udev->dev);
> - if (udev->pm_usage_cnt > 0)
> + if (udev->pm_usage_cnt > 0 || udev->pm_quirks & USB_PM_DISCONNECT)
> return -EBUSY;
> if (udev->actconfig) {
> for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) {
You don't need to make this change here. Instead, when
USB_QUIRK_NO_AUTOSUSPEND is set you can simply increment
udev->pm_usage_cnt. Or if you prefer not to violate the layering, call
usb_autoresume_device().
Alan Stern
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
