Re: [RFC][PATCH] dwc3: gadget: Fix losing list items in dwc3_gadget_ep_cleanup_completed_requests()

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

John Stultz wrote:
> In commit d25d85061bd8 ("usb: dwc3: gadget: Use
> list_replace_init() before traversing lists"), a local list_head
> was introduced to process the started_list items to avoid races.
> 
> However, in dwc3_gadget_ep_cleanup_completed_requests() if
> dwc3_gadget_ep_cleanup_completed_request() fails, we break early,
> causing the items on the local list_head to be lost.
> 
> This issue showed up as problems on the db845c/RB3 board, where
> adb connetions would fail, showing the device as "offline".
> 
> This patch tries to fix the issue by if we are returning early
> we splice in the local list head back into the started_list
> and return (avoiding an infinite loop, as the started_list is
> now non-null).
> 
> Not sure if this is fully correct, but seems to work for me so I
> wanted to share for feedback.
> 
> Cc: Wesley Cheng <wcheng@codeaurora.org>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Jack Pham <jackp@codeaurora.org>
> Cc: Thinh Nguyen <thinh.nguyen@synopsys.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Amit Pundir <amit.pundir@linaro.org>
> Cc: YongQin Liu <yongqin.liu@linaro.org>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Petri Gynther <pgynther@google.com>
> Cc: linux-usb@vger.kernel.org
> Fixes: d25d85061bd8 ("usb: dwc3: gadget: Use list_replace_init() before traversing lists")
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/usb/dwc3/gadget.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index b8d4b2d327b23..a73ebe8e75024 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2990,6 +2990,12 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
>  			break;
>  	}
>  
> +	if (!list_empty(&local)) {
> +		list_splice_tail(&local, &dep->started_list);
> +		/* Return so we don't hit the restart case and loop forever */
> +		return;
> +	}
> +
>  	if (!list_empty(&dep->started_list))
>  		goto restart;
>  }
> 

No, we should revert the change for
dwc3_gadget_ep_cleaup_completed_requests(). As I mentioned previously,
we don't cleanup the entire started_list. If the original problem is due
to disconnection in the middle of request completion, then we can just
check for pullup_connected and exit the loop and let the
dwc3_remove_requests() do the cleanup.

BR,
Thinh