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

[Thinh Nguyen]

John Stultz wrote:
> On Mon, Aug 9, 2021 at 3:44 PM Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>>> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>>> Cc: Jack Pham <jackp@xxxxxxxxxxxxxx>
>>> Cc: Thinh Nguyen <thinh.nguyen@xxxxxxxxxxxx>
>>> Cc: Todd Kjos <tkjos@xxxxxxxxxx>
>>> Cc: Amit Pundir <amit.pundir@xxxxxxxxxx>
>>> Cc: YongQin Liu <yongqin.liu@xxxxxxxxxx>
>>> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
>>> Cc: Petri Gynther <pgynther@xxxxxxxxxx>
>>> Cc: linux-usb@xxxxxxxxxxxxxxx
>>> Fixes: d25d85061bd8 ("usb: dwc3: gadget: Use list_replace_init() before traversing lists")
>>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>>> ---
>>>  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