Re: [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 20.5.2020 13.18, Kai-Heng Feng wrote:
> USB2 devices with LPM enabled may interrupt the system suspend:
> [  932.510475] usb 1-7: usb suspend, wakeup 0
> [  932.510549] hub 1-0:1.0: hub_suspend
> [  932.510581] usb usb1: bus suspend, wakeup 0
> [  932.510590] xhci_hcd 0000:00:14.0: port 9 not suspended
> [  932.510593] xhci_hcd 0000:00:14.0: port 8 not suspended
> ..
> [  932.520323] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03

400e03 = Connected, Enabled, U0 with port ink state change flag (PLC) set.

> ..
> [  932.591405] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
> [  932.591414] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
> [  932.591418] PM: Device 0000:00:14.0 failed to suspend async: error -16
> 
> During system suspend, USB core will let HC suspends the device if it
> doesn't have remote wakeup enabled and doesn't have any children.
> However, from the log above we can see that the usb 1-7 doesn't get bus
> suspended due to not in U0. After a while the port finished U2 -> U0
> transition, interrupts the suspend process.

In USB2 HW link PM the PLC flag should not be set in U2Exit -> U0 transitions.
Only case we should see a port change event is U2Entry -> U0 due to STALL or
error/timeout. (see xhci 4.23.5.1.1.1)

> 
> The observation is that after disabling LPM, port doesn't transit to U0
> immediately and can linger in U2. xHCI spec 4.23.5.2 states that the
> maximum exit latency for USB2 LPM should be BESL + 10us. The BESL for
> the affected device is advertised as 400us, which is still not enough
> based on my testing result.
> 
> So let's use the maximum permitted latency, 10000, to poll for U0
> status to solve the issue.

I don't recall all details, but it could be that disabling LPM before suspend
is unnecessary. 
At least xhci should be able to set a port to U3 from U1 and U2 (see xhci 4.15.1)
so that is one change that could be done to xhci_bus_suspend()

Also just noticed that we are not really checking L1S field in PORTPMSC register, 
this should tell if there was an issue with USB2 lpm state transitions, and
perhaps we should disable lpm for that device. 

Does the L1S field show anything unuaual in your case?
That could explain the port event with the PLC bit set.

I think we can avoid a readl_poll_timeout() solution in this case.

-Mathias