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

[Wesley Cheng <wcheng@codeaurora.org>]

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@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
> 

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://lore.kernel.org/linux-usb/1620716636-12422-1-git-send-email-wcheng@codeaurora.org/

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project