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

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

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