diff mbox series

usb: dwc3: gadget: Handle EP0 request dequeuing properly

Message ID 20231121232003.20249-1-quic_wcheng@quicinc.com
State New
Headers
Series usb: dwc3: gadget: Handle EP0 request dequeuing properly |

Commit Message

Wesley Cheng Nov. 21, 2023, 11:20 p.m. UTC 
  Current EP0 dequeue path will share the same as other EPs.  However, there
are some special considerations that need to be made for EP0 transfers:

  - EP0 transfers never transition into the started_list
  - EP0 only has one active request at a time

In case there is a vendor specific control message for a function over USB
FFS, then there is no guarantee on the timeline which the DATA/STATUS stage
is responded to.  While this occurs, any attempt to end transfers on
non-control EPs will end up having the DWC3_EP_DELAY_STOP flag set, and
defer issuing of the end transfer command.  If the USB FFS application
decides to timeout the control transfer, or if USB FFS AIO path exits, the
USB FFS driver will issue a call to usb_ep_dequeue() for the ep0 request.

In case of the AIO exit path, the AIO FS blocks until all pending USB
requests utilizing the AIO path is completed.  However, since the dequeue
of ep0 req does not happen properly, all non-control EPs with the
DWC3_EP_DELAY_STOP flag set will not be handled, and the AIO exit path will
be stuck waiting for the USB FFS data endpoints to receive a completion
callback.

Fix is to utilize dwc3_ep0_reset_state() in the dequeue API to ensure EP0
is brought back to the SETUP state, and ensures that any deferred end
transfer commands are handled.  This also will end any active transfers
on EP0, compared to the previous implementation which directly called
giveback only.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/gadget.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Thinh Nguyen Dec. 5, 2023, 1:46 a.m. UTC | #1 
On Tue, Nov 21, 2023, Wesley Cheng wrote:
> Current EP0 dequeue path will share the same as other EPs.  However, there
> are some special considerations that need to be made for EP0 transfers:
> 
>   - EP0 transfers never transition into the started_list
>   - EP0 only has one active request at a time
> 
> In case there is a vendor specific control message for a function over USB
> FFS, then there is no guarantee on the timeline which the DATA/STATUS stage
> is responded to.  While this occurs, any attempt to end transfers on
> non-control EPs will end up having the DWC3_EP_DELAY_STOP flag set, and
> defer issuing of the end transfer command.  If the USB FFS application
> decides to timeout the control transfer, or if USB FFS AIO path exits, the
> USB FFS driver will issue a call to usb_ep_dequeue() for the ep0 request.
> 
> In case of the AIO exit path, the AIO FS blocks until all pending USB
> requests utilizing the AIO path is completed.  However, since the dequeue
> of ep0 req does not happen properly, all non-control EPs with the
> DWC3_EP_DELAY_STOP flag set will not be handled, and the AIO exit path will
> be stuck waiting for the USB FFS data endpoints to receive a completion
> callback.
> 
> Fix is to utilize dwc3_ep0_reset_state() in the dequeue API to ensure EP0
> is brought back to the SETUP state, and ensures that any deferred end
> transfer commands are handled.  This also will end any active transfers
> on EP0, compared to the previous implementation which directly called
> giveback only.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/dwc3/gadget.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 858fe4c299b7..88d8d589f014 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2103,7 +2103,17 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  
>  	list_for_each_entry(r, &dep->pending_list, list) {
>  		if (r == req) {
> -			dwc3_gadget_giveback(dep, req, -ECONNRESET);
> +			/*
> +			 * Explicitly check for EP0/1 as dequeue for those
> +			 * EPs need to be handled differently.  Control EP
> +			 * only deals with one USB req, and giveback will
> +			 * occur during dwc3_ep0_stall_and_restart().  EP0
> +			 * requests are never added to started_list.
> +			 */
> +			if (dep->number > 1)
> +				dwc3_gadget_giveback(dep, req, -ECONNRESET);
> +			else
> +				dwc3_ep0_reset_state(dwc);
>  			goto out;
>  		}
>  	}

Should we add Fixes tag?

Thanks,
Thinh
Wesley Cheng Dec. 5, 2023, 8:10 p.m. UTC | #2 
Hi Thinh,

On 12/4/2023 5:46 PM, Thinh Nguyen wrote:
> On Tue, Nov 21, 2023, Wesley Cheng wrote:
>> Current EP0 dequeue path will share the same as other EPs.  However, there
>> are some special considerations that need to be made for EP0 transfers:
>>
>>    - EP0 transfers never transition into the started_list
>>    - EP0 only has one active request at a time
>>
>> In case there is a vendor specific control message for a function over USB
>> FFS, then there is no guarantee on the timeline which the DATA/STATUS stage
>> is responded to.  While this occurs, any attempt to end transfers on
>> non-control EPs will end up having the DWC3_EP_DELAY_STOP flag set, and
>> defer issuing of the end transfer command.  If the USB FFS application
>> decides to timeout the control transfer, or if USB FFS AIO path exits, the
>> USB FFS driver will issue a call to usb_ep_dequeue() for the ep0 request.
>>
>> In case of the AIO exit path, the AIO FS blocks until all pending USB
>> requests utilizing the AIO path is completed.  However, since the dequeue
>> of ep0 req does not happen properly, all non-control EPs with the
>> DWC3_EP_DELAY_STOP flag set will not be handled, and the AIO exit path will
>> be stuck waiting for the USB FFS data endpoints to receive a completion
>> callback.
>>
>> Fix is to utilize dwc3_ep0_reset_state() in the dequeue API to ensure EP0
>> is brought back to the SETUP state, and ensures that any deferred end
>> transfer commands are handled.  This also will end any active transfers
>> on EP0, compared to the previous implementation which directly called
>> giveback only.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   drivers/usb/dwc3/gadget.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 858fe4c299b7..88d8d589f014 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2103,7 +2103,17 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>>   
>>   	list_for_each_entry(r, &dep->pending_list, list) {
>>   		if (r == req) {
>> -			dwc3_gadget_giveback(dep, req, -ECONNRESET);
>> +			/*
>> +			 * Explicitly check for EP0/1 as dequeue for those
>> +			 * EPs need to be handled differently.  Control EP
>> +			 * only deals with one USB req, and giveback will
>> +			 * occur during dwc3_ep0_stall_and_restart().  EP0
>> +			 * requests are never added to started_list.
>> +			 */
>> +			if (dep->number > 1)
>> +				dwc3_gadget_giveback(dep, req, -ECONNRESET);
>> +			else
>> +				dwc3_ep0_reset_state(dwc);
>>   			goto out;
>>   		}
>>   	}
> 
> Should we add Fixes tag?
> 

Sure will add and resubmit.

Thanks
Wesley Cheng
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 858fe4c299b7..88d8d589f014 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2103,7 +2103,17 @@  static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	list_for_each_entry(r, &dep->pending_list, list) {
 		if (r == req) {
-			dwc3_gadget_giveback(dep, req, -ECONNRESET);
+			/*
+			 * Explicitly check for EP0/1 as dequeue for those
+			 * EPs need to be handled differently.  Control EP
+			 * only deals with one USB req, and giveback will
+			 * occur during dwc3_ep0_stall_and_restart().  EP0
+			 * requests are never added to started_list.
+			 */
+			if (dep->number > 1)
+				dwc3_gadget_giveback(dep, req, -ECONNRESET);
+			else
+				dwc3_ep0_reset_state(dwc);
 			goto out;
 		}
 	}