[PATCH] usb: host: xhci: wait USB2 port enter suspend for bus suspend

[PATCH] usb: host: xhci: wait USB2 port enter suspend for bus suspend

[Li Jun]

If the connected USB2 device wakeup is not enabled/supported, the link
state may still be U0 when do xhci bus suspend, after we suspend ports
in U0, we need give time to device to enter suspend before do further
suspend operations (e.g. system suspend), otherwise we may enter system
suspend with link state at U0.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/host/xhci-hub.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index c799ca5..1e054d0 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1598,6 +1598,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
 	struct xhci_hub *rhub;
 	struct xhci_port **ports;
 	u32 portsc_buf[USB_MAXCHILDREN];
+	bool wait_port_enter_u3 = false;
 	bool wake_enabled;
 
 	rhub = xhci_get_rhub(hcd);
@@ -1706,12 +1707,17 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
 				xhci_stop_device(xhci, slot_id, 1);
 				spin_lock_irqsave(&xhci->lock, flags);
 			}
+			wait_port_enter_u3 = true;
 		}
 		writel(portsc_buf[port_index], ports[port_index]->addr);
 	}
 	hcd->state = HC_STATE_SUSPENDED;
 	bus_state->next_statechange = jiffies + msecs_to_jiffies(10);
 	spin_unlock_irqrestore(&xhci->lock, flags);
+
+	if (wait_port_enter_u3)
+		usleep_range(5000, 10000);
+
 	return 0;
 }
 
-- 
2.7.4

RE: [PATCH] usb: host: xhci: wait USB2 port enter suspend for bus suspend

[Jun Li]

Hi,

> -----Original Message-----
> From: Jun Li <jun.li@nxp.com>
> Sent: Tuesday, November 3, 2020 7:23 PM
> To: mathias.nyman@intel.com
> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Peter Chen
> <peter.chen@nxp.com>
> Subject: [PATCH] usb: host: xhci: wait USB2 port enter suspend for bus suspend
> 
> If the connected USB2 device wakeup is not enabled/supported, the link state
> may still be U0 when do xhci bus suspend, after we suspend ports in U0, we
> need give time to device to enter suspend before do further suspend operations
> (e.g. system suspend), otherwise we may enter system suspend with link state
> at U0.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  drivers/usb/host/xhci-hub.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index c799ca5..1e054d0 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1598,6 +1598,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>  	struct xhci_hub *rhub;
>  	struct xhci_port **ports;
>  	u32 portsc_buf[USB_MAXCHILDREN];
> +	bool wait_port_enter_u3 = false;
>  	bool wake_enabled;
> 
>  	rhub = xhci_get_rhub(hcd);
> @@ -1706,12 +1707,17 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>  				xhci_stop_device(xhci, slot_id, 1);
>  				spin_lock_irqsave(&xhci->lock, flags);
>  			}
> +			wait_port_enter_u3 = true;
>  		}
>  		writel(portsc_buf[port_index], ports[port_index]->addr);
>  	}
>  	hcd->state = HC_STATE_SUSPENDED;
>  	bus_state->next_statechange = jiffies + msecs_to_jiffies(10);
>  	spin_unlock_irqrestore(&xhci->lock, flags);
> +
> +	if (wait_port_enter_u3)
> +		usleep_range(5000, 10000);
> +
>  	return 0;
>  }
> 
> --
> 2.7.4

A gentle ping.

Thanks
Li Jun

Re: [PATCH] usb: host: xhci: wait USB2 port enter suspend for bus suspend

[Mathias Nyman]

Hi

On 1.12.2020 8.12, Jun Li wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Jun Li <jun.li@nxp.com>
>> Sent: Tuesday, November 3, 2020 7:23 PM
>> To: mathias.nyman@intel.com
>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Peter Chen
>> <peter.chen@nxp.com>
>> Subject: [PATCH] usb: host: xhci: wait USB2 port enter suspend for bus suspend
>>
>> If the connected USB2 device wakeup is not enabled/supported, the link state
>> may still be U0 when do xhci bus suspend, after we suspend ports in U0, we
>> need give time to device to enter suspend before do further suspend operations
>> (e.g. system suspend), otherwise we may enter system suspend with link state
>> at U0.


What side-effects have you observed if bus suspend returns while a port is still
transitioning to U3?

I can't recall why we end up with ports in U0 in bus suspend anymore.
I think that in system suspend the link should be put to U3 already when the usb device is
suspended, before the bus suspends, even if it doesn't support remote wakeup.

Do you know the reason why the device is in U0 in bus suspend in your case?
Could that be the real problem that needs to be fixed? 

