On Sun, Oct 11, 2020 at 08:12:29AM +0200, Martin Pieuchot wrote:
> On 09/10/20(Fri) 12:37, Jonathon Fletcher wrote:
> > In xhci_xfer_get_trb, the count of transfer buffers in the pipe
> > (xp->free_trbs) is always decremented but the count of transfer buffers
> > used in the transfer (xx->ntrb) is not incremented for zero-length
> > transfers. The result of this is that, at the end of a zero length
> > transfer, xp->free_trbs has 'lost' one.
> >
> > Over time, this mismatch of unconditional decrement (xp->free_trbs) vs
> > conditional increment (xx->ntrb) results in xhci_device_*_start returning
> > USBD_NOMEM.
> >
> > The patch below works around this by only decrementing xp->free_trbs in the
> > cases when xx->ntrb is incremented.
>
> 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.
>
> > Index: xhci.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/xhci.c,v
> > retrieving revision 1.119
> > diff -u -p -u -r1.119 xhci.c
> > --- xhci.c 31 Jul 2020 19:27:57 -0000 1.119
> > +++ xhci.c 9 Oct 2020 19:11:45 -0000
> > @@ -1836,7 +1836,6 @@ xhci_xfer_get_trb(struct xhci_softc *sc,
> > struct xhci_xfer *xx = (struct xhci_xfer *)xfer;
> >
> > KASSERT(xp->free_trbs >= 1);
> > - xp->free_trbs--;
> > *togglep = xp->ring.toggle;
> >
> > switch (last) {
> > @@ -1847,11 +1846,13 @@ xhci_xfer_get_trb(struct xhci_softc *sc,
> > xp->pending_xfers[xp->ring.index] = xfer;
> > xx->index = -2;
> > xx->ntrb += 1;
> > + xp->free_trbs--;
> > break;
> > case 1: /* This will terminate a chain. */
> > xp->pending_xfers[xp->ring.index] = xfer;
> > xx->index = xp->ring.index;
> > xx->ntrb += 1;
> > + xp->free_trbs--;
> > break;
> > }
> >
> >
>
