Here's the problem, described as briefly as I can:
When 1-0:1.0 is suspended, remote wakeup is enabled or not,
based on the setting of the wakeup_enable flag for usb1.
And when usb1 is suspended, the wakeup_enable flag for usb1
isn't checked at all.
That's obviously incorrect behavior, unless you assume that usb1 and
1-0:1.0 are always suspended and resumed together. And even then it's
illogical. Compare it to the behavior of an external hub:
When 1-4:1.0 is suspended, remote wakeup is not enabled.
And when 1-4 is suspended, remote wakeup is enabled or not,
based on the setting of the wakeup_enable flag for 1-4.
On Wed, 5 Oct 2005, David Brownell wrote:
> > So okay, suppose 1-4.2 has been suspended and we suspend 1-4:1.0. What's
> > the result?
...
> > (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.
I didn't assume anything; I said "If ...". Clearly that "if" will not
hold unless the device has been enabled for wakeup.
> And also that
> device_may_wakeup(1-4) is also true, otherwise it would never
> forward the event in any case.
That's actually an interesting point. Note first that if 1-4 isn't
suspended then its wakeup setting doesn't matter, and the event is
reported in the status URB as usual. But what if it _is_ suspended?
According to the USB spec, the downstream port enters the Restart_S state
(Figure 11-10 and 11.5.1.12) and sets the C_PORT_SUSPEND bit, causing the
internal port to propagate the K signal upstream. 11.4.4 says that this
happens whenever C_PORT_SUSPEND is set for any port, or if the hub is
enabled as a wakeup source and any Port Change or Hub Change bit is set.
So downstream wakeup requests get repeated upstream whether or not the hub
is enabled for wakeup. Port change events generate wakeup requests only
when the hub is enabled.
Also interesting is that the sequence of state transitions is not the same
if the hub's upstream port is suspended and its downstream port isn't (as
in a "global" suspend). It's still true, though, that resume signals are
repeated upstream whether or not the hub is enabled for wakeup.
> > 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:
...
> > (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.
It's not the same thing at all -- go back and reread (3). (3) says that
if 1-4.2 sends a wakeup signal, nothing much will happen. Here (3') says
that if 1-4 sends a wakeup signal, lots of stuff will happen.
> > 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.
Okay, let's assume those patches have been finished up and applied.
> > 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.
Just like (3), yes. Very different from (3').
> > As before, let's continue by suspending usb1. The result is:
>
> As before, but adding (3'n) as a variant assumption.
...
> > (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.
It _could_ be examined, but in the current code it _isn't_. All that
happens is usb_generic_suspend calls usb_suspend_device, which calls
__usb_suspend_device, which checks that the interface is already suspended
and then returns. It does _not_ look at any wakeup_enabled flags.
> > 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).
I described above how (3) and (3') are radically different. Your comment
on (5') doesn't alter the fact that it is different from (5).
> > 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).
External hubs don't suspend their downstream ports when the _interface_ is
suspended! They don't even know that anything has happened. All
disabled ports, for instance, remain capable of supplying full power.
> > (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.
khubd is not supposed to learn about the event. Just like in (3), where
it never learns about a wakeup request from a child device.
> > 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.
Yes I am. That's the whole point -- you've moved most of the
functionality that formerly went with suspending the upstream link into
the routine for suspending the downstream link. The effects of this
change show up precisely when one link is suspended and the other isn't.
And even though root hubs don't have a real upstream link, they do have an
associated stub usb_device.
> 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.
Maybe it won't behave _well_, but it should behave _consistently_ for
external hubs and root hubs. This isn't a necessary difference at all.
It can easily be set right be arranging for remote wakeup to be enabled on
a root hub when the stub usb_device is suspended, not when the interface
is suspended.
> > 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.
>
> Those events don't go upstream. They're internal to the HCD.
> So in that "system" sense they're not really wakeup events ... for
> all that they use those hardware wakeup signaling. (At least,
> they do if the root hub doesn't have the bugs that some have...)
Now we're running into confusion over whether a remote wakeup signal
constitutes a "wakeup event". If the system is asleep, clearly it does.
But if the device is selectively suspended ... then what do you call it?
A "selective resume request"?
> I guess I just don't see any point to that. Though you may be focussing
> more on _external_ software trying to manage the root hub power than
> the autosuspend/autoresume mechanism already working in OHCI.
That's exactly right. The autosuspend/autoresume mechanism won't have any
problem because it will always update the stub device and the interface
together. The possibility for confusion arises mostly for external
software (and also for programmers trying to figure out why the code is so
illogical!).
> (I came
> across a note from someone who said that made a notable difference in
> battery life on their laptop, FWIW.)
Of the two levels involved (PCI device and root hub), do you know which
had a larger impact on the overall power savings?
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
