[+cc Rafael, Lukas]
On Wed, Oct 05, 2016 at 10:45:22AM -0400, Alan Stern wrote:
> [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-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
