| Message ID | 20230227232035.13759-1-quic_wcheng@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: gadget: Add 100uS delay after end transfer command without IOC | expand |
On Mon, Feb 27, 2023, Wesley Cheng wrote: > Previously, there was a 100uS delay inserted after issuing an end transfer > command for specific controller revisions. This was due to the fact that > there was a GUCTL2 bit field which enabled synchronous completion of the > end transfer command once the CMDACT bit was cleared in the DEPCMD > register. Since this bit does not exist for all controller revisions, add > the delay back in. > > An issue was seen where the USB request buffer was unmapped while the DWC3 > controller was still accessing the TRB. However, it was confirmed that the > end transfer command was successfully submitted. (no end transfer timeout) Currently we only check for command active, not completion on teardown. > In situations, such as dwc3_gadget_soft_disconnect() and > __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which > will issue the end transfer command, and follow up with > dwc3_gadget_giveback(). At least for the USB ep disable path, it is > required for any pending and started requests to be completed and returned > to the function driver in the same context of the disable call. Without > the GUCTL2 bit, it is not ensured that the end transfer is completed before > the buffers are unmapped. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> This is expected. We're supposed to make sure the End Transfer command complete before accessing the request. Usually on device/endpoint teardown, the gadget drivers don't access the stale/incomplete requests with -ESHUTDOWN status. There will be problems if we do, and we haven't fixed that. Adding 100uS may not apply for every device, and we don't need to do that for every End Transfer command. Can you try this untested diff instead: diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 30408bafe64e..5ae5ff4c8858 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1962,6 +1962,34 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc) return DWC3_DSTS_SOFFN(reg); } +static int dwc3_poll_ep_completion(struct dwc3_ep *dep) +{ + if (!list_empty(&dep->started_list)) { + struct dwc3_request *req; + int timeout = 500; + + req = next_request(&dep->started_list); + while(--timeout) { + /* + * Note: don't check the last enqueued TRB in case + * of short transfer. Check first TRB of a started + * request instead. + */ + if (!(req->trb->ctrl & DWC3_TRB_CTRL_HWO)) + break; + + udelay(2); + } + if (!timeout) { + dev_warn(dep->dwc->dev, + "%s is still in-progress\n", dep->name); + return -ETIMEDOUT; + } + } + + return 0; +} + /** * __dwc3_stop_active_transfer - stop the current active transfer * @dep: isoc endpoint @@ -2003,10 +2031,12 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int WARN_ON_ONCE(ret); dep->resource_index = 0; - if (!interrupt) + if (!interrupt) { + ret = dwc3_poll_ep_completion(dep); dep->flags &= ~DWC3_EP_TRANSFER_STARTED; - else if (!ret) + } else if (!ret) { dep->flags |= DWC3_EP_END_TRANSFER_PENDING; + } dep->flags &= ~DWC3_EP_DELAY_STOP; return ret; Thanks, Thinh
On Tue, Feb 28, 2023, Thinh Nguyen wrote: > On Mon, Feb 27, 2023, Wesley Cheng wrote: > > Previously, there was a 100uS delay inserted after issuing an end transfer > > command for specific controller revisions. This was due to the fact that > > there was a GUCTL2 bit field which enabled synchronous completion of the > > end transfer command once the CMDACT bit was cleared in the DEPCMD > > register. Since this bit does not exist for all controller revisions, add > > the delay back in. > > > > An issue was seen where the USB request buffer was unmapped while the DWC3 > > controller was still accessing the TRB. However, it was confirmed that the > > end transfer command was successfully submitted. (no end transfer timeout) > > Currently we only check for command active, not completion on teardown. > > > In situations, such as dwc3_gadget_soft_disconnect() and > > __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which > > will issue the end transfer command, and follow up with > > dwc3_gadget_giveback(). At least for the USB ep disable path, it is > > required for any pending and started requests to be completed and returned > > to the function driver in the same context of the disable call. Without > > the GUCTL2 bit, it is not ensured that the end transfer is completed before > > the buffers are unmapped. > > > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > > This is expected. We're supposed to make sure the End Transfer command > complete before accessing the request. Usually on device/endpoint > teardown, the gadget drivers don't access the stale/incomplete requests > with -ESHUTDOWN status. There will be problems if we do, and we haven't > fixed that. > > Adding 100uS may not apply for every device, and we don't need to do > that for every End Transfer command. Can you try this untested diff > instead: > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 30408bafe64e..5ae5ff4c8858 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1962,6 +1962,34 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc) > return DWC3_DSTS_SOFFN(reg); > } > > +static int dwc3_poll_ep_completion(struct dwc3_ep *dep) > +{ > + if (!list_empty(&dep->started_list)) { > + struct dwc3_request *req; > + int timeout = 500; > + > + req = next_request(&dep->started_list); > + while(--timeout) { > + /* > + * Note: don't check the last enqueued TRB in case > + * of short transfer. Check first TRB of a started > + * request instead. > + */ > + if (!(req->trb->ctrl & DWC3_TRB_CTRL_HWO)) > + break; > + > + udelay(2); > + } > + if (!timeout) { > + dev_warn(dep->dwc->dev, > + "%s is still in-progress\n", dep->name); > + return -ETIMEDOUT; > + } > + } > + > + return 0; > +} > + > /** > * __dwc3_stop_active_transfer - stop the current active transfer > * @dep: isoc endpoint > @@ -2003,10 +2031,12 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int > WARN_ON_ONCE(ret); > dep->resource_index = 0; > > - if (!interrupt) > + if (!interrupt) { > + ret = dwc3_poll_ep_completion(dep); Actually, the TRB status may not get updated, so this may not work, instead of polling, may need to add the delay here instead. > dep->flags &= ~DWC3_EP_TRANSFER_STARTED; > - else if (!ret) > + } else if (!ret) { > dep->flags |= DWC3_EP_END_TRANSFER_PENDING; > + } > > dep->flags &= ~DWC3_EP_DELAY_STOP; > return ret; > > Thanks, Thinh
Hi Thinh, On 2/27/2023 7:10 PM, Thinh Nguyen wrote: > On Tue, Feb 28, 2023, Thinh Nguyen wrote: >> On Mon, Feb 27, 2023, Wesley Cheng wrote: >>> Previously, there was a 100uS delay inserted after issuing an end transfer >>> command for specific controller revisions. This was due to the fact that >>> there was a GUCTL2 bit field which enabled synchronous completion of the >>> end transfer command once the CMDACT bit was cleared in the DEPCMD >>> register. Since this bit does not exist for all controller revisions, add >>> the delay back in. >>> >>> An issue was seen where the USB request buffer was unmapped while the DWC3 >>> controller was still accessing the TRB. However, it was confirmed that the >>> end transfer command was successfully submitted. (no end transfer timeout) >> >> Currently we only check for command active, not completion on teardown. >> >>> In situations, such as dwc3_gadget_soft_disconnect() and >>> __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which >>> will issue the end transfer command, and follow up with >>> dwc3_gadget_giveback(). At least for the USB ep disable path, it is >>> required for any pending and started requests to be completed and returned >>> to the function driver in the same context of the disable call. Without >>> the GUCTL2 bit, it is not ensured that the end transfer is completed before >>> the buffers are unmapped. >>> >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> >> This is expected. We're supposed to make sure the End Transfer command >> complete before accessing the request. Usually on device/endpoint >> teardown, the gadget drivers don't access the stale/incomplete requests >> with -ESHUTDOWN status. There will be problems if we do, and we haven't >> fixed that. >> >> Adding 100uS may not apply for every device, and we don't need to do >> that for every End Transfer command. Can you try this untested diff >> instead: >> Thanks for the code suggestion. >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 30408bafe64e..5ae5ff4c8858 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1962,6 +1962,34 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc) >> return DWC3_DSTS_SOFFN(reg); >> } >> >> +static int dwc3_poll_ep_completion(struct dwc3_ep *dep) >> +{ >> + if (!list_empty(&dep->started_list)) { >> + struct dwc3_request *req; >> + int timeout = 500; >> + >> + req = next_request(&dep->started_list); >> + while(--timeout) { >> + /* >> + * Note: don't check the last enqueued TRB in case >> + * of short transfer. Check first TRB of a started >> + * request instead. >> + */ >> + if (!(req->trb->ctrl & DWC3_TRB_CTRL_HWO)) >> + break; >> + >> + udelay(2); >> + } >> + if (!timeout) { >> + dev_warn(dep->dwc->dev, >> + "%s is still in-progress\n", dep->name); >> + return -ETIMEDOUT; >> + } >> + } >> + >> + return 0; >> +} >> + >> /** >> * __dwc3_stop_active_transfer - stop the current active transfer >> * @dep: isoc endpoint >> @@ -2003,10 +2031,12 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int >> WARN_ON_ONCE(ret); >> dep->resource_index = 0; >> >> - if (!interrupt) >> + if (!interrupt) { >> + ret = dwc3_poll_ep_completion(dep); > > Actually, the TRB status may not get updated, so this may not work, > instead of polling, may need to add the delay here instead. > Yeah, I just gave it a try, and I get the ETIMEDOUT error all the time. Don't think we can utilize the HWO bit here. Thanks Wesley Cheng
Hi Thinh, On 2/27/2023 8:02 PM, Thinh Nguyen wrote: > On Mon, Feb 27, 2023, Wesley Cheng wrote: >> Hi Thinh, >> >> On 2/27/2023 7:10 PM, Thinh Nguyen wrote: >>> On Tue, Feb 28, 2023, Thinh Nguyen wrote: >>>> On Mon, Feb 27, 2023, Wesley Cheng wrote: >>>>> Previously, there was a 100uS delay inserted after issuing an end transfer >>>>> command for specific controller revisions. This was due to the fact that >>>>> there was a GUCTL2 bit field which enabled synchronous completion of the >>>>> end transfer command once the CMDACT bit was cleared in the DEPCMD >>>>> register. Since this bit does not exist for all controller revisions, add >>>>> the delay back in. >>>>> >>>>> An issue was seen where the USB request buffer was unmapped while the DWC3 >>>>> controller was still accessing the TRB. However, it was confirmed that the >>>>> end transfer command was successfully submitted. (no end transfer timeout) >>>> >>>> Currently we only check for command active, not completion on teardown. >>>> >>>>> In situations, such as dwc3_gadget_soft_disconnect() and >>>>> __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which >>>>> will issue the end transfer command, and follow up with >>>>> dwc3_gadget_giveback(). At least for the USB ep disable path, it is >>>>> required for any pending and started requests to be completed and returned >>>>> to the function driver in the same context of the disable call. Without >>>>> the GUCTL2 bit, it is not ensured that the end transfer is completed before >>>>> the buffers are unmapped. >>>>> >>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >>>> >>>> This is expected. We're supposed to make sure the End Transfer command >>>> complete before accessing the request. Usually on device/endpoint >>>> teardown, the gadget drivers don't access the stale/incomplete requests >>>> with -ESHUTDOWN status. There will be problems if we do, and we haven't >>>> fixed that. >>>> >>>> Adding 100uS may not apply for every device, and we don't need to do >>>> that for every End Transfer command. Can you try this untested diff >>>> instead: >>>> >> >> Thanks for the code suggestion. >> >>>> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index 30408bafe64e..5ae5ff4c8858 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -1962,6 +1962,34 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc) >>>> return DWC3_DSTS_SOFFN(reg); >>>> } >>>> +static int dwc3_poll_ep_completion(struct dwc3_ep *dep) >>>> +{ >>>> + if (!list_empty(&dep->started_list)) { >>>> + struct dwc3_request *req; >>>> + int timeout = 500; >>>> + >>>> + req = next_request(&dep->started_list); >>>> + while(--timeout) { >>>> + /* >>>> + * Note: don't check the last enqueued TRB in case >>>> + * of short transfer. Check first TRB of a started >>>> + * request instead. >>>> + */ >>>> + if (!(req->trb->ctrl & DWC3_TRB_CTRL_HWO)) >>>> + break; >>>> + >>>> + udelay(2); >>>> + } >>>> + if (!timeout) { >>>> + dev_warn(dep->dwc->dev, >>>> + "%s is still in-progress\n", dep->name); >>>> + return -ETIMEDOUT; >>>> + } >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> /** >>>> * __dwc3_stop_active_transfer - stop the current active transfer >>>> * @dep: isoc endpoint >>>> @@ -2003,10 +2031,12 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int >>>> WARN_ON_ONCE(ret); >>>> dep->resource_index = 0; >>>> - if (!interrupt) >>>> + if (!interrupt) { >>>> + ret = dwc3_poll_ep_completion(dep); >>> >>> Actually, the TRB status may not get updated, so this may not work, >>> instead of polling, may need to add the delay here instead. >>> >> >> Yeah, I just gave it a try, and I get the ETIMEDOUT error all the time. >> Don't think we can utilize the HWO bit here. >> > > I may be over complicating things here. With ForceRM, the controller > only updates the last TRB it processed. We don't care about performance > much during teardown. That would mean more codes for something that's > not need. > Yes :) that is what I encountered as well. I tried a few other things, but it opened a whole new set of topics that needed to be discussed further. Hence why I proposed the simple delay, since this happens only in the teardown path as you mentioned. > Can you add a delay here instead? Make sure it's at least 1ms and > applicable for dwc_usb32 also. > Sure, I will update the delay to 1ms and also add USB32 check. Thanks Wesley Cheng
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 3c63fa97a680..a4c02e4f7f7c 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1699,6 +1699,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc) */ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt) { + struct dwc3 *dwc = dep->dwc; struct dwc3_gadget_ep_cmd_params params; u32 cmd; int ret; @@ -1722,10 +1723,13 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int WARN_ON_ONCE(ret); dep->resource_index = 0; - if (!interrupt) + if (!interrupt) { + if (DWC3_IP_IS(DWC31) || DWC3_VER_IS_PRIOR(DWC3, 310A)) + udelay(100); dep->flags &= ~DWC3_EP_TRANSFER_STARTED; - else if (!ret) + } else if (!ret) { dep->flags |= DWC3_EP_END_TRANSFER_PENDING; + } dep->flags &= ~DWC3_EP_DELAY_STOP; return ret; @@ -3774,7 +3778,11 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, * enabled, the EndTransfer command will have completed upon * returning from this function. * - * This mode is NOT available on the DWC_usb31 IP. + * This mode is NOT available on the DWC_usb31 IP. In this + * case, if the IOC bit is not set, then delay by 100uS + * after issuing the EndTransfer command. This allows for the + * controller to handle the command completely before DWC3 + * remove requests attempts to unmap USB request buffers. */ __dwc3_stop_active_transfer(dep, force, interrupt);
Previously, there was a 100uS delay inserted after issuing an end transfer command for specific controller revisions. This was due to the fact that there was a GUCTL2 bit field which enabled synchronous completion of the end transfer command once the CMDACT bit was cleared in the DEPCMD register. Since this bit does not exist for all controller revisions, add the delay back in. An issue was seen where the USB request buffer was unmapped while the DWC3 controller was still accessing the TRB. However, it was confirmed that the end transfer command was successfully submitted. (no end transfer timeout) In situations, such as dwc3_gadget_soft_disconnect() and __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which will issue the end transfer command, and follow up with dwc3_gadget_giveback(). At least for the USB ep disable path, it is required for any pending and started requests to be completed and returned to the function driver in the same context of the disable call. Without the GUCTL2 bit, it is not ensured that the end transfer is completed before the buffers are unmapped. Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> --- drivers/usb/dwc3/gadget.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)