> -----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.
>
> 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@xxxxxxx>
> >> ---
> >> 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
