diff mbox series

usb: dwc3: gadget: Wait for ep0 xfers to complete during dequeue

Message ID 20220309004148.12061-1-quic_wcheng@quicinc.com (mailing list archive)
State Superseded
Headers show
Series usb: dwc3: gadget: Wait for ep0 xfers to complete during dequeue | expand

Commit Message

Wesley Cheng March 9, 2022, 12:41 a.m. UTC 
From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

If the request being dequeued is currently active, then the current
logic is to issue a stop transfer command, and allow the command
completion to cleanup the cancelled list.  The DWC3 controller will
run into an end transfer command timeout if there is an ongoing EP0
transaction.  If this is the case, wait for the EP0 completion event
before proceeding to retry the endxfer command again.

Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Patch discussion below:
   https://lore.kernel.org/linux-usb/1644836933-141376-1-git-send-email-dh10.jung@samsung.com/T/#t

 drivers/usb/dwc3/core.h   |  2 +-
 drivers/usb/dwc3/ep0.c    | 14 ++++++++++++++
 drivers/usb/dwc3/gadget.c | 13 ++++++++-----
 drivers/usb/dwc3/gadget.h |  1 +
 4 files changed, 24 insertions(+), 6 deletions(-)

Comments

Greg Kroah-Hartman March 9, 2022, 10:21 a.m. UTC | #1 
On Tue, Mar 08, 2022 at 04:41:48PM -0800, Wesley Cheng wrote:
> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> If the request being dequeued is currently active, then the current
> logic is to issue a stop transfer command, and allow the command
> completion to cleanup the cancelled list.  The DWC3 controller will
> run into an end transfer command timeout if there is an ongoing EP0
> transaction.  If this is the case, wait for the EP0 completion event
> before proceeding to retry the endxfer command again.
> 
> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>


You sent this twice?  What is the differences between the patches?

And as you sent it, your signed-off-by needs to be at the end, as per
the kernel documentation.

> ---
>  Patch discussion below:
>    https://lore.kernel.org/linux-usb/1644836933-141376-1-git-send-email-dh10.jung@samsung.com/T/#t

So this is a v2?


> 
>  drivers/usb/dwc3/core.h   |  2 +-
>  drivers/usb/dwc3/ep0.c    | 14 ++++++++++++++
>  drivers/usb/dwc3/gadget.c | 13 ++++++++-----
>  drivers/usb/dwc3/gadget.h |  1 +
>  4 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index eb9c1efced05..f557f5f36a7f 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -736,7 +736,7 @@ struct dwc3_ep {
>  #define DWC3_EP_FIRST_STREAM_PRIMED	BIT(10)
>  #define DWC3_EP_PENDING_CLEAR_STALL	BIT(11)
>  #define DWC3_EP_TXFIFO_RESIZED		BIT(12)
> -
> +#define DWC3_EP_DELAY_STOP             BIT(13)

Why did you loose the blank line?

>  	/* This last one is specific to EP0 */
>  #define DWC3_EP0_DIR_IN			BIT(31)
>  
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 658739410992..1064be5518f6 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -271,6 +271,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>  {
>  	struct dwc3_ep			*dep;
>  	int				ret;
> +	int                             i;
>  
>  	complete(&dwc->ep0_in_setup);
>  
> @@ -279,6 +280,19 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>  			DWC3_TRBCTL_CONTROL_SETUP, false);
>  	ret = dwc3_ep0_start_trans(dep);
>  	WARN_ON(ret < 0);
> +	for (i = 2; i < DWC3_ENDPOINTS_NUM; i++) {
> +		struct dwc3_ep *dwc3_ep;
> +
> +		dwc3_ep = dwc->eps[i];
> +		if (!dwc3_ep)
> +			continue;
> +
> +		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
> +			continue;
> +
> +		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
> +		dwc3_stop_active_transfer(dwc3_ep, true, true);
> +	}
>  }
>  
>  static struct dwc3_ep *dwc3_wIndex_to_dep(struct dwc3 *dwc, __le16 wIndex_le)
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a0c883f19a41..ccef508b1296 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -654,9 +654,6 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
>  	return dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_SETEPCONFIG, &params);
>  }
>  
> -static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> -		bool interrupt);
> -
>  /**
>   * dwc3_gadget_calc_tx_fifo_size - calculates the txfifo size value
>   * @dwc: pointer to the DWC3 context
> @@ -1899,6 +1896,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  	 */
>  	if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
>  	    (dep->flags & DWC3_EP_WEDGE) ||
> +	    (dep->flags & DWC3_EP_DELAY_STOP) ||
>  	    (dep->flags & DWC3_EP_STALL)) {
>  		dep->flags |= DWC3_EP_DELAY_START;
>  		return 0;
> @@ -2033,6 +2031,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  		if (r == req) {
>  			struct dwc3_request *t;
>  
> +			if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status)
> +				dep->flags |= DWC3_EP_DELAY_STOP;
> +
>  			/* wait until it is processed */
>  			dwc3_stop_active_transfer(dep, true, true);
>  
> @@ -2116,7 +2117,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>  		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
>  			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_STALLED);
>  
> -		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
> +		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING ||
> +		    (dep->flags & DWC3_EP_DELAY_STOP)) {
>  			dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
>  			return 0;
>  		}
> @@ -3596,7 +3598,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>  	}
>  }
>  
> -static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> +void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	bool interrupt)

