On Thu, Dec 17, 2009 at 3:20 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 17 Dec 2009, Dan Streetman wrote:
>
>> Currently a USB device's wakeup settings are initialized when the device
>> is set to a configured state using device_init_wakeup(), but this is not
>> correct as wakeup is split into "capable" (can_wakeup) and "enabled"
>> (should_wakeup). The settings should be initialized instead in the device
>> initialization (usb_new_device) with the "capable" setting disabled and the
>> "enabled" setting enabled. The "capable" setting should be set based on the
>> device being configured or unconfigured.
>>
>> This patch retains the sysfs power/wakeup setting of a USB device over a
>> USB device re-configuration, which can happen (for example) after a
>> suspend/resume cycle.
>
>> - /* Increment the parent's count of unsuspended children */
>> - if (udev->parent)
>> + if (udev->parent) {
>
> Why don't you do this for root hubs too? They can be unconfigured,
> just like anything else. Do you want them not to be wakeup-enabled for
> some reason?
Well, they can be unconfigured but their wakeup setting isn't changed
based on their configuration status - the comment in
usb_set_device_state says:
/* root hub wakeup capabilities are managed out-of-band
* and may involve silicon errata ... ignore them here.
*/
And their wakeup settings are only modified (as far as I can tell) in
hcd.c in usb_add_hcd where they are initialized to enabled. They
aren't modified by any other common code. So they should always be
wakeup-capable.
>
>> + /* Increment the parent's count of unsuspended children */
>> usb_autoresume_device(udev->parent);
>>
>> + /* Initialize wakeup and disable wakeup capable;
>> + * wakeup capability is set by configured state,
>> + * wakeup enabled/disabled is set by sysfs control
>> + */
>> + device_init_wakeup(&udev->dev, 1);
>> + device_set_wakeup_capable(&udev->dev, 0);
>
> This code sequence looks strange. You're saying that the device can
> issue remote wakeups and then that it can't. It would make more sense
> to do:
>
> device_init_wakeup(&udev->dev, 0);
> device_set_wakeup_enable(&udev->dev, 1);
>
> With a corresponding change to the comment, of course.
Sounds good, I'll send an updated patch.
>
> 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
>
--
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
