Re: xhci(4) isoc: fix bogus handling of chained TRBs

[sc . dying]

hi,

On 2020/07/15 21:28, sc.dy...@gmail.com wrote:
> hi,
> 
> The patch works well on Intel PCH xhci, but not on AMD Bolton xHCI.
> I'll investigate this problem.

Bolton xHCI sometimes raises following events.
(I added printf Completion Code.)

#246 cc 13 remain 0 type 5 origlen 2048 frlengths[6] 2048
#247 cc 1 remain 0 type 1 origlen 1024 frlengths[6] 3072

It seems this behavior violates the spec. 4.10.1.1.
When the remain of 1. TRB is 0, CC should be SUCCESS, not SHORT_PKT,
and HC should not raise interrupt.
The problem is how many length 2. TRB transferred.
If real CC of 1. TRB = SUCCESS, we should add 2. TRB length to frlengths.
If SHORT_PKT, we should keep frlengths as is.
Actually with the latter assumption I can see video smoothly w/o errors.

I changed your patch to skip adding to frlengths and actlen
if previous TRB is processed AND remain == 0.


--- sys/dev/usb/xhci.c.orig     Sun Apr  5 10:12:37 2020
+++ sys/dev/usb/xhci.c  Fri Jul 17 23:30:30 2020
@@ -931,7 +931,7 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xh
 {
        struct usbd_xfer *skipxfer;
        struct xhci_xfer *xx = (struct xhci_xfer *)xfer;
-       int trb0_idx, frame_idx = 0;
+       int trb0_idx, frame_idx = 0, prev_trb_processed = 0;
 
        KASSERT(xx->index >= 0);
        trb0_idx =
@@ -945,6 +945,7 @@ xhci_event_xfer_isoc(struct usbd_xfer *xfer, struct xh
                if (trb0_idx++ == (xp->ring.ntrb - 1))
                        trb0_idx = 0;
        }
+       xp->ring.trb_processed[trb_idx] = 1;
 
        /*
         * If we queued two TRBs for a frame and this is the second TRB,
@@ -958,15 +959,19 @@ 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) {
+               prev_trb_processed = xp->ring.trb_processed[trb0_idx];
+               if (prev_trb_processed == 0) {
                        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 (!(prev_trb_processed != 0 && remain == 0)) {
+               xfer->frlengths[frame_idx] +=
+                   XHCI_TRB_LEN(letoh32(xp->ring.trbs[trb_idx].trb_status)) -
+                   remain;
+               xfer->actlen += xfer->frlengths[frame_idx];
+       }
 
        if (xx->index != trb_idx)
                return (1);
@@ -1696,6 +1701,9 @@ xhci_ring_alloc(struct xhci_softc *sc, struct xhci_rin
 
        ring->ntrb = ntrb;
 
+       ring->trb_processed = malloc(ring->ntrb * sizeof(*ring->trb_processed),
+           M_DEVBUF, M_NOWAIT);
+
        xhci_ring_reset(sc, ring);
 
        return (0);
@@ -1704,6 +1712,8 @@ xhci_ring_alloc(struct xhci_softc *sc, struct xhci_rin
 void
 xhci_ring_free(struct xhci_softc *sc, struct xhci_ring *ring)
 {
+       free(ring->trb_processed, M_DEVBUF,
+           ring->ntrb * sizeof(*ring->trb_processed));
        usbd_dma_contig_free(&sc->sc_bus, &ring->dma);
 }
 
@@ -1715,6 +1725,8 @@ xhci_ring_reset(struct xhci_softc *sc, struct xhci_rin
        size = ring->ntrb * sizeof(struct xhci_trb);
 
        memset(ring->trbs, 0, size);
+       memset(ring->trb_processed, 0,
+           ring->ntrb * sizeof(*ring->trb_processed));
 
        ring->index = 0;
        ring->toggle = XHCI_TRB_CYCLE;
@@ -1799,6 +1811,8 @@ xhci_ring_produce(struct xhci_softc *sc, struct xhci_r
                ring->index = 0;
                ring->toggle ^= 1;
        }
+
+       ring->trb_processed[(trb - &ring->trbs[0])] = 0;
 
        return (trb);
 }