On Mon, 26 Sep 2005, David Brownell wrote:
> I think further discussion on these points probably belongs in
> some thread with a relevant title ... :)
Done.
Before going any farther into this, I want to make the grounds for
discussion more precise. So I'll start off with some context, some
undisputed facts, and some definitions. Then some comparisons, and on
into the discussion proper.
To begin with, let's agree that we're considering only runtime PM. For
system PM none of this matters, because during a system sleep _all_
devices get suspended and afterward they _all_ get resumed.
Let's also agree to assume that CONFIG_USB_SUSPEND is set. The
possibility of it being unset is an aberration that will go away once
everything is working okay.
An external USB device goes into its suspended state (actually, one of
three suspended states -- DEFAULT-SUSPEND, ADDRESS-SUSPEND, and
CONFIGURED-SUSPEND -- but we don't need to worry about the details) when
it hasn't received any packets for more than a few milliseconds. This can
happen for several possible reasons:
(A) One of the ports between the device and the host was suspended;
(B) One of the ports between the device and the host was disabled;
(C) The host stopped sending packets.
(A) and (C) are the ones we care about; (B) will show up as a disconnect
event. Furthermore, (C) doesn't normally happen without (A) happening
first. So I'll concentrate on (A).
(A) occurs when a Set-Feature(PORT_SUSPEND) request is sent to one of the
device's ancestor hubs. But this fact is encapsulated in hub.c; as far as
usbcore is concerned (A) happens when usb_suspend_device is called. And
even that fact should remain encapsulated in usbcore. As far as the rest
of the system, the user, and other programmers are concerned, (A)
happens when dpm_runtime_suspend or the equivalent is called. For
example, when the user writes to /sys/.../power/state.
You mentioned before that we should avoid probing inside the black-box of
a hub or host controller. So I'll take your advice and adopt a very
"behaviorist" point of view (in the sense used by psychologists). I'll
focus mainly on subroutine calls and the resulting device behavior.
Therefore let's accept the following definitions:
"Suspend an external USB device" means calling dpm_runtime_suspend
(or the equivalent) for the struct device embedded in the
usb_device.
"Suspend a root hub" means calling dpm_runtime_suspend (or the
equivalent) for the struct device embedded in the root hub's
usb_device.
"Suspend an interface" means calling dpm_runtime_suspend (or the
equivalent) for the struct device embedded in the usb_interface.
That's very unambiguous, and it ignores the details of the underlying
implementations. Note that calling dpm_runtime_suspend boils down to
calling usb_generic_suspend, which in turn may or may not call
usb_suspend_device.
Let's agree that "hub_suspend" refers to the routine in hub.c. I'll use
"hcd->hub_suspend" for the HCD method, but I don't like that name as it is
too similar to the other. (Even if nothing else comes out of this, I'm
going to send in a patch renaming the method to something like
"bus_suspend".)
To help make the discussion more concrete, let's say that
/sys/devices/pci0000:00/0000:00:1d.0/usb1/1-4
is an external hub, 1-4:1.0 is its interface, and 1-4.2 is a child device.
Also, usb1 is the usb_device for the root hub and 1-0:1.0 is the root
hub's interface. 0000:00:1d.0 is the pci_device for the host controller.
The main idea behind this discussion has been that root hubs ought to
behave like external hubs, to a very large extent. So let's consider
first how an external hub behaves in response to various stimuli.
When you suspend an external hub's interface, the suspend call will fail
if all the enabled ports aren't already suspended. Suspending a root
hub's interface does the same thing; it even uses the same code.
So okay, suppose 1-4.2 has been suspended and we suspend 1-4:1.0. What's
the result?
(1) The hub driver unlinks the status URB. Hence it can no longer be
aware of port status changes.
(2) 1-4 itself is not suspended. Users can send URBs to it via usbfs
and it will respond. For example, you could run lsusb on it.
(3) If 1-4.2 should send a remote wakeup request, port 2 on 1-4 will
automatically turn off its Suspend feature. The PORT_C_SUSPEND
status bit will be set, but khubd won't learn about it.
Note that (3) actually represents a weak spot in out current model. It's
an example showing how the hub interface can be suspended at a time when
an enabled port is not suspended.
Let's continue now by suspending 1-4. The result is:
(4) The usb_device for 1-4 is marked USB_STATE_SUSPENDED. URBs
submitted to it will fail with -EHOSTUNREACH. Even if usbcore
didn't do this check, URBs would still quickly fail with a
low-level protocol error.
(5) If the wakeup-enabled flag is set, 1-4 will be enabled for remote
wakeup. Whenever a port status change occurs, the device will
send a wakeup request upstream.
Now compare (1) - (5) with the corresponding behaviors when we suspend a
root hub's interface and the root hub. Assuming 1-4 has been suspended,
after suspending 1-0:1.0 we get:
(1') The hub driver unlinks the status URB. Hence it can no longer be
aware of port status changes. It also calls hcd->hub_suspend.
(2') usb1 itself is not suspended. Users can send URBs to it via usbfs
and it will respond. For example, you could run lsusb on it.
(3') If 1-4 should send a remote wakeup request, the HCD will
automatically call usb_hcd_resume_root_hub, which will cause
hcd->hub_resume to run and 1-0:1.0 to be resumed. Then 1-4
would be resumed as well, when the wakeup request showed up
in the root hub's port status.
In fact this last part ought to be true only when an appropriate
wakeup-enabled flag is set (which one -- the one in usb1 or in 1-0:1.0?).
Once that check has been implemented, we will have:
(3'n) If wakeup-enabled isn't set, then a remote wakeup request from 1-4
won't cause anything to happen at all.
As before, let's continue by suspending usb1. The result is:
(4') The usb_device for usb1 is marked USB_STATE_SUSPENDED. URBs
submitted to it will fail with -EHOSTUNREACH. If usbcore didn't
do this check, URBs would probably succeed (depending perhaps on
whether or not the HC was also suspended).
(5') The wakeup-enabled flag's setting for usb1 won't be examined.
Root-hub remote wakeup will remain enabled or not according to
whether we are in (3') or (3'n).
(I trust those capsule summaries are correct and agree with what your
patches do. Let me know if I made a mistake.)
Clearly there are some major differences between (1)-(5) and (1')-(5').
Let's consider them in order.
(1) vs. (1'): The difference is the call to hcd->hub_suspend.
This can be regarded as little more than a power-saving optimization.
External hubs don't have the luxury of reducing power to their downstream
halves during an interface suspend, but root hubs do. As such, the
difference is innocuous. The one false note is that hcd->hub_suspend
enables remote wakeup for the root hub; see below.
(2) vs. (2'): No difference.
(3) vs. (3'): This is substantial. At the very least, the HCD
should not call usb_hcd_hub_resume when a remote wakeup request comes in.
In fact, if hcd->hub_suspend didn't enable remote wakeup for the root hub,
the difference in behavior would be a lot smaller. Then we'd always be in
the (3'n) case, which is much closer to (3).
(4) vs. (4'): No significant difference.
(5) vs. (5'): This is a glaring difference. To rectify it
requires adding a new HCD method to be called from usb_suspend_device, one
that enables root-hub remote wakeup depending on the wakeup-enable flag
setting.
To sum up, the important differences are the ones between (3)/(3') and
(5)/(5'). We can greatly reduce those differences by: deciding that
hcd->hub_suspend does not enable remote wakeup, and adding a new HCD
method that does nothing but enable remote wakeup. Alternatively, we
could reduce them by calling hcd->hub_suspend from usb_suspend_device like
we used to, instead of from hub_suspend.
All right. Having said all of that, I'll respond to some of your
comments.
> That means that suspending the hub interface driver should affect all
> of the downstream ports on that hub.
Or rather, the hub interface driver should not allow itself to be
suspended unless all the enabled downstream ports on that hub are already
suspended. Which is what your patches do.
> A different driver controls the
> upstream link ... maybe a different instance of the hub driver (for
> USB links), maybe an HCD (for links using some other bus).
Yes. But controlling the upstream link is not the same as controlling the
device's response to messages sent over that link. (Even though
usb_suspend_device resides inside hub.c, it's not called by khubd.)
> However, it's important to remember that root hubs really are NOT the same
> as other hubs. There may be several places where reality demands recognition,
> beyond the fundamental one that the upstream data and signal links use PCI
> (or some other non-USB bus).
I accept that. However I don't believe the differences discussed above in
(3') and (5') fall into this "essential" category. Particularly since
there are easy ways to eliminate them.
> That means it would be nonsensical (illogical, inconsistent) to support
> access to *any* downstream ports/devices when the hub driver is suspended.
>
> Because such access would mean it's not the hub driver which is actually
> managing those ports ... and if it isn't, who is? And why do we have a
> hub driver then? Something would need to vainish in a puff of illogic!!
("Oh dear, I hadn't thought of that!" says God.) :-)
Agreed. But there can be times when usbcore's idea of the port state
differs from the actual state; see (3) above. And we don't need to call
hcd->hub_suspend to disable access to the root hub ports; it suffices to
suspend them individually.
> Well, there's no remote wakeup signaling to affect, either; no "K"
> possibility, since it's not a four-wire link using USB signaling.
Just because the signals aren't in the form used by the USB bus (K states)
doesn't mean they can't exist.
> For that matter, there's no requirement so _support_ "system" wakeup!
I'm interested in runtime, not system, wakeup.
> (Where remote wakeup mechanisms are only one part of the stack that
> needs to work. On PCs, it needs to cascade through S1/S3/... through
> ACPI to Linux. I've seen the wakeups arriving, but then ACPI borks;
> that's actually progress from the last time I tested this.)
You left out a third type of wakeup signal, assuming the HC isn't
suspended: an interrupt from the HC with the Resume Detect bit set in some
status register. This is in fact precisely the signal we would get when
the root hub is suspended, the HC isn't, and a port status change occurs.
These signals should be controllable by the same sort of wakeup_enable
flag as the others.
> Don't think so much about usb_suspend_device(); a call to that isn't
> a very interesting event.
Yes; the interesting events are the calls to dpm_runtime_suspend, which is
what gets called when you write to /sys/.../power/state.
> We want it *not to matter* very much if a
> usb link is suspended or not ... because the minute it has Real Work
> to do, it should wake right back up.
Only if wakeup_enabled is on.
> There's no way you can monitor the D+ and D- signals upstream of any
> root hub. Since the definition of the USB suspend state is in terms
> of those signals (within a VBUS power session), usb_suspend_device()
> can't do that to a root hub.
Root hubs don't have to obey the definition of the USB suspend state
because they aren't USB devices. However they should act in much the same
manner as USB devices when we call dpm_runtime_suspend.
> > I would be a lot happier if you renamed hcd->hub_suspend to
> > hcd->bus_suspend and explicitly defined it as leaving remote wakeup turned
> > off. Then there would have to be a new callback, hcd->rh_suspend, which
> > would do nothing but enable remote wakeup,
>
> I guess I don't see any confusion. The hub driver's suspend method works
> only on the resources it manages: downstream ports. Same for all hubs
> (root or external). As part of entering USB_STATE_SUSPEND, the device
> on the other end of that link may be told to enable remote wakeup.
The confusion is over whether the "hub" in "hcd->hub_suspend" is analogous
to the "hub" in "hub_suspend" or to the "hub" in "root hub"; that is,
whether it refers to a (root) hub interface/driver or a (root) hub device.
All along you've apparently thought of it as the HCD's component of an
interface suspend -- just as hub_suspend is the hub driver's component --
whereas I've thought of it as the HCD's component of the device suspend.
By definition (see above), the first means it gets called at the same time
as hub_suspend and the second means it gets called as part of
usb_suspend_device.
Or to put it another way, is it called "hcd->hub_suspend" because it is
logically part of the hub driver's hub_suspend, or because it suspends the
root hub?
That's why I prefer "bus_suspend", to make it clear that the routine
suspends an entire bus. Or even "global_suspend", since that's
appropriate, although I know you don't like it.
And I still say that we need to separate out remote-wakeup-enable for the
root hub from suspending the root hub's interface.
Alan Stern
-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
