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

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

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