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

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

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.

> 
> 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.

BR,
Thinh