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

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

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.

> 
> > 
> > > +				"%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.

Thanks,
Thinh