On Thu, Sep 27, 2012 at 10:31 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 27 Sep 2012, Ming Lei wrote:
>
>> From: Ming Lei <tom.leiming@xxxxxxxxx>
>>
>> The hub status endpoint has a long 'bInterval', which is 255ms
>> for FS/LS device and 256ms for HS device according to USB 2.0 spec,
>> so the device connection change may be reported later more than 255ms
>> via status pipe.
>>
>> The connection change in hub may have been happened already on the
>> downstream ports, but no status URB completes when it is killed
>> in hub_suspend(auto), so the connection change may be missed by some
>> buggy hub devices, which won't generate remote wakeup signal after
>> their remote wakeup is enabled and they are put into suspend state.
>>
>> The problem can be observed at least on the below Genesys Logic, Inc.
>> hub devices:
>>
>> 0x05e3,0x0606
>> 0x05e3,0x0608
>>
>> In theory, there is no way to fix the problem completely, but we
>> can make it less likely to occur by this patch.
>>
>> This patch introduces one quirk of HUB_QUIRK_CHECK_PORTS_AUTOSUSPEND
>> to check ports' change during hub_suspend(auto) for the buggy
>> devices. If ports' change is found, terminate the auto suspend and
>> return to working state.
>>
>> So for the buggy hubs, if the connection change happend before
>> the ports' check, it can be handled correctly. If it happens between
>> the ports' check and enabling remote wakeup/entering suspend, it
>> will be missed. Considered the interval is quite short, it is very
>> less likely to happen during the window.
>
> One remark and one tiny problem...
>
>> @@ -3176,6 +3198,17 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
>> return -EBUSY;
>> }
>> }
>> +
>> + if (hdev->do_remote_wakeup && hub->quirk_check_port_auto_suspend) {
>> + /* check if there are changes pending on hub ports */
>> + if (check_ports_changed(hub)) {
>> + if (!PMSG_IS_AUTO(msg) &&
>> + device_may_wakeup(&hdev->dev))
>
> You don't need to check device_may_wakeup(); the core has already taken
> care of it. If device_may_wakeup() returns 0 then do_remote_wakeup
> will be 0 also.
Yes, will remove the unnecessary check on device_may_wakeup().
>
>> + pm_wakeup_event(&hdev->dev, 2000);
>> + return -EBUSY;
>> + }
>> + }
>
> The !PMSG_IS_AUTO case should not return -EBUSY, because
> usb_suspend_both() will ignore the return value. It should proceed
> with the rest of the suspend.
That is a big problem wrt. this patch, :-(
But as discussed in previous thread, usb_suspend_both() should return
failure and stop suspend for the situation, so that the wakeup event can
be handled asap.
Thanks,
--
Ming Lei
--
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
