On Fri, 23 Sep 2005, David Brownell wrote: > > Mostly in these, although maybe in a few of the others as well. > > Those are in fact the guts of the fixes, notably pm-root2.patch ...
Actually that was pretty much okay, except for this one question about hub_resume. It was a big set of patches; there hasn't been time to review it carefully... > > The main thing is moving hcd->hub_suspend into hub.c:hub_suspend. I > > firmly believe that the model you're using for root hubs isn't as logical > > or as consistent with external hubs as the one I outlined. > > Seems plenty logical to me: hub suspend code handles hub suspend, > whether or not it's a root hub. Ditto with resume code. The But hub.c:hub_suspend (which is what you mean by "hub suspend code", I trust?) _doesn't_ handle hub suspend. It handles hub _interface_ suspend. Well, let's leave this to the other email thread. > externally testable invariants then become the same in both cases, > which certainly improves consistency ... > > In fact doing it the other way has always bothered me as being > internally inconsistent. I recall, when you first posted the USB suspend patches, sending a suggestion that the root hub special-case stuff be moved to a different subroutine (can't remember which, probably hub_port_suspend). > > Another thing is the way you're doing autosuspend in OHCI (and probably > > EHCI as well -- I haven't checked). There's nothing new about this, but > > these patches just seem to move farther down the wrong path. > > That's just making things work again the way they used to work. Some of > the recent usbcore patches broke OHCI resume horribly ... EHCI too, though > it doesn't do that sort of autosuspend. > > If by "wrong path" you mean "not the khubd-driven approach that's been > slightly discussed" ... well, we don't have any such code yet, and it's > not OK to leave OHCI broken until we do. Okay, but this should be regarded only as a temporary fix. The old code went to some lengths to carry out an autosuspend from within an interrupt context, and this just piles on more of the same. Instead, the interrupt-level code should arrange for some thread to do a real suspend, in process context. That thread could be khubd, or it could be something else. Originally I started using khubd because it seemed like a natural choice for hub-related actions. Now I realize that all sorts of other drivers will be faced with the same problem: runtime suspend or resume actions started at interrupt time but needing a process context. Obviously they won't use khubd. They could use keventd or a separate kernel thread reserved just for these power management things. The real problem will be figuring out at interrupt time where to allocate a struct work_struct. Probably end up pre-allocating one... > > Also, what goes wrong when you try to use the FREEZE/SUSPEND distinction? > > You mentioned only that it doesn't work. > > Consider that a pure FREEZE doesn't go through all the gyrations to > shut down the hardware. So its in-memory state doesn't reflect any > such gyrations ... that's the _entire_ point of using FREEZE instead > of suspend: avoiding extra work, just idling the hardware instead of > shutting it down like the SUSPEND state(s) do. > > But that means swusp will write garbage state out to disk, with device > state marked as "frozen". Then the resume paths will restore that garbage. > Unhealthy. > > I've commented before that the whole swsusp "freeze" thing seemed to be > not-well-thought-out, and ill-defined. I had hoped not to be proven right > on that; in vain, it seems. I can see how this causes trouble. I'll try posting a message on the PM development list to see how other people think it should be handled. > > For the sake of making forward progress, I won't object to having these > > patches applied. But there are a few parts I will want to alter or change > > back. > > There will things that need to change a bit; there are cleanups I've > not sent yet, for example. And I expect the usual sort of glitches to > appear. > > But when you say "change back", that fires up my trouble radar. Things > are working sanely now in all my tests, so "change back" implies that > some now-finally-fixed bugs will reappear ... I should have said "implement differently", which implies getting rid of some of the changes made here and then adding new ones. Your usb_suspend_root_hub and __hub_quiesce routines are examples of things that shouldn't be there. Alan Stern ------------------------------------------------------- SF.Net email is sponsored by: Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php _______________________________________________ [email protected] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
