From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
Wesley Cheng <wcheng@codeaurora.org>,
John Stultz <john.stultz@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
Felipe Balbi <balbi@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Alan Stern <stern@rowland.harvard.edu>,
Jack Pham <jackp@codeaurora.org>, Todd Kjos <tkjos@google.com>,
Amit Pundir <amit.pundir@linaro.org>,
YongQin Liu <yongqin.liu@linaro.org>,
Sumit Semwal <sumit.semwal@linaro.org>,
Petri Gynther <pgynther@google.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: Re: [RFC][PATCH] dwc3: gadget: Fix losing list items in dwc3_gadget_ep_cleanup_completed_requests()
Date: Tue, 10 Aug 2021 23:40:02 +0000 [thread overview]
Message-ID: <89bb9ef4-f85e-5ccf-9288-780efec72f5c@synopsys.com> (raw)
In-Reply-To: <915337ae-07d4-6a19-ec98-a8b88c5d9743@synopsys.com>
Hi Wesley,
Thinh Nguyen wrote:
> 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@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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1620716636-12422-1-git-send-email-wcheng@codeaurora.org/__;!!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.
>>
Can you try this?
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 706246d93a00..17b2d8d4efb4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2029,6 +2029,13 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
dwc3_gadget_giveback(dep, req, -ECONNRESET);
break;
}
+
+ /*
+ * The endpoint is disabled, let the dwc3_remove_requests()
+ * handle the cleanup.
+ */
+ if (!dep->endpoint.desc)
+ break;
}
}
@@ -3402,6 +3409,13 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
req, status);
if (ret)
break;
+
+ /*
+ * The endpoint is disabled, let the dwc3_remove_requests()
+ * handle the cleanup.
+ */
+ if (!dep->endpoint.desc)
+ break;
}
}
If needed, you can also use your change while(!list_empty(started_list)) along with this for future proof.
BR,
Thinh
next prev parent reply other threads:[~2021-08-10 23:40 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-29 7:33 [PATCH] usb: dwc3: gadget: Use list_replace_init() before traversing lists Wesley Cheng
2021-07-29 8:09 ` Felipe Balbi
2021-07-29 8:45 ` Wesley Cheng
2021-07-29 9:31 ` Felipe Balbi
2021-07-29 14:20 ` Alan Stern
2021-08-09 21:04 ` John Stultz
2021-08-09 22:31 ` [RFC][PATCH] dwc3: gadget: Fix losing list items in dwc3_gadget_ep_cleanup_completed_requests() John Stultz
2021-08-09 22:44 ` Thinh Nguyen
2021-08-09 22:53 ` John Stultz
2021-08-09 22:57 ` Thinh Nguyen
2021-08-10 6:05 ` Greg Kroah-Hartman
2021-08-10 7:11 ` Greg Kroah-Hartman
2021-08-10 17:11 ` Wesley Cheng
2021-08-10 20:14 ` Thinh Nguyen
2021-08-10 20:17 ` Thinh Nguyen
2021-08-10 23:40 ` Thinh Nguyen [this message]
2021-08-09 21:26 ` [PATCH] usb: dwc3: gadget: Use list_replace_init() before traversing lists John Stultz
2021-08-09 22:07 ` Thinh Nguyen
2021-08-10 3:12 ` Ray Chi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=89bb9ef4-f85e-5ccf-9288-780efec72f5c@synopsys.com \
--to=thinh.nguyen@synopsys.com \
--cc=amit.pundir@linaro.org \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jackp@codeaurora.org \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=pgynther@google.com \
--cc=stern@rowland.harvard.edu \
--cc=sumit.semwal@linaro.org \
--cc=tkjos@google.com \
--cc=wcheng@codeaurora.org \
--cc=yongqin.liu@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).