On 2.12.2020 8.58, Jun Li wrote:
>
>
>> -----Original Message-----
>> From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>> Sent: Wednesday, December 2, 2020 7:55 AM
>> To: Jun Li <jun.li@xxxxxxx>; mathias.nyman@xxxxxxxxx
>> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; Peter Chen
>> <peter.chen@xxxxxxx>
>> 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@xxxxxxx>
>>>> Sent: Tuesday, November 3, 2020 7:23 PM
>>>> To: mathias.nyman@xxxxxxxxx
>>>> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; Peter Chen
>>>> <peter.chen@xxxxxxx>
>>>> 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
