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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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.
> I was not able to reproduce this bug locally in any version.
> > 
> > > 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
> > > 
> > Cherry-picking the upstream patch like this will be difficult to debug
> > as I can't determine the corresponding changes related to the usb gadget
> > core API and the dwc3.
> 
> 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

Thanks,
Thinh