>>
>> Signed-off-by: Li Jun <jun.li@nxp.com>
>> ---
>>  drivers/usb/host/xhci-hub.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index c799ca5..1e054d0 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -1598,6 +1598,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>>  	struct xhci_hub *rhub;
>>  	struct xhci_port **ports;
>>  	u32 portsc_buf[USB_MAXCHILDREN];
>> +	bool wait_port_enter_u3 = false;
>>  	bool wake_enabled;
>>
>>  	rhub = xhci_get_rhub(hcd);
>> @@ -1706,12 +1707,17 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>>  				xhci_stop_device(xhci, slot_id, 1);
>>  				spin_lock_irqsave(&xhci->lock, flags);
>>  			}
>> +			wait_port_enter_u3 = true;

I don't think "wait_port_enter_u3" is needed. If xhci_bus_suspend() needs 
to set a port link to U3 it will also set a bit in bus_state->bus_suspended

>>  		}
>>  		writel(portsc_buf[port_index], ports[port_index]->addr);
>>  	}
>>  	hcd->state = HC_STATE_SUSPENDED;
>>  	bus_state->next_statechange = jiffies + msecs_to_jiffies(10);
>>  	spin_unlock_irqrestore(&xhci->lock, flags);
>> +
>> +	if (wait_port_enter_u3)
>> +		usleep_range(5000, 10000);

First thought we should poll the register(s) and wait for ports to enter U3,
but the more common case where a device is suspended and link put to U3 with a 
USB2 SetPortFeature(PORT_SUSPEND) also just sleeps for 10ms, so doing it
for this special case should be ok as well. 

if (bus_state->bus_suspended)
	usleep_range(5000, 10000)

-Mathias

RE: [PATCH] usb: host: xhci: wait USB2 port enter suspend for bus suspend

[Jun Li]

> -----Original Message-----
> From: Mathias Nyman <mathias.nyman@linux.intel.com>
> Sent: Wednesday, December 2, 2020 7:55 AM
> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com
> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Peter Chen
> <peter.chen@nxp.com>
> Subject: Re: [PATCH] usb: host: xhci: wait USB2 port enter suspend for bus
> suspend
> 
> Hi
> 
> On 1.12.2020 8.12, Jun Li wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Jun Li <jun.li@nxp.com>
> >> Sent: Tuesday, November 3, 2020 7:23 PM
> >> To: mathias.nyman@intel.com
> >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Peter Chen
> >> <peter.chen@nxp.com>
> >> Subject: [PATCH] usb: host: xhci: wait USB2 port enter suspend for
> >> bus suspend
> >>
> >> If the connected USB2 device wakeup is not enabled/supported, the
> >> link state may still be U0 when do xhci bus suspend, after we suspend
> >> ports in U0, we need give time to device to enter suspend before do
> >> further suspend operations (e.g. system suspend), otherwise we may
> >> enter system suspend with link state at U0.
> 
> 
> What side-effects have you observed if bus suspend returns while a port is
> still transitioning to U3?

I found a real problem on remote wakeup by USB2 device disconnect
on root port, that device(e.g. Udisk) itself does not support remote
wakeup, the remote wakeup has problem if I enable USB2 DPDM wakeup
when USB2 bus at U0.

> 
> I can't recall why we end up with ports in U0 in bus suspend anymore.
> I think that in system suspend the link should be put to U3 already when
> the usb device is suspended, before the bus suspends, even if it doesn't
> support remote wakeup.

I also thought so but actually not, see below in usb_port_suspend():

  12         if (hub_is_superspeed(hub->hdev))
  13                 status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3); 
  14 
  15         /*
  16          * For system suspend, we do not need to enable the suspend feature
  17          * on individual USB-2 ports.  The devices will automatically go
  18          * into suspend a few ms after the root hub stops sending packets.
  19          * The USB 2.0 spec calls this "global suspend".
  20          *
  21          * However, many USB hubs have a bug: They don't relay wakeup requests
  22          * from a downstream port if the port's suspend feature isn't on.
  23          * Therefore we will turn on the suspend feature if udev or any of its
  24          * descendants is enabled for remote wakeup.
  25          */
  26         else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
  27                 status = set_port_feature(hub->hdev, port1,
  28                                 USB_PORT_FEAT_SUSPEND);
  29         else {
  30                 really_suspend = false;
  31                 status = 0;
  32         }

usb_wakeup_enabled_descendants(udev) > 0 is not true, if the device itself
does not support remote wakeup.

> 
> Do you know the reason why the device is in U0 in bus suspend in your case?
> Could that be the real problem that needs to be fixed?

See above.

