Patchwork [Lucid] USB: EHCI: go back to using the system clock for QH unlinks

login
register
mail settings
Submitter Ming Lei
Date March 18, 2012, 9:23 a.m.
Message ID <20120318172323.48e19095@tom-ThinkPad-T410>
Download mbox | patch
Permalink /patch/147356/
State New
Headers show

Comments

Ming Lei - March 18, 2012, 9:23 a.m.
From b75858acae39d4ed518bf82d4d03a6b9f685dc81 Mon Sep 17 00:00:00 2001
From: Alan Stern <stern@rowland.harvard.edu>
Date: Tue, 5 Jul 2011 12:34:05 -0400
Subject: [PATCH] USB: EHCI: go back to using the system clock for QH unlinks

BugLink: http://bugs.launchpad.net/bugs/624510

This is a backport of commit 004c19682884d4f40000ce1ded53f4a1d0b18206(upstream).

This patch (as1477) fixes a problem affecting a few types of EHCI
controller.  Contrary to what one might expect, these controllers
automatically stop their internal frame counter when no ports are
enabled.  Since ehci-hcd currently relies on the frame counter for
determining when it should unlink QHs from the async schedule, those
controllers run into trouble: The frame counter stops and the QHs
never get unlinked.

Some systems have also experienced other problems traced back to
commit b963801164618e25fbdc0cd452ce49c3628b46c8 (USB: ehci-hcd unlink
speedups), which made the original switch from using the system clock
to using the frame counter.  It never became clear what the reason was
for these problems, but evidently it is related to use of the frame
counter.

To fix all these problems, this patch more or less reverts that commit
and goes back to using the system clock.  But this can't be done
cleanly because other changes have since been made to the scan_async()
subroutine.  One of these changes involved the tricky logic that tries
to avoid rescanning QHs that have already been seen when the scanning
loop is restarted, which happens whenever an URB is given back.
Switching back to clock-based unlinks would make this logic even more
complicated.

Therefore the new code doesn't rescan the entire async list whenever a
giveback occurs.  Instead it rescans only the current QH and continues
on from there.  This requires the use of a separate pointer to keep
track of the next QH to scan, since the current QH may be unlinked
while the scanning is in progress.  That new pointer must be global,
so that it can be adjusted forward whenever the _next_ QH gets
unlinked.  (uhci-hcd uses this same trick.)

Simplification of the scanning loop removes a level of indentation,
which accounts for the size of the patch.  The amount of code changed
is relatively small, and it isn't exactly a reversion of the
b963801164 commit.

This fixes Bugzilla #624510.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Tested-by: Matej Kenda <matejken@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/usb/host/ehci-hcd.c |    8 ++---
 drivers/usb/host/ehci-q.c   |   82 +++++++++++++++++++++----------------------
 drivers/usb/host/ehci.h     |    4 ++-
 3 files changed, 46 insertions(+), 48 deletions(-)
Stefan Bader - March 19, 2012, 2:53 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 18.03.2012 10:23, Ming Lei wrote:
> From b75858acae39d4ed518bf82d4d03a6b9f685dc81 Mon Sep 17 00:00:00 2001 
> From: Alan Stern <stern@rowland.harvard.edu> Date: Tue, 5 Jul 2011 12:34:05
> -0400 Subject: [PATCH] USB: EHCI: go back to using the system clock for QH
> unlinks
> 
> BugLink: http://bugs.launchpad.net/bugs/624510
> 
> This is a backport of commit
> 004c19682884d4f40000ce1ded53f4a1d0b18206(upstream).

While not incorrect, the two lines may be documenting the flow of the patch
better when in the sob block.

