On Tue, 29 Dec 2009, Dan Streetman wrote:
> 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...
The revised version is okay.
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
