| Message ID | 20240816062017.970364-1-quic_prashk@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: dwc3: Avoid waking up gadget during startxfer | expand |
On Fri, Aug 16, 2024, Prashanth K wrote: > When operating in High-Speed, it is observed that DSTS[USBLNKST] doesn't > update link state immediately after receiving the wakeup interrupt. Since > wakeup event handler calls the resume callbacks, there is a chance that > function drivers can perform an ep queue, which in turn tries to perform > remote wakeup from send_gadget_ep_cmd(STARTXFER). This happens because > DSTS[[21:18] wasn't updated to U0 yet, it's observed that the latency of > DSTS can be in order of milli-seconds. Hence avoid calling gadget_wakeup > during startxfer to prevent unnecessarily issuing remote wakeup to host. > > Fixes: c36d8e947a56 ("usb: dwc3: gadget: put link to U0 before Start Transfer") > Cc: <stable@vger.kernel.org> > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Signed-off-by: Prashanth K <quic_prashk@quicinc.com> > --- > v2: Refactored the patch as suggested in v1 discussion. > > drivers/usb/dwc3/gadget.c | 24 ------------------------ > 1 file changed, 24 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 89fc690fdf34..3f634209c5b8 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -327,30 +327,6 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd, > dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); Can you capture the notes I provided explaining why we can issue Start Transfer without checking for L1/L2/U3 states on top of this function? Thanks, Thinh > } > > - 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 > -- > 2.25.1 >
On 17-08-24 03:19 am, Thinh Nguyen wrote: > On Fri, Aug 16, 2024, Prashanth K wrote: >> When operating in High-Speed, it is observed that DSTS[USBLNKST] doesn't >> update link state immediately after receiving the wakeup interrupt. Since >> wakeup event handler calls the resume callbacks, there is a chance that >> function drivers can perform an ep queue, which in turn tries to perform >> remote wakeup from send_gadget_ep_cmd(STARTXFER). This happens because >> DSTS[[21:18] wasn't updated to U0 yet, it's observed that the latency of >> DSTS can be in order of milli-seconds. Hence avoid calling gadget_wakeup >> during startxfer to prevent unnecessarily issuing remote wakeup to host. >> >> Fixes: c36d8e947a56 ("usb: dwc3: gadget: put link to U0 before Start Transfer") >> Cc: <stable@vger.kernel.org> >> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Signed-off-by: Prashanth K <quic_prashk@quicinc.com> >> --- >> v2: Refactored the patch as suggested in v1 discussion. >> >> drivers/usb/dwc3/gadget.c | 24 ------------------------ >> 1 file changed, 24 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 89fc690fdf34..3f634209c5b8 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -327,30 +327,6 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd, >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > > Can you capture the notes I provided explaining why we can issue Start > Transfer without checking for L1/L2/U3 states on top of this function? > I also thought the same initially, but didnt add since Greg usually adds the patch link to commit, so that the discussion would be captured in git log. I will add it on top of dwc3_send_gadget_ep_cmd and send V3, let me know if there's any other suggestion. Thanks, Prashanth K
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 89fc690fdf34..3f634209c5b8 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -327,30 +327,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 operating in High-Speed, it is observed that DSTS[USBLNKST] doesn't update link state immediately after receiving the wakeup interrupt. Since wakeup event handler calls the resume callbacks, there is a chance that function drivers can perform an ep queue, which in turn tries to perform remote wakeup from send_gadget_ep_cmd(STARTXFER). This happens because DSTS[[21:18] wasn't updated to U0 yet, it's observed that the latency of DSTS can be in order of milli-seconds. Hence avoid calling gadget_wakeup during startxfer to prevent unnecessarily issuing remote wakeup to host. Fixes: c36d8e947a56 ("usb: dwc3: gadget: put link to U0 before Start Transfer") Cc: <stable@vger.kernel.org> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Signed-off-by: Prashanth K <quic_prashk@quicinc.com> --- v2: Refactored the patch as suggested in v1 discussion. drivers/usb/dwc3/gadget.c | 24 ------------------------ 1 file changed, 24 deletions(-)