Message-ID: <20240813233043.uhsxocjr2pn4ujle@synopsys.com>
Date: Tue, 13 Aug 2024 23:30:51 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Prashanth K <quic_prashk@...cinc.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: Fix latency of DSTS while receiving wakeup
 event

On Tue, Aug 13, 2024, Prashanth K wrote:
> 
> 
> On 08-08-24 05:36 am, Thinh Nguyen wrote:
> 
> 
> > > And turns out, retries 1500 to 15000 (worst case), which can range from 3ms
> > > to 30ms. By this time, control can reach startXfer, where it tries to
> > > perform remote-wakeup even if host just resumed the gadget.
> > 
> > Polling for 20K time is a bit much, and this will vary depending on
> > different setup. This is something that I want to fix in the wakeup()
> > ops and keep everything async.
> > 
> This was done as part of experiment, just to determine the latency in DSTS.
> And it was around 3-30ms. Saw rhis same behaviour when polling DSTS in
> __dwc3_gadget_wakeup(sync)
> 
> > > 
> > > For SS case, this retries count was consistently 1, it was passing in first
> > > try itself. But unfortunately doesn't behave the same way in HS.
> > > 
> > > > GUSB2PHYCFG.suspendusb2 turns on the signal required to complete a
> > > > command within 50us. This happens within the timeout required for an
> > > > endpoint command. As a result, there's no need to perform remote wakeup.
> > > > 
> > > > For usb3 speed, if it's in U3, the gadget is in suspend anyway. There
> > > > will be no ep_queue to trigger the Start Transfer command.
> > > > 
> > > > You can just remove the whole Start Transfer check for remote wakeup
> > > > completely.
> > > > 
> > > Sorry, i didnt understand your suggestion. The startxfer check is needed as
> > > per databook, but we also need to handle the latency seen in DSTS when
> > > operating in HS.
> > > 
> > 
> > usb_ep_queue should not trigger remote wakeup; it should be done by
> > wakeup() ops. The programming guide just noted that the Start Transfer
> > command should not be issued while in L1/L2/U3. It suggested to wake up
> > the host to bring it out of L1/L2/U3 state so the command can go
> > through.
> > 
> > My suggestion is to remove the L1/L2/U3 check in
> > dwc3_send_gadget_ep_cmd(), and it will still work fine with reasons
> > noted previously. So, just do this:
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 0ea2ca0f0d28..6ef6c4ef2a7b 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -411,30 +411,6 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
> >                          dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> >          }
> > 
> > -       if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) {
> > -               int link_state;
> > -
> > -               /*
> > -                * Initiate remote wakeup if the link state is in U3 when
> > -                * operating in SS/SSP or L1/L2 when operating in HS/FS. If the
> > -                * link state is in U1/U2, no remote wakeup is needed. The Start
> > -                * Transfer command will initiate the link recovery.
> > -                */
> > -               link_state = dwc3_gadget_get_link_state(dwc);
> > -               switch (link_state) {
> > -               case DWC3_LINK_STATE_U2:
> > -                       if (dwc->gadget->speed >= USB_SPEED_SUPER)
> > -                               break;
> > -
> > -                       fallthrough;
> > -               case DWC3_LINK_STATE_U3:
> > -                       ret = __dwc3_gadget_wakeup(dwc, false);
> > -                       dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
> > -                                       ret);
> > -                       break;
> > -               }
> > -       }
> > -
> >          /*
> >           * For some commands such as Update Transfer command, DEPCMDPARn
> >           * registers are reserved. Since the driver often sends Update Transfer
> > 
> > When we receive the wakeup event, then the device is no longer in
> > L1/L2/U3. The Start Tranfer command should go through. >
> Ok will do this, I hope there won't be any corner cases where the link is
> down when start_xfer happens. I was not really sure about the history, thats
> why tried to incorporate my fix into the above IF check.
> 

It was initially implemented verbatim base on the Start Transfer command
suggestion from the programming guide without considering the dwc3
driver flow. First dwc3 checks for U1/U2/U3 state. Then we fixed to only
check for L1/L2/U3 state, but it's still not right. I've had this on my
TODO list for awhile and haven't made an update since it's not critical.

> > We do have an issue where if the function driver issues remote wakeup,
> > the link may not transition before ep_queue() because wakeup() can be
> > async. In that case, you probably want to keep the usb_requests in the
> > pending_list until the link_state transitions out of low power.
> > 
> > The other thing that I noted previously is that I want to fix is the
> > wakeup() ops. Currently it can be async or synchronous. We should keep
> > it consistent and make it async throughout.
> > 
> Sounds like a good idea, we can move the req to pending list, then issue
> async wakeup, and queue it back once linksts_change interrupt indicates
> L0/U0. Special care is needed in dwc3_gadget_func_wakeup() when making it
> async.
> 

Yes. That would be great.

Thanks,
Thinh

Powered by blists - more mailing lists