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

[Frode Isaksen <fisaksen@baylibre.com>]

On 4/10/25 2:09 AM, Thinh Nguyen wrote:
> On Wed, Apr 09, 2025, Thinh Nguyen wrote:
>> On Tue, Apr 08, 2025, Frode Isaksen wrote:
>>> On 4/8/25 1:44 AM, Thinh Nguyen wrote:
>>>> On Thu, Apr 03, 2025, Frode Isaksen wrote:
>>>>> 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:
>>>> What causes the crash? I'm still not clear whether you were able to
>>>> reproduced this on mainline, or any of the new stable tree.
> <snip>
>
>>> I was not able to reproduce this bug locally in any version.
> Sorry, I totally missed this response...
>
> <snip>
>
>>> There were first a WARNING:
>>>
>>> <4>[ 341.860109] WARNING: CPU: 0 PID: 8548 at drivers/usb/dwc3/ep0.c:1077
>>> dwc3_ep0_interrupt+0x1e8/0x340
>>>
>>> Here:
>>>
>>> static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep
>>> *dep) { WARN_ON(dwc3_ep0_start_control_status(dep)); } and then the crash:
>>>
>>> <3>[ 351.674418] list_add double add: new=ffffff87ab2c7950,
>>> prev=ffffff87ab2c7950, next=ffffff87ab485b60.
>>>
>>> <6>[ 351.674437] ------------[ cut here ]------------
>>>
>>> <2>[ 351.674439] kernel BUG at lib/list_debug.c:35!
>>>
>>> <0>[ 351.674447] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>>
>>> <6>[ 351.675047] Call trace:
>>>
>>> <6>[ 351.675052] __list_add_valid+0x70/0xc0
>>>
>>> <6>[ 351.675060] __dwc3_gadget_ep0_queue+0x70/0x224
>>>
>>> <6>[ 351.675064] dwc3_ep0_handle_status+0x118/0x200
>>>
>>> <6>[ 351.675068] dwc3_ep0_inspect_setup+0x144/0x32c
>>>
>>> <6>[ 351.675073] dwc3_ep0_interrupt+0xac/0x340
>>>
>>> <6>[ 351.675078] dwc3_process_event_entry+0x90/0x724
>>>
>>> <6>[ 351.675082] dwc3_process_event_buf+0x7c/0x33c
>>>
>>> <6>[ 351.675086] dwc3_thread_interrupt+0x58/0x8c
>>>
>>> <6>[ 351.675092] irq_thread_fn+0x54/0xd4
>>>
>>> This is all I have..
>>>
>> Hm... which kernel version was this reproduced?
>>
>> Are you using the usb composite framework? The control transfers are
>> driven by the host, and the usb gadget composite framework should not
>> hit scenarios like this.
>>
>> Can you capture the tracepoints* for dwc3?
>> [*] https://docs.kernel.org/driver-api/usb/dwc3.html#reporting-bugs
> Ignore this request since you can't reproduce this bug. But can you help
> answer the question above about whether you used the usb composite
> framework?
>
> Thanks,
> Thinh

Yes, I thinks it's using the composite framework, as there are two 
interfaces XRSP and ADB.

Thanks,

Frode