Hi,
On 2025/03/22 1:58, biguncle...@gmail.com wrote:
From: Maksim Kiselev <biguncle...@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(+)
