[ Your post was quite long and my response will be ]
[ split in the middle at a natural break ... ]
> 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.
Those are essentially the same point. One thing to remember is that
we need to keep supporting CONFIG_USB_SUSPEND=n for some time yet;
if for no other reason than to avoid changing too much of a kinda
complicated system all at once. It'll take a while yet to shake
out the various config-specific bugs.
> 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;
Well, actually FOUR suspend states as shown in the USB spec, but on the
host side we only really need to handle the "address" and "configured"
variants.
> ... So I'll concentrate on (A).
Yes. They all boil down to that, although there are multiple scenarios
which can produce that result.
> (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;
Or the host/<hcd>-hub.c support, which implements that bit for root hubs.
There are also hosts that only support global suspend, it seems -- because
of errata, or otherwise.
> as far as
> usbcore is concerned (A) happens when usb_suspend_device is called. And
> even that fact should remain encapsulated in usbcore.
Fine. So clearly that will not apply to root hubs, since their
parent is never a hub.c interface.
> 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.
Subroutines are usbcore implementation artifacts, sandy ground on which
to build. I was aiming at the bedrock hardware constraints instead...
still behavioral, but not as malleable.
> 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.
To the hardware, it means suspending its upstream (USB) port.
In terms of usbcore framework, we've also required that all
the interfaces have been suspended too.
Agreed, those calls should be doing that today; easy to test
using sysfs.
> "Suspend a root hub" means calling dpm_runtime_suspend (or the
> equivalent) for the struct device embedded in the root hub's
> usb_device.
No... let's be consistent, and say suspending a hub just means
suspending its upstream port ... on PCs, this is a PCI device.
Remember the three faces of an HCD: upstream bus, downstream ports,
and stub usb_device. We have hardware constraints related to the
upstream and downstream links.
> "Suspend an interface" means calling dpm_runtime_suspend (or the
> equivalent) for the struct device embedded in the usb_interface.
Yes. And thus usb_driver->suspend() ... with the responsibility to
stop using ALL hardware resources associated with that interface
(including access to ep0).
> 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".)
OK by me. And maybe get rid of that hcd.c glue for it; I think we can
just ignore the last usb_bus driver (CRIS).
> 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.
Right, and from the outside all we see is four wires of USB signal.
> So okay, suppose 1-4.2 has been suspended and we suspend 1-4:1.0. What's
> the result?
I'll assume 1-4.1, 1-4.3, and so on are already suspended or disabled.
> (1) The hub driver unlinks the status URB. Hence it can no longer be
> aware of port status changes.
Right. The hub driver is no longer managing its resources, that is
to say "hub ports".
> (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.
Right. Because the upstream link was not suspended.
> (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.
Right. Because the hub driver is no longer managing those resources,
yet the forwarding mechanism (suspend upstream link) was disabled.
Note that you're assuming that device_may_wakeup(1-4.2) is true,
otherwise that device won't be issuing wakeups. And also that
device_may_wakeup(1-4) is also true, otherwise it would never
forward the event in any case.
> 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.
What it shows is a device state that we want to avoid. And one
that's actually easy to avoid: for devices that issue wakeups,
autosuspend the device as soon as all interfaces are 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.
Right.
> 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.
Right, same thing.
> (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.
Right, same thing and same assumptions about wakeup being enabled.
> 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?).
I have patches in the works to update some of this. They didn't
seem like immediate priorities.
- The hcd can_wakeup and remote_wakeup flags need to move into
struct device
- Resume path recursion must vanish, except (!) within HCDs when
USB_SUSPEND=n.
These need some care because of situations like PCI devices which issue
wakeup events without PCI PME#, BIOS interactions, chip errata, and all
that sort of "good fun".
Re "which one" ... "usb1" clearly, since there is no other flag. See the
appended script, convenient for scanning sysfs for the devices which could
issue wake events. This is for the stub device.
> 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.
Again, just like for an external hub.
> As before, let's continue by suspending usb1. The result is:
As before, but adding (3'n) as a variant assumption.
> (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).
Again, the same.
> (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).
Not so; the only wakeup flag that could be set would be for usb1,
so that's the only one that _could_ be examined.
> (I trust those capsule summaries are correct and agree with what your
> patches do. Let me know if I made a mistake.)
See above.
> Clearly there are some major differences between (1)-(5) and (1')-(5').
> Let's consider them in order.
I didn't actually notice any differences ... see above (mostly).
> 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.
There's no difference: all hubs suspend their downstream ports; that's
the basic requirement. Suspending any port implies any device there cut
its power usage from 200mA to 0.2mA (or whatever).
> The one false note is that hcd->hub_suspend
> enables remote wakeup for the root hub; see below.
Not so; remote wakeup can always be disabled. Clearly, autosuspend
depends on its use ... for "autoresume".
> (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.
The HCD is what gets the IRQ though; how _else_ is khubd ever going
to learn about that event? Remember that it only does that as part
of autoresume.
> To sum up, the important differences are the ones between (3)/(3') and
> (5)/(5').
I don't see any differences, once the descriptions have been corrected.
> 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.
You're making a big deal about the "odd" state where the downstream
links are suspended but the upstream one is not. That will never
behave well. And given that HCD upstream links obey non-USB rules,
this is another case where root hubs are necessarily different.
- Dave
> All right. Having said all of that, I'll respond to some of your
> comments.
-------------------------------------------------------
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
