| Message ID | 6bacec56ecabb2c6e49a09cedfcac281fdc97de0.1650593829.git.Thinh.Nguyen@synopsys.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c96683798e272366866a5c0ce3073c0b5a256db7 |
| Headers | show |
| Series | usb: dwc3: gadget: Rework pullup | expand |
On Thu, Apr 21, 2022 at 07:22:50PM -0700, Thinh Nguyen wrote: > Since we can't guarantee that the host won't send new Setup packet > before going through the device-initiated disconnect, don't prepare > beyond the Setup stage and keep the device in EP0_SETUP_PHASE. This > ensures that the device-initated disconnect sequence can go through > gracefully. Note that the controller won't service the End Transfer > command if it can't DMA out the Setup packet. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/usb/dwc3/ep0.c | 2 +- > drivers/usb/dwc3/gadget.c | 29 +++++++++++++++++------------ > 2 files changed, 18 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c > index 1064be5518f6..c47c696316dd 100644 > --- a/drivers/usb/dwc3/ep0.c > +++ b/drivers/usb/dwc3/ep0.c > @@ -813,7 +813,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc, > int ret = -EINVAL; > u32 len; > > - if (!dwc->gadget_driver) > + if (!dwc->gadget_driver || !dwc->connected) > goto out; > > trace_dwc3_ctrl_req(ctrl); > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index a86225dbaa2c..e5f07c0e8ad9 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2505,6 +2505,23 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc) > spin_lock_irqsave(&dwc->lock, flags); > dwc->connected = false; > > + /* > + * Per databook, when we want to stop the gadget, if a control transfer > + * is still in process, complete it and get the core into setup phase. > + */ > + if (dwc->ep0state != EP0_SETUP_PHASE) { > + int ret; > + > + reinit_completion(&dwc->ep0_in_setup); > + > + spin_unlock_irqrestore(&dwc->lock, flags); > + ret = wait_for_completion_timeout(&dwc->ep0_in_setup, > + msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT)); > + spin_lock_irqsave(&dwc->lock, flags); > + if (ret == 0) > + dev_warn(dwc->dev, "timed out waiting for SETUP phase\n"); DWC3_PULL_UP_TIMEOUT is 500 msec. If the ongoing control transfer is delayed (dwc3::delayed_status), for whatever reason, would there be a problem? Thanks, Pavan
Hi, Pavan Kondeti wrote: > On Thu, Apr 21, 2022 at 07:22:50PM -0700, Thinh Nguyen wrote: >> Since we can't guarantee that the host won't send new Setup packet >> before going through the device-initiated disconnect, don't prepare >> beyond the Setup stage and keep the device in EP0_SETUP_PHASE. This >> ensures that the device-initated disconnect sequence can go through >> gracefully. Note that the controller won't service the End Transfer >> command if it can't DMA out the Setup packet. >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/usb/dwc3/ep0.c | 2 +- >> drivers/usb/dwc3/gadget.c | 29 +++++++++++++++++------------ >> 2 files changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >> index 1064be5518f6..c47c696316dd 100644 >> --- a/drivers/usb/dwc3/ep0.c >> +++ b/drivers/usb/dwc3/ep0.c >> @@ -813,7 +813,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc, >> int ret = -EINVAL; >> u32 len; >> >> - if (!dwc->gadget_driver) >> + if (!dwc->gadget_driver || !dwc->connected) >> goto out; >> >> trace_dwc3_ctrl_req(ctrl); >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index a86225dbaa2c..e5f07c0e8ad9 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2505,6 +2505,23 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc) >> spin_lock_irqsave(&dwc->lock, flags); >> dwc->connected = false; >> >> + /* >> + * Per databook, when we want to stop the gadget, if a control transfer >> + * is still in process, complete it and get the core into setup phase. >> + */ >> + if (dwc->ep0state != EP0_SETUP_PHASE) { >> + int ret; >> + >> + reinit_completion(&dwc->ep0_in_setup); >> + >> + spin_unlock_irqrestore(&dwc->lock, flags); >> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup, >> + msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT)); >> + spin_lock_irqsave(&dwc->lock, flags); >> + if (ret == 0) >> + dev_warn(dwc->dev, "timed out waiting for SETUP phase\n"); > > DWC3_PULL_UP_TIMEOUT is 500 msec. If the ongoing control transfer is delayed > (dwc3::delayed_status), for whatever reason, would there be a problem? > Sorry for the delayed response. I was away. If the control transfer takes longer than 500ms, then we'd get this timed out warning. However, it should be fine because 1) If the function driver hasn't sent the status, then the host won't be sending a new SETUP packet. 2) If the delayed status was sent and completed immediately after the timeout but before the dwc3_gadget_soft_disconnect holding the spin_lock, then we may see End Transfer command timeout. It may not look like the cleanup was done gracefully, but that should be fine. The command should be able to complete once the spin_lock is released and Setup packet handled. The controller should halt within the polling period. 3) If the host misbehaves and ignores the status stage/abort the control transfer to send a new setup packet, I don't think the current dwc3 driver handles that case properly. But that should be for a separate patch fix. BR, Thinh
Hi Thinh, On 5/23/2022 4:22 PM, Thinh Nguyen wrote: > Hi, > > Pavan Kondeti wrote: >> On Thu, Apr 21, 2022 at 07:22:50PM -0700, Thinh Nguyen wrote: >>> Since we can't guarantee that the host won't send new Setup packet >>> before going through the device-initiated disconnect, don't prepare >>> beyond the Setup stage and keep the device in EP0_SETUP_PHASE. This >>> ensures that the device-initated disconnect sequence can go through >>> gracefully. Note that the controller won't service the End Transfer >>> command if it can't DMA out the Setup packet. >>> >>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>> --- >>> drivers/usb/dwc3/ep0.c | 2 +- >>> drivers/usb/dwc3/gadget.c | 29 +++++++++++++++++------------ >>> 2 files changed, 18 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >>> index 1064be5518f6..c47c696316dd 100644 >>> --- a/drivers/usb/dwc3/ep0.c >>> +++ b/drivers/usb/dwc3/ep0.c >>> @@ -813,7 +813,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc, >>> int ret = -EINVAL; >>> u32 len; >>> >>> - if (!dwc->gadget_driver) >>> + if (!dwc->gadget_driver || !dwc->connected) >>> goto out; >>> >>> trace_dwc3_ctrl_req(ctrl); >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index a86225dbaa2c..e5f07c0e8ad9 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2505,6 +2505,23 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc) >>> spin_lock_irqsave(&dwc->lock, flags); >>> dwc->connected = false; >>> >>> + /* >>> + * Per databook, when we want to stop the gadget, if a control transfer >>> + * is still in process, complete it and get the core into setup phase. >>> + */ >>> + if (dwc->ep0state != EP0_SETUP_PHASE) { >>> + int ret; >>> + >>> + reinit_completion(&dwc->ep0_in_setup); >>> + >>> + spin_unlock_irqrestore(&dwc->lock, flags); >>> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup, >>> + msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT)); >>> + spin_lock_irqsave(&dwc->lock, flags); >>> + if (ret == 0) >>> + dev_warn(dwc->dev, "timed out waiting for SETUP phase\n"); >> >> DWC3_PULL_UP_TIMEOUT is 500 msec. If the ongoing control transfer is delayed >> (dwc3::delayed_status), for whatever reason, would there be a problem? >> > > Sorry for the delayed response. I was away. > > If the control transfer takes longer than 500ms, then we'd get this > timed out warning. However, it should be fine because > > 1) If the function driver hasn't sent the status, then the host won't be > sending a new SETUP packet. > > 2) If the delayed status was sent and completed immediately after the > timeout but before the dwc3_gadget_soft_disconnect holding the > spin_lock, then we may see End Transfer command timeout. It may not look > like the cleanup was done gracefully, but that should be fine. The > command should be able to complete once the spin_lock is released and > Setup packet handled. The controller should halt within the polling period. > > 3) If the host misbehaves and ignores the status stage/abort the control > transfer to send a new setup packet, I don't think the current dwc3 > driver handles that case properly. But that should be for a separate > patch fix. > In the trace that I sent you where the controller halt fails, it is due to the above condition that Pavan mentioned. We're in a situation where if the function driver dequeues an USB request, and we are not in the proper ep0state to handle, we'll set the DWC3_EP_DELAY_STOP flag. Soon after, if a soft disconnect occurs, and we're in a situation where delayed_status == 1, then most likely, we'll see the SETUP packet timeout (depending on when the function queues the status phase) and proceed w/ stop active xfers and gadget. Since we do not wait for the delayed stop condition to be handled before attempting to halt the controller, we'll run into a timeout when clearing Run/Stop. In this situation, this is why you don't see the endxfer command being send for endpoints. Thanks Wesley Cheng
Wesley Cheng wrote: > Hi Thinh, > > On 5/23/2022 4:22 PM, Thinh Nguyen wrote: >> Hi, >> >> Pavan Kondeti wrote: >>> On Thu, Apr 21, 2022 at 07:22:50PM -0700, Thinh Nguyen wrote: >>>> Since we can't guarantee that the host won't send new Setup packet >>>> before going through the device-initiated disconnect, don't prepare >>>> beyond the Setup stage and keep the device in EP0_SETUP_PHASE. This >>>> ensures that the device-initated disconnect sequence can go through >>>> gracefully. Note that the controller won't service the End Transfer >>>> command if it can't DMA out the Setup packet. >>>> >>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>>> --- >>>> drivers/usb/dwc3/ep0.c | 2 +- >>>> drivers/usb/dwc3/gadget.c | 29 +++++++++++++++++------------ >>>> 2 files changed, 18 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >>>> index 1064be5518f6..c47c696316dd 100644 >>>> --- a/drivers/usb/dwc3/ep0.c >>>> +++ b/drivers/usb/dwc3/ep0.c >>>> @@ -813,7 +813,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 >>>> *dwc, >>>> int ret = -EINVAL; >>>> u32 len; >>>> - if (!dwc->gadget_driver) >>>> + if (!dwc->gadget_driver || !dwc->connected) >>>> goto out; >>>> trace_dwc3_ctrl_req(ctrl); >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index a86225dbaa2c..e5f07c0e8ad9 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -2505,6 +2505,23 @@ static int dwc3_gadget_soft_disconnect(struct >>>> dwc3 *dwc) >>>> spin_lock_irqsave(&dwc->lock, flags); >>>> dwc->connected = false; >>>> + /* >>>> + * Per databook, when we want to stop the gadget, if a control >>>> transfer >>>> + * is still in process, complete it and get the core into setup >>>> phase. >>>> + */ >>>> + if (dwc->ep0state != EP0_SETUP_PHASE) { >>>> + int ret; >>>> + >>>> + reinit_completion(&dwc->ep0_in_setup); >>>> + >>>> + spin_unlock_irqrestore(&dwc->lock, flags); >>>> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup, >>>> + msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT)); >>>> + spin_lock_irqsave(&dwc->lock, flags); >>>> + if (ret == 0) >>>> + dev_warn(dwc->dev, "timed out waiting for SETUP phase\n"); >>> >>> DWC3_PULL_UP_TIMEOUT is 500 msec. If the ongoing control transfer is >>> delayed >>> (dwc3::delayed_status), for whatever reason, would there be a problem? >>> >> >> Sorry for the delayed response. I was away. >> >> If the control transfer takes longer than 500ms, then we'd get this >> timed out warning. However, it should be fine because >> >> 1) If the function driver hasn't sent the status, then the host won't be >> sending a new SETUP packet. >> >> 2) If the delayed status was sent and completed immediately after the >> timeout but before the dwc3_gadget_soft_disconnect holding the >> spin_lock, then we may see End Transfer command timeout. It may not look >> like the cleanup was done gracefully, but that should be fine. The >> command should be able to complete once the spin_lock is released and >> Setup packet handled. The controller should halt within the polling >> period. >> >> 3) If the host misbehaves and ignores the status stage/abort the control >> transfer to send a new setup packet, I don't think the current dwc3 >> driver handles that case properly. But that should be for a separate >> patch fix. >> > > In the trace that I sent you where the controller halt fails, it is due > to the above condition that Pavan mentioned. We're in a situation where > if the function driver dequeues an USB request, and we are not in the > proper ep0state to handle, we'll set the DWC3_EP_DELAY_STOP flag. > > Soon after, if a soft disconnect occurs, and we're in a situation where > delayed_status == 1, then most likely, we'll see the SETUP packet > timeout (depending on when the function queues the status phase) and > proceed w/ stop active xfers and gadget. Since we do not wait for the > delayed stop condition to be handled before attempting to halt the > controller, we'll run into a timeout when clearing Run/Stop. In this > situation, this is why you don't see the endxfer command being send for > endpoints. > I see. If that's the case, then the End Transfer command won't be sent. Can you try this: diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 793c4aaf85a2..a10e0cb11385 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -292,7 +292,6 @@ void dwc3_ep0_out_start(struct dwc3 *dwc) if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP)) continue; - dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP; dwc3_stop_active_transfer(dwc3_ep, true, true); } } diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index ee8e8974302d..ff7aa7402b5b 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1984,6 +1984,7 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int else if (!ret) dep->flags |= DWC3_EP_END_TRANSFER_PENDING; + dep->flags &= ~DWC3_EP_DELAY_STOP; return ret; } @@ -4232,8 +4233,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE) return; + if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP)) + return; + if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) || - (dep->flags & DWC3_EP_DELAY_STOP) || (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) return; This makes sure that we issue End Transfer command to active endpoints. There's a small chance that the End Transfer command gets timed out if somehow the status was sent and new setup packet was immediately received, but the command should go through once the Setup packet is handled during the polling. Please help test. Thanks, Thinh
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 1064be5518f6..c47c696316dd 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -813,7 +813,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc, int ret = -EINVAL; u32 len; - if (!dwc->gadget_driver) + if (!dwc->gadget_driver || !dwc->connected) goto out; trace_dwc3_ctrl_req(ctrl); diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index a86225dbaa2c..e5f07c0e8ad9 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2505,6 +2505,23 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc) spin_lock_irqsave(&dwc->lock, flags); dwc->connected = false; + /* + * Per databook, when we want to stop the gadget, if a control transfer + * is still in process, complete it and get the core into setup phase. + */ + if (dwc->ep0state != EP0_SETUP_PHASE) { + int ret; + + reinit_completion(&dwc->ep0_in_setup); + + spin_unlock_irqrestore(&dwc->lock, flags); + ret = wait_for_completion_timeout(&dwc->ep0_in_setup, + msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT)); + spin_lock_irqsave(&dwc->lock, flags); + if (ret == 0) + dev_warn(dwc->dev, "timed out waiting for SETUP phase\n"); + } + /* * In the Synopsys DesignWare Cores USB3 Databook Rev. 3.30a * Section 4.1.8 Table 4-7, it states that for a device-initiated @@ -2537,18 +2554,6 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) return 0; dwc->softconnect = is_on; - /* - * Per databook, when we want to stop the gadget, if a control transfer - * is still in process, complete it and get the core into setup phase. - */ - if (!is_on && dwc->ep0state != EP0_SETUP_PHASE) { - reinit_completion(&dwc->ep0_in_setup); - - ret = wait_for_completion_timeout(&dwc->ep0_in_setup, - msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT)); - if (ret == 0) - dev_warn(dwc->dev, "timed out waiting for SETUP phase\n"); - } /* * Avoid issuing a runtime resume if the device is already in the
Since we can't guarantee that the host won't send new Setup packet before going through the device-initiated disconnect, don't prepare beyond the Setup stage and keep the device in EP0_SETUP_PHASE. This ensures that the device-initated disconnect sequence can go through gracefully. Note that the controller won't service the End Transfer command if it can't DMA out the Setup packet. Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- drivers/usb/dwc3/ep0.c | 2 +- drivers/usb/dwc3/gadget.c | 29 +++++++++++++++++------------ 2 files changed, 18 insertions(+), 13 deletions(-)