On Wed, 14 Sep 2005, David Brownell wrote:
> > > The parameter isn't needed, or even usable, without the recursion
> > > removed in the previous patch.
> >
> > These patches look great, and I intend to go through them carefully. At
> > first glance, there appears to be a small problem in the pm-usbsusp patch:
>
> Not with that patch ... it's actually in the CURRENT kernel unless it's
> got CONFIG_USB_SUSPEND set!
>
> Notice the FIXME added by pm-config.patch; it's part of the underlying
> issue ... which by my reading isn't in this routine (yet). (And it's
> unrelated to any usb_suspend_device parameter; $SUBJECT is irrelevant
> other than "patch".)
(Yes, sorry about the $SUBJECT -- I replied to the message containing the
objectionable patch instead of the 0/5 summary message.)
There are in fact two distinct but related problems here. One is the
problem you mention, in which usbcore doesn't call the root-hub-suspend
methods if CONFIG_USB_SUSPEND isn't set. But I was talking about a
different problem. Even when CONFIG_USB_SUSPEND is set, your patch will
prevent usbcore from suspending root hubs during FREEZE events. The
patch should have said something more like this:
+ udev = to_usb_device(dev);
+ if (message.event == PM_EVENT_FREEZE && udev->parent) {
+ dev->power.power_state = message;
+ return 0;
+ }
+ return usb_suspend_device (udev);
> This seems related to two OHCI regressions I noticed, by the way.
>
> In one case, resume-via-reset seems broken without CONFIG_USB_SUSPEND;
> the root hub timer doesn't restart correctly. (It tries to resubmit
> the URB while the hub is suspended, fails, then dies.) In the other,
> wakeup is broken since khubd never notices USB_PORT_STAT_C_SUSPEND;
> as if the root hub timer stopped (new behavior?) and didn't restart.
It looks like the first problem is a logical error in the USB resume code:
Device states are set back to USB_STATE_CONFIGURED _after_ the devices are
resumed. Naturally, URBs can complete as soon as the physical resume is
done, and completion handlers may try to resubmit -- but they will fail
until the state is set back. Clearly the state needs to be changed
_before_ the resume is done.
I don't understand the second problem. Why would you expect wakeup to
work when CONFIG_USB_SUSPEND isn't set? Why should it have to work at
all in that case?
> > You skip calling usb_suspend_device for FREEZE events. That's good for
> > external devices, but it's not good for root hubs. They really do need to
> > be suspended even for FREEZE events, since FREEZE requires no interrupts
> > or DMA.
>
> Remember that HCDs consist of two hardware interfaces, which can normally
> be managed somewhat separately:
>
> - Root hub, which is the down-stream link. These are managed through
> calls into the HCD: hub_suspend(), hub_resume(), the code handling
> control requests, status polling, and so on.
>
> - Controller, the upstream link. Sometimes controllers support
> lower power modes than even "root hub suspended" (like "off", or
> even just "clock off"), or they're like PCI (or PCMCIA) and have
> additional control protocols to follow.
>
> Now, the FIXME that I mentioned is basically that suspend/resume linkage
> to the root hubs needs reworking. Today, it's completely unused unless
> CONFIG_USB_SUSPEND is set ... so all the HCDs have code inside the
> controller suspend/resume calls to cope with that.
>
> That is: root hubs currently never see FREEZE except indirectly through
> the upstream link ... except with the non-default CONFIG_USB_SUSPEND.
Right. And with your new patch they won't see FREEZE at all, since that
indirect upstream link gets used only when CONFIG_USB_SUSPEND isn't set.
(At least, that's how uhci-hcd works; maybe other HCDs are different.)
> I suspect the right fix will involve adding to the code which you quoted,
> by calling hcd->hub_suspend(); similarly on resume. That'll be a nice
> step towards shrinking what CONFIG_USB_SUSPEND covers, and removing bugs
> associated with the "other" setting of that option.
If we just got rid of CONFIG_USB_SUSPEND entirely, this wouldn't be an
issue... :-)
Maybe a better fix for this problem would be to make the non-USB_SUSPEND
version of usb_suspend_device more than just an empty placeholder. If the
device is a root hub, do perform the hcd->hub_suspend call. And likewise
for usb_resume_device, of course. Then all that special indirect upstream
stuff could be removed from the HCDs.
> > This will be easy to fix, of course.
>
> Just changing all the HCDs, the remote wakeup support, and the normal
> suspend/resume paths ... and retesting them all both with and without
> CONFIG_USB_SUSPEND, root hub timers, etc.
>
> Not easy enough to be 2.6.14 material, I suspect!!
I meant that the alteration to your pm-usbsusp patch (given above) was
easy.
> > Bringing up this next point may be a mistake, but bear with me... Now
> > that we're switching to a model where the recursion for sleep transitions
> > is handled entirely by the PM core (and recursion for runtime PM is
> > handled by some as-yet unspecified means), there appears to be no longer
> > any need for the "locktree" protocol. Do you agree?
>
> Not at first glance. It's that "unspecified means" thing; we'll still
> need to be able to resume devices, and hence possibly their parents and
> up the tree to the root hub. Maybe it could be replaced later by
> something similar though. :)
Okay, we'll see. I believe that resuming devices and their ancestors will
prove simple enough; the difficulty comes in when you want to do selective
subtree suspends.
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
