On Sun, Oct 11, 2020 at 12:14:22PM +0200, Patrick Wildt wrote:
> 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?
xp->free_trbs is used for accounting at the pipe level.
xx->ntrb is also used in ring index calculations and
changing that would have larger effects.
> 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.
There are two (or more) bugs.
Patrick is correct about the major issue - zero length transfers
use a transfer buffer and it is not correctly accounted for. I
do not have a patch for this and I agree with Patrick that it
will be bigger than a one-liner.
My minor patch works around an accounting mismatch between the
pipe (xp->free_trbs) and the transfer (xx->ntrb). This accounting
mismatch will cause a xhci controller to stop working (via
USBD_NOMEM) after a fixed number of zero-length transfers
following pipe initialization. This patch does not change
the major issue that Patrick describes.
> > 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;
> > > }
> > >
> > >
> >
