On Tue, 1 Mar 2016, Daniel Fraga wrote:
> On Tue, 1 Mar 2016 17:15:56 -0500 (EST)
> Alan Stern <st...@rowland.harvard.edu> wrote:
>
> > Don't worry about the Elan driver. Instead, let's see if this patch
> > fixes the problem.
>
> Yes, this patch fixed the problem. I can suspend and resume
> without those repeated "reset" messages ;)
>
> It appears just 1 time and that's it:
>
> [ 644.691934] usb 3-1.6: reset full-speed USB device number 6 using ehci-pci
>
> Now it's fine :) But if you still need more tests, just ask.
That patch fixed the problem, but I'm not sure it's the best solution.
Time to ask the usbhid maintainers.
Jiri and Benjamin: Daniel's problem boils down to this: He's got an
Elan touchscreen that he doesn't use. The touchscreen driver isn't
even loaded. As a result, the kernel never calls usbhid_start() and
usbhid->urbin never gets allocated.
That would be okay, except that hid_post_reset() unconditionally
hid_start_in() and then reports an error because there is no input URB
to start! Looking through the code, it appears that HID_STARTED isn't
checked consistently. It does get checked in the hid_resume() path but
not in the hid_reset_resume() or hid_post_reset() paths.
And even then, it's not clear that the check in hid_resume() is in the
right spot. If HID_STARTED isn't set then the code skips calling
hid_resume_common(), which means the HID_SUSPENDED bit doesn't get
cleared. Overall, the use of the flag bits in these callback routines
could use some auditing.
One more thing: usbhid_start() carefully looks for an IN endpoint and
uses it for usbhid_urbin. But there's no check for devices that don't
have any IN endpoints (such as a device that relies entirely on polling
over the control endpoint, or a device that is output-only). Is this
supposed to be an error? If it isn't, then does hid_start_in() need to
check that usbhid->urbin isn't NULL before trying to submit it?
The patch that fixed Daniel's problem is repeated below for your
convenience. Is this the right approach? If you think it is, I'll
submit it officially.
Alan Stern
Index: usb-4.4/drivers/hid/usbhid/hid-core.c
===================================================================
--- usb-4.4.orig/drivers/hid/usbhid/hid-core.c
+++ usb-4.4/drivers/hid/usbhid/hid-core.c
@@ -83,6 +83,7 @@ static int hid_start_in(struct hid_devic
spin_lock_irqsave(&usbhid->lock, flags);
if ((hid->open > 0 || hid->quirks & HID_QUIRK_ALWAYS_POLL) &&
+ usbhid->urbin &&
!test_bit(HID_DISCONNECTED, &usbhid->iofl) &&
!test_bit(HID_SUSPENDED, &usbhid->iofl) &&
!test_and_set_bit(HID_IN_RUNNING, &usbhid->iofl)) {
@@ -1457,10 +1458,13 @@ static int hid_post_reset(struct usb_int
clear_bit(HID_RESET_PENDING, &usbhid->iofl);
spin_unlock_irq(&usbhid->lock);
hid_set_idle(dev, intf->cur_altsetting->desc.bInterfaceNumber, 0, 0);
- status = hid_start_in(hid);
- if (status < 0)
- hid_io_error(hid);
- usbhid_restart_queues(usbhid);
+
+ if (test_bit(HID_STARTED, &usbhid->iofl)) {
+ status = hid_start_in(hid);
+ if (status < 0)
+ hid_io_error(hid);
+ usbhid_restart_queues(usbhid);
+ }
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
