Hello,
machine with Asus P4P800-VM mainboard wakes up immediately after:
echo mem >/sys/power/state
or even
echo disk >/sys/power/state (only when /sys/power/disk is set to "platform",
which is the default)
The problem disappears when unloading ehci_hcd module. There are no USB
devices attached. The problem seems to be something like this:
http://www.mail-archive.com/linux-usb-devel%40lists.sourceforge.net/msg54499.html
/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default
/sys/devices/pci0000:00/0000:00:1d.7/usb1/power/wakeup is enabled by default,
changing it to disabled fixes the problem
The board has 4 USBPWxx jumpers (USBPW12, USBPW34, USBPW56 and USBPW78) that
selects 5V or 5VSB as power to the corresponding USB connectors. When they're
all set to 5VSB, the immediate wakeup does not appear. When any of them is
set to 5V, it wakes up immediately after entering suspend. I guess that the
loss of power to any of the ports (5V power is turned off in suspend) is
detected as some event (overcurrent or device connect?)
"ignore_oc=1" parameter to ehci_hcd modules does not change anything.
--
Ondrej Zary
Disable wake on overcurrent in EHCI.
This fixes immediate resume from standby on boards where port power is lost
in standby which triggers overcurrent detection. At least Asus P4P800 boards
are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.
Tested on Asus P4P800-VM.
Signed-off-by: Ondrej Zary <[email protected]>
--- linux-source-2.6.33-orig/drivers/usb/host/ehci-hub.c 2010-02-24 19:52:17.000000000 +0100
+++ linux-source-2.6.33/drivers/usb/host/ehci-hub.c 2010-04-27 16:04:37.000000000 +0200
@@ -180,10 +180,10 @@ static int ehci_bus_suspend (struct usb_
* set), the port change detection will finally fix it.
*/
if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
+ t2 |= PORT_WKDISC_E;
t2 &= ~PORT_WKCONN_E;
} else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
+ t2 |= PORT_WKCONN_E;
t2 &= ~PORT_WKDISC_E;
}
} else
@@ -912,7 +912,7 @@ static int ehci_hub_control (
* mode if we have hostpc feature
*/
temp &= ~PORT_WKCONN_E;
- temp |= PORT_WKDISC_E | PORT_WKOC_E;
+ temp |= PORT_WKDISC_E;
ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
if (hostpc_reg) {
spin_unlock_irqrestore(&ehci->lock, flags);
--
Ondrej Zary
The previous patch was not enough as it worked only when there were no USB
devices connected. With a bus-powered device connected, power loss causes
disconnect which wakes up the machine immediately again.
Does anyone know why is this enabled by default? No other USB host driver
(except oxu210hp-hcd which is based on EHCI) does that. This also means that
suspended laptop wakes up when disconnecting a mouse? I don't have any to test
right now. Wakeup on USB connect makes a bit more sense but I'd say that it's
still not a good idea to enable it by default.
If we don't need that, perhaps the following patch should be applied.
Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost
in standby which triggers overcurrent detection and disconnect if a
bus-powered device is connected. At least Asus P4P800 boards are affected when
any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.
Tested on Asus P4P800-VM.
Signed-off-by: Ondrej Zary <[email protected]>
--- linux-source-2.6.33-orig/drivers/usb/host/ehci-hub.c 2010-02-24 19:52:17.000000000 +0100
+++ linux-source-2.6.33/drivers/usb/host/ehci-hub.c 2010-04-27 18:17:36.000000000 +0200
@@ -173,21 +173,16 @@ static int ehci_bus_suspend (struct usb_
}
/* enable remote wakeup on all ports */
+ t2 &= ~PORT_WAKE_BITS;
if (hcd->self.root_hub->do_remote_wakeup) {
/* only enable appropriate wake bits, otherwise the
* hardware can not go phy low power mode. If a race
* condition happens here(connection change during bits
* set), the port change detection will finally fix it.
*/
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }
- } else
- t2 &= ~PORT_WAKE_BITS;
+ if (!(t1 & PORT_CONNECT))
+ t2 |= PORT_WKCONN_E;
+ }
if (t1 != t2) {
ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
--
Ondrej Zary
On Tue, 27 Apr 2010, Ondrej Zary wrote:
> The previous patch was not enough as it worked only when there were no USB
> devices connected. With a bus-powered device connected, power loss causes
> disconnect which wakes up the machine immediately again.
You said earlier that the host controller was disabled for remote
wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled
by default"). So even though the root hub might issue a wakeup
request, the controller hardware should not forward that request to the
PCI bus and it should not cause the system to wake up.
> Does anyone know why is this enabled by default?
Why _what_ is enabled? Detection of disconnects? Because otherwise
your computer wouldn't realize anything had happened when a suspended
USB device was unplugged from a suspended root hub.
> No other USB host driver
> (except oxu210hp-hcd which is based on EHCI) does that.
Look again -- they all do. (All the HCDs that support suspend/resume,
anyway.)
> This also means that
> suspended laptop wakes up when disconnecting a mouse?
No, for the reason I described above. The controller is aware of the
wakeup request but doesn't generate a PME# event. Likewise for desktop
systems.
> I don't have any to test
> right now. Wakeup on USB connect makes a bit more sense but I'd say that it's
> still not a good idea to enable it by default.
>
> If we don't need that, perhaps the following patch should be applied.
>
>
> Disable wake on overcurrent and disconnect in EHCI.
> This fixes immediate resume from standby on boards where port power is lost
> in standby which triggers overcurrent detection and disconnect if a
> bus-powered device is connected. At least Asus P4P800 boards are affected when
> any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.
Why would you want to change the jumper settings? Host controllers are
_supposed_ to supply 5V power during system suspend.
Alan Stern
On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:
> On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > The previous patch was not enough as it worked only when there were no
> > USB devices connected. With a bus-powered device connected, power loss
> > causes disconnect which wakes up the machine immediately again.
>
> You said earlier that the host controller was disabled for remote
> wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled
> by default"). So even though the root hub might issue a wakeup
> request, the controller hardware should not forward that request to the
> PCI bus and it should not cause the system to wake up.
Maybe it should not - but it wakes up this board and probably also P4P800,
P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497
> > Does anyone know why is this enabled by default?
>
> Why _what_ is enabled? Detection of disconnects? Because otherwise
> your computer wouldn't realize anything had happened when a suspended
> USB device was unplugged from a suspended root hub.
That's not disconnect detection - that's wakeup on disconnect. If I understand
EHCI 1.0 specification correctly, disconnect detection should work regardless
of the state of this bit:
| PORTSC bit 21: Wake on Disconnect Enable (WKDSCNNT_E):
| R/W. Default = 0b.
| Writing this bit to a one enables the port to be sensitive to device
| disconnects as wake-up events. See Section 4.3 for effects of this bit on
| resume event behavior. Refer to Section 4.3.1 for operational model.
And it really does. With this patch applied, system does not wake up when a
device is disconnected during suspend. When I wake up the system manually,
the disconnect is detected immediately (does not matter
> > No other USB host driver
> > (except oxu210hp-hcd which is based on EHCI) does that.
>
> Look again -- they all do. (All the HCDs that support suspend/resume,
> anyway.)
>
> > This also means that
> > suspended laptop wakes up when disconnecting a mouse?
>
> No, for the reason I described above. The controller is aware of the
> wakeup request but doesn't generate a PME# event. Likewise for desktop
> systems.
>
> > I don't have any to test
> > right now. Wakeup on USB connect makes a bit more sense but I'd say that
> > it's still not a good idea to enable it by default.
> >
> > If we don't need that, perhaps the following patch should be applied.
> >
> >
> > Disable wake on overcurrent and disconnect in EHCI.
> > This fixes immediate resume from standby on boards where port power is
> > lost in standby which triggers overcurrent detection and disconnect if a
> > bus-powered device is connected. At least Asus P4P800 boards are affected
> > when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.
>
> Why would you want to change the jumper settings? Host controllers are
> _supposed_ to supply 5V power during system suspend.
Maybe because I don't want all my USB devices to be powered when the system is
turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power
to USB ports.
--
Ondrej Zary
On Tue, Apr 27, 2010 at 10:46:48PM +0200, Ondrej Zary wrote:
> > Why would you want to change the jumper settings? Host controllers are
> > _supposed_ to supply 5V power during system suspend.
>
> Maybe because I don't want all my USB devices to be powered when the system is
> turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power
> to USB ports.
On the contrary, it does. Some of us use this mode to charge devices :)
thanks,
greg k-h
On Wednesday 28 April 2010 17:41:30 Alan Stern wrote:
> On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:
> > > On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > > > The previous patch was not enough as it worked only when there were
> > > > no USB devices connected. With a bus-powered device connected, power
> > > > loss causes disconnect which wakes up the machine immediately again.
> > >
> > > You said earlier that the host controller was disabled for remote
> > > wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled
> > > by default"). So even though the root hub might issue a wakeup
> > > request, the controller hardware should not forward that request to the
> > > PCI bus and it should not cause the system to wake up.
> >
> > Maybe it should not - but it wakes up this board and probably also
> > P4P800, P4P800-E and P4C800-E at least:
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497
>
> Okay, evidently the hardware or firmware on these boards is buggy.
> Other systems do not have the same problem, as far as I know.
It works fine in Windows.
Now I took another machine - IBM ThinkCentre M51 (i915+ICH6). USB ports are
powered in suspend here so it does not resume immediately. But
connecting/disconnecting an USB device wakes it up from suspend. Only in
Linux, not in Windows.
> > > > Does anyone know why is this enabled by default?
> > >
> > > Why _what_ is enabled? Detection of disconnects? Because otherwise
> > > your computer wouldn't realize anything had happened when a suspended
> > > USB device was unplugged from a suspended root hub.
> >
> > That's not disconnect detection - that's wakeup on disconnect.
>
> True; I oversimplified. Furthermore, starting in 2.6.34, the wakeup
> settings during system sleep (suspend or hibernation) can be different
> from the settings during autosuspend, so you can have root hubs enabled
> for wakeup during autosuspend but not during system sleep.
>
> > If I understand
> > EHCI 1.0 specification correctly, disconnect detection should work
> > regardless
> >
> > of the state of this bit:
> > | PORTSC bit 21: Wake on Disconnect Enable (WKDSCNNT_E):
> > | R/W. Default = 0b.
> > | Writing this bit to a one enables the port to be sensitive to device
> > | disconnects as wake-up events. See Section 4.3 for effects of this bit
> > | on resume event behavior. Refer to Section 4.3.1 for operational model.
> >
> > And it really does. With this patch applied, system does not wake up when
> > a device is disconnected during suspend. When I wake up the system
> > manually, the disconnect is detected immediately (does not matter
>
> It's worth pointing out that EHCI is different in this respect from
> OHCI and UHCI; the older controllers do not have the capability to
> enable or disable wakeup independently for connect, disconnect, and
> overcurrent events. They are all or nothing. So are external USB
> hubs.
>
> > > > If we don't need that, perhaps the following patch should be applied.
> > > >
> > > >
> > > > Disable wake on overcurrent and disconnect in EHCI.
> > > > This fixes immediate resume from standby on boards where port power
> > > > is lost in standby which triggers overcurrent detection and
> > > > disconnect if a bus-powered device is connected. At least Asus P4P800
> > > > boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is
> > > > set to 5V.
> > >
> > > Why would you want to change the jumper settings? Host controllers are
> > > _supposed_ to supply 5V power during system suspend.
> >
> > Maybe because I don't want all my USB devices to be powered when the
> > system is turned off. I doubt that laptop in suspend-to-RAM (on battery)
> > provides power to USB ports.
>
> This depends on how your system was turned off. During suspend or
> hibernation, you _should_ want USB devices to be powered (and some
> people do, as Greg pointed out). During a normal system shutdown, the
> USB buses should not be powered.
>
> Regardless, I don't think a kernel patch is the way to solve your
> problem. Changing the wakeup setting for the root hub will do what you
> want, and that setting is explicitly intended to be controlled by
> userspace (after all, that's why it is exposed in sysfs). The initial
> value is only a reasonable default; you can certainly add scripts or
> udev rules to disable wakeup on your EHCI root hub.
Yes, I can work around that. But many users can't. This is an attempt to make
it "just work".
I'm trying to say that the "wakeup on everything" is not a good thing (if not
a bug). Who needs it? I can't imagine any real use for it. It clearly breaks
suspend on some systems and is annoying on other. Who expects that
disconnecting a device should wake up sleeping machine?
When all these three wakeups (overcurrent, connect, disconnect) are disabled,
we lose nothing. Connect/disconnect detection works fine after wakeup. Wakeup
by USB devices (not by connect/disconnect but by the device itself signaling
a resume) is completely independent of this (according to EHCI
specification).
--
Ondrej Zary
On Tue, 27 Apr 2010, Ondrej Zary wrote:
> On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:
> > On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > > The previous patch was not enough as it worked only when there were no
> > > USB devices connected. With a bus-powered device connected, power loss
> > > causes disconnect which wakes up the machine immediately again.
> >
> > You said earlier that the host controller was disabled for remote
> > wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled
> > by default"). So even though the root hub might issue a wakeup
> > request, the controller hardware should not forward that request to the
> > PCI bus and it should not cause the system to wake up.
>
> Maybe it should not - but it wakes up this board and probably also P4P800,
> P4P800-E and P4C800-E at least:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497
Okay, evidently the hardware or firmware on these boards is buggy.
Other systems do not have the same problem, as far as I know.
> > > Does anyone know why is this enabled by default?
> >
> > Why _what_ is enabled? Detection of disconnects? Because otherwise
> > your computer wouldn't realize anything had happened when a suspended
> > USB device was unplugged from a suspended root hub.
>
> That's not disconnect detection - that's wakeup on disconnect.
True; I oversimplified. Furthermore, starting in 2.6.34, the wakeup
settings during system sleep (suspend or hibernation) can be different
from the settings during autosuspend, so you can have root hubs enabled
for wakeup during autosuspend but not during system sleep.
> If I understand
> EHCI 1.0 specification correctly, disconnect detection should work regardless
> of the state of this bit:
> | PORTSC bit 21: Wake on Disconnect Enable (WKDSCNNT_E):
> | R/W. Default = 0b.
> | Writing this bit to a one enables the port to be sensitive to device
> | disconnects as wake-up events. See Section 4.3 for effects of this bit on
> | resume event behavior. Refer to Section 4.3.1 for operational model.
>
> And it really does. With this patch applied, system does not wake up when a
> device is disconnected during suspend. When I wake up the system manually,
> the disconnect is detected immediately (does not matter
It's worth pointing out that EHCI is different in this respect from
OHCI and UHCI; the older controllers do not have the capability to
enable or disable wakeup independently for connect, disconnect, and
overcurrent events. They are all or nothing. So are external USB
hubs.
> > > If we don't need that, perhaps the following patch should be applied.
> > >
> > >
> > > Disable wake on overcurrent and disconnect in EHCI.
> > > This fixes immediate resume from standby on boards where port power is
> > > lost in standby which triggers overcurrent detection and disconnect if a
> > > bus-powered device is connected. At least Asus P4P800 boards are affected
> > > when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.
> >
> > Why would you want to change the jumper settings? Host controllers are
> > _supposed_ to supply 5V power during system suspend.
>
> Maybe because I don't want all my USB devices to be powered when the system is
> turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power
> to USB ports.
This depends on how your system was turned off. During suspend or
hibernation, you _should_ want USB devices to be powered (and some
people do, as Greg pointed out). During a normal system shutdown, the
USB buses should not be powered.
Regardless, I don't think a kernel patch is the way to solve your
problem. Changing the wakeup setting for the root hub will do what you
want, and that setting is explicitly intended to be controlled by
userspace (after all, that's why it is exposed in sysfs). The initial
value is only a reasonable default; you can certainly add scripts or
udev rules to disable wakeup on your EHCI root hub.
Alan Stern
On Thursday 29 April 2010 19:45:33 Alan Stern wrote:
> On Thu, 29 Apr 2010, Alan Stern wrote:
> > > > > > You said earlier that the host controller was disabled for remote
> > > > > > wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is
> > > > > > disabled by default"). So even though the root hub might issue a
> > > > > > wakeup request, the controller hardware should not forward that
> > > > > > request to the PCI bus and it should not cause the system to wake
> > > > > > up.
> > > > >
> > > > > Maybe it should not - but it wakes up this board and probably also
> > > > > P4P800, P4P800-E and P4C800-E at least:
> > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497
> > > >
> > > > Okay, evidently the hardware or firmware on these boards is buggy.
> > > > Other systems do not have the same problem, as far as I know.
> >
> > I take this back. The same thing happens on my machine (Intel ICH5
> > chipset). I'd guess there is a bug in the PCI or ACPI subsystem, but
> > more testing is needed before I can be sure. Somehow the PCI or
> > platform wakeup mechanism gets activated even when it is supposed to be
> > disabled.
>
> This patch fixes the problem on my system. Does it work for you?
> I still think that disabling wakeup at the PCI or platform level should
> override the port wakeup flags, but apparently it doesn't.
Thanks for the patch. When applied to 2.6.33 kernel, the ehci-fsl.c part fails
but that does not matter. Asus P4P800-VM now suspends correctly even when the
jumpers are set to 5V, both with and without USB device. Also device (un)plug
does not wake up the system anymore. So the patch works fine.
Yes, this wakeup seems to work differently on different systems. For example,
Asus A7V8X-X [VIA] (which has these 5V/5VSB jumpers too) is not affected by
this (it wakes up dead but that's another problem). Also Asus Eee PC 701
(ICH6) and ASRock 939Dual-VSTA (ULi) are not affected. So maybe it affects
only ICH4/5/6 chips (only some revisions?). Or it's also board-dependent.
> Alek, can you confirm that the changes in this patch are okay for the
> Moorestown EHCI controller? I had to guess at how to handle the
> HOSTPCx register settings.
>
> Alan Stern
>
>
>
> Index: usb-2.6/drivers/usb/host/ehci-hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-hub.c
> +++ usb-2.6/drivers/usb/host/ehci-hub.c
> @@ -106,6 +106,47 @@ static void ehci_handover_companion_port
> ehci->owned_ports = 0;
> }
>
> +static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
> +{
> + int port;
> +
> + /* enable remote wakeup on all ports, if allowed */
> + port = HCS_N_PORTS(ehci->hcs_params);
> + while (port--) {
> + u32 __iomem *reg = &ehci->regs->port_status[port];
> + u32 t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
> + u32 t2 = t1 & ~PORT_WAKE_BITS;
> +
> + /* only enable appropriate wake bits, otherwise the
> + * hardware can not go phy low power mode. If a race
> + * condition happens here(connection change during bits
> + * set), the port change detection will finally fix it.
> + */
> + if (device_may_wakeup(ehci_to_hcd(ehci)->self.controller)) {
> + if (t1 & PORT_CONNECT)
> + t2 |= PORT_WKOC_E | PORT_WKDISC_E;
> + else
> + t2 |= PORT_WKOC_E | PORT_WKCONN_E;
> + ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
> + port + 1, t1, t2);
> + }
> + ehci_writel(ehci, t2, reg);
> + }
> +}
> +
> +static void ehci_clear_wakeup_flags(struct ehci_hcd *ehci)
> +{
> + int port;
> +
> + port = HCS_N_PORTS(ehci->hcs_params);
> + while (port--) {
> + u32 __iomem *reg = &ehci->regs->port_status[port];
> + u32 t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
> +
> + ehci_writel(ehci, t1 & ~PORT_WAKE_BITS, reg);
> + }
> +}
> +
> static int ehci_bus_suspend (struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> @@ -170,26 +211,6 @@ static int ehci_bus_suspend (struct usb_
> else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
> t2 |= PORT_SUSPEND;
> set_bit(port, &ehci->bus_suspended);
> - }
> -
> - /* enable remote wakeup on all ports */
> - if (hcd->self.root_hub->do_remote_wakeup) {
> - /* only enable appropriate wake bits, otherwise the
> - * hardware can not go phy low power mode. If a race
> - * condition happens here(connection change during bits
> - * set), the port change detection will finally fix it.
> - */
> - if (t1 & PORT_CONNECT) {
> - t2 |= PORT_WKOC_E | PORT_WKDISC_E;
> - t2 &= ~PORT_WKCONN_E;
> - } else {
> - t2 |= PORT_WKOC_E | PORT_WKCONN_E;
> - t2 &= ~PORT_WKDISC_E;
> - }
> - } else
> - t2 &= ~PORT_WAKE_BITS;
> -
> - if (t1 != t2) {
> ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
> port + 1, t1, t2);
> ehci_writel(ehci, t2, reg);
> @@ -907,12 +928,7 @@ static int ehci_hub_control (
>
> || (temp & PORT_RESET) != 0)
>
> goto error;
>
> - /* After above check the port must be connected.
> - * Set appropriate bit thus could put phy into low power
> - * mode if we have hostpc feature
> - */
> - temp &= ~PORT_WKCONN_E;
> - temp |= PORT_WKDISC_E | PORT_WKOC_E;
> + temp &= ~PORT_WAKE_BITS;
> ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
> if (hostpc_reg) {
> spin_unlock_irqrestore(&ehci->lock, flags);
> Index: usb-2.6/drivers/usb/host/ehci-pci.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-pci.c
> +++ usb-2.6/drivers/usb/host/ehci-pci.c
> @@ -284,23 +284,15 @@ static int ehci_pci_suspend(struct usb_h
> msleep(10);
>
> /* Root hub was already suspended. Disable irq emission and
> - * mark HW unaccessible, bail out if RH has been resumed. Use
> - * the spinlock to properly synchronize with possible pending
> - * RH suspend or resume activity.
> - *
> - * This is still racy as hcd->state is manipulated outside of
> - * any locks =P But that will be a different fix.
> + * mark HW unaccessible. The PM and USB cores make sure that
> + * the root hub is either suspended or stopped.
> */
> spin_lock_irqsave (&ehci->lock, flags);
> - if (hcd->state != HC_STATE_SUSPENDED) {
> - rc = -EINVAL;
> - goto bail;
> - }
> + ehci_set_wakeup_flags(ehci);
> ehci_writel(ehci, 0, &ehci->regs->intr_enable);
> (void)ehci_readl(ehci, &ehci->regs->intr_enable);
>
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> - bail:
> spin_unlock_irqrestore (&ehci->lock, flags);
>
> // could save FLADJ in case of Vaux power loss
> @@ -330,6 +322,7 @@ static int ehci_pci_resume(struct usb_hc
> !hibernated) {
> int mask = INTR_MASK;
>
> + ehci_clear_wakeup_flags(ehci);
> if (!hcd->self.root_hub->do_remote_wakeup)
> mask &= ~STS_PCD;
> ehci_writel(ehci, mask, &ehci->regs->intr_enable);
> Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
> +++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
> @@ -215,26 +215,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
> msleep(10);
>
> /* Root hub was already suspended. Disable irq emission and
> - * mark HW unaccessible, bail out if RH has been resumed. Use
> - * the spinlock to properly synchronize with possible pending
> - * RH suspend or resume activity.
> - *
> - * This is still racy as hcd->state is manipulated outside of
> - * any locks =P But that will be a different fix.
> + * mark HW unaccessible. The PM and USB cores make sure that
> + * the root hub is either suspended or stopped.
> */
> spin_lock_irqsave(&ehci->lock, flags);
> - if (hcd->state != HC_STATE_SUSPENDED) {
> - rc = -EINVAL;
> - goto bail;
> - }
> + ehci_set_wakeup_flags(ehci);
> ehci_writel(ehci, 0, &ehci->regs->intr_enable);
> (void)ehci_readl(ehci, &ehci->regs->intr_enable);
>
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>
> au1xxx_stop_ehc();
> -
> -bail:
> spin_unlock_irqrestore(&ehci->lock, flags);
>
> // could save FLADJ in case of Vaux power loss
> @@ -264,6 +255,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
> if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
> int mask = INTR_MASK;
>
> + ehci_clear_wakeup_flags(ehci);
> if (!hcd->self.root_hub->do_remote_wakeup)
> mask &= ~STS_PCD;
> ehci_writel(ehci, mask, &ehci->regs->intr_enable);
> Index: usb-2.6/drivers/usb/host/ehci-fsl.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
> +++ usb-2.6/drivers/usb/host/ehci-fsl.c
> @@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
> struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> void __iomem *non_ehci = hcd->regs;
>
> + ehci_set_wakeup_flags(ehci);
> if (!fsl_deep_sleep())
> return 0;
>
> @@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> void __iomem *non_ehci = hcd->regs;
>
> + ehci_clear_wakeup_flags(ehci);
> if (!fsl_deep_sleep())
> return 0;
--
Ondrej Zary
On Thu, 29 Apr 2010, Alan Stern wrote:
> > > > > You said earlier that the host controller was disabled for remote
> > > > > wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled
> > > > > by default"). So even though the root hub might issue a wakeup
> > > > > request, the controller hardware should not forward that request to the
> > > > > PCI bus and it should not cause the system to wake up.
> > > >
> > > > Maybe it should not - but it wakes up this board and probably also
> > > > P4P800, P4P800-E and P4C800-E at least:
> > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497
> > >
> > > Okay, evidently the hardware or firmware on these boards is buggy.
> > > Other systems do not have the same problem, as far as I know.
>
> I take this back. The same thing happens on my machine (Intel ICH5
> chipset). I'd guess there is a bug in the PCI or ACPI subsystem, but
> more testing is needed before I can be sure. Somehow the PCI or
> platform wakeup mechanism gets activated even when it is supposed to be
> disabled.
This patch fixes the problem on my system. Does it work for you?
I still think that disabling wakeup at the PCI or platform level should
override the port wakeup flags, but apparently it doesn't.
Alek, can you confirm that the changes in this patch are okay for the
Moorestown EHCI controller? I had to guess at how to handle the
HOSTPCx register settings.
Alan Stern
Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
+++ usb-2.6/drivers/usb/host/ehci-hub.c
@@ -106,6 +106,47 @@ static void ehci_handover_companion_port
ehci->owned_ports = 0;
}
+static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
+{
+ int port;
+
+ /* enable remote wakeup on all ports, if allowed */
+ port = HCS_N_PORTS(ehci->hcs_params);
+ while (port--) {
+ u32 __iomem *reg = &ehci->regs->port_status[port];
+ u32 t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+ u32 t2 = t1 & ~PORT_WAKE_BITS;
+
+ /* only enable appropriate wake bits, otherwise the
+ * hardware can not go phy low power mode. If a race
+ * condition happens here(connection change during bits
+ * set), the port change detection will finally fix it.
+ */
+ if (device_may_wakeup(ehci_to_hcd(ehci)->self.controller)) {
+ if (t1 & PORT_CONNECT)
+ t2 |= PORT_WKOC_E | PORT_WKDISC_E;
+ else
+ t2 |= PORT_WKOC_E | PORT_WKCONN_E;
+ ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
+ port + 1, t1, t2);
+ }
+ ehci_writel(ehci, t2, reg);
+ }
+}
+
+static void ehci_clear_wakeup_flags(struct ehci_hcd *ehci)
+{
+ int port;
+
+ port = HCS_N_PORTS(ehci->hcs_params);
+ while (port--) {
+ u32 __iomem *reg = &ehci->regs->port_status[port];
+ u32 t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+
+ ehci_writel(ehci, t1 & ~PORT_WAKE_BITS, reg);
+ }
+}
+
static int ehci_bus_suspend (struct usb_hcd *hcd)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
@@ -170,26 +211,6 @@ static int ehci_bus_suspend (struct usb_
else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
t2 |= PORT_SUSPEND;
set_bit(port, &ehci->bus_suspended);
- }
-
- /* enable remote wakeup on all ports */
- if (hcd->self.root_hub->do_remote_wakeup) {
- /* only enable appropriate wake bits, otherwise the
- * hardware can not go phy low power mode. If a race
- * condition happens here(connection change during bits
- * set), the port change detection will finally fix it.
- */
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }
- } else
- t2 &= ~PORT_WAKE_BITS;
-
- if (t1 != t2) {
ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
port + 1, t1, t2);
ehci_writel(ehci, t2, reg);
@@ -907,12 +928,7 @@ static int ehci_hub_control (
|| (temp & PORT_RESET) != 0)
goto error;
- /* After above check the port must be connected.
- * Set appropriate bit thus could put phy into low power
- * mode if we have hostpc feature
- */
- temp &= ~PORT_WKCONN_E;
- temp |= PORT_WKDISC_E | PORT_WKOC_E;
+ temp &= ~PORT_WAKE_BITS;
ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
if (hostpc_reg) {
spin_unlock_irqrestore(&ehci->lock, flags);
Index: usb-2.6/drivers/usb/host/ehci-pci.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-pci.c
+++ usb-2.6/drivers/usb/host/ehci-pci.c
@@ -284,23 +284,15 @@ static int ehci_pci_suspend(struct usb_h
msleep(10);
/* Root hub was already suspended. Disable irq emission and
- * mark HW unaccessible, bail out if RH has been resumed. Use
- * the spinlock to properly synchronize with possible pending
- * RH suspend or resume activity.
- *
- * This is still racy as hcd->state is manipulated outside of
- * any locks =P But that will be a different fix.
+ * mark HW unaccessible. The PM and USB cores make sure that
+ * the root hub is either suspended or stopped.
*/
spin_lock_irqsave (&ehci->lock, flags);
- if (hcd->state != HC_STATE_SUSPENDED) {
- rc = -EINVAL;
- goto bail;
- }
+ ehci_set_wakeup_flags(ehci);
ehci_writel(ehci, 0, &ehci->regs->intr_enable);
(void)ehci_readl(ehci, &ehci->regs->intr_enable);
clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
- bail:
spin_unlock_irqrestore (&ehci->lock, flags);
// could save FLADJ in case of Vaux power loss
@@ -330,6 +322,7 @@ static int ehci_pci_resume(struct usb_hc
!hibernated) {
int mask = INTR_MASK;
+ ehci_clear_wakeup_flags(ehci);
if (!hcd->self.root_hub->do_remote_wakeup)
mask &= ~STS_PCD;
ehci_writel(ehci, mask, &ehci->regs->intr_enable);
Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
+++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
@@ -215,26 +215,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
msleep(10);
/* Root hub was already suspended. Disable irq emission and
- * mark HW unaccessible, bail out if RH has been resumed. Use
- * the spinlock to properly synchronize with possible pending
- * RH suspend or resume activity.
- *
- * This is still racy as hcd->state is manipulated outside of
- * any locks =P But that will be a different fix.
+ * mark HW unaccessible. The PM and USB cores make sure that
+ * the root hub is either suspended or stopped.
*/
spin_lock_irqsave(&ehci->lock, flags);
- if (hcd->state != HC_STATE_SUSPENDED) {
- rc = -EINVAL;
- goto bail;
- }
+ ehci_set_wakeup_flags(ehci);
ehci_writel(ehci, 0, &ehci->regs->intr_enable);
(void)ehci_readl(ehci, &ehci->regs->intr_enable);
clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
au1xxx_stop_ehc();
-
-bail:
spin_unlock_irqrestore(&ehci->lock, flags);
// could save FLADJ in case of Vaux power loss
@@ -264,6 +255,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
int mask = INTR_MASK;
+ ehci_clear_wakeup_flags(ehci);
if (!hcd->self.root_hub->do_remote_wakeup)
mask &= ~STS_PCD;
ehci_writel(ehci, mask, &ehci->regs->intr_enable);
Index: usb-2.6/drivers/usb/host/ehci-fsl.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
+++ usb-2.6/drivers/usb/host/ehci-fsl.c
@@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
void __iomem *non_ehci = hcd->regs;
+ ehci_set_wakeup_flags(ehci);
if (!fsl_deep_sleep())
return 0;
@@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
void __iomem *non_ehci = hcd->regs;
+ ehci_clear_wakeup_flags(ehci);
if (!fsl_deep_sleep())
return 0;
On Wed, 28 Apr 2010, Ondrej Zary wrote:
> On Wednesday 28 April 2010 17:41:30 Alan Stern wrote:
> > On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > > On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:
> > > > On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > > > > The previous patch was not enough as it worked only when there were
> > > > > no USB devices connected. With a bus-powered device connected, power
> > > > > loss causes disconnect which wakes up the machine immediately again.
> > > >
> > > > You said earlier that the host controller was disabled for remote
> > > > wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled
> > > > by default"). So even though the root hub might issue a wakeup
> > > > request, the controller hardware should not forward that request to the
> > > > PCI bus and it should not cause the system to wake up.
> > >
> > > Maybe it should not - but it wakes up this board and probably also
> > > P4P800, P4P800-E and P4C800-E at least:
> > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497
> >
> > Okay, evidently the hardware or firmware on these boards is buggy.
> > Other systems do not have the same problem, as far as I know.
I take this back. The same thing happens on my machine (Intel ICH5
chipset). I'd guess there is a bug in the PCI or ACPI subsystem, but
more testing is needed before I can be sure. Somehow the PCI or
platform wakeup mechanism gets activated even when it is supposed to be
disabled.
> It works fine in Windows.
How can you tell? How do you know what values Windows writes into the
EHCI port control registers?
> > Regardless, I don't think a kernel patch is the way to solve your
> > problem. Changing the wakeup setting for the root hub will do what you
> > want, and that setting is explicitly intended to be controlled by
> > userspace (after all, that's why it is exposed in sysfs). The initial
> > value is only a reasonable default; you can certainly add scripts or
> > udev rules to disable wakeup on your EHCI root hub.
>
> Yes, I can work around that. But many users can't. This is an attempt to make
> it "just work".
It should "just work" already. The fact that it doesn't means there is
a bug. At the moment I can't be sure where the bug is -- but even if
it is in ehci-hcd, your suggested patch isn't the right fix.
> I'm trying to say that the "wakeup on everything" is not a good thing (if not
> a bug). Who needs it?
I don't know, and neither do you. But the USB spec requires this
behavior from external hubs, so evidently _somebody_ thought it was a
good idea.
> I can't imagine any real use for it. It clearly breaks
> suspend on some systems and is annoying on other.
If everything was working properly, the machine wouldn't wake up when a
disconnect occurred.
> Who expects that
> disconnecting a device should wake up sleeping machine?
Perhaps the same people who expect that plugging in a device should
wake up a sleeping machine?
> When all these three wakeups (overcurrent, connect, disconnect) are disabled,
> we lose nothing. Connect/disconnect detection works fine after wakeup. Wakeup
> by USB devices (not by connect/disconnect but by the device itself signaling
> a resume) is completely independent of this (according to EHCI
> specification).
What about UHCI or OHCI? What about external hubs?
In short, why should EHCI behave differently from everything else?
Alan Stern