On Wed, Jul 6, 2011 at 12:34 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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 #32432.
>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> CC: <stable@xxxxxxxxxx>
>
> ---
>
> drivers/usb/host/ehci-hcd.c | 8 +---
> drivers/usb/host/ehci-q.c | 86 +++++++++++++++++++++-----------------------
> drivers/usb/host/ehci.h | 3 +
> 3 files changed, 47 insertions(+), 50 deletions(-)
>
> Index: usb-3.0/drivers/usb/host/ehci.h
> ===================================================================
> --- usb-3.0.orig/drivers/usb/host/ehci.h
> +++ usb-3.0/drivers/usb/host/ehci.h
> @@ -75,6 +75,7 @@ struct ehci_hcd { /* one per controlle
> struct ehci_qh *async;
> struct ehci_qh *dummy; /* For AMD quirk use */
> struct ehci_qh *reclaim;
> + struct ehci_qh *qh_scan_next;
> unsigned scanning : 1;
>
> /* periodic schedule support */
> @@ -117,7 +118,6 @@ struct ehci_hcd { /* one per controlle
> struct timer_list iaa_watchdog;
> struct timer_list watchdog;
> unsigned long actions;
> - unsigned stamp;
> unsigned periodic_stamp;
> unsigned random_frame;
> unsigned long next_statechange;
> @@ -343,6 +343,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
> Index: usb-3.0/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- usb-3.0.orig/drivers/usb/host/ehci-hcd.c
> +++ usb-3.0/drivers/usb/host/ehci-hcd.c
> @@ -94,7 +94,8 @@ static const char hcd_name [] = "ehci_hc
> #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
> @@ -152,10 +153,7 @@ timer_action(struct ehci_hcd *ehci, enum
> 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);
> Index: usb-3.0/drivers/usb/host/ehci-q.c
> ===================================================================
> --- usb-3.0.orig/drivers/usb/host/ehci-q.c
> +++ usb-3.0/drivers/usb/host/ehci-q.c
> @@ -1231,6 +1231,8 @@ static void start_unlink_async (struct e
>
> 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 */
> @@ -1256,53 +1258,49 @@ static void scan_async (struct ehci_hcd
> struct ehci_qh *qh;
> enum ehci_timer_action action = TIMER_IO_WATCHDOG;
>
> - ehci->stamp = ehci_readl(ehci, &ehci->regs->frame_index);
Interesting, this may fix a possible performance bug[1] too.
I guess that if the HC hw doesn't obey the rule of Interrupt Threshold
Control, then two sequential scan_async may be called in same uframe, the
qh_completions for the 2nd URB will be missed and io watchdog will be
triggered, so degrade performance a lot.
[1], https://bugs.launchpad.net/bugs/624510
> 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.)
> - */
> - 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->qh_next.qh;
> - } while (qh);
> + 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().
> + */
> + 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;
> + }
> +
> + /* 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);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
