| Message ID | 20240820121524.1084983-1-quic_prashk@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] usb: dwc3: Avoid waking up gadget during startxfer | expand |
On 21-08-24 04:08 am, Thinh Nguyen wrote: > On Tue, Aug 20, 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> >> --- >> v3: Added notes on top the function definition. >> v2: Refactored the patch as suggested in v1 discussion. >> >> drivers/usb/dwc3/gadget.c | 31 +++++++------------------------ >> 1 file changed, 7 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 89fc690fdf34..d4f2f0e1f031 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -287,6 +287,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async); >> * >> * Caller should handle locking. This function will issue @cmd with given >> * @params to @dep and wait for its completion. >> + * >> + * According to databook, if the link is in L1/L2/U3 while issuing StartXfer command, >> + * software must bring the link back to L0/U0 by performing remote wakeup. But we don't > > Change "L0" -> "On" state > >> + * expect ep_queue to trigger a remote wakeup; instead it should be done by wakeup ops. >> + * >> + * After receiving wakeup event, device should no longer be in U3, and any link >> + * transition afterwards needs to be adressed with wakeup ops. >> */ > > You're missing the explanation for the case of L1. Please incorporate > this snippet (reword as necessary to fit in the rest of your comment): > > While operating in usb2 speed, if the device is in low power link state > (L1/L2), the Start Transfer command may not complete and timeout. The > programming guide suggested to initiate remote wakeup to bring the > device to ON state, allowing the command to go through. However, since > issuing a command in usb2 speed requires the clearing of > GUSB2PHYCFG.suspendusb2, this turns on the signal required (in 50us) to > complete a command. This should happen within the command timeout set by > the driver. No extra handling is needed. > > Special note: if wakeup() ops is triggered for remote wakeup, care > should be taken should the Start Transfer command needs to be sent soon > after. The wakeup() ops is asynchronous and the link state may not > transition to U0 link state yet. > > Does this sound good? (Didnt want to spam with new patches) "According to databook, while issuing StartXfer command if the link is in L1/L2/U3, then the command may not complete and timeout, hence software must bring the link back to ON state by performing remote wakeup. However, since issuing a command in USB2 speeds requires the clearing of GUSB2PHYCFG.suspendusb2, which turns on the signal required to complete the given command (usually within 50us). This should happen within the command timeout set by driver. Hence we don't expect to trigger a remote wakeup from here; instead it should be done by wakeup ops. Special note: If wakeup() ops is triggered for remote wakeup, care should be taken if StartXfer command needs to be sent soon after. The wakeup() ops is asynchronous and the link state may not transition to U0 link state yet. After receiving wakeup event, device would no longer be in U3, and any link transition afterwards needs to be adressed with wakeup ops." Thanks, Prashanth K
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 89fc690fdf34..d4f2f0e1f031 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -287,6 +287,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async); * * Caller should handle locking. This function will issue @cmd with given * @params to @dep and wait for its completion. + * + * According to databook, if the link is in L1/L2/U3 while issuing StartXfer command, + * software must bring the link back to L0/U0 by performing remote wakeup. But we don't + * expect ep_queue to trigger a remote wakeup; instead it should be done by wakeup ops. + * + * After receiving wakeup event, device should no longer be in U3, and any link + * transition afterwards needs to be adressed with wakeup ops. */ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd, struct dwc3_gadget_ep_cmd_params *params) @@ -327,30 +334,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> --- v3: Added notes on top the function definition. v2: Refactored the patch as suggested in v1 discussion. drivers/usb/dwc3/gadget.c | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-)