On Tuesday 10 May 2005 8:28 am, Alan Stern wrote: > On Mon, 9 May 2005, David Brownell wrote: > > > > This patch does that conversion, and it also has the other tweak. > > Worked for me ... you? > > Hmmm... There's a couple of problems with this. Not with the tweak; I > won't have a chance to test that for a few days but there's no reason to > think it won't work. However the way you're using resume_root_hub doesn't > match what I had in mind when it was originally written.
However it's what I had in mind when I agreed with you that such code would be useful, and it did pass the basic sysfs-induced test scenarios with USB_SUSPEND defined. > No doubt the problem was caused at least in part by insufficient > documentation. These are tricky matters, and it's hard to write things > down in a form that everyone will understand. The short explanation is > that resume_root_hub does a "usbcore" resume, not just a "root hub" > resume. > > I'll skip the details. Suffice it to say that when khubd sees the > resume_root_hub flag, it fulfills the request by calling > usb_resume_device. That's why the code was conditioned on USB_SUSPEND -- > because otherwise usb_resume_device doesn't do anything. Arguably khubd > should call the HCD's hub_resume method directly when USB_SUSPEND isn't > configured. Right now it doesn't, so your patch won't work as expected in > that case. And that'd explain why it worked in light testing with USB_SUSPEND. But ... if it doesn't work without USB_SUSPEND, then what would be the point of such a routine? > The second issue, which might or might not be a problem, is that your > patch calls resume_root_hub to resume from an autosuspend, i.e., a > situation where the root hub was suspended without going through > usb_suspend_device. So it's not at all clear whether using > usb_resume_device to wake up is a good idea. (More honestly, it is > clear that it's not a good idea!) See above ... I thought the whole point of that routine was to help that resume-from-autosuspend case. At least I did when the idea originally came up ... in conjunction with the timer fixes, an autosuspended root hub could finally do a "real suspend" from the usbcore POV. > These two issues are somewhat related, and the underlying theme is the > need to support autosuspend even when CONFIG_USB_SUSPEND is off. If you > changed the way the driver does autosuspend then the issues could be > avoided. For instance, the UHCI driver makes both autosuspend and > autoresume completely transparent to usbcore. It doesn't change any > externally visible flags, it doesn't even lock the root hub device while > making the transitions! When the transition can be atomic -- e.g. use only an internal spinlock -- that can work. But supporting the USB_SUSPEND=n case does make complications ... though even with USB_SUSPEND, there could still be a need to wait for URBs to be reported back from the HC after all drivers suspend. For now, I'm not willing to have separate and subtly different suspend codepaths. (This is an area -- the donelist, which can hold completed transactons for up to seven msec -- where OHCI is at a disadvantage compared to EHCI and UHCI. At least, so long as the driver actually _uses_ the donelist instead of just scanning TDs as they're marked completed...) > And since it knows there are no USB devices > listening it can avoid doing lengthy signalling during the transitions, so > they take place entirely in_interrupt. Hence there's no need for a > process context and no need for resume_root_hub. Once we change the policy and make resume not recurse, that will become so. Remote wakeup logic should handle whatever recursion is truly necessary. - Dave ------------------------------------------------------- This SF.Net email is sponsored by Oracle Space Sweepstakes Want to be the first software developer in space? Enter now for the Oracle Space Sweepstakes! http://ads.osdn.com/?ad_id=7393&alloc_id=16281&op=click _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
