Re: [PATCH] usb: dwc3: ep0: prevent dwc3_request from being queued twice

[Frode Isaksen <fisaksen@baylibre.com>]

On 4/3/25 12:21 AM, Thinh Nguyen wrote:
> On Wed, Apr 02, 2025, Frode Isaksen wrote:
>> On 4/2/25 1:49 AM, Thinh Nguyen wrote:
>>> On Thu, Mar 27, 2025, Frode Isaksen wrote:
>>>> From: Frode Isaksen <frode@meta.com>
>>>>
>>>> Prevent dwc3_request from being queued twice, by checking
>>>> req->status.
>>>> Similar to commit b2b6d601365a ("usb: dwc3: gadget: prevent
>>>> dwc3_request from being queued twice") for non-ep0 endpoints.
>>>> Crash log:
>>>> list_add double add: new=ffffff87ab2c7950, prev=ffffff87ab2c7950,
>>>>    next=ffffff87ab485b60.
>>>> kernel BUG at lib/list_debug.c:35!
>>>> Call trace:
>>>> __list_add_valid+0x70/0xc0
>>>> __dwc3_gadget_ep0_queue+0x70/0x224
>>>> dwc3_ep0_handle_status+0x118/0x200
>>>> dwc3_ep0_inspect_setup+0x144/0x32c
>>>> dwc3_ep0_interrupt+0xac/0x340
>>>> dwc3_process_event_entry+0x90/0x724
>>>> dwc3_process_event_buf+0x7c/0x33c
>>>> dwc3_thread_interrupt+0x58/0x8c
>>>>
>>>> Signed-off-by: Frode Isaksen <frode@meta.com>
>>>> ---
>>>> This bug was discovered, tested and fixed (no more crashes seen) on
>>>> Meta Quest 3 device. Also tested on T.I. AM62x board.
>>>>
>>>>    drivers/usb/dwc3/ep0.c    | 5 +++++
>>>>    drivers/usb/dwc3/gadget.c | 1 +
>>>>    2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>> index 666ac432f52d..e26c3a62d470 100644
>>>> --- a/drivers/usb/dwc3/ep0.c
>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>> @@ -91,6 +91,11 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
>>>>    {
>>>>    	struct dwc3		*dwc = dep->dwc;
>>>> +	if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED,
>>> Let's not use WARN. Perhaps dev_warn?
>> I copied the coding style from commit b2b6d601365a ("usb: dwc3: gadget:
>> prevent
>>
>> dwc3_request from being queued twice"), so maybe a follow-up patch to change from WARN to dev_warn for the two patches ?
> We can just use dev_warn here, we don't need 2 separate patches for this
> change. The other place can be reworked in a separate patch.
OK
>
>>>> +				"%s: request %pK already in flight\n",
>>>> +				dep->name, &req->request))
>>>> +		return -EINVAL;
>>>> +
>>>>    	req->request.actual	= 0;
>>>>    	req->request.status	= -EINPROGRESS;
>>>>    	req->epnum		= dep->number;
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 89a4dc8ebf94..c34446d8c54f 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -3002,6 +3002,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
>>>>    	dwc->ep0_bounced = false;
>>>>    	dwc->link_state = DWC3_LINK_STATE_SS_DIS;
>>>>    	dwc->delayed_status = false;
>>>> +	dwc->ep0_usb_req.status = DWC3_REQUEST_STATUS_UNKNOWN;
>>>>    	dwc3_ep0_out_start(dwc);
>>>>    	dwc3_gadget_enable_irq(dwc);
>>>> -- 
>>>> 2.48.1
>>>>
>>> I'm still not clear how this can happen. Are you testing this against
>>> mainline? Can you provide the dwc3 tracepoints?
>> I was not able to reproduce this locally. Was seen on 5.10, tested on 6.1
>> and 6.6.
>>
> Without understanding how this can happen and why it is needed, we
> should not just apply this. Kernel v5.10, v6.1, and v6.6 are really old.
> We have a lot of fixes since then. Please see if this is reproducible
> using mainline.

We have applied all relevant patches from mainline to ep0.c, in order to 
try to fix this crash:

566bc793bdd7 usb: dwc3: ep0: Don't clear ep0 DWC3_EP_TRANSFER_STARTED
371d3fc577db usb: dwc3: ep0: Don't reset resource alloc flag (including ep0)
d819a0bbb493 usb: dwc3: ep0: Don't reset resource alloc flag
2eb3ba38bf88 usb: dwc3: gadget: Rewrite endpoint allocation flow
cbfcf517dc42 FROMGIT: usb: dwc: ep0: Update request status in 
dwc3_ep0_stall_restart
4fc39328579e UPSTREAM: usb: dwc3: Avoid unmapping USB requests if 
endxfer is not complete
5dc06419d8a6 UPSTREAM: usb: dwc3: Do not service EP0 and conndone events 
if soft disconnected
75a4f0b5e1f4 UPSTREAM: usb: dwc3: ep0: Properly handle 
setup_packet_pending scenario in data stage
a79e848e5299 UPSTREAM: usb: dwc3: ep0: Don't prepare beyond Setup stage
910e9e60492a UPSTREAM: usb: dwc3: Fix ep0 handling when getting reset 
while doing control transfer
fe513e1c2685 UPSTREAM: usb: dwc3: gadget: Wait for ep0 xfers to complete 
during dequeueThan

Thanks,

Frode

>
> Thanks,
> Thinh