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;
Sender: wcheng=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org B0D4FC4338F
Message-ID: <d8eb4c9d-3c68-b7f0-2dd6-600a542a64d1@codeaurora.org>
Date:   Wed, 9 Mar 2022 11:01:14 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH] usb: dwc3: gadget: Wait for ep0 xfers to complete during
 dequeue
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>,
        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
References: <20220309004148.12061-1-quic_wcheng@quicinc.com>
 <Yih/tapu/JMRgBqT@kroah.com>
From:   Wesley Cheng <wcheng@codeaurora.org>
In-Reply-To: <Yih/tapu/JMRgBqT@kroah.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project