Re: Regression in f_fastboot due to commit 6a92e9827650 ("usb: ci_udc: Check ci_ep->desc before use")

["Lothar Waßmann" <LW@KARO-electronics.de>]

Hi,

On Fri, 10 Jul 2026 21:27:48 +0000 Petr Beneš wrote:
> Hello Lothar and Mattijs,
> 
> I hope web outlook doesn't render my response unreadable.
> ________________________________________
> From: Mattijs Korpershoek <mkorpershoek@kernel.org>
> Sent: Thursday, July 9, 2026 17:15
> To: Lothar Waßmann; u-boot@lists.denx.de
> Cc: Petr Beneš
> Subject: Re: Regression in f_fastboot due to commit 6a92e9827650 ("usb: ci_udc: Check ci_ep->desc before use")
> 
> Hi Lothar,
> 
> On Wed, Jul 08, 2026 at 07:12, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> 
> > Hi,
> >
> > commit 6a92e9827650 ("usb: ci_udc: Check ci_ep->desc before use") has
> > the side effect that an empty buffer is being passed on to
> > rx_handler_command() in drivers/usb/gadget/f_fastboot.c upon
> > termination of a fastboot session which leads to a
> > 'command  not recognized' error message being printed on the console.  
> 
> Thank you for reporting this.
> 
> >
> > Obviously without the commit the offending USB request had been silently
> > dropped but is now delivered to the upper levels.  
> 
> Well, it was not silently dropped in case of the ethernet gadget, it was silently corrupting memory.
> 
> >
> > I'm not sure what the correct fix would be.
> > Should the fastboot driver ignore the empty command buffer or should
> > the "Moreover, the patch gets rid of possible outstanding requests
> > if the endpoint's state changes to disabled." part of the commit be
> > reverted?  
> 
> Hmm, maybe I got it wrong. The endpoint got disabled, so what should happen with requests?
> 
> linux/usb/gadget.h reads:
> 
>  * @complete: Function called when request completes, so this request and
>  *      its buffer may be re-used.
>  *      Reads terminate with a short packet, or when the buffer fills,
>  *      whichever comes first.  When writes terminate, some data bytes
>  *      will usually still be in flight (often in a hardware fifo).
>  *      Errors (for reads or writes) stop the queue from advancing
>  *      until the completion function returns, so that any transfers
>  *      invalidated by the error may first be dequeued.
> 
> Petr, did you observe similar behaviour with the ums gadget?
> 
> I didn't see any wrongdoing of the ethernet gadget.
> >
> >
I found a solution for the problem. The ci_udc driver checks the req
status for -EINPROGRESS in several places, but does never initialize the
status to that value.

This patch alleviates the problem:
Index: a/drivers/usb/gadget/ci_udc.c
===================================================================
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -530,6 +530,9 @@ static void ci_ep_submit_next_request(st
 	head = ci_get_qh(num, in);
 
 	ci_req = list_first_entry(&ci_ep->queue, struct ci_req, queue);
+
+	ci_req->req.status = -EINPROGRESS;
+
 	len = ci_req->req.length;
 
 	head->next = (unsigned long)item;
@@ -753,6 +756,9 @@ static void handle_ep_complete(struct ci
 		ci_ep_submit_next_request(ci_ep);
 
 	ci_req->req.actual = ci_req->req.length - len;
+	if (ci_req->req.status == -EINPROGRESS)
+		ci_req->req.status = 0;
+
 	ci_debounce(ci_req, in);
 
 	DBG("ept%d %s req %p, complete %x\n",

This is similar to how the Linux driver behaves.

I tested with fastboot and mass-storage gadget.


Lothar Waßmann