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

[Thinh Nguyen]

Thinh Nguyen wrote:
> Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 8/9/2021 3:57 PM, Thinh Nguyen wrote:
>>> 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
>>>
>>
>> Another suggestion would just be to replace the loop with a while() loop
>> and using list_entry() instead.  That was what was discussed in the
>> earlier patch series which also addresses the problem as well.  Issue
>> here is the tmp variable still carries a stale request after the dwc3
>> giveback is called.  We can avoid that by always fetching the
>> list_entry() instead of relying on list_for_each_safe()
>>
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1620716636-12422-1-git-send-email-wcheng@xxxxxxxxxxxxxx/__;!!A4F2R9G_pg!P0E1pv3C0PStDepKyy8iqKgUaOhDy0ZDhYdz-_cZwnJRQjNjvw0MdJQCdU6Xwnt3YAs_$ 
>>
> 
> This should work, but the awkward thing is 2 loops from 2 separate
> threads competing to remove/giveback the requests and may report mix status.
> 

It's fine with me.

BR,
Thinh