On Tue, Nov 12, 2019 at 10:45:39AM +0100, Gerhard Roth wrote:
> Hi,
>
> xhci's calculation of 'xfer->actlen' is wrong if the xfer was split into
> multiple TRBs. That's because the code just looks at the remainder
> reported by the status TRB. However, this remainder only refers to the
> total size of this single TRB; not to the total size of the xfer.
>
> Example: assume a 16 kb xfer is split into two TRBs for 8 kb each.
> Then we get a short read of 6 kb. The status TRB will refer
> to the first TRB with remainder set to 2 kb. Currently xhci
> would assume that actlen is 'xfer->length' (16 kb) minus the
> remainder (2 kb). That yields a wrong actlen of 14 kb instead
> of 6 kb.
>
> The patch below fixes this by adding up all the remainders of all the
> transfer TRBs up to the current one.
>
> Gerhard
>
Very goood catch, this is very similar to the issue we had with isoc
transfers. Diff looks good to me!
>
> Index: sys/dev/usb/xhci.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/xhci.c,v
> retrieving revision 1.106
> diff -u -p -u -p -r1.106 xhci.c
> --- sys/dev/usb/xhci.c 6 Oct 2019 17:30:00 -0000 1.106
> +++ sys/dev/usb/xhci.c 12 Nov 2019 09:29:47 -0000
> @@ -810,6 +810,28 @@ xhci_event_xfer(struct xhci_softc *sc, u
> xhci_xfer_done(xfer);
> }
>
> +uint32_t
> +xhci_xfer_length_generic(struct xhci_xfer *xx, struct xhci_pipe *xp,
> + int trb_idx)
> +{
> + int trb0_idx;
> + uint32_t len = 0;
> +
> + KASSERT(xx->index >= 0);
> + trb0_idx =
> + ((xx->index + xp->ring.ntrb) - xx->ntrb) % (xp->ring.ntrb - 1);
> +
> + while (1) {
> + len +=
> + le32toh(XHCI_TRB_LEN(xp->ring.trbs[trb0_idx].trb_status));
> + if (trb0_idx == trb_idx)
> + break;
> + if (++trb0_idx == xp->ring.ntrb)
> + trb0_idx = 0;
> + }
> + return len;
> +}
> +
> int
> xhci_event_xfer_generic(struct xhci_softc *sc, struct usbd_xfer *xfer,
> struct xhci_pipe *xp, uint32_t remain, int trb_idx,
> @@ -823,12 +845,22 @@ xhci_event_xfer_generic(struct xhci_soft
> * This might be the last TRB of a TD that ended up
> * with a Short Transfer condition, see below.
> */
> - if (xfer->actlen == 0)
> - xfer->actlen = xfer->length - remain;
> + if (xfer->actlen == 0) {
> + if (remain)
> + xfer->actlen =
> + xhci_xfer_length_generic(xx, xp, trb_idx) -
> + remain;
> + else
> + xfer->actlen = xfer->length;
> + }
> xfer->status = USBD_NORMAL_COMPLETION;
> break;
> case XHCI_CODE_SHORT_XFER:
> - xfer->actlen = xfer->length - remain;
> + /*
> + * Use values from the transfer TRB instead of the status TRB.
> + */
> + xfer->actlen = xhci_xfer_length_generic(xx, xp, trb_idx) -
> + remain;
> /*
> * If this is not the last TRB of a transfer, we should
> * theoretically clear the IOC at the end of the chain
>
