On Thu, 7 Nov 2013, Sarah Sharp wrote: > > Probably the current code is wrong. The wakeup bits don't need to be > > set during port suspend or root-hub suspend, but only during controller > > suspend -- they get used only when the controller is not in D0. I > > guess we should change both drivers. > > Ok, let me see if I understand this issue correctly. You're pretty close. Some of the more obscure details need some clarification. > The wakeup file in /sys/bus/pci/devices/../power/wakeup for the xHCI PCI > host controller is supposed to control whether PMEs are enabled when the > device is placed into D3. The power/wakeup file setting is (currently) relevant only for system sleep, i.e, S3/S4. It is not used when a controller gets put into D3 (i.e., runtime suspend) if the system as a whole is going to remain in S0. The setting is supposed to control whether the controller can wake the system up from S3/S4. This does indeed mean that it controls whether PME is enabled, but it's more than that. Some platforms include other mechanisms in addition to PME whereby a PCI device can wake up the system; those other mechanisms are also supposed to be controlled by power/wakeup. > The value in that file will correspond to the > do_wakeup variable passed to xhci_pci_suspend. During a system sleep transition it will. However, if the system remains awake while the host controller goes into runtime suspend, the do_wakeup argument to xhci_pci_suspend will be set regardless of the power/wakeup setting. (Note: That's how things work now. Rafael plans to change this in the near future. Under the new arrangement, power/wakeup will be able to store three possible values: "enabled", "disabled", and "off". When the value is "enabled" or "disabled", things will continue to work as described above. When the value is "off", do_wakeup will be clear for both system sleep and runtime suspend.) > Each of the two xHCI roothubs have separate files in > /sys/bus/usb/devices../usbN/power/wakeup. That controls whether the > roothub is allowed to set the "wake on" flags for the roothub ports. Not exactly; they control whether or not connect/disconnect/overcurrent events on the respective root hubs should cause the controller to ask the OS to resume the root hub. This means that while a root hub is in runtime suspend and the controller is in D0, the power/wakeup setting controls whether or not Port-Status-Change interrupts are enabled for that root hub. While the controller is in D3, on the other hand, the settings do indeed control the various wake_on flags for the ports. > Those file values are reflected in hcd->self.root_hub->do_remote_wakeup, > which xhci_bus_suspend checks before enabling the wake on flags for each > roothub port. It will set those flags when the bus is suspended, even > if PMEs are disabled for the xHCI PCI device. That's right. Note that according to the explanation above, the wake_on flags don't need to be set until xhci_suspend is called, although setting them in xhci_bus_suspend doesn't really hurt. > The problem is that buggy Intel EHCI and xHCI silicon will assert the > PME status in S3 if the remote wake up flags are set, even if PME enable > is not set. Therefore, we need to fix this by not setting the remote > wakeup flags for the ports unless the PCI device has PMEs enabled. I'm not sure that "buggy" is the right term. The motherboards are probably meant to work that way -- whether it should be considered a bug is open to question. Also, you should be referring to PME# rather than PME Status. They are two separate things. (In fact, the spec requires the controller to turn on PME Status when one of these port-status-change events happens, even if the wake_on flag is off.) PME# is the signal which actually wakes up the system or causes an interrupt. Finally, the problem is that these controllers wake up the system from S3. In other words, that is the observed behavior; I can't tell whether the wakeup occurs because PME# is asserted or if some other mechanism is in use. (I suspect the latter, but there doesn't seem to be any reliable way to tell for certain.) > In order to fix the xHCI driver, we would remove setting the wake on > bits from xhci_bus_suspend. Then, in xhci_pci_suspend, if do_wakeup was > set, we would check hcd->self.root_hub->do_remote_wakeup for each of the > roothubs, and enable the wake on flags if that was set. Correct? Right. Or maybe do this in xhci_suspend, because wakeup settings are platform-independent. And then clear the wake_on flags in xhci__resume. > One more question: what happens if the PCI wakeup file is set to > disabled while the host is in D3? Will the PCI device be resumed and > then suspended with do_wakeup set to false? No, changes to the sysfs attribute don't send any sort of notification to the driver. They don't take effect until the next time the controller is suspended. Currently this doesn't make any difference, because the power/wakeup setting matters only for system suspend, not runtime suspend. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
