On Mon, 8 Jun 2009, Matthew Garrett wrote:
> If I turn on autosuspend on qcserial (which claims to support it), the
> card fails to resume. The following bodge makes it work:
>
> diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
> index 7528b8d..f8c02ba 100644
> --- a/drivers/usb/serial/qcserial.c
> +++ b/drivers/usb/serial/qcserial.c
> @@ -15,6 +15,7 @@
> #include <linux/tty_flip.h>
> #include <linux/usb.h>
> #include <linux/usb/serial.h>
> +#include <linux/usb/quirks.h>
>
> #define DRIVER_AUTHOR "Qualcomm Inc"
> #define DRIVER_DESC "Qualcomm USB Serial driver"
> @@ -51,6 +52,14 @@ static struct usb_device_id id_table[] = {
> };
> MODULE_DEVICE_TABLE(usb, id_table);
>
> +static int qc_reset_resume(struct usb_interface *intf) {
> + struct usb_device *udev = interface_to_usbdev(intf);
> +
> + intf->needs_binding = 1;
> + udev->quirks &= USB_QUIRK_RESET_RESUME;
> + return 0;
> +}
This looks rather strange. Are you sure you don't mean "|=" instead of
"&="? Anyway, this code doesn't belong here. The reset_resume routine
is supposed to do whatever is necessary to restore any device state
that was lost when the device was reset.
> +
> static struct usb_driver qcdriver = {
> .name = "qcserial",
> .probe = usb_serial_probe,
> @@ -58,6 +67,7 @@ static struct usb_driver qcdriver = {
> .id_table = id_table,
> .suspend = usb_serial_suspend,
> .resume = usb_serial_resume,
> + .reset_resume = qc_reset_resume,
> .supports_autosuspend = true,
> };
>
> @@ -66,6 +76,7 @@ static int qcprobe(struct usb_serial *serial, const struct usb_device_id *id)
> int retval = -ENODEV;
> __u8 nintf;
> __u8 ifnum;
> + struct usb_device *udev = interface_to_usbdev(serial->interface);
>
> dbg("%s", __func__);
>
> @@ -74,6 +85,9 @@ static int qcprobe(struct usb_serial *serial, const struct usb_device_id *id)
> ifnum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
> dbg("This Interface = %d", ifnum);
>
> + udev->quirks &= USB_QUIRK_RESET_RESUME;
> + udev->reset_resume = 1;
> +
> switch (nintf) {
> case 1:
> /* QDL mode */
This is peculiar and roundabout.
> but I'm left with a few questions. Firstly, is there a better way for a
> driver to indicate that it wants reset_resume? I'd rather not add it to
> the quirks list since that means duplicating all the IDs in the driver
> and there's a risk of them getting out of sync.
You don't have to add all the IDs to the quirks list; all you need to
do is put
udev->quirks |= USB_QUIRK_RESET_RESUME;
in the driver's probe routine. The result will be the same.
> Secondly, is there a
> better way to do this in the first place?
Better way to do what? Indicate that the device should always get a
reset-resume? No; see above.
> I've got no documentation for
> the card, but there's no relevant state kept when there isn't an open
> interface.
But what about when there _is_ an open interface? The reset_resume
method has to handle all cases.
Alan Stern
--
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
