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

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

On Thu, Apr 10, 2025, Frode Isaksen wrote:
> 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://urldefense.com/v3/__https://docs.kernel.org/driver-api/usb/dwc3.html*reporting-bugs__;Iw!!A4F2R9G_pg!a99eDyoLRAn8bMgn6L9hZr1e0Dv214xF-wMGKgw8j-n78id3zRmMhpfG_0sNYa9gHgqgOrtqpb2r6WPO3eR42lZL$
> > 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.
> 

Ok... it's unfortunate that we can't reproduce this.

Base on your change in __dwc3_gadget_start for this:
+	dwc->ep0_usb_req.status = DWC3_REQUEST_STATUS_UNKNOWN;

I suspect the problem occurs during bind/unbind where
soft-disconnect/connect occurs. We have a lot of fixes to dwc3 since the
kernel 5.10 related to soft-disconnect handling that may have resolved
the issue you reported.

We should not just override the request status and ignore the ep0 queue.
The control transfer state from the driver may not be matching with the
controller, things may already be broken here.

If you run into this issue again, we can take a look and properly
resolve it. But I don't think we should handle it this way.

Thanks,
Thinh