kern/50249: XHCI driver missing delay after address set

[webpages%sprow.co.uk@localhost]

>Number:         50249
>Category:       kern
>Synopsis:       XHCI driver missing delay after address set
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 16 10:40:00 +0000 2015
>Originator:     Sprow
>Release:        
>Organization:
>Environment:
>Description:
The notes in 4.6.5 of the XHCI spec say (via a reference to the USB2 spec section 9.2.6.3) that it is software's responsibility to add an address settling delay after the address is set. 

The XHCI driver doesn't have one, despite large chunks of xhci_new_device() being copied from usbd_new_device() which does.

Rename state variable xr_cs to xr_rcs to avoid confusion with the 'command stop (CS)' bit which is in the same CRCR register.
>How-To-Repeat:

>Fix:
See attached patch.
Line 1578: insert the address settling delay in the same place it is in the main USB stack, even though it was actually set in xhci_init_slot() a few lines earlier.
Others lines: rename of xr_cs to xr_rcs.

--- xhcivar-1_4.h	2015-09-16 11:18:00 +0100
+++ xhcivar.h	2015-09-16 10:18:06 +0100
@@ -45,5 +45,5 @@
 	u_int xr_ntrb;			/* number of elements for above */
 	u_int xr_ep;			/* enqueue pointer */
-	u_int xr_cs;			/* cycle state */
+	u_int xr_rcs;			/* ring cycle state */
 	bool is_halted;
 };

--- xhci-1_29.c	2015-09-16 10:48:24 +0100
+++ xhci.c	2015-09-16 10:16:56 +0100
@@ -114,5 +114,5 @@
 #define XHCI_EVENT_RING_TRBS 256
 #define XHCI_EVENT_RING_SEGMENTS 1
-#define XHCI_TRB_3_ED_BIT XHCI_TRB_3_ISP_BIT
+#define XHCI_TRB_3_ED_BIT XHCI_TRB_3_ISP_BIT /* Event data */
 
 static usbd_status xhci_open(usbd_pipe_handle);
@@ -858,5 +858,5 @@
 	xhci_op_write_8(sc, XHCI_DCBAAP, DMAADDR(&sc->sc_dcbaa_dma, 0));
 	xhci_op_write_8(sc, XHCI_CRCR, xhci_ring_trbp(&sc->sc_cr, 0) |
-	    sc->sc_cr.xr_cs);
+	    sc->sc_cr.xr_rcs);
 
 #if 0
@@ -1111,5 +1111,5 @@
 
 	xr->xr_ep = 0;
-	xr->xr_cs = 1;
+	xr->xr_rcs = 1;
 
 	trb.trb_0 = xhci_ring_trbp(xr, 0) | 1; /* XXX */
@@ -1330,7 +1330,7 @@
 
 	i = er->xr_ep;
-	j = er->xr_cs;
+	j = er->xr_rcs;
 
-	DPRINTFN(16, "xr_ep %d xr_cs %d", i, j, 0, 0);
+	DPRINTFN(16, "xr_ep %d xr_rcs %d", i, j, 0, 0);
 
 	while (1) {
@@ -1353,5 +1353,5 @@
 
 	er->xr_ep = i;
-	er->xr_cs = j;
+	er->xr_rcs = j;
 
 	xhci_rt_write_8(sc, XHCI_ERDP(0), xhci_ring_trbp(er, er->xr_ep) |
@@ -1578,4 +1578,6 @@
 		   we can't yet cope with */
 		KASSERT(addr >= 1 && addr <= 127);
+		/* Allow device time to set new address */
+		usb_delay_ms(dev->bus, USB_SET_ADDRESS_SETTLE);
 		dev->address = addr;
 		/* XXX dev->address not necessarily unique on bus */
@@ -1649,5 +1651,5 @@
 	xr->xr_ntrb = ntrb;
 	xr->xr_ep = 0;
-	xr->xr_cs = 1;
+	xr->xr_rcs = 1;
 	memset(xr->xr_trb, 0, size);
 	usb_syncmem(&xr->xr_dma, 0, size, BUS_DMASYNC_PREWRITE);
@@ -1671,5 +1673,5 @@
 	size_t i;
 	u_int ri;
-	u_int cs;
+	u_int rcs;
 	uint64_t parameter;
 	uint32_t status;
@@ -1686,8 +1688,8 @@
 	}
 
-	DPRINTFN(12, "%p xr_ep 0x%x xr_cs %u", xr, xr->xr_ep, xr->xr_cs, 0);
+	DPRINTFN(12, "%p xr_ep 0x%x xr_rcs %u", xr, xr->xr_ep, xr->xr_rcs, 0);
 
 	ri = xr->xr_ep;
-	cs = xr->xr_cs;
+	rcs = xr->xr_rcs;
 
 	/*
@@ -1709,5 +1711,5 @@
 		status = 0;
 		control = XHCI_TRB_3_TYPE_SET(XHCI_TRB_TYPE_LINK) |
-		    XHCI_TRB_3_TC_BIT | (cs ? XHCI_TRB_3_CYCLE_BIT : 0);
+		    XHCI_TRB_3_TC_BIT | (rcs ? XHCI_TRB_3_CYCLE_BIT : 0);
 		xhci_trb_put(&xr->xr_trb[ri], htole64(parameter),
 		    htole32(status), htole32(control));
@@ -1716,7 +1718,7 @@
 		xr->xr_cookies[ri] = NULL;
 		xr->xr_ep = 0;
-		xr->xr_cs ^= 1;
+		xr->xr_rcs ^= 1;
 		ri = xr->xr_ep;
-		cs = xr->xr_cs;
+		rcs = xr->xr_rcs;
 	}
 
@@ -1729,5 +1731,5 @@
 		control = trbs[i].trb_3;
 
-		if (cs) {
+		if (rcs) {
 			control |= XHCI_TRB_3_CYCLE_BIT;
 		} else {
@@ -1750,5 +1752,5 @@
 		control = trbs[i].trb_3;
 
-		if (xr->xr_cs) {
+		if (xr->xr_rcs) {
 			control |= XHCI_TRB_3_CYCLE_BIT;
 		} else {
@@ -1764,7 +1766,7 @@
 
 	xr->xr_ep = ri;
-	xr->xr_cs = cs;
+	xr->xr_rcs = rcs;
 
-	DPRINTFN(12, "%p xr_ep 0x%x xr_cs %u", xr, xr->xr_ep, xr->xr_cs, 0);
+	DPRINTFN(12, "%p xr_ep 0x%x xr_rcs %u", xr, xr->xr_ep, xr->xr_rcs, 0);
 }