On Wed, 23 Dec 2020 17:35:36 +0100
Patrick Wildt <patr...@blueri.se> wrote:
> 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.
Right, makes sense.
Should we call variable and comment like that so it's clear?
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 17:10:40 -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->zerotd;
xx->index = -1;
xx->ntrb = 0;
+ xx->zerotd = 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->zerotd += 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 17:10:40 -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 zerotd; /* Is zero len TD required? */
};
struct xhci_ring {
