On 2020/06/24 11:57, Patrick Wildt wrote:
> On Tue, Jun 16, 2020 at 06:55:27AM +0000, [email protected] wrote:
>> hi,
>>
>> The function xhci_event_xfer_isoc() of sys/dev/usb/xhci.c at line 954
>> does not work with zero length multi-TRB inbound transfer.
>>
>> 949 /*
>> 950 * If we queued two TRBs for a frame and this is the
>> second TRB,
>> 951 * check if the first TRB needs accounting since it
>> might not have
>> 952 * raised an interrupt in case of full data received.
>> 953 */
>> 954 if ((letoh32(xp->ring.trbs[trb_idx].trb_flags) &
>> XHCI_TRB_TYPE_MASK) ==
>> 955 XHCI_TRB_TYPE_NORMAL) {
>> 956 frame_idx--;
>> 957 if (trb_idx == 0)
>> 958 trb0_idx = xp->ring.ntrb - 2;
>> 959 else
>> 960 trb0_idx = trb_idx - 1;
>> 961 if (xfer->frlengths[frame_idx] == 0) {
>> 962 xfer->frlengths[frame_idx] =
>> XHCI_TRB_LEN(letoh32(
>> 963
>> xp->ring.trbs[trb0_idx].trb_status));
>> 964 }
>> 965 }
>> 966
>> 967 xfer->frlengths[frame_idx] +=
>> 968
>> XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain;
>> 969 xfer->actlen += xfer->frlengths[frame_idx];
>>
>> When a multi-TRB inbound transfer TD completes with transfer length = 0,
>> the HC should generate two events: 1st event for ISOCH TRB /w ISP|CHAIN
>> and 2nd event for NORMAL TRB w/ ISP|IOC.
>> Transfer Length field (it's remain length, actually) of each event is
>> same as requested length is, i.e., transferred length is 0.
>> So when the first event raises the frlengths is set to 0 at line 967.
>> It's correct.
>> On second event, as the comment describes, xhci.c tries to calculate
>> the 1st TRB xfer length at lines 954-965. The requested length of
>> 1st TRB is stored into frlengths -- even though the xfer len is 0.
>>
>> If frlengths = 0, we cannot distinguish the case the first event is
>> not raised from the case the transferred length is 0.
>> The frlengths is already 0 so the requested length of 1st TRB is stored.
>
> That's a really good find! I actually do wonder if we could have the
> same issue with the non-isoc transfers, when the first TRB throws a
> short and then we get another event for the last TRB.
It may occur on non-isoc pipes and should be fixed.
I have not observed problems on non-isoc pipes, but it's because
the devices I saw did not do zero-length xfer on non-isoc pipes,
I guess.
>
> Maybe it would make sense to record the idx of the last TRB that we have
> received an event for? Then we could check if we already processed that
> TRB.
At first I recorded such an index and compared with current idx, but
I noticed it would be expressed as a bit.
Do you have any plan to use the software's dequeue pointer for other
purposes?
>
> Patrick
>
>> For example, I applied debug printf [*1], and run
>> mplayer tv:// for my webcam.
>> I see...
>>
>> #25 remain 1024 type 5 origlen 1024 frlengths[25] 0
>> #25 (omitted) frlen[25] 1024
>> #26 remain 2048 type 1 origlen 2048 frlengths[25] 1024
>>
>> These console logs show a 3072 bytes frame is splitted into
>> two TRBs and got 0 bytes. The first TRB transfers 0 bytes and
>> the second TRB transfers 0, too, but it results 1024 bytes.
>>
>> My proposal patch [*2] adds a flag to xhci_xfer that indicates the
>> TRB processed by xhci.c previously has CHAIN bit, and updates the
>> frlengths only when that flag is not set.
>>
>>
>>
>> [*1]
>> debug printf.
>> It shows only splitted isochronous TDs.
>>
>> --- sys/dev/usb/xhci.c.orig Sun Apr 5 10:12:37 2020
>> +++ sys/dev/usb/xhci.c Fri May 29 04:13:36 2020
>> @@ -961,12 +961,23 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xh
>> if (xfer->frlengths[frame_idx] == 0) {
>> xfer->frlengths[frame_idx] = XHCI_TRB_LEN(letoh32(
>> xp->ring.trbs[trb0_idx].trb_status));
>> + printf("#%d (omitted) frlen[%d] %u\n",
>> + trb0_idx, frame_idx, xfer->frlengths[frame_idx]);
>> }
>> }
>>
>> xfer->frlengths[frame_idx] +=
>> XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain;
>> xfer->actlen += xfer->frlengths[frame_idx];
>> + uint32_t trb_flags = letoh32(xp->ring.trbs[trb_idx].trb_flags);
>> + if ((trb_flags & XHCI_TRB_CHAIN) ||
>> + (trb_flags & XHCI_TRB_TYPE_MASK) == XHCI_TRB_TYPE_NORMAL) {
>> + printf("#%d remain %u type %u origlen %u frlengths[%d] %hu\n",
>> + trb_idx, remain,
>> + XHCI_TRB_TYPE(trb_flags),
>> + XHCI_TRB_LEN(le32toh(xp->ring.trbs[trb_idx].trb_status)),
>> + frame_idx, xfer->frlengths[frame_idx]);
>> + }
>>
>> if (xx->index != trb_idx)
>> return (1);
>>
>> [*2]
>> patch
>>
>> --- sys/dev/usb/xhcivar.h.orig Sun Oct 6 21:19:28 2019
>> +++ sys/dev/usb/xhcivar.h Fri May 22 04:19:57 2020
>> @@ -40,6 +40,7 @@ struct xhci_xfer {
>> struct usbd_xfer xfer;
>> int index; /* Index of the last TRB */
>> size_t ntrb; /* Number of associated TRBs */
>> + bool trb_chained;
>> };
>>
>> struct xhci_ring {
>> --- sys/dev/usb/xhci.c.orig Sun Apr 5 10:12:37 2020
>> +++ sys/dev/usb/xhci.c Thu Jun 4 05:39:03 2020
>> @@ -958,15 +958,23 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xh
>> trb0_idx = xp->ring.ntrb - 2;
>> else
>> trb0_idx = trb_idx - 1;
>> - if (xfer->frlengths[frame_idx] == 0) {
>> + if (xfer->frlengths[frame_idx] == 0 && !xx->trb_chained) {
>> xfer->frlengths[frame_idx] = XHCI_TRB_LEN(letoh32(
>> xp->ring.trbs[trb0_idx].trb_status));
>> }
>> }
>>
>> - xfer->frlengths[frame_idx] +=
>> - XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) - remain;
>> - xfer->actlen += xfer->frlengths[frame_idx];
>> + if (!xx->trb_chained) {
>> + xfer->frlengths[frame_idx] +=
>> + XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) -
>> + remain;
>> + xfer->actlen += xfer->frlengths[frame_idx];
>> + }
>> +
>> + if (letoh32(xp->ring.trbs[trb_idx].trb_flags) & XHCI_TRB_CHAIN)
>> + xx->trb_chained = true;
>> + else
>> + xx->trb_chained = false;
>>
>> if (xx->index != trb_idx)
>> return (1);
>>
