fix xhci 'actlen' calculation

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

fix xhci 'actlen' calculation

Gerhard Roth-2
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


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

Reply | Threaded
Open this post in threaded view
|

Re: fix xhci 'actlen' calculation

Patrick Wildt-3
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
>

Reply | Threaded
Open this post in threaded view
|

Re: fix xhci 'actlen' calculation

Patrick Wildt-3
On Tue, Nov 12, 2019 at 01:43:28PM +0100, Patrick Wildt wrote:

> 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!

Unfortunately this is *not* ok.  It turns out that this doesn't work
for short ctrl transfers.  Compared to bulk/intr transfers, ctrl xfers
have a setup, data and status trb.  Now your diff will sum all TRBs,
but the thing is that only the data TRB is actually short. :(  But I
think this should be easily fixable, I'll have a look.

Reply | Threaded
Open this post in threaded view
|

Re: fix xhci 'actlen' calculation

Patrick Wildt-3
On Tue, Nov 12, 2019 at 07:00:00PM +0100, Patrick Wildt wrote:

> On Tue, Nov 12, 2019 at 01:43:28PM +0100, Patrick Wildt wrote:
> > 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!
>
> Unfortunately this is *not* ok.  It turns out that this doesn't work
> for short ctrl transfers.  Compared to bulk/intr transfers, ctrl xfers
> have a setup, data and status trb.  Now your diff will sum all TRBs,
> but the thing is that only the data TRB is actually short. :(  But I
> think this should be easily fixable, I'll have a look.

This changes the diff so that it only sums actual data TRBs (NORMAL and
DATA).

That makes my umb(4) device behave again.

Patrick

diff --git a/sys/dev/usb/xhci.c b/sys/dev/usb/xhci.c
index f9d7eb3a401..c801bd96ad1 100644
--- a/sys/dev/usb/xhci.c
+++ b/sys/dev/usb/xhci.c
@@ -810,6 +810,29 @@ xhci_event_xfer(struct xhci_softc *sc, uint64_t paddr, uint32_t status,
  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, type;
+
+ trb0_idx =
+    ((xx->index + xp->ring.ntrb) - xx->ntrb) % (xp->ring.ntrb - 1);
+
+ while (1) {
+ type = xp->ring.trbs[trb0_idx].trb_flags & XHCI_TRB_TYPE_MASK;
+ if (type == XHCI_TRB_TYPE_NORMAL || type == XHCI_TRB_TYPE_DATA)
+ 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,
@@ -819,16 +842,22 @@ xhci_event_xfer_generic(struct xhci_softc *sc, struct usbd_xfer *xfer,
 
  switch (code) {
  case XHCI_CODE_SUCCESS:
- /*
- * 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