[Adding Bjorn and linux-pci to the CC: list]
In short, Pierre's USB host controller doesn't send wakeup signals from
runtime suspend, because the firmware limits the runtime-suspend state
to D0 and the controller can't issue PME# from the D0 state. In this
situation we would prefer to avoid suspending the controller at all,
rather than have it go into runtime suspend and then stop working.
On Wed, 5 Oct 2016, Mathias Nyman wrote:
> On 04.10.2016 17:11, Alan Stern wrote:
> > On Tue, 4 Oct 2016, Mathias Nyman wrote:
> >
> >> On 03.10.2016 23:54, Pierre de Villemereuil wrote:
> >>> Hi Mathias,
> >>>
> >>> Just to know: does that mean the firmware from Asus is faulty in here? Do you
> >>> think I should contact Asus about this?
> >>>
> >>
> >> Probably, _S0W, _DSM and XFLT code for xHCI are useless in that ACPI DSDT firmware version.
> >>
> >> But we still want the driver to handle cases like this.
> >> Just wrote the patch.
> >> Adding Alan Stern to CC for PM sanity feedback on it:
> >>
> >> ---
> >>
> >> Author: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> >> Date: Tue Oct 4 13:07:59 2016 +0300
> >>
> >> xhci: Prevent a non-responsive xhci suspend state.
> >>
> >> ACPI may limit the lowest possible runtime suspend PCI D-state to D0.
> >> If xHC is not capable of generating PME# events in D0 we end
> >> up with a non-responsive runtime suspended host without PME# capability
> >> and with interrupts disabled, unable to detect or wake up when a
> >> new usb device is connected.
> >>
> >> This was triggered on a ASUS TP301UA machine.
> >>
> >> Reported-by: Pierre de Villemereuil <flyos@xxxxxxxxxx>
> >> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> >>
> >> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> >> index d7b0f97..08b021c 100644
> >> --- a/drivers/usb/host/xhci-pci.c
> >> +++ b/drivers/usb/host/xhci-pci.c
> >> @@ -396,6 +396,11 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> >> if (xhci->quirks & XHCI_SSIC_PORT_UNUSED)
> >> xhci_ssic_port_unused_quirk(hcd, true);
> >>
> >> + /* Prevent a non-responsive PCI D0 state without PME# wake capability */
> >> + if (pci_choose_state(pdev, PMSG_AUTO_SUSPEND) == PCI_D0)
> >> + if (!pci_pme_capable(pdev, PCI_D0))
> >> + return -EBUSY;
> >> +
> >> ret = xhci_suspend(xhci, do_wakeup);
> >> if (ret && (xhci->quirks & XHCI_SSIC_PORT_UNUSED))
> >> xhci_ssic_port_unused_quirk(hcd, false);
> >
> > I've got three comments about this proposal.
> >
> > First, this logic should not apply during system suspend, only during
> > runtime suspend. You don't want to abort putting a laptop to sleep
> > just because the xHCI controller can't generate wakeup signals.
>
> Yes, I assume it would be ok change usb core to pass down something like
> pm_message_t to suspend_common() so that we could do this.
> hcd_pci_runetime_suspend() -> suspend_common() -> hcd->driver->pci_suspend()
>
> Assuming this workaround should stay in xhci-pci.c
If necessary, it could be moved into hcd_pci_runtime_suspend(). But I
would prefer to put it in the PCI core.
> > Second, this really has nothing to do with the D0 state. The same
> > logic should apply to whatever state is chosen for runtime suspend: If
> > the controller can't generate PME# wakeup signals in that state then
> > the suspend should be aborted.
>
> PCI code actually does this for us, it will walk down the D-states until
> it finds one that support PME#, but stop at D0 end return D0 even if D0
> does not support PME#.
That sounds like a bug. Or rather, accepting D0 as the target state
when it doesn't support PME# is the bug.
> Unfortunately that is done in a static function in pci/pci.c.
>
> static pci_power_t pci_target_state(struct pci_dev *dev)
> {
> pci_power_t target_state = PCI_D3hot;
>
> if (platform_pci_power_manageable(dev)) {
> /*
> * Call the platform to choose the target state of the device
> * and enable wake-up from this state if supported.
> */
> pci_power_t state = platform_pci_choose_state(dev);
>
> switch (state) {
> case PCI_POWER_ERROR:
> case PCI_UNKNOWN:
> break;
> case PCI_D1:
> case PCI_D2:
> if (pci_no_d1d2(dev))
> break;
> default:
> target_state = state;
> }
> } else if (!dev->pm_cap) {
> target_state = PCI_D0;
> } else if (device_may_wakeup(&dev->dev)) {
> /*
> * Find the deepest state from which the device can generate
> * wake-up events, make it the target state and enable device
> * to generate PME#.
> */
> if (dev->pme_support) {
> while (target_state
> && !(dev->pme_support & (1 << target_state)))
> target_state--;
> }
> }
>
> return target_state;
> }
>
> The best I can do from xhci is call pci_choose_state() which will call
> platform_pci_choose_state(), but won't do the PME# checking and
> D-state decrement to D0 .
>
> If pci_choose_state() returns D0 from a runtime suspend callback
> (and yes, should make sure its runtime suspend, not system suspend)
> I can pretty much assume pci_target_state will do the same.
>
> >
> > Third, the same reasoning applies to every PCI device that relies on
> > PME#, not just to xHCI controllers. Therefore the new code should be
> > added to drivers/pci/pci-driver.c:pci_pm_runtime_suspend(), not to
> > xhci-pci.c.
>
> Yes, that would be the preferred solution.
> I was not sure we can do that. pci-driver.c pci_pm_runtime_suspend() will call
> pci_finish_runtime_suspend()
> pci_target_state() // returns D0,
> pci_set_power_state() // to D0, which is the state we probably are already in.
>
> So it won't really do much. The device PCI device is disabled in usb hcd suspend
> hcd-pci.c:
> suspend_common()
> ...
> pci_disable_device(pci_dev)
>
> Should we anyway disable runtime PM in PCI if the PCI device has wakeup enabled
> and the target state is D0 without PME# support?
First, the device_may_wakeup() test applies only to suspend suspend,
not to runtime suspend. The current PM design assumes that wakeup will
always be enabled during runtime suspend (if the device supports it).
Second, there is a potential problem because some PCI devices have
other platform-based mechanisms for sending wakeup signals, instead of
using PME#. Such devices probably don't support standard PCI wakeup at
all. (A good example is the UHCI controllers on Intel's old chipsets.)
We don't want to prevent them from going into runtime suspend.
Now, maybe this isn't an issue -- I don't know enough about the details
of how the PCI core handles runtime suspend and how it coordinates with
the platform code.
Hopefully Bjorn can fill in the missing details.
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