This is a horrid api (2 booleans?)  But you aren't adding it so I guess
we can live with it :(

thanks,

greg k-h
Wesley Cheng March 9, 2022, 7:01 p.m. UTC | #2 
Hi Greg,

On 3/9/2022 2:21 AM, Greg KH wrote:
> On Tue, Mar 08, 2022 at 04:41:48PM -0800, Wesley Cheng wrote:
>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>
>> If the request being dequeued is currently active, then the current
>> logic is to issue a stop transfer command, and allow the command
>> completion to cleanup the cancelled list.  The DWC3 controller will
>> run into an end transfer command timeout if there is an ongoing EP0
>> transaction.  If this is the case, wait for the EP0 completion event
>> before proceeding to retry the endxfer command again.
>>
>> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> 
> You sent this twice?  What is the differences between the patches?
> 
Sorry for sending it twice.  It was a mistake on my end, where I thought
the patch wasn't getting sent out by our email server, so I sent it out
several times to check.  Ended up just showing up after 30 mins or so :/.
> And as you sent it, your signed-off-by needs to be at the end, as per
> the kernel documentation.
> 
I'll fix this.
>> ---
>>  Patch discussion below:
>>    https://lore.kernel.org/linux-usb/1644836933-141376-1-git-send-email-dh10.jung@samsung.com/T/#t
> 
> So this is a v2?
> 
> 
This is an entirely different change, but the change was discussed in
the above thread.
>>
>>  drivers/usb/dwc3/core.h   |  2 +-
>>  drivers/usb/dwc3/ep0.c    | 14 ++++++++++++++
>>  drivers/usb/dwc3/gadget.c | 13 ++++++++-----
>>  drivers/usb/dwc3/gadget.h |  1 +
>>  4 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index eb9c1efced05..f557f5f36a7f 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -736,7 +736,7 @@ struct dwc3_ep {
>>  #define DWC3_EP_FIRST_STREAM_PRIMED	BIT(10)
>>  #define DWC3_EP_PENDING_CLEAR_STALL	BIT(11)
>>  #define DWC3_EP_TXFIFO_RESIZED		BIT(12)
>> -
>> +#define DWC3_EP_DELAY_STOP             BIT(13)
> 
> Why did you loose the blank line?
> 
I can add that back in.

Thanks
Wesley Cheng

>>  	/* This last one is specific to EP0 */
>>  #define DWC3_EP0_DIR_IN			BIT(31)
>>  
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 658739410992..1064be5518f6 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -271,6 +271,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>  {
>>  	struct dwc3_ep			*dep;
>>  	int				ret;
>> +	int                             i;
>>  
>>  	complete(&dwc->ep0_in_setup);
>>  
>> @@ -279,6 +280,19 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>  			DWC3_TRBCTL_CONTROL_SETUP, false);
>>  	ret = dwc3_ep0_start_trans(dep);
>>  	WARN_ON(ret < 0);
>> +	for (i = 2; i < DWC3_ENDPOINTS_NUM; i++) {
>> +		struct dwc3_ep *dwc3_ep;
>> +
>> +		dwc3_ep = dwc->eps[i];
>> +		if (!dwc3_ep)
>> +			continue;
>> +
>> +		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>> +			continue;
>> +
>> +		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>> +		dwc3_stop_active_transfer(dwc3_ep, true, true);
>> +	}
>>  }
>>  
>>  static struct dwc3_ep *dwc3_wIndex_to_dep(struct dwc3 *dwc, __le16 wIndex_le)
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index a0c883f19a41..ccef508b1296 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -654,9 +654,6 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
>>  	return dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_SETEPCONFIG, &params);
>>  }
>>  
>> -static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>> -		bool interrupt);
>> -
>>  /**
>>   * dwc3_gadget_calc_tx_fifo_size - calculates the txfifo size value
>>   * @dwc: pointer to the DWC3 context
>> @@ -1899,6 +1896,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>>  	 */
>>  	if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
>>  	    (dep->flags & DWC3_EP_WEDGE) ||
>> +	    (dep->flags & DWC3_EP_DELAY_STOP) ||
>>  	    (dep->flags & DWC3_EP_STALL)) {
>>  		dep->flags |= DWC3_EP_DELAY_START;
>>  		return 0;
>> @@ -2033,6 +2031,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>>  		if (r == req) {
>>  			struct dwc3_request *t;
>>  
>> +			if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status)
>> +				dep->flags |= DWC3_EP_DELAY_STOP;
>> +
>>  			/* wait until it is processed */
>>  			dwc3_stop_active_transfer(dep, true, true);
>>  
>> @@ -2116,7 +2117,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>>  		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
>>  			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_STALLED);
>>  
>> -		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
>> +		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING ||
>> +		    (dep->flags & DWC3_EP_DELAY_STOP)) {
>>  			dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
>>  			return 0;
>>  		}
>> @@ -3596,7 +3598,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>>  	}
>>  }
>>  
>> -static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>> +void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>  	bool interrupt)
> 
> This is a horrid api (2 booleans?)  But you aren't adding it so I guess
> we can live with it :(
> 
> thanks,
> 
> greg k-h
Thinh Nguyen March 9, 2022, 8:03 p.m. UTC | #3 
Hi,

Wesley Cheng wrote:
> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> If the request being dequeued is currently active, then the current
> logic is to issue a stop transfer command, and allow the command
> completion to cleanup the cancelled list.  The DWC3 controller will
> run into an end transfer command timeout if there is an ongoing EP0
> transaction.  If this is the case, wait for the EP0 completion event
> before proceeding to retry the endxfer command again.

Can you fix the commit message as follow:

If a Setup packet is received but yet to DMA out, the controller will
not process the End Transfer command of any endpoint. Polling of its
DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a
command timeout.

This may occur if the driver doesn’t service the completion interrupt of
the control status stage yet due to system latency, then it won’t
prepare TRB and start the transfer for the next Setup Stage. To the host
side, the control transfer had completed, and the host can send a new
Setup packet at this point.

In the meanwhile, if the driver receives an async call to dequeue a
request (triggering End Transfer) to any endpoint, then the driver will
service that End transfer first, blocking the control status stage
completion handler. Since no TRB is available for the Setup stage, the
Setup packet can’t be DMA’ed out and the End Transfer gets hung.

The driver must not block setting up of the Setup stage. So track and
only issue the End Transfer command only when there’s Setup TRB prepared
so that the controller can DMA out the Setup packet. Delay the End
transfer command until there's no Setup TRB available. This is
applicable to all DWC_usb3x IPs.



> 
> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  Patch discussion below:
>    https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1644836933-141376-1-git-send-email-dh10.jung@samsung.com/T/*t__;Iw!!A4F2R9G_pg!MaN26sw4EKQDdKItFw7sVISIz8Za9Q2o2FrId0OYviaOyTkQRrAVuIYFI-Eb_XkR7SGP$ 
> 
>  drivers/usb/dwc3/core.h   |  2 +-
>  drivers/usb/dwc3/ep0.c    | 14 ++++++++++++++
>  drivers/usb/dwc3/gadget.c | 13 ++++++++-----
>  drivers/usb/dwc3/gadget.h |  1 +
>  4 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index eb9c1efced05..f557f5f36a7f 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -736,7 +736,7 @@ struct dwc3_ep {
>  #define DWC3_EP_FIRST_STREAM_PRIMED	BIT(10)
>  #define DWC3_EP_PENDING_CLEAR_STALL	BIT(11)
>  #define DWC3_EP_TXFIFO_RESIZED		BIT(12)
> -
> +#define DWC3_EP_DELAY_STOP             BIT(13)
>  	/* This last one is specific to EP0 */
>  #define DWC3_EP0_DIR_IN			BIT(31)
>  
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 658739410992..1064be5518f6 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -271,6 +271,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>  {
>  	struct dwc3_ep			*dep;
>  	int				ret;
> +	int                             i;
>  
>  	complete(&dwc->ep0_in_setup);
>  
> @@ -279,6 +280,19 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>  			DWC3_TRBCTL_CONTROL_SETUP, false);
>  	ret = dwc3_ep0_start_trans(dep);
>  	WARN_ON(ret < 0);
> +	for (i = 2; i < DWC3_ENDPOINTS_NUM; i++) {
> +		struct dwc3_ep *dwc3_ep;
> +
> +		dwc3_ep = dwc->eps[i];
> +		if (!dwc3_ep)
> +			continue;
> +
> +		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
> +			continue;
> +
> +		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
> +		dwc3_stop_active_transfer(dwc3_ep, true, true);
> +	}
>  }
>  
>  static struct dwc3_ep *dwc3_wIndex_to_dep(struct dwc3 *dwc, __le16 wIndex_le)
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a0c883f19a41..ccef508b1296 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -654,9 +654,6 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
>  	return dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_SETEPCONFIG, &params);
>  }
>  
> -static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> -		bool interrupt);
> -
>  /**
>   * dwc3_gadget_calc_tx_fifo_size - calculates the txfifo size value
>   * @dwc: pointer to the DWC3 context
> @@ -1899,6 +1896,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  	 */
>  	if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
>  	    (dep->flags & DWC3_EP_WEDGE) ||
> +	    (dep->flags & DWC3_EP_DELAY_STOP) ||
>  	    (dep->flags & DWC3_EP_STALL)) {
>  		dep->flags |= DWC3_EP_DELAY_START;
>  		return 0;
> @@ -2033,6 +2031,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  		if (r == req) {
>  			struct dwc3_request *t;
>  

Add a comment here:

If a Setup packet is received but yet to DMA out, the controller will
not process the End Transfer command of any endpoint. Polling of its
DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a
timeout. Delay issuing the End Transfer command until the Setup TRB is
prepared.

> +			if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status)
> +				dep->flags |= DWC3_EP_DELAY_STOP;
> +
>  			/* wait until it is processed */
>  			dwc3_stop_active_transfer(dep, true, true);
>  
> @@ -2116,7 +2117,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>  		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
>  			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_STALLED);
>  
> -		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
> +		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING ||
> +		    (dep->flags & DWC3_EP_DELAY_STOP)) {
>  			dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
>  			return 0;
>  		}
> @@ -3596,7 +3598,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>  	}
>  }
>  
> -static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> +void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	bool interrupt)
>  {
>  	struct dwc3_gadget_ep_cmd_params params;
> @@ -3604,6 +3606,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	int ret;
>  
>  	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
> +		(dep->flags & DWC3_EP_DELAY_STOP) ||

minor nit: can you fix alignment.

>  	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>  		return;
>  
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 77df4b6d6c13..f763380e672e 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -116,6 +116,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>  		gfp_t gfp_flags);
>  int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol);
>  void dwc3_ep0_send_delayed_status(struct dwc3 *dwc);
> +void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt);
>  
>  /**
>   * dwc3_gadget_ep_get_transfer_index - Gets transfer index from HW

Thanks,
Thinh
Thinh Nguyen March 9, 2022, 8:11 p.m. UTC | #4 
Thinh Nguyen wrote:
> Hi,
> 
> Wesley Cheng wrote:
>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>
>> If the request being dequeued is currently active, then the current
>> logic is to issue a stop transfer command, and allow the command
>> completion to cleanup the cancelled list.  The DWC3 controller will
>> run into an end transfer command timeout if there is an ongoing EP0
>> transaction.  If this is the case, wait for the EP0 completion event
>> before proceeding to retry the endxfer command again.
> 
> Can you fix the commit message as follow:
> 
> If a Setup packet is received but yet to DMA out, the controller will
> not process the End Transfer command of any endpoint. Polling of its
> DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a
> command timeout.
> 
> This may occur if the driver doesn’t service the completion interrupt of
> the control status stage yet due to system latency, then it won’t
> prepare TRB and start the transfer for the next Setup Stage. To the host
> side, the control transfer had completed, and the host can send a new
> Setup packet at this point.
> 
> In the meanwhile, if the driver receives an async call to dequeue a
> request (triggering End Transfer) to any endpoint, then the driver will
> service that End transfer first, blocking the control status stage
> completion handler. Since no TRB is available for the Setup stage, the
> Setup packet can’t be DMA’ed out and the End Transfer gets hung.
> 
> The driver must not block setting up of the Setup stage. So track and
> only issue the End Transfer command only when there’s Setup TRB prepared
> so that the controller can DMA out the Setup packet. Delay the End
> transfer command until there's no Setup TRB available. This is

typo:
Delay the End transfer command *if* there's no Setup TRB available.


> applicable to all DWC_usb3x IPs.
>
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index eb9c1efced05..f557f5f36a7f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -736,7 +736,7 @@  struct dwc3_ep {
 #define DWC3_EP_FIRST_STREAM_PRIMED	BIT(10)
 #define DWC3_EP_PENDING_CLEAR_STALL	BIT(11)
 #define DWC3_EP_TXFIFO_RESIZED		BIT(12)
-
+#define DWC3_EP_DELAY_STOP             BIT(13)
 	/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN			BIT(31)
 
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 658739410992..1064be5518f6 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -271,6 +271,7 @@  void dwc3_ep0_out_start(struct dwc3 *dwc)
 {
 	struct dwc3_ep			*dep;
 	int				ret;
+	int                             i;
 
 	complete(&dwc->ep0_in_setup);
 
@@ -279,6 +280,19 @@  void dwc3_ep0_out_start(struct dwc3 *dwc)
 			DWC3_TRBCTL_CONTROL_SETUP, false);
 	ret = dwc3_ep0_start_trans(dep);
 	WARN_ON(ret < 0);
+	for (i = 2; i < DWC3_ENDPOINTS_NUM; i++) {
+		struct dwc3_ep *dwc3_ep;
+
+		dwc3_ep = dwc->eps[i];
+		if (!dwc3_ep)
+			continue;
+
+		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
+			continue;
+
+		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
+		dwc3_stop_active_transfer(dwc3_ep, true, true);
+	}
 }
 
 static struct dwc3_ep *dwc3_wIndex_to_dep(struct dwc3 *dwc, __le16 wIndex_le)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a0c883f19a41..ccef508b1296 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -654,9 +654,6 @@  static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
 	return dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_SETEPCONFIG, &params);
 }
 
-static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
-		bool interrupt);
-
 /**
  * dwc3_gadget_calc_tx_fifo_size - calculates the txfifo size value
  * @dwc: pointer to the DWC3 context
@@ -1899,6 +1896,7 @@  static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 	 */
 	if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
 	    (dep->flags & DWC3_EP_WEDGE) ||
