Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 9 Mar 2022 11:21:41 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Wesley Cheng <quic_wcheng@quicinc.com>
Cc:     balbi@kernel.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, quic_jackp@quicinc.com,
        Thinh.Nguyen@synopsys.com
Subject: Re: [PATCH] usb: dwc3: gadget: Wait for ep0 xfers to complete during
 dequeue
Message-ID: <Yih/tapu/JMRgBqT@kroah.com>
References: <20220309004148.12061-1-quic_wcheng@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220309004148.12061-1-quic_wcheng@quicinc.com>
Precedence: bulk

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