Re: [PATCH v2] usb: dwc3: gadget: Replace list_for_each_entry_safe() if using giveback

From: Felipe Balbi
Date: Wed Jul 28 2021 - 01:54:01 EST

Hi Wesley,

Wesley Cheng <wcheng@xxxxxxxxxxxxxx> writes:
>>>>> Sorry for the late response. So I tried with a list_replace_init() to
>>>>> within the list_for_each_entry_safe() loop to update tmp w/ the
>>>>> cancelled_list list head, but the issue was still observed. This is
>>>>> because we can't replace the reference the loop already has stored in
>>>>> tmp, which is simply updated as the current item on the next iteration.
>>>>> I believe this is what you were trying to achieve?
>>>> Was wondering if you had any further inputs on this change? As
>>>> mentioned, I tried a few things with list_replace_init(), which did not
>>>> work.
>>> Sorry for the ping. Is this change OK to add as is? We've been running
>>> into this instance pretty frequently during our testing, so just wanted
>>> to close on the proper changes being merged upstream.
>> The idea is this:
>> struct list_head local;
>> spin_lock_irq(&lock);
>> list_replace_init(&dwc->cancelled_list, &local);
>> spin_unlock_irq(&lock);
>> list_for_each_entry_safe(req, tmp, &local, list) {
>> /* ... */
>> }
>> It looks to me this should work fine, no? You can also follow what
>> drivers/usb/core/hcd.c is doing in usb_giveback_urb_bh() and restarting
>> if dwc->cancelled_list is not empty after list_for_each_entry_safe().
>> Can you give that one a shot?
> Great, thanks for this suggestion! Now I understand what you were
> referring to. I gave this a try and it works well. Will prepare a
> change to replace both places with list_replace_init()

this is great news :-) Thanks for trying it out