[PATCH 1/2] xhci: Suspend ports to U3 directly from U1 or U2

[PATCH 1/2] xhci: Suspend ports to U3 directly from U1 or U2

[Kai-Heng Feng]

xHCI spec "4.15.1 Port Suspend" states that port can be put to U3 as long
as Enabled bit is set and from U0, U1 or U2 state.

Currently only USB_PORT_FEAT_LINK_STATE puts port to U3 directly, let's
do the same for USB_PORT_FEAT_SUSPEND and bus suspend case.

This is particularly useful for USB2 devices, which may take a very long
time to switch USB2 LPM on and off.

Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/usb/host/xhci-hub.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index f37316d2c8fa..f9375b77d17d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1196,15 +1196,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		/* FIXME: What new port features do we need to support? */
 		switch (wValue) {
 		case USB_PORT_FEAT_SUSPEND:
-			temp = readl(ports[wIndex]->addr);
-			if ((temp & PORT_PLS_MASK) != XDEV_U0) {
-				/* Resume the port to U0 first */
-				xhci_set_link_state(xhci, ports[wIndex],
-							XDEV_U0);
-				spin_unlock_irqrestore(&xhci->lock, flags);
-				msleep(10);
-				spin_lock_irqsave(&xhci->lock, flags);
-			}
 			/* In spec software should not attempt to suspend
 			 * a port unless the port reports that it is in the
 			 * enabled (PED = ‘1’,PLS < ‘3’) state.
@@ -1645,8 +1636,10 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
 			xhci_dbg(xhci, "Bus suspend bailout, port over-current detected\n");
 			return -EBUSY;
 		}
-		/* suspend ports in U0, or bail out for new connect changes */
-		if ((t1 & PORT_PE) && (t1 & PORT_PLS_MASK) == XDEV_U0) {
+		/* suspend ports in U0/U1/U2, or bail out for new connect changes */
+		if ((t1 & PORT_PE) && ((t1 & PORT_PLS_MASK) == XDEV_U0 ||
+				       (t1 & PORT_PLS_MASK) == XDEV_U1 ||
+				       (t1 & PORT_PLS_MASK) == XDEV_U2)) {
 			if ((t1 & PORT_CSC) && wake_enabled) {
 				bus_state->bus_suspended = 0;
 				spin_unlock_irqrestore(&xhci->lock, flags);
-- 
2.17.1

[PATCH 2/2] USB: hub: Suspend and resume port with LPM enabled

[Kai-Heng Feng]

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
..
[  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, because it requires a longer period for
disabling LPM. After a while the port finished its U2 -> U0 transition,
interrupts the suspend process.

Though PLC shouldn't be set for U2 -> U0 case, we can avoid all that by
directly put the port from U0/U1/U2 to U3, and solves this issue.

Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/usb/core/hub.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b1e14beaac5f..882b54df6ef5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3285,9 +3285,6 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		}
 	}
 
-	/* disable USB2 hardware LPM */
-	usb_disable_usb2_hardware_lpm(udev);
-
 	if (usb_disable_ltm(udev)) {
 		dev_err(&udev->dev, "Failed to disable LTM before suspend\n");
 		status = -ENOMEM;
@@ -3323,9 +3320,6 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		/* Try to enable USB3 LTM again */
 		usb_enable_ltm(udev);
  err_ltm:
-		/* Try to enable USB2 hardware LPM again */
-		usb_enable_usb2_hardware_lpm(udev);
-
 		if (udev->do_remote_wakeup)
 			(void) usb_disable_remote_wakeup(udev);
  err_wakeup:
@@ -3606,9 +3600,6 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 		dev_dbg(&udev->dev, "can't resume, status %d\n", status);
 		hub_port_logical_disconnect(hub, port1);
 	} else  {
-		/* Try to enable USB2 hardware LPM */
-		usb_enable_usb2_hardware_lpm(udev);
-
 		/* Try to enable USB3 LTM */
 		usb_enable_ltm(udev);
 	}
-- 
2.17.1

Re: [PATCH 2/2] USB: hub: Suspend and resume port with LPM enabled

[Sergei Shtylyov]

Hello!

On 10.06.2020 9:42, 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
> ..
> [  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

    Suspend. Perhaps can be fixed while applying...

> 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, because it requires a longer period for
> disabling LPM. After a while the port finished its U2 -> U0 transition,
> interrupts the suspend process.
> 
> Though PLC shouldn't be set for U2 -> U0 case, we can avoid all that by
> directly put the port from U0/U1/U2 to U3, and solves this issue.
            ^ putting                            ^ it?

> Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
[...]

MBR, Sergei

Re: [PATCH 1/2] xhci: Suspend ports to U3 directly from U1 or U2

[Alan Stern]

On Wed, Jun 10, 2020 at 02:42:30PM +0800, Kai-Heng Feng wrote:
> xHCI spec "4.15.1 Port Suspend" states that port can be put to U3 as long
> as Enabled bit is set and from U0, U1 or U2 state.
> 
> Currently only USB_PORT_FEAT_LINK_STATE puts port to U3 directly, let's
> do the same for USB_PORT_FEAT_SUSPEND and bus suspend case.
> 
> This is particularly useful for USB2 devices, which may take a very long
> time to switch USB2 LPM on and off.

Have these two patches been tested with a variety of USB-2.0 and USB-2.1 
devices?

Alan Stern

Re: [PATCH 1/2] xhci: Suspend ports to U3 directly from U1 or U2

[Kai-Heng Feng]

> On Jun 10, 2020, at 22:32, Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> On Wed, Jun 10, 2020 at 02:42:30PM +0800, Kai-Heng Feng wrote:
>> xHCI spec "4.15.1 Port Suspend" states that port can be put to U3 as long
>> as Enabled bit is set and from U0, U1 or U2 state.
>> 
>> Currently only USB_PORT_FEAT_LINK_STATE puts port to U3 directly, let's
>> do the same for USB_PORT_FEAT_SUSPEND and bus suspend case.
>> 
>> This is particularly useful for USB2 devices, which may take a very long
>> time to switch USB2 LPM on and off.
> 
> Have these two patches been tested with a variety of USB-2.0 and USB-2.1 
> devices?

I tested some laptops around and they work fine.
Only internally connected USB devices like USB Bluetooth and USB Camera have USB2 LPM enabled, so this patch won't affect external connected devices.

Kai-Heng

> 
> Alan Stern

Re: [PATCH 1/2] xhci: Suspend ports to U3 directly from U1 or U2

[Mathias Nyman]

On 10.6.2020 18.43, Kai-Heng Feng wrote:
> 
> 
>> On Jun 10, 2020, at 22:32, Alan Stern <stern@rowland.harvard.edu> wrote:
>>
>> On Wed, Jun 10, 2020 at 02:42:30PM +0800, Kai-Heng Feng wrote:
>>> xHCI spec "4.15.1 Port Suspend" states that port can be put to U3 as long
>>> as Enabled bit is set and from U0, U1 or U2 state.
>>>
>>> Currently only USB_PORT_FEAT_LINK_STATE puts port to U3 directly, let's
>>> do the same for USB_PORT_FEAT_SUSPEND and bus suspend case.
>>>
>>> This is particularly useful for USB2 devices, which may take a very long
>>> time to switch USB2 LPM on and off.
>>
>> Have these two patches been tested with a variety of USB-2.0 and USB-2.1 
>> devices?
> 
> I tested some laptops around and they work fine.
> Only internally connected USB devices like USB Bluetooth and USB Camera have USB2 LPM enabled, so this patch won't affect external connected devices.
> 

Took a fresh look at the USB2 side and it's not as clear as the USB3 case, where
we know the hub must support transition to U3 from any other state. [1]

Supporting link state transition to U3 (USB2 L2) from any other U state for USB2 seems
to be xHCI specific feature. xHC hardware will make sure it goes via the U0 state.

I have no clue about other hosts (or hubs), USB2 LPM ECN just shows that link
state transitions to L1 or L2 should always goes via L0.
It's possible this has to be done in software by disabling USB2 LPM before suspending the device.

So I guess the original suggestion to wait for link state to reach U0 before
is a better solution. Sorry about my hasty suggestion.

Kai-Heng, does it help if you fist manually set the link to U0 before disabling
USB2 LPM (set PLS to 0 before clearing the HLE bit). Does it transition to U0
any faster, or get rid of the extra port event with PLC:U0?

-Mathias

 
[1]
    USB3.1 spec (10.16.2.10) Set Port feature:   
   "If the value is 3, then host software wants to selectively suspend the
    device connected to this port. The hub shall transition the link to U3
    from any of the other U states using allowed link state transitions.
    If the port is not already in the U0 state, then it shall transition the
    port to the U0 state and then initiate the transition to U3.

Re: [PATCH 1/2] xhci: Suspend ports to U3 directly from U1 or U2

[Kai-Heng Feng]

> On Jun 10, 2020, at 23:58, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
> 
> On 10.6.2020 18.43, Kai-Heng Feng wrote:
>> 
>> 
>>> On Jun 10, 2020, at 22:32, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> 
>>> On Wed, Jun 10, 2020 at 02:42:30PM +0800, Kai-Heng Feng wrote:
>>>> xHCI spec "4.15.1 Port Suspend" states that port can be put to U3 as long
>>>> as Enabled bit is set and from U0, U1 or U2 state.
>>>> 
>>>> Currently only USB_PORT_FEAT_LINK_STATE puts port to U3 directly, let's
>>>> do the same for USB_PORT_FEAT_SUSPEND and bus suspend case.
>>>> 
>>>> This is particularly useful for USB2 devices, which may take a very long
>>>> time to switch USB2 LPM on and off.
>>> 
>>> Have these two patches been tested with a variety of USB-2.0 and USB-2.1 
>>> devices?
>> 
>> I tested some laptops around and they work fine.
>> Only internally connected USB devices like USB Bluetooth and USB Camera have USB2 LPM enabled, so this patch won't affect external connected devices.
>> 
> 
> Took a fresh look at the USB2 side and it's not as clear as the USB3 case, where
> we know the hub must support transition to U3 from any other state. [1]
> 
> Supporting link state transition to U3 (USB2 L2) from any other U state for USB2 seems
> to be xHCI specific feature. xHC hardware will make sure it goes via the U0 state.
> 
> I have no clue about other hosts (or hubs), USB2 LPM ECN just shows that link
> state transitions to L1 or L2 should always goes via L0.
> It's possible this has to be done in software by disabling USB2 LPM before suspending the device.

Is there any host or hub other than xHCI support USB2 LPM? Seem like only xHCI implements it.

> 
> So I guess the original suggestion to wait for link state to reach U0 before
> is a better solution. Sorry about my hasty suggestion.
> 
> Kai-Heng, does it help if you fist manually set the link to U0 before disabling
> USB2 LPM (set PLS to 0 before clearing the HLE bit). Does it transition to U0
> any faster, or get rid of the extra port event with PLC:U0?

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 03b64b73eb99..0b8db13e79e4 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4385,7 +4385,7 @@ static int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
        struct xhci_hcd *xhci = hcd_to_xhci(hcd);
        struct xhci_port **ports;
        __le32 __iomem  *pm_addr, *hlpm_addr;
-       u32             pm_val, hlpm_val, field;
+       u32             portsc, pm_val, hlpm_val, field;
        unsigned int    port_num;
        unsigned long   flags;
        int             hird, exit_latency;
@@ -4463,6 +4463,15 @@ static int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
                /* flush write */
                readl(pm_addr);
        } else {
+               portsc = readl(ports[port_num]->addr);
+               if ((portsc & PORT_PE) && ((portsc & PORT_PLS_MASK) == XDEV_U1 ||
+                                       (portsc & PORT_PLS_MASK) == XDEV_U2)) {
+                       xhci_set_link_state(xhci, ports[port_num], XDEV_U0);
+                       spin_unlock_irqrestore(&xhci->lock, flags);
+                       dev_info(&udev->dev, "DEBUG: SLEEP\n");
+                       msleep(100);
+                       spin_lock_irqsave(&xhci->lock, flags);
+               }
                pm_val &= ~(PORT_HLE | PORT_RWE | PORT_HIRD_MASK | PORT_L1DS_MASK);
                writel(pm_val, pm_addr);
                /* flush write */

If there's a long enough delay for U0 before clearing HLE, then the PLC can be avoided.
It's not any faster though.

Kai-Heng

> 
> -Mathias
> 
> 
> [1]
>    USB3.1 spec (10.16.2.10) Set Port feature:   
>   "If the value is 3, then host software wants to selectively suspend the
>    device connected to this port. The hub shall transition the link to U3
>    from any of the other U states using allowed link state transitions.
>    If the port is not already in the U0 state, then it shall transition the
>    port to the U0 state and then initiate the transition to U3.