kern/51202: XHCI driver sends stale TRBs from a pipe previously opened

[webpages%sprow.co.uk@localhost]

>Number:         51202
>Category:       kern
>Synopsis:       XHCI driver sends stale TRBs from a pipe previously opened
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue May 31 09:20:00 +0000 2016
>Originator:     Sprow
>Release:        
>Organization:
>Environment:
>Description:
When a pipe is opened (eg. for output) TRBs are written into the endpoint's transfer ring and sent by ringing the doorbell. However, when that pipe is closed the (previously sent) TRBs are left in the ring.
Subsequently re-opening that pipe and sending further output results in the previous (now stale) TRBs being re-sent since they are still there.

Simple example case:
* open endpoint for bulk output
* do two transfers => two TRBs now in the ring
* close endpoint
* open same endpoint again for bulk output => same ring is used
* do one transfer => two are sent, the new one plus the 2nd old one

There are 3 places where the transfer ring is dequeued:
1 explicit: in xhci_set_dequeue().
2 implicit: when the EP is configured, and when the slot is set up (search for XHCI_EPCTX_2_DCS_SET).

It is the case when the EP is configured that results in the stale TRBs; the driver needs to wipe its state of the ring like it does in the other 2 cases.
>How-To-Repeat:
Attached patch splits out the host side dequeue into a function and calls it as required from xhci_set_dequeue() and when configuring endpoints. In principle the 3rd copy (in xhci_ring_init) could also be replaced and the call to kmem_zalloc changed to be a kmem_alloc.
>Fix:
--- xhci-1_46.c	2016-05-30 22:44:14 +0100
+++ xhci.c	2016-05-31 08:56:10 +0100
@@ -142,6 +142,7 @@
 static usbd_status xhci_reset_endpoint(struct usbd_pipe *);
 static usbd_status xhci_stop_endpoint(struct usbd_pipe *);
 
+static void xhci_host_dequeue(struct xhci_ring * const);
 static usbd_status xhci_set_dequeue(struct usbd_pipe *);
 
 static usbd_status xhci_do_command(struct xhci_softc * const,
@@ -1508,6 +1509,9 @@
 	cp = xhci_slot_get_icv(sc, xs, xhci_dci_to_ici(dci));
 	xhci_setup_endp_ctx(pipe, cp);
 
+	/* configure ep context performs an implicit dequeue */
+	xhci_host_dequeue(&xs->xs_ep[dci].xe_tr);
+
 	/* sync input contexts before they are read from memory */
 	usb_syncmem(&xs->xs_ic_dma, 0, sc->sc_pgsz, BUS_DMASYNC_PREWRITE);
 	hexdump("input control context", xhci_slot_get_icv(sc, xs, 0),
@@ -1600,6 +1604,19 @@
 	return err;
 }
 
+static void
+xhci_host_dequeue(struct xhci_ring * const xr)
+{
+	/* When dequeueing the controller, update our struct copy too */
+	memset(xr->xr_trb, 0, xr->xr_ntrb * XHCI_TRB_SIZE);
+	usb_syncmem(&xr->xr_dma, 0, xr->xr_ntrb * XHCI_TRB_SIZE,
+	    BUS_DMASYNC_PREWRITE);
+	memset(xr->xr_cookies, 0, xr->xr_ntrb * sizeof(*xr->xr_cookies));
+
+	xr->xr_ep = 0;
+	xr->xr_cs = 1;
+}
+
 /*
  * Set TR Dequeue Pointer.
  * xCHI 1.1  4.6.10  6.4.3.9
@@ -1619,13 +1636,7 @@
 	XHCIHIST_FUNC(); XHCIHIST_CALLED();
 	DPRINTFN(4, "slot %u dci %u", xs->xs_idx, dci, 0, 0);
 
-	memset(xr->xr_trb, 0, xr->xr_ntrb * XHCI_TRB_SIZE);
-	usb_syncmem(&xr->xr_dma, 0, xr->xr_ntrb * XHCI_TRB_SIZE,
-	    BUS_DMASYNC_PREWRITE);
-	memset(xr->xr_cookies, 0, xr->xr_ntrb * sizeof(*xr->xr_cookies));
-
-	xr->xr_ep = 0;
-	xr->xr_cs = 1;
+	xhci_host_dequeue(xr);
 
 	/* set DCS */
 	trb.trb_0 = xhci_ring_trbp(xr, 0) | 1; /* XXX */
@@ -2898,6 +2909,10 @@
 	*(uint64_t *)(&cp[2]) = htole64(
 	    xhci_ring_trbp(&xs->xs_ep[XHCI_DCI_EP_CONTROL].xe_tr, 0) |
 	    XHCI_EPCTX_2_DCS_SET(1));
+	/* this performs an implicit dequeue of the ep transfer ring,
+	 * but the call to xhci_ring_init above has dequeued the host
+	 * view of the structures already. 
+	 */
 	cp[4] = htole32(
 		XHCI_EPCTX_4_AVG_TRB_LEN_SET(8)
 		);