NetBSD Problem Report #51202

From www@NetBSD.org  Tue May 31 09:17:45 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 6B3217A2AE
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 31 May 2016 09:17:45 +0000 (UTC)
Message-Id: <20160531091744.559547ABDC@mollari.NetBSD.org>
Date: Tue, 31 May 2016 09:17:44 +0000 (UTC)
From: webpages@sprow.co.uk
Reply-To: webpages@sprow.co.uk
To: gnats-bugs@NetBSD.org
Subject: XHCI driver sends stale TRBs from a pipe previously opened
X-Send-Pr-Version: www-1.0

>Number:         51202
>Category:       kern
>Synopsis:       XHCI driver sends stale TRBs from a pipe previously opened
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    skrll
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue May 31 09:20:00 +0000 2016
>Closed-Date:    Sun Jun 05 10:46:08 +0000 2016
>Last-Modified:  Sun Jun 05 10:50: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)
 		);

>Release-Note:

>Audit-Trail:
From: Sprow <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/51202: XHCI driver sends stale TRBs from a pipe previously opened
Date: Tue, 31 May 2016 11:05:26 +0100

 Forgot to mention: this wouldn't be seen with clients that open and sit on a
 pipe, such as umass. 

 It's occasional/temporary clients such as printing that are, eg. sending a
 ~5k printer dump direct to the printer bulk endpoint you'd get 1 copy the
 first time, 2 copies the second time, etc...

From: Sprow <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/51202: XHCI driver sends stale TRBs from a pipe previously opened
Date: Tue, 31 May 2016 13:55:49 +0100

 For completeness, xhci_ring_init sharing code for the host side dequeue too.


 --- xhci_1-46.c	2016-05-30 22:44:14 +0100
 +++ xhci.c	2016-05-31 12:52:30 +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 */
 @@ -2449,13 +2460,10 @@
  	if (err)
  		return err;
  	mutex_init(&xr->xr_lock, MUTEX_DEFAULT, IPL_SOFTUSB);
 -	xr->xr_cookies = kmem_zalloc(sizeof(*xr->xr_cookies) * ntrb, KM_SLEEP);
 +	xr->xr_cookies = kmem_alloc(sizeof(*xr->xr_cookies) * ntrb, KM_SLEEP);
  	xr->xr_trb = xhci_ring_trbv(xr, 0);
  	xr->xr_ntrb = ntrb;
 -	xr->xr_ep = 0;
 -	xr->xr_cs = 1;
 -	memset(xr->xr_trb, 0, size);
 -	usb_syncmem(&xr->xr_dma, 0, size, BUS_DMASYNC_PREWRITE);
 +	xhci_host_dequeue(xr);
  	xr->is_halted = false;

  	return USBD_NORMAL_COMPLETION;
 @@ -2898,6 +2906,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)
  		);

Responsible-Changed-From-To: kern-bug-people->skrll
Responsible-Changed-By: skrll@NetBSD.org
Responsible-Changed-When: Sat, 04 Jun 2016 20:53:53 +0000
Responsible-Changed-Why:
take


From: "Nick Hudson" <skrll@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51202 CVS commit: src/sys/dev/usb
Date: Sun, 5 Jun 2016 09:16:02 +0000

 Module Name:	src
 Committed By:	skrll
 Date:		Sun Jun  5 09:16:02 UTC 2016

 Modified Files:
 	src/sys/dev/usb: xhci.c

 Log Message:
 PR/51202: XHCI driver sends stale TRBs from a pipe previously opened

 A similar patch was sent to me by t-hash and the xhci_setup_ctx is taken
 from that.


 To generate a diff of this commit:
 cvs rdiff -u -r1.54 -r1.55 src/sys/dev/usb/xhci.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Sun, 05 Jun 2016 09:34:41 +0000
State-Changed-Why:
A similar patch was committed.  OK to close?


From: Sprow <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: netbsd-bugs@netbsd.org,
	gnats-admin@netbsd.org,
	skrll@NetBSD.org
Subject: Re: kern/51202 (XHCI driver sends stale TRBs from a pipe previously opened)
Date: Sun, 05 Jun 2016 11:36:41 +0100

 On 05 Jun, <skrll@NetBSD.org> wrote:
 > Synopsis: XHCI driver sends stale TRBs from a pipe previously opened
 >
 > State-Changed-From-To: open->feedback
 > State-Changed-By: skrll@NetBSD.org
 > State-Changed-When: Sun, 05 Jun 2016 09:34:41 +0000
 > State-Changed-Why:
 > A similar patch was committed.  OK to close?

 I think you missed an instance in xhci_set_dequeue(). Seems safer to replace
 that with a call to xhci_host_dequeue() too since then there's only one place
 to maintain where that set of 5 dequeuing steps happens.

 Otherwise, looks fine.

State-Changed-From-To: feedback->closed
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Sun, 05 Jun 2016 10:46:08 +0000
State-Changed-Why:
Full fix committed


From: "Nick Hudson" <skrll@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51202 CVS commit: src/sys/dev/usb
Date: Sun, 5 Jun 2016 10:45:16 +0000

 Module Name:	src
 Committed By:	skrll
 Date:		Sun Jun  5 10:45:16 UTC 2016

 Modified Files:
 	src/sys/dev/usb: xhci.c

 Log Message:
 PR/51202: XHCI driver sends stale TRBs from a pipe previously opened

 Use xhci_host_dequeue in xhci_set_dequeue


 To generate a diff of this commit:
 cvs rdiff -u -r1.55 -r1.56 src/sys/dev/usb/xhci.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.