Re: [PATCH] usb: gadget: dummy_hcd: fix data corruption with queued requests

[Alan Stern]

On Sun, Mar 15, 2026 at 10:03:07AM +0100, Sebastian Urban wrote:
> The transfer() function in dummy_hcd iterates over all queued gadget
> requests for a given endpoint via list_for_each_entry(). When the
> per-frame bandwidth budget is exhausted mid-request, leaving a
> partially-transferred gadget request, the loop continues to the next
> queued request instead of stopping.

Why do you say this?  Did you not see the code (at line 1445 
approximately):

		if (unlikely(len == 0))
			is_short = 1;
		else {
			/* not enough bandwidth left? */
			if (limit < ep->ep.maxpacket && limit < len)
				break;

It does break out of the loop when there is not enough space remaining 
in the bandwidth budget for another transaction.  But it does so at the 
start of the iteration following the last allowed transfer, rather than 
at the end of last transfer's iteration (as your patch does).

Furthermore, this test can't be removed, as there might not be enough 
space remaining for even a single transaction.

>  This breaks data ordering in the
> URB transfer buffer.
> 
> Two consequences:
> 
>   1. Data corruption: bytes from subsequent requests are written into
>      the URB buffer ahead of the remaining bytes from the incomplete
>      request. On the next timer tick the incomplete request resumes,
>      appending its remaining data after the out-of-order bytes.
> 
>   2. Premature URB completion: if the next request is a ZLP or shorter
>      than the remaining host buffer, it triggers the is_short path and
>      completes the URB before all data has been transferred.
> 
> Fix this by breaking out of the loop when the current request has
> remaining data (req->req.actual < req->req.length). The request
> resumes on the next timer tick, preserving data ordering.

As far as I can tell, this change is not necessary.  While it would 
avoid executing a few extra statements in the unlikely case where a 
request spans multiple URBs, in most situations it just adds a redundant 
test.

Alan Stern

> Signed-off-by: Sebastian Urban <surban@xxxxxxxxxx>
> ---
>  drivers/usb/gadget/udc/dummy_hcd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
> index 1cefca660..0eead4a85 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -1534,6 +1534,12 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
>  		/* rescan to continue with any other queued i/o */
>  		if (rescan)
>  			goto top;
> +
> +		/* request not fully transferred; stop iterating to
> +		 * preserve data ordering across queued requests.
> +		 */
> +		if (req->req.actual < req->req.length)
> +			break;
>  	}
>  	return sent;
>  }
> -- 
> 2.53.0
> 
>