xhci: set chain bit in link TRBs

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

xhci: set chain bit in link TRBs

Patrick Wildt-3
Hi,

on my i.MX8M machine which features a DWC3 xHCI 1.10 controller I have
seen this error while installing base64.tgz or running fsck:

umass0: Invalid CSW: sig 0x43425355 should be 0x53425355

As it turns out using a USB protocol analyzer, the transfers actually
seem fine, the USB mass storage is not responding nonsense.  By further
looking into it I realized that our xhci(4) is told the transfer was
completed, even though the buffer had not been touched.

Further debugging revealed that the issue occured when a transfer that
spans multiple TRBs loops over the ring.  This means one TRB is using
idx 254, the next TRB is the link TRB which does not contain data and
sits at idx 255, and the following data TRB is using idx 0.

Transfers that comprise of multiple TRBs must have the chain bit set in
all but the last TRB.  Now in this case the link TRB which sits in the
middle does not get the chain bit set, and thus processing stops.

Somewhere I have seen code that always sets the chain bit in the link
TRB, so I'm wondering if we should follow that too.  For now I think the
easiest fix for this is to set the chain bit in the link TRB if the
transfer spans multiple TRBs.

Feedback?

Patrick

diff --git a/sys/dev/usb/xhci.c b/sys/dev/usb/xhci.c
index 1078acb20c1..e6966655b48 100644
--- a/sys/dev/usb/xhci.c
+++ b/sys/dev/usb/xhci.c
@@ -91,7 +91,8 @@ int xhci_ring_alloc(struct xhci_softc *, struct xhci_ring *, size_t,
 void xhci_ring_free(struct xhci_softc *, struct xhci_ring *);
 void xhci_ring_reset(struct xhci_softc *, struct xhci_ring *);
 struct xhci_trb *xhci_ring_consume(struct xhci_softc *, struct xhci_ring *);
-struct xhci_trb *xhci_ring_produce(struct xhci_softc *, struct xhci_ring *);
+struct xhci_trb *xhci_ring_produce(struct xhci_softc *, struct xhci_ring *,
+    int);
 
 struct xhci_trb *xhci_xfer_get_trb(struct xhci_softc *, struct usbd_xfer*,
     uint8_t *, int);
@@ -1584,7 +1585,7 @@ xhci_ring_consume(struct xhci_softc *sc, struct xhci_ring *ring)
 }
 
 struct xhci_trb*
-xhci_ring_produce(struct xhci_softc *sc, struct xhci_ring *ring)
+xhci_ring_produce(struct xhci_softc *sc, struct xhci_ring *ring, int last)
 {
  struct xhci_trb *trb = &ring->trbs[ring->index];
 
@@ -1605,6 +1606,9 @@ xhci_ring_produce(struct xhci_softc *sc, struct xhci_ring *ring)
     BUS_DMASYNC_POSTWRITE);
 
  lnk->trb_flags ^= htole32(XHCI_TRB_CYCLE);
+ lnk->trb_flags &= htole32(~XHCI_TRB_CHAIN);
+ if (!last)
+ lnk->trb_flags |= htole32(XHCI_TRB_CHAIN);
 
  bus_dmamap_sync(ring->dma.tag, ring->dma.map, TRBOFF(ring, lnk),
     sizeof(struct xhci_trb), BUS_DMASYNC_PREWRITE);
@@ -1633,7 +1637,7 @@ xhci_xfer_get_trb(struct xhci_softc *sc, struct usbd_xfer *xfer,
  xx->ntrb += 1;
 
  *togglep = xp->ring.toggle;
- return (xhci_ring_produce(sc, &xp->ring));
+ return (xhci_ring_produce(sc, &xp->ring, last));
 }
 
 int
@@ -1646,7 +1650,7 @@ xhci_command_submit(struct xhci_softc *sc, struct xhci_trb *trb0, int timeout)
 
  trb0->trb_flags |= htole32(sc->sc_cmd_ring.toggle);
 
- trb = xhci_ring_produce(sc, &sc->sc_cmd_ring);
+ trb = xhci_ring_produce(sc, &sc->sc_cmd_ring, 1);
  if (trb == NULL)
  return (EAGAIN);
  trb->trb_paddr = trb0->trb_paddr;

Reply | Threaded
Open this post in threaded view
|

Re: xhci: set chain bit in link TRBs

Stefan Sperling-5
On Thu, Feb 21, 2019 at 10:50:40AM +0100, Patrick Wildt wrote:

> Hi,
>
> on my i.MX8M machine which features a DWC3 xHCI 1.10 controller I have
> seen this error while installing base64.tgz or running fsck:
>
> umass0: Invalid CSW: sig 0x43425355 should be 0x53425355
>
> As it turns out using a USB protocol analyzer, the transfers actually
> seem fine, the USB mass storage is not responding nonsense.  By further
> looking into it I realized that our xhci(4) is told the transfer was
> completed, even though the buffer had not been touched.
>
> Further debugging revealed that the issue occured when a transfer that
> spans multiple TRBs loops over the ring.  This means one TRB is using
> idx 254, the next TRB is the link TRB which does not contain data and
> sits at idx 255, and the following data TRB is using idx 0.
>
> Transfers that comprise of multiple TRBs must have the chain bit set in
> all but the last TRB.  Now in this case the link TRB which sits in the
> middle does not get the chain bit set, and thus processing stops.
>
> Somewhere I have seen code that always sets the chain bit in the link
> TRB, so I'm wondering if we should follow that too.  For now I think the
> easiest fix for this is to set the chain bit in the link TRB if the
> transfer spans multiple TRBs.
>
> Feedback?

