On Fri, Dec 18, 2009 at 5:25 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 18 Dec 2009, Dan Streetman wrote:
>
>> >> - /* 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.
>
> Okay, I get it. That makes sense. It's kind of awkward since the code
> in usb_set_device_state uses the ATT_WAKEUP bit in bmAttributes to
> determine whether a device is wakeup-capable, whereas rh_call_control
> in hcd.c uses the device's wakeup-capable bit to determine whether to
> set ATT_WAKEUP in bmAttributes. In other words, the flow of
> information is backward for root hubs as compared with other devices.
>
> I guess we can live with that little piece of confusion...
>
>> >> + /* 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.
>
> On second thought, the values are initialized to 0 anyway when the
> structure is created, right? So the device_init_wakeup call setting
> them to 0 isn't needed.
Not technically because we know what the device_init_wakeup
implementation does, but IMHO it doesn't hurt anything to call
device_init_wakeup with 0 and it is good to make clear this is the
point in code where init is supposed to happen...
>
> 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
