Messages in this thread | | | From | Thinh Nguyen <> | Subject | Re: [RFC][PATCH] dwc3: gadget: Fix losing list items in dwc3_gadget_ep_cleanup_completed_requests() | Date | Mon, 9 Aug 2021 22:57:27 +0000 |
| |
John Stultz wrote: > On Mon, Aug 9, 2021 at 3:44 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: >> >> 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. > > 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
| |