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.
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
