Re: [PATCH] usb: dwc3: gadget: Fix logical condition

[Jack Pham <jackp@codeaurora.org>]

Hi Tejas & Felipe,

On Wed, Nov 13, 2019 at 11:45:16AM +0530, Tejas Joglekar wrote:
> This patch corrects the condition to kick the transfer without
> giving back the requests when either request has remaining data
> or when there are pending SGs. The && check was introduced during
> spliting up the dwc3_gadget_ep_cleanup_completed_requests() function.
> 
> Fixes: f38e35dd84e2 ("usb: dwc3: gadget: split dwc3_gadget_ep_cleanup_completed_requests()")
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Tejas Joglekar <joglekar@synopsys.com>
> ---
>  drivers/usb/dwc3/gadget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 86dc1db788a9..e07159e06f9a 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2485,7 +2485,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>  
>  	req->request.actual = req->request.length - req->remaining;
>  
> -	if (!dwc3_gadget_ep_request_completed(req) &&
> +	if (!dwc3_gadget_ep_request_completed(req) ||
>  			req->num_pending_sgs) {
>  		__dwc3_gadget_kick_transfer(dep);
>  		goto out;

Been staring at this for a while--I think I see a potential issue but
not sure if it is or not.

If this condition is true and causes an early return, the 'ret' value
could be 0 which could allow the caller in cleanup_completed_requests()
to continue looping over the started_list and calling
cleanup_completed_request() again on the next req. But we just issued
another START or UPDATE transfer command on the previous incomplete req
and now the loop continued to try to reclaim the next TRB (and increment
the dequeue pointer and whatnot) when it might actually be in progress.

According to the code before f38e35dd84e2,

	list_for_each_entry_safe(req, tmp, &dep->started_list, list) {

	...
		if (!dwc3_gadget_ep_request_completed(req) ||
				req->num_pending_sgs) {
			__dwc3_gadget_kick_transfer(dep);
			break;
		}

The 'goto out' used to be a 'break', which terminates the list loop. But
with the refactored code, the loop can only terminate if 'ret' is
non-zero.

I haven't seen any real issue with the code as-is yet, but was just
wondering if the 'goto out' should be replaced with a return 1?

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