+	    (dep->flags & DWC3_EP_DELAY_STOP) ||
 	    (dep->flags & DWC3_EP_STALL)) {
 		dep->flags |= DWC3_EP_DELAY_START;
 		return 0;
@@ -2033,6 +2031,9 @@  static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 		if (r == req) {
 			struct dwc3_request *t;
 
+			if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status)
+				dep->flags |= DWC3_EP_DELAY_STOP;
+
 			/* wait until it is processed */
 			dwc3_stop_active_transfer(dep, true, true);
 
@@ -2116,7 +2117,8 @@  int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
 		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
 			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_STALLED);
 
-		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
+		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING ||
+		    (dep->flags & DWC3_EP_DELAY_STOP)) {
 			dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
 			return 0;
 		}
@@ -3596,7 +3598,7 @@  static void dwc3_reset_gadget(struct dwc3 *dwc)
 	}
 }
 
-static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
+void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	bool interrupt)
 {
 	struct dwc3_gadget_ep_cmd_params params;
@@ -3604,6 +3606,7 @@  static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	int ret;
 
 	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
+		(dep->flags & DWC3_EP_DELAY_STOP) ||
 	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
 		return;
 
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 77df4b6d6c13..f763380e672e 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -116,6 +116,7 @@  int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
 		gfp_t gfp_flags);
 int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol);
 void dwc3_ep0_send_delayed_status(struct dwc3 *dwc);
+void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt);
 
 /**
  * dwc3_gadget_ep_get_transfer_index - Gets transfer index from HW