Am Wed, Dec 23, 2020 at 10:44:21AM +0100 schrieb Marcus Glocker:
> On Wed, 23 Dec 2020 09:47:44 +0100
> Marcus Glocker <mar...@nazgul.ch> wrote:
>
> > On Tue, 22 Dec 2020 20:55:41 +0100
> > Marcus Glocker <mar...@nazgul.ch> wrote:
> >
> > > > > Did you consider incrementing xx->ntrb instead?
> > >
> > > >That doesn't work either, because the status completion code needs
> > > >xx->ntrb to be correct for the data TD to be handled correctly.
> > > >Incrementing xx->ntrb means the number of TRBs for the data TD is
> > > >incorrect, since it includes the (optional) zero TD's TRB.
> > > >
> > > >In this case the zero TD allocates a TRB but doesn't do proper
> > > >accounting, and currently there's no place where this could be
> > > >accounted properly.
> > > >
> > > >In the end it's all software, so I guess the diff will simply have
> > > >to be bigger than just a one-liner.
> > >
> > > > > With the diff below the produced TRB isn't accounted which might
> > > > > lead
> > > > > to and off-by-one.
> > >
> > > Sorry, I missed this thread and had to re-grab the last mail from
> > > MARC.
> > >
> > > Can't we just take account of the zero trb separately?
> >
> > We also want to reset the zerotrb.
>
> Re-thinking this again I think we should only increase the zerotrb to
> avoid again a possible miss match for free_trbs, and leave the
> responsibility to the caller of xhci_xfer_get_trb() to request the
> right amount of zerotrb.
>
>
> Index: dev/usb/xhci.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/xhci.c,v
> retrieving revision 1.119
> diff -u -p -u -p -r1.119 xhci.c
> --- dev/usb/xhci.c 31 Jul 2020 19:27:57 -0000 1.119
> +++ dev/usb/xhci.c 23 Dec 2020 09:38:58 -0000
> @@ -1135,8 +1135,10 @@ xhci_xfer_done(struct usbd_xfer *xfer)
> i = (xp->ring.ntrb - 1);
> }
> xp->free_trbs += xx->ntrb;
> + xp->free_trbs += xx->zerotrb;
> xx->index = -1;
> xx->ntrb = 0;
> + xx->zerotrb = 0;
>
> timeout_del(&xfer->timeout_handle);
> usb_rem_task(xfer->device, &xfer->abort_task);
> @@ -1842,6 +1844,7 @@ xhci_xfer_get_trb(struct xhci_softc *sc,
> switch (last) {
> case -1: /* This will be a zero-length TD. */
> xp->pending_xfers[xp->ring.index] = NULL;
> + xx->zerotrb += 1;
> break;
> case 0: /* This will be in a chain. */
> xp->pending_xfers[xp->ring.index] = xfer;
> Index: dev/usb/xhcivar.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/xhcivar.h,v
> retrieving revision 1.11
> diff -u -p -u -p -r1.11 xhcivar.h
> --- dev/usb/xhcivar.h 6 Oct 2019 17:30:00 -0000 1.11
> +++ dev/usb/xhcivar.h 23 Dec 2020 09:38:58 -0000
> @@ -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 */
> + size_t zerotrb; /* Is zero len TRB required? */
It's a zero-length TD, not TRB. I mean, it indeed is a zero-legth TRB,
but the important thing is that it's part of an extra TD. So at least
update the comment, maybe even the variable name.
The difference is that a TD means that it's a separate transfer. It
also completes seperately from the TD before. In theory xfer done will
be called on the initial TD, not on the zero TD, which means that we
could have a race where our accounting "frees" the zero TD, even though
the controller isn't there yet. In practice I think this is not an
issue, the ring's hopefully long enough that we don't immediately reuse
the TRB that we just freed.
So, I think the approach taken in this diff is fine, the code looks
good, only the naming I think can be improved. Maybe really just call
it zerotd, then it also fits with the comment.
> };
>
> struct xhci_ring {
>
