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