So is this patch good, or does it need any other changes?
On Fri, Dec 18, 2009 at 5:51 PM, Dan Streetman <ddstreet@xxxxxxxx> wrote:
> 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
