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

John Stultz wrote:

On Mon, Aug 9, 2021 at 3:44 PM Thinh Nguyen [off-list ref] wrote:

quoted

John Stultz wrote:

quoted

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 <redacted>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Jack Pham <redacted>
Cc: Thinh Nguyen <thinh.nguyen@synopsys.com>
Cc: Todd Kjos <redacted>
Cc: Amit Pundir <redacted>
Cc: YongQin Liu <yongqin.liu@linaro.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Petri Gynther <redacted>
Cc: linux-usb@vger.kernel.org
Fixes: d25d85061bd8 ("usb: dwc3: gadget: Use list_replace_init() before traversing lists")
Signed-off-by: John Stultz <redacted>
---
 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.

Ok, sorry, I didn't read your mail in depth until I had this patch
sent out. If a revert of d25d85061bd8 is the better fix, I'm fine with
that too.

thanks
-john

IMO, we should revert this patch for now since it will cause regression.
We can review and test a proper fix at a later time.

Thanks,
Thinh