> 
> This patch (as1477) fixes a problem affecting a few types of EHCI 
> controller.  Contrary to what one might expect, these controllers 
> automatically stop their internal frame counter when no ports are enabled.
> Since ehci-hcd currently relies on the frame counter for determining when
> it should unlink QHs from the async schedule, those controllers run into
> trouble: The frame counter stops and the QHs never get unlinked.
> 
> Some systems have also experienced other problems traced back to commit
> b963801164618e25fbdc0cd452ce49c3628b46c8 (USB: ehci-hcd unlink speedups),
> which made the original switch from using the system clock to using the
> frame counter.  It never became clear what the reason was for these
> problems, but evidently it is related to use of the frame counter.
> 
> To fix all these problems, this patch more or less reverts that commit and
> goes back to using the system clock.  But this can't be done cleanly
> because other changes have since been made to the scan_async() subroutine.
> One of these changes involved the tricky logic that tries to avoid
> rescanning QHs that have already been seen when the scanning loop is
> restarted, which happens whenever an URB is given back. Switching back to
> clock-based unlinks would make this logic even more complicated.
> 
> Therefore the new code doesn't rescan the entire async list whenever a 
> giveback occurs.  Instead it rescans only the current QH and continues on
> from there.  This requires the use of a separate pointer to keep track of
> the next QH to scan, since the current QH may be unlinked while the
> scanning is in progress.  That new pointer must be global, so that it can
> be adjusted forward whenever the _next_ QH gets unlinked.  (uhci-hcd uses
> this same trick.)
> 
> Simplification of the scanning loop removes a level of indentation, which
> accounts for the size of the patch.  The amount of code changed is
> relatively small, and it isn't exactly a reversion of the b963801164
> commit.
> 
> This fixes Bugzilla #624510.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

The upstream patch has a Cc to stable here. This should not get dropped:
CC: <stable@kernel.org>

> Tested-by: Matej Kenda <matejken@gmail.com> Signed-off-by: Greg
> Kroah-Hartman <gregkh@suse.de>

BugLink: http://bugs.launchpad.net/bugs/624510

(backported from 004c19682884d4f40000ce1ded53f4a1d0b18206 upstream)
Signed-off-by: Ming Lei <ming.lei@canonical.com>

