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

[Sebastian Urban]

On 3/15/26 16:28, Alan Stern wrote:
> On Sun, Mar 15, 2026 at 11:21:26AM -0400, Alan Stern wrote:
>> On Sun, Mar 15, 2026 at 03:06:21PM +0000, Sebastian Urban wrote:
>>> On 3/15/26 15:44, Alan Stern wrote:
>>>> 		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).
>>>>
>>> You're right that the existing bandwidth check at line 1444 handles the
>>> general case of a non-zero request following a partial transfer.
>>>
>>> However, if a ZLP follows a partially-transferred request, the check is
>>> skipped because of the unlikely(len == 0) branch, leading to a false
>>> complete of the transfer with a shortened length.
>>
>> How can a ZLP follow a partially transferred request?  What follows a
>> partially transferred request is always the remainder of that request,
>> which by definition must have length > 0.
> 
> Oh, I see now what you mean.  The whole thing uses
> loop_for_each_entry(), so it always proceeds to the next request in the
> queue even if the current request isn't finished.  Instead of doing
> that, it should always handle the first request in the queue.
> 
> The loop should be rewritten; it should be more like
> 
> 	while (!list_empty(&ep->queue)) {
> 		req = list_first_entry(&ep->queue);
> 		...
> 
> Then it would behave as expected.
> 
I agree that the loop should only ever process the first matching entry. 
The break I added at the end of the loop body achieves exactly that. 
  
  


I kept list_for_each_entry rather than rewriting to while + 
list_first_entry because the stream ID filtering at line 1421 uses 
continue to skip non-matching requests:

       if (dummy_ep_stream_en(dum_hcd, urb)) {
           if ((urb->stream_id != req->req.stream_id))
               continue;
       }

This wouldn't work with list_first_entry. A structural rewrite would 
need a separate scan to find the first matching request, which seems 
risky for a bug fix. If you'd prefer that approach I'm happy to do it as 
a follow-up patch.

Sebastian