> 
> >>
> >> Signed-off-by: Li Jun <jun.li@nxp.com>
> >> ---
> >>  drivers/usb/host/xhci-hub.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/usb/host/xhci-hub.c
> >> b/drivers/usb/host/xhci-hub.c index c799ca5..1e054d0 100644
> >> --- a/drivers/usb/host/xhci-hub.c
> >> +++ b/drivers/usb/host/xhci-hub.c
> >> @@ -1598,6 +1598,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
> >>  	struct xhci_hub *rhub;
> >>  	struct xhci_port **ports;
> >>  	u32 portsc_buf[USB_MAXCHILDREN];
> >> +	bool wait_port_enter_u3 = false;
> >>  	bool wake_enabled;
> >>
> >>  	rhub = xhci_get_rhub(hcd);
> >> @@ -1706,12 +1707,17 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
> >>  				xhci_stop_device(xhci, slot_id, 1);
> >>  				spin_lock_irqsave(&xhci->lock, flags);
> >>  			}
> >> +			wait_port_enter_u3 = true;
> 
> I don't think "wait_port_enter_u3" is needed. If xhci_bus_suspend() needs
> to set a port link to U3 it will also set a bit in bus_state->bus_suspended
> 

Agree after check.

> >>  		}
> >>  		writel(portsc_buf[port_index], ports[port_index]->addr);
> >>  	}
> >>  	hcd->state = HC_STATE_SUSPENDED;
> >>  	bus_state->next_statechange = jiffies + msecs_to_jiffies(10);
> >>  	spin_unlock_irqrestore(&xhci->lock, flags);
> >> +
> >> +	if (wait_port_enter_u3)
> >> +		usleep_range(5000, 10000);
> 
> First thought we should poll the register(s) and wait for ports to enter
> U3, but the more common case where a device is suspended and link put to
> U3 with a
> USB2 SetPortFeature(PORT_SUSPEND) also just sleeps for 10ms, so doing it
> for this special case should be ok as well.
> 
> if (bus_state->bus_suspended)
> 	usleep_range(5000, 10000)

I will send your proposal if no more comments.

Thanks
Li Jun
> 
> -Mathias

Re: [PATCH] usb: host: xhci: wait USB2 port enter suspend for bus suspend

[Mathias Nyman]

On 2.12.2020 8.58, Jun Li wrote:
> 
> 
>> -----Original Message-----
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Sent: Wednesday, December 2, 2020 7:55 AM
>> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com
>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Peter Chen
>> <peter.chen@nxp.com>
>> Subject: Re: [PATCH] usb: host: xhci: wait USB2 port enter suspend for bus
>> suspend
>>
>> Hi
>>
>> On 1.12.2020 8.12, Jun Li wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Jun Li <jun.li@nxp.com>
>>>> Sent: Tuesday, November 3, 2020 7:23 PM
>>>> To: mathias.nyman@intel.com
>>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Peter Chen
>>>> <peter.chen@nxp.com>
>>>> Subject: [PATCH] usb: host: xhci: wait USB2 port enter suspend for
>>>> bus suspend
>>>>
>>>> If the connected USB2 device wakeup is not enabled/supported, the
>>>> link state may still be U0 when do xhci bus suspend, after we suspend
>>>> ports in U0, we need give time to device to enter suspend before do
>>>> further suspend operations (e.g. system suspend), otherwise we may
>>>> enter system suspend with link state at U0.
>>
>>
>> What side-effects have you observed if bus suspend returns while a port is
>> still transitioning to U3?
> 
> I found a real problem on remote wakeup by USB2 device disconnect
> on root port, that device(e.g. Udisk) itself does not support remote
> wakeup, the remote wakeup has problem if I enable USB2 DPDM wakeup
> when USB2 bus at U0.
> 
>>
>> I can't recall why we end up with ports in U0 in bus suspend anymore.
>> I think that in system suspend the link should be put to U3 already when
>> the usb device is suspended, before the bus suspends, even if it doesn't
>> support remote wakeup.
> 
> I also thought so but actually not, see below in usb_port_suspend():
> 
>   12         if (hub_is_superspeed(hub->hdev))
>   13                 status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3); 
>   14 
>   15         /*
>   16          * For system suspend, we do not need to enable the suspend feature
>   17          * on individual USB-2 ports.  The devices will automatically go
>   18          * into suspend a few ms after the root hub stops sending packets.
>   19          * The USB 2.0 spec calls this "global suspend".
>   20          *
>   21          * However, many USB hubs have a bug: They don't relay wakeup requests
>   22          * from a downstream port if the port's suspend feature isn't on.
>   23          * Therefore we will turn on the suspend feature if udev or any of its
>   24          * descendants is enabled for remote wakeup.
>   25          */
>   26         else if (PMSG_IS_AUTO(msg) || usb_wakeup_enabled_descendants(udev) > 0)
>   27                 status = set_port_feature(hub->hdev, port1,
>   28                                 USB_PORT_FEAT_SUSPEND);
>   29         else {
>   30                 really_suspend = false;
>   31                 status = 0;
>   32         }
> 
> usb_wakeup_enabled_descendants(udev) > 0 is not true, if the device itself
> does not support remote wakeup.
> 

You're right, link isn't set to U3 in this case. 

...
>>
>> if (bus_state->bus_suspended)
>> 	usleep_range(5000, 10000)
> 
> I will send your proposal if no more comments.

Yes, thanks, no more comments.

-Mathias