Messages in this thread | | | Subject | Re: [PATCH] usb: dwc3: gadget: Replace list_for_each_entry_safe() if using giveback | From | Wesley Cheng <> | Date | Sat, 8 May 2021 01:15:49 -0700 |
| |
On 5/7/2021 8:45 PM, Peter Chen wrote: > On 21-05-07 11:42:03, Wesley Cheng wrote: >> The list_for_each_entry_safe() macro saves the current item (n) and >> the item after (n+1), so that n can be safely removed without >> corrupting the list. However, when traversing the list and removing >> items using gadget giveback, the DWC3 lock is briefly released,
Hi Peter,
Thanks for the review.
> > I see dwc3_gadget_del_and_unmap_request remove the list, the lock is > still held there. Am I something wrong? >
The scenario the issue happens in is say the follow thread is running the sequence below:
Thread#1: __dwc3_gadget_ep_set_halt() - CLEAR HALT -> dwc3_gadget_ep_cleanup_cancelled_requests() ->list_for_each_entry_safe() ->dwc3_gadget_giveback() ->dwc3_gadget_del_and_unmap_request()-n deleted cancelled_list ->spin_unlock
Thread#2: dwc3_gadget_pullup() ->waiting for dwc3 spin_lock ->Thread#1 released lock ->dwc3_stop_active_transfers() ->dwc3_remove_requests() ->fetches n+1 item from cancelled_list (n removed by thread#1) ->dwc3_gadget_giveback() ->dwc3_gadget_del_and_unmap_request()-n+1 deleted cancelled_list ->spin_unlock
So now, if thread#1 takes the DWC3 lock again, it will continue to item n+1, which was already removed by thread#2, leading to a double list removal. We saw this issue on our platform after enabling list debug.
Thanks Wesley Cheng
> Peter > >> allowing other routines to execute. There is a situation where while >> items are being removed from the cancelled_list using >> dwc3_gadget_ep_cleanup_cancelled_requests(), the pullup disable >> routine is running in parallel (due to UDC unbind). As the cleanup >> routine removes n, and the pullup disable removes n+1, once the >> cleanup retakes the DWC3 lock, it references a request who was already >> removed/handled. With list debug enabled, this leads to a panic. >> Ensure all instances of the macro are replaced where gadget giveback >> is used. >> >> Fixes: d4f1afe5e896 ("usb: dwc3: gadget: move requests to cancelled_list") >> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >> --- >> drivers/usb/dwc3/gadget.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index dd80e5c..efa939b 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1737,10 +1737,10 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *r >> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep) >> { >> struct dwc3_request *req; >> - struct dwc3_request *tmp; >> struct dwc3 *dwc = dep->dwc; >> >> - list_for_each_entry_safe(req, tmp, &dep->cancelled_list, list) { >> + while (!list_empty(&dep->cancelled_list)) { >> + req = next_request(&dep->cancelled_list); >> dwc3_gadget_ep_skip_trbs(dep, req); >> switch (req->status) { >> case DWC3_REQUEST_STATUS_DISCONNECTED: >> @@ -2935,11 +2935,11 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep, >> const struct dwc3_event_depevt *event, int status) >> { >> struct dwc3_request *req; >> - struct dwc3_request *tmp; >> >> - list_for_each_entry_safe(req, tmp, &dep->started_list, list) { >> + while (!list_empty(&dep->started_list)) { >> int ret; >> >> + req = next_request(&dep->started_list); >> ret = dwc3_gadget_ep_cleanup_completed_request(dep, event, >> req, status); >> if (ret) >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >> >
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
| |