Is this why the #if 0 isochronous transfer code doesn't work?

Reply | Threaded
Open this post in threaded view
|

Re: xhci: set chain bit in link TRBs

Patrick Wildt-3
On Thu, Feb 21, 2019 at 03:56:57PM +0100, Stefan Sperling wrote:

> On Thu, Feb 21, 2019 at 10:50:40AM +0100, Patrick Wildt wrote:
> > Hi,
> >
> > on my i.MX8M machine which features a DWC3 xHCI 1.10 controller I have
> > seen this error while installing base64.tgz or running fsck:
> >
> > umass0: Invalid CSW: sig 0x43425355 should be 0x53425355
> >
> > As it turns out using a USB protocol analyzer, the transfers actually
> > seem fine, the USB mass storage is not responding nonsense.  By further
> > looking into it I realized that our xhci(4) is told the transfer was
> > completed, even though the buffer had not been touched.
> >
> > Further debugging revealed that the issue occured when a transfer that
> > spans multiple TRBs loops over the ring.  This means one TRB is using
> > idx 254, the next TRB is the link TRB which does not contain data and
> > sits at idx 255, and the following data TRB is using idx 0.
> >
> > Transfers that comprise of multiple TRBs must have the chain bit set in
> > all but the last TRB.  Now in this case the link TRB which sits in the
> > middle does not get the chain bit set, and thus processing stops.
> >
> > Somewhere I have seen code that always sets the chain bit in the link
> > TRB, so I'm wondering if we should follow that too.  For now I think the
> > easiest fix for this is to set the chain bit in the link TRB if the
> > transfer spans multiple TRBs.
> >
> > Feedback?
>
> Is this why the #if 0 isochronous transfer code doesn't work?
>

That's possible.  Do you have a device you could test this with?

Reply | Threaded
Open this post in threaded view
|

Re: xhci: set chain bit in link TRBs

Peter Hessler
On 2019 Feb 21 (Thu) at 16:19:22 +0100 (+0100), Patrick Wildt wrote:
:On Thu, Feb 21, 2019 at 03:56:57PM +0100, Stefan Sperling wrote:
:> On Thu, Feb 21, 2019 at 10:50:40AM +0100, Patrick Wildt wrote:
:> > Hi,
:> >
:> > on my i.MX8M machine which features a DWC3 xHCI 1.10 controller I have
:> > seen this error while installing base64.tgz or running fsck:
:> >
:> > umass0: Invalid CSW: sig 0x43425355 should be 0x53425355
:> >
:> > As it turns out using a USB protocol analyzer, the transfers actually
:> > seem fine, the USB mass storage is not responding nonsense.  By further
:> > looking into it I realized that our xhci(4) is told the transfer was
:> > completed, even though the buffer had not been touched.
:> >
:> > Further debugging revealed that the issue occured when a transfer that
:> > spans multiple TRBs loops over the ring.  This means one TRB is using
:> > idx 254, the next TRB is the link TRB which does not contain data and
:> > sits at idx 255, and the following data TRB is using idx 0.
:> >
:> > Transfers that comprise of multiple TRBs must have the chain bit set in
:> > all but the last TRB.  Now in this case the link TRB which sits in the
:> > middle does not get the chain bit set, and thus processing stops.
:> >
:> > Somewhere I have seen code that always sets the chain bit in the link
:> > TRB, so I'm wondering if we should follow that too.  For now I think the
:> > easiest fix for this is to set the chain bit in the link TRB if the
:> > transfer spans multiple TRBs.
:> >
:> > Feedback?
:>
:> Is this why the #if 0 isochronous transfer code doesn't work?
:>
:
:That's possible.  Do you have a device you could test this with?
:

tested here with uvideo0:

xhci0 at pci0 dev 20 function 0 "Intel 100 Series xHCI" rev 0x21: msi, xHCI 1.0
usb0 at xhci0: USB revision 3.0
uhub0 at usb0 configuration 1 interface 0 "Intel xHCI root hub" rev 3.00/1.00 addr 1
...
uvideo0 at uhub0 port 8 configuration 1 interface 0 "Chicony Electronics Co.,Ltd. Integrated Camera" rev 2.00/0.06 addr 3
video0 at uvideo0
...
xhci0: NULL xfer pointer
xhci0: wrong trb index (4227450112) max is 255
xhci0: wrong trb index (4227450112) max is 255
xhci0: NULL xfer pointer



--
Q:  How many Martians does it take to screw in a lightbulb?
A:  One and a half.