> --- drivers/usb/host/ehci-hcd.c |    8 ++--- drivers/usb/host/ehci-q.c   |
> 82 +++++++++++++++++++++---------------------- drivers/usb/host/ehci.h
> |    4 ++- 3 files changed, 46 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c 
> index 7b2e99c..8d17f780 100644 --- a/drivers/usb/host/ehci-hcd.c +++
> b/drivers/usb/host/ehci-hcd.c @@ -84,7 +84,8 @@ static const char	hcd_name
> [] = "ehci_hcd"; #define EHCI_IAA_MSECS		10		/* arbitrary */ #define
> EHCI_IO_JIFFIES		(HZ/10)		/* io watchdog > irq_thresh */ #define
> EHCI_ASYNC_JIFFIES	(HZ/20)		/* async idle timeout */ -#define
> EHCI_SHRINK_FRAMES	5		/* async qh unlink delay */ +#define
> EHCI_SHRINK_JIFFIES	(DIV_ROUND_UP(HZ, 200) + 1) +						/* 200-ms async qh
> unlink delay */
> 
> /* Initial IRQ latency:  faster than hw default */ static int
> log2_irq_thresh = 0;		// 0 to 6 @@ -139,10 +140,7 @@ timer_action(struct
> ehci_hcd *ehci, enum ehci_timer_action action) break; /* case
> TIMER_ASYNC_SHRINK: */ default: -			/* add a jiffie since we synch against
> the -			 * 8 KHz uframe counter. -			 */ -			t =
> DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1; +			t =
> EHCI_SHRINK_JIFFIES; break; } mod_timer(&ehci->watchdog, t + jiffies); diff
> --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c index
> 0ee5b4b..3b8fa18 100644 --- a/drivers/usb/host/ehci-q.c +++
> b/drivers/usb/host/ehci-q.c @@ -1204,6 +1204,8 @@ static void
> start_unlink_async (struct ehci_hcd *ehci, struct ehci_qh *qh)
> 
> prev->hw->hw_next = qh->hw->hw_next; prev->qh_next = qh->qh_next; +	if
> (ehci->qh_scan_next == qh) +		ehci->qh_scan_next = qh->qh_next.qh; wmb ();
> 
> /* If the controller isn't running, we don't have to wait for it */ @@
> -1229,53 +1231,49 @@ static void scan_async (struct ehci_hcd *ehci) struct
> ehci_qh		*qh; enum ehci_timer_action	action = TIMER_IO_WATCHDOG;
> 
> -	ehci->stamp = ehci_readl(ehci, &ehci->regs->frame_index); 
> timer_action_done (ehci, TIMER_ASYNC_SHRINK); -rescan: stopped =
> !HC_IS_RUNNING(ehci_to_hcd(ehci)->state); -	qh = ehci->async->qh_next.qh; -
> if (likely (qh != NULL)) { -		do { -			/* clean any finished work for this
> qh */ -			if (!list_empty(&qh->qtd_list) && (stopped || -					qh->stamp !=
> ehci->stamp)) { -				int temp; - -				/* unlinks could happen here;
> completion -				 * reporting drops the lock.  rescan using -				 * the
> latest schedule, but don't rescan -				 * qhs we already finished (no
> looping) -				 * unless the controller is stopped. -				 */ -				qh =
> qh_get (qh); -				qh->stamp = ehci->stamp; -				temp = qh_completions
> (ehci, qh); -				if (qh->needs_rescan) -					unlink_async(ehci, qh); -
> qh_put (qh); -				if (temp != 0) { -					goto rescan; -				} -			}
> 
> -			/* unlink idle entries, reducing DMA usage as well -			 * as HCD
> schedule-scanning costs.  delay for any qh -			 * we just scanned, there's
> a not-unusual case that it -			 * doesn't stay idle for long. -			 * (plus,
> avoids some kind of re-activation race.) +	ehci->qh_scan_next =
> ehci->async->qh_next.qh; +	while (ehci->qh_scan_next) { +		qh =
> ehci->qh_scan_next; +		ehci->qh_scan_next = qh->qh_next.qh; + rescan: +		/*
> clean any finished work for this qh */ +		if (!list_empty(&qh->qtd_list))
> { +			int temp; + +			/* +			 * Unlinks could happen here; completion
> reporting +			 * drops the lock.  That's why ehci->qh_scan_next +			 *
> always holds the next qh to scan; if the next qh +			 * gets unlinked then
> ehci->qh_scan_next is adjusted +			 * in start_unlink_async(). */ -			if
> (list_empty(&qh->qtd_list) -					&& qh->qh_state == QH_STATE_LINKED) { -
> if (!ehci->reclaim && (stopped || -					((ehci->stamp - qh->stamp) &
> 0x1fff) -						>= EHCI_SHRINK_FRAMES * 8)) -					start_unlink_async(ehci,
> qh); -				else -					action = TIMER_ASYNC_SHRINK; -			} +			qh =
> qh_get(qh); +			temp = qh_completions(ehci, qh); +			if (qh->needs_rescan) 
> +				unlink_async(ehci, qh); +			qh->unlink_time = jiffies +
> EHCI_SHRINK_JIFFIES; +			qh_put(qh); +			if (temp != 0) +				goto rescan; +
> }
> 
> -			qh = qh->qh_next.qh; -		} while (qh); +		/* unlink idle entries,
> reducing DMA usage as well +		 * as HCD schedule-scanning costs.  delay for
> any qh +		 * we just scanned, there's a not-unusual case that it +		 *
> doesn't stay idle for long. +		 * (plus, avoids some kind of re-activation
> race.) +		 */ +		if (list_empty(&qh->qtd_list) +				&& qh->qh_state ==
> QH_STATE_LINKED) { +			if (!ehci->reclaim && (stopped || +
> time_after_eq(jiffies, qh->unlink_time))) +				start_unlink_async(ehci,
> qh); +			else +				action = TIMER_ASYNC_SHRINK; +		} } if (action ==
> TIMER_ASYNC_SHRINK) timer_action (ehci, TIMER_ASYNC_SHRINK); diff --git
> a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index 5b3ca74..ad8a65e
> 100644 --- a/drivers/usb/host/ehci.h +++ b/drivers/usb/host/ehci.h @@ -74,6
> +74,7 @@ struct ehci_hcd {			/* one per controller */ /* async schedule
> support */ struct ehci_qh		*async; struct ehci_qh		*reclaim; +	struct
> ehci_qh		*qh_scan_next; unsigned		scanning : 1;
> 
> /* periodic schedule support */ @@ -116,7 +117,7 @@ struct ehci_hcd {			/*
> one per controller */ struct timer_list	iaa_watchdog; struct timer_list
> watchdog; unsigned long		actions; -	unsigned		stamp; +	unsigned
> periodic_stamp; unsigned		random_frame; unsigned long		next_statechange; 
> ktime_t			last_periodic_enable; @@ -335,6 +336,7 @@ struct ehci_qh { struct
> ehci_qh		*reclaim;	/* next to reclaim */
> 
> struct ehci_hcd		*ehci; +	unsigned long		unlink_time;
> 
> /* * Do NOT use atomic operations for QH refcounting. On some CPUs

Ok, looks to be like the upstream change. It was deemed stable material, too.
But either because it did not apply or because it went into the black whole of
what is the old stable@kernel.org address.

Acked-by: Stefan Bader <stefan.bader@canonical.com>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBCgAGBQJPZ0hQAAoJEOhnXe7L7s6jx8oP/jcsWGYer7OQNYKyyrUS1jiO
/ZS+N4iAzdt85mGUPVKqNckZuQQLmeKRZ/pNyCww3uM7KnKCEqjqZKZMr5WjeJaa
FReRUyGKLjvv37rHgJLCpCXyDUyTcBmSIj9D5nEn+pdgrDauLATxxWDdTgq2E2tj
k8JNunr25iqey3bAfz8Z2xm9hfpRJITqlYVsBQRLn9INHhESpnCwyZBvq4lI1NKE
lKGQ4tIV2lX2ll0Ykpgyezr5DmDe4OQC0+WTLiWyVbIbNGzmvBs0TfyHLIei+4sM
U3gxLtxbTBTuSZdMZx+MUDmO10B37/DQwZ3OejjQqsBZAV5YRLK0OOJHW1FLxX9+
UkA087SPhgJNtVEcXrwnmRvqZz69awdXkzOYdRLd9g3u8eTMSNUdY3hYmePvLQ9c
PYUh3buVDDmV+bGTHar9FfqZjoXTBo+t1afDXIuPR1pSwDu5pzTdW+wXx1ks/5MQ
tM8Goth13OpOFEPo5vmy+zR0NolY6R17JYzUsbT6gbJRc6ZZx4G8f9d0iXXDPywN
OUt97kAGV4f5Xchvz3HtJMvgQp2AsqhPvOnYQoEpjjM2YAomJsv9hyJJlEon0m2L
3Tp+hJW8Ek8NWZWVH1j8UTVoR06hguUopU0SZaymhrSoHq6cv/Vq4gl5l0y2Vm7l
VwChRBfHgjaH0xoJSW9G
=oTZa
-----END PGP SIGNATURE-----
Tim Gardner - March 19, 2012, 3:09 p.m.
Applied per Stefan's comments

Patch

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 7b2e99c..8d17f780 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -84,7 +84,8 @@  static const char	hcd_name [] = "ehci_hcd";
 #define EHCI_IAA_MSECS		10		/* arbitrary */
 #define EHCI_IO_JIFFIES		(HZ/10)		/* io watchdog > irq_thresh */
 #define EHCI_ASYNC_JIFFIES	(HZ/20)		/* async idle timeout */
-#define EHCI_SHRINK_FRAMES	5		/* async qh unlink delay */
+#define EHCI_SHRINK_JIFFIES	(DIV_ROUND_UP(HZ, 200) + 1)
+						/* 200-ms async qh unlink delay */
 
 /* Initial IRQ latency:  faster than hw default */
 static int log2_irq_thresh = 0;		// 0 to 6
@@ -139,10 +140,7 @@  timer_action(struct ehci_hcd *ehci, enum ehci_timer_action action)
 			break;
 		/* case TIMER_ASYNC_SHRINK: */
 		default:
-			/* add a jiffie since we synch against the
-			 * 8 KHz uframe counter.
-			 */
-			t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
+			t = EHCI_SHRINK_JIFFIES;
 			break;
 		}
 		mod_timer(&ehci->watchdog, t + jiffies);
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 0ee5b4b..3b8fa18 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -1204,6 +1204,8 @@  static void start_unlink_async (struct ehci_hcd *ehci, struct ehci_qh *qh)
 
 	prev->hw->hw_next = qh->hw->hw_next;
 	prev->qh_next = qh->qh_next;
+	if (ehci->qh_scan_next == qh)
+		ehci->qh_scan_next = qh->qh_next.qh;
 	wmb ();
 
 	/* If the controller isn't running, we don't have to wait for it */
@@ -1229,53 +1231,49 @@  static void scan_async (struct ehci_hcd *ehci)
 	struct ehci_qh		*qh;
 	enum ehci_timer_action	action = TIMER_IO_WATCHDOG;
 
-	ehci->stamp = ehci_readl(ehci, &ehci->regs->frame_index);
 	timer_action_done (ehci, TIMER_ASYNC_SHRINK);
-rescan:
 	stopped = !HC_IS_RUNNING(ehci_to_hcd(ehci)->state);
-	qh = ehci->async->qh_next.qh;
-	if (likely (qh != NULL)) {
-		do {
-			/* clean any finished work for this qh */
-			if (!list_empty(&qh->qtd_list) && (stopped ||
-					qh->stamp != ehci->stamp)) {
-				int temp;
-
-				/* unlinks could happen here; completion
-				 * reporting drops the lock.  rescan using
-				 * the latest schedule, but don't rescan
-				 * qhs we already finished (no looping)
-				 * unless the controller is stopped.
-				 */
-				qh = qh_get (qh);
-				qh->stamp = ehci->stamp;
-				temp = qh_completions (ehci, qh);
-				if (qh->needs_rescan)
-					unlink_async(ehci, qh);
-				qh_put (qh);
-				if (temp != 0) {
-					goto rescan;
-				}
-			}
 
-			/* unlink idle entries, reducing DMA usage as well
-			 * as HCD schedule-scanning costs.  delay for any qh
-			 * we just scanned, there's a not-unusual case that it
-			 * doesn't stay idle for long.
-			 * (plus, avoids some kind of re-activation race.)
+	ehci->qh_scan_next = ehci->async->qh_next.qh;
+	while (ehci->qh_scan_next) {
+		qh = ehci->qh_scan_next;
+		ehci->qh_scan_next = qh->qh_next.qh;
+ rescan:
+		/* clean any finished work for this qh */
+		if (!list_empty(&qh->qtd_list)) {
+			int temp;
+
+			/*
+			 * Unlinks could happen here; completion reporting
+			 * drops the lock.  That's why ehci->qh_scan_next
+			 * always holds the next qh to scan; if the next qh
+			 * gets unlinked then ehci->qh_scan_next is adjusted
+			 * in start_unlink_async().
 			 */
-			if (list_empty(&qh->qtd_list)
-					&& qh->qh_state == QH_STATE_LINKED) {
-				if (!ehci->reclaim && (stopped ||
-					((ehci->stamp - qh->stamp) & 0x1fff)
-						>= EHCI_SHRINK_FRAMES * 8))
-					start_unlink_async(ehci, qh);
-				else
-					action = TIMER_ASYNC_SHRINK;
-			}
+			qh = qh_get(qh);
+			temp = qh_completions(ehci, qh);
+			if (qh->needs_rescan)
+				unlink_async(ehci, qh);
+			qh->unlink_time = jiffies + EHCI_SHRINK_JIFFIES;
+			qh_put(qh);
+			if (temp != 0)
+				goto rescan;
+		}
 
-			qh = qh->qh_next.qh;
-		} while (qh);
+		/* unlink idle entries, reducing DMA usage as well
+		 * as HCD schedule-scanning costs.  delay for any qh
+		 * we just scanned, there's a not-unusual case that it
+		 * doesn't stay idle for long.
+		 * (plus, avoids some kind of re-activation race.)
+		 */
+		if (list_empty(&qh->qtd_list)
+				&& qh->qh_state == QH_STATE_LINKED) {
+			if (!ehci->reclaim && (stopped ||
+					time_after_eq(jiffies, qh->unlink_time)))
+				start_unlink_async(ehci, qh);
+			else
+				action = TIMER_ASYNC_SHRINK;
+		}
 	}
 	if (action == TIMER_ASYNC_SHRINK)
 		timer_action (ehci, TIMER_ASYNC_SHRINK);
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 5b3ca74..ad8a65e 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -74,6 +74,7 @@  struct ehci_hcd {			/* one per controller */
 	/* async schedule support */
 	struct ehci_qh		*async;
 	struct ehci_qh		*reclaim;
+	struct ehci_qh		*qh_scan_next;
 	unsigned		scanning : 1;
 
 	/* periodic schedule support */
@@ -116,7 +117,7 @@  struct ehci_hcd {			/* one per controller */
 	struct timer_list	iaa_watchdog;
 	struct timer_list	watchdog;
 	unsigned long		actions;
-	unsigned		stamp;
+	unsigned		periodic_stamp;
 	unsigned		random_frame;
 	unsigned long		next_statechange;
 	ktime_t			last_periodic_enable;
@@ -335,6 +336,7 @@  struct ehci_qh {
 	struct ehci_qh		*reclaim;	/* next to reclaim */
 
 	struct ehci_hcd		*ehci;
+	unsigned long		unlink_time;
 
 	/*
 	 * Do NOT use atomic operations for QH refcounting. On some CPUs