[PATCH v1 0/1] USB xHCI wrong act_len calculation in case of using multipe TRB

[marcan]

Hi,

On 2025/03/22 1:58, bigunclemax at gmail.com wrote:
> From: Maksim Kiselev <bigunclemax at gmail.com>
> 
> Hello everyone!
> 
> I've encountered an issue where the actual length of received data is
> calculated incorrectly in the case of a multiple TRB request.
> 
> Below, I'll try to describe the essence of the problem:
> 
> A USB-ethernet adapter ASIX ax88179 is connected to my board Li4pi,
> and I'm sending a DHCP request to the server via dhpc command.
> 
> The response from the DHCP server always has the same length (0x168).
> However, in some cases, I noticed that the received response had
> an incorrect length and network subsystem is completely ingnore
> incoming packets.
> 
> The problem turned out to be in the xhci_bulk_tx() function.
> I added some debugging[1] to better understand what's happening.
> 
> Here's the log from a working case:
> ```
>      dev=000000057f786b90, pipe=c0010283, buffer=000000057f787540, length=20480
>      PUSH. trb_len: 0x5000
>      POP. trb_len: 0x4e98, comp_code: 0xd
>      udev->act_len: 0x168
> ```
> 
> And here's the log from a non-working case:
> ```
>      dev=000000057f78b610, pipe=c0010283, buffer=000000057f78bfc0, length=20480
>      PUSH. trb_len: 0x4040
>      PUSH. trb_len: 0xfc0
>      POP. trb_len: 0x3ed8, comp_code: 0xd
>      POP. trb_len: 0x0, comp_code: 0x1
>      udev->act_len: 0x1128

This looks like a bug in your host controller. Completion code 0xd is 
"short packet", which is as expected. However, completion code 0x1 is 
"success", which is unexpected. According to xHCI 4.10.1.1.2:

> If the Short Packet occurred while processing a Transfer TRB with only an ISP
> flag set, then two events shall be generated for the transfer; one for the Transfer
> TRB that the Short Packet occurred on, and a second for the last TRB with the
> IOC flag set. Table 6-38 defines the Completion Code and TRB Transfer Length
> for the first event. In the second event, the Completion Code shall be set to
> Short Packet, and the TRB Transfer Length should be set to the same value that
> was reported by the initial Short Packet Event.

So the second event should have had completion code 0xd too.

What host controller is this? Though this is kind of irrelevant, because 
the correct fix would ignore this quirk anyway (see below)

> ```
> As you can see, in the second case, the buffer spans a 64KB boundary
> (buffer=000000057f78bfc0 + 0x5000).
> Therefore, it is split into two TRBs with lengths 0x4040 and 0xfc0.
> 
> Then, act_len is calculated as the difference between length and
> trans_event.transfer_len:
> ```
> 0x5000 - 0x3ed8 = 0x1128
> ```
> 
> However, it seems that this is not entirely correct,
> and act_len should be calculated as:
> 
> ```
> trb_buff_len - trans_event.transfer_len
> (0x4040 - 0x3ed8 = 0x168)
> ```
> 
> 0x168 is the correct length, which is the same as the length
> obtained in the first case where network rx works fine.
> 
> Also I looked at the Linux xhci-ring code where urb->actual_length
> calculated as:
> [2]
> ```
>      requested = td->urb->transfer_buffer_length;
>      remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
> ```
> [3]
> ```
>      td->urb->actual_length = requested - remaining;
> ```
> 
> Perhaps I missed something or misunderstood, so I would appreciate any help.

You're looking at the control transfer path. This is the bulk path:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/xhci-ring.c?h=v6.14-rc7#n2604

There you can see there are two branches. If the event happened on the 
last TRB, just subtract the remaining bytes from the total transfer 
size. If the event happened earlier (short packet with completely 
untransferred TRBs remaining), then you need to add up the TRB lengths 
up to and including the current TRB, and subtract the event remaining 
bytes from *that* (excluding any subsequent TRBs). This both fixes your 
issue and ignores the apparent bug in your host controller (since you're 
supposed to do all this processing when the short TRB is processed and 
completely ignore the second event on the last TRB).

So your patch is incorrect, it fixes the specific case you are hitting 
but breaks other split TRB cases. A more complex patch is needed that 
does this:

- In the if branch (short packet in non final TRB case, first event):
-- Add up the TRB lengths up to and including the event's associated TRB
-- Call record_transfer_result() passing that partial sum as length 
(instead of available_length) and the current event (first of the two)
-- Set a flag
- Later, before returning from the function, check the flag and do NOT 
call record_transfer_result() again on the final event, since the length 
was already calculated earlier and the length field of the final event 
is not useful (it is either wrong as in your case, or per spec a repeat 
of the short TRB's length field, but this information is useless without 
the TRB index that was short, which is not the final TRB).

> 
> ---
> 
> [1] Patch for debug output
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 89d2e54f20a..b1433a16a99 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -791,6 +791,8 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,
>                  trb_fields[2] = length_field;
>                  trb_fields[3] = field | TRB_TYPE(TRB_NORMAL);
>   
> +               debug("PUSH. trb_len: 0x%x\n", trb_buff_len);
> +
>                  last_transfer_trb_addr = queue_trb(ctrl, ring, (num_trbs > 1), trb_fields);
>   
>                  --num_trbs;
> @@ -816,6 +818,10 @@ again:
>                  return -ETIMEDOUT;
>          }
>   
> +       debug("POP. trb_len: 0x%x, comp_code: 0x%x\n",
> +             (int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len)),
> +             GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len)));
> +
>          if ((uintptr_t)(le64_to_cpu(event->trans_event.buffer)) !=
>              (uintptr_t)last_transfer_trb_addr) {
>                  available_length -=
> @@ -831,6 +837,7 @@ again:
>          available_length -= first_trb_trimmed_sz;
>   
>          record_transfer_result(udev, event, available_length);
> +       debug("udev->act_len: 0x%x\n", udev->act_len);
>          xhci_acknowledge_event(ctrl);
>          xhci_inval_cache((uintptr_t)buffer, length);
>          xhci_dma_unmap(ctrl, buf_64, length);
> 
> 
> [2] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/xhci-ring.c?h=v6.14-rc7#n2346
> [3] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/xhci-ring.c?h=v6.14-rc7#n2413
> 
> Maksim Kiselev (1):
>    usb: xhci: fix calculation of act_len in case of using multipe TRB
> 
>   drivers/usb/host/xhci-ring.c | 5 +++++
>   1 file changed, 5 insertions(+)
>