On Tue, Jun 16, 2020 at 06:55:27AM +0000, sc.dy...@gmail.com 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.
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.
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);
>
