On Wed, 4 May 2005, David Brownell wrote:
> On Sunday 01 May 2005 7:14 pm, Alan Stern wrote:
> > David:
> >
> > I found part of the source for the trouble I've had with root-hub
> > suspend/resume on OHCI. It's these two lines near the end of
> > ohci-hub.c:ohci_hub_suspend():
> >
> > if (status == 0)
> > hcd->state = HC_STATE_SUSPENDED;
>
> I think I remember why that's there. It pairs the earlier line
> in the same function to set hcd->state to QUIESCING. And the reason
> for that is because hcd->state is the only hook we have for, erm,
> quiescing the traffic going into the HCD.
Whatever the reason for it being there, it's causing this problem.
> That is, whenever a root hub starts to suspend, whether that's
> because of (a) some internal state transition, (b) a request to
> that root hub's /sys/.../power/state file, (c) a side effect
> of suspending the HCD by /sys/.../power/state when USB_SUSPEND
> is not enabled, or even (d) a side effect of suspending the HCD
> as part of entering some system sleep state ... new URBs going
> into the HCD must be blocked.
>
> We have no other way to throttle down traffic just now...
Do we really need to throttle down traffic from within usbcore? Won't it
be enough to rely on the HCDs to reject submissions when they are unable
to accept them? (Plus the fact that thanks to all the cleanups made in
the last couple of years, hardly any submissions are made at inappropriate
times.)
> I think a better way of looking at this is that the two notions
> may not yet been fully teased apart.
If you really want to do this from within usbcore, consider restricting
hcd->state to only three possible values:
Okay to submit and unlink (running normally);
Okay to unlink but not to submit (quiescing);
Not okay to unlink or submit (any other state).
No part of usbcore outside of hcd-pci.c should care about anything else.
And while we're at it, make it clear who owns hcd->state -- either the HCD
or usbcore does, and the other shouldn't write to it at all.
Any additional information needed by hcd-pci.c or the HCDs (such as
whether IRQs are enabled) can be stored in a separate new field.
> However, it seems I've been interpreting hcd->state as representing
> the parts of "root hub suspended" that are a superset of what the
> root_hub->state == USB_STATE_SUSPENDED means. And leaving the other
> parts to bus-specific logic.
>
>
> One complication is that there's _also_ a need to represent the
> state of the HC itself. Including cases like:
<snip>
> There's also the struct device dev->power.power_state field, which
> has always been problematic (vergin on useless) since the PM core
> doesn't manage it coherently. It's never really been usable as
> anything other than a boolean ... not powerful enough to distinguish
> cases like [4] in current kernels, either.
Apart from the PCI bus glue, all of that stuff is private to the HCDs.
It shouldn't be folded in along with a field used elsewhere in usbcore.
And even the PCI glue info should be kept separate from things used only
by HCDs. Right now hcd->state mixes all these different categories
together in a confusing and error-prone way.
> > The comment indicates that hcd->state refers to the root hub, but the code
> > calls the suspend routine for the host controller.
>
> It's consistent with EHCI and OHCI managing hcd->state as part of
> root hub suspend logic, though, as well as the next comment:
>
> /* entry with CONFIG_USB_SUSPEND, or hcds that autosuspend: the
> * controller and/or root hub will already have been suspended,
> * but it won't be ready for a PCI resume call.
> *
> * FIXME only CONFIG_USB_SUSPEND guarantees hub_suspend() will
> * have been called, otherwise root hub timers still run ...
> */
>
> And if you look at how those routines work for those HCDs, all they
> really do is suspend or resume the root hub ... modulo that timer issue.
Forget about timer issues. That problem has already been solved in
principle, even if the solution has yet to find its way into all the HCDs.
For UHCI, the controller-level suspend and resume routines _don't_ affect
the root hub (unless CONFIG_USB_SUSPEND isn't set). But they do handle a
few other things: check for HC died, disable/enable IRQ generation,
enable/disable non-PME# wakeup (not yet written), reconfigure the
controller, and even reset it if the BIOS or anything else has changed its
state.
> Those two "host controller" suspend/resume calls are effectively
> there only for the PCI based controllers, and nothing they currently
> do seems HC-specific. (Other than the PMAC hooks, which seem to
> reflect deficiencies in the PCI suspend/resume framework... that
> stuff should be handled using generic PCI platform hooks.) I'd
> be happy to see those hooks vanish.
You mean the suspend/resume routines in hcd-pci.c? Once a suitable
generic PCI power driver is available they shouldn't be needed.
> > Evidently this confusion needs to be straightened out. I wouldn't be
> > surprised if it's responsible for some other odd things we've been seeing.
>
> Could be, but I'm still curious why I've never seen the problem
> you saw. Could it be that you've been working under the assumption
> that hcd->state is more like pci_dev->power_state than like an
> augmented root_hub->state? Any changes in RC3 haven't been very
> testable by me so far, though I've started to poke around with
> coGITo and so on.
I haven't been working under any assumptions. I just used the 2.6.12-rc3
patch from the usual place at kernel.org. Although I haven't tried it,
probably the same thing would happen with vanilla 2.6.11.
The only out-of-the-ordinary thing I did was
echo -n 3 >/sys/devices/pci.../usb1/power/state
before plugging a device into the controller. This should be easy for you
to test.
Alan Stern
-------------------------------------------------------
This SF.Net email is sponsored by: NEC IT Guy Games.
Get your fingers limbered up and give it your best shot. 4 great events, 4
opportunities to win big! Highest score wins.NEC IT Guy Games. Play to
win an NEC 61 plasma display. Visit http://www.necitguy.com/?r=20
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
