On Sat, Jun 29, 2013 at 1:36 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Fri, 28 Jun 2013, Ming Lei wrote: > >> ehci-hcd currently unlinks an interrupt QH when it becomes empty, that >> is, after its last URB completes. This works well because in almost >> all cases, the completion handler for an interrupt URB resubmits the >> URB; therefore the QH doesn't become empty and doesn't get unlinked. >> >> When we start using tasklets for URB completion, this scheme won't work >> as well. The resubmission won't occur until the tasklet runs, which >> will be some time after the completion is queued with the tasklet. >> During that delay, the QH will be empty and so will be unlinked >> unnecessarily. >> >> To prevent this problem, this patch adds a 5-ms time delay before empty >> interrupt QHs are unlinked. Most often, during that time the interrupt >> URB will be resubmitted and thus we can avoid unlinking the QH. > > There a few, relatively minor issues... > >> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c >> index 246e124..e2fccc0 100644 >> --- a/drivers/usb/host/ehci-hcd.c >> +++ b/drivers/usb/host/ehci-hcd.c >> @@ -484,6 +484,7 @@ static int ehci_init(struct usb_hcd *hcd) >> ehci->periodic_size = DEFAULT_I_TDPS; >> INIT_LIST_HEAD(&ehci->async_unlink); >> INIT_LIST_HEAD(&ehci->async_idle); >> + INIT_LIST_HEAD(&ehci->intr_unlink_wait); >> INIT_LIST_HEAD(&ehci->intr_unlink); >> INIT_LIST_HEAD(&ehci->intr_qh_list); >> INIT_LIST_HEAD(&ehci->cached_itd_list); > > The ehci_endpoint_disable() routine can be improved. In the > QH_STATE_LINNKED or QH_STATE_COMPLETING case, we now need to handle > interrupt QHs -- the comment about "periodic qh self-unlinks on empty" > isn't entirely correct any more, because now the unlink doesn't occur > until the QH has been empty for 5 ms. Actually, almost all interrupt URBs are resubmitted in its completion handler, that means they are basically stopped by unlinking before disabling endpoint, so I am wondering if we need to consider improving ehci_endpoint_disable() on interrupt endpoint. > > To start with, I think the QH_STATE_COMPLETING case label can be moved > to the QH_STATE_UNLINK section. That case should never happen anyway; > endpoints aren't supposed to be disabled while they are in use. Yes, since all URBs on the endpoint should be in unlinking or unlinked when doing endpoint_disable(). > > Next, the search through the async list can be removed. If an async QH > is in the LINKED state then it must be on the async list. Likewise, if > an intr QH is in the LINKED state then it must be on the intr_qh_list. Right. But I suggest to do the above on in another patch because most of the change is not much related with this patch. > So all we have to do is figure out whether the QH is async or intr. If > it is async, call start_unlink_async(), otherwise call > start_unlink_intr(). We shouldn't have to wait 5 ms for an interrupt > QH to be unlinked. OK, how about the below change? diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index fe27038..0cdaf7f 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -944,6 +944,7 @@ ehci_endpoint_disable (struct usb_hcd *hcd, struct usb_host_endpoint *ep) struct ehci_hcd *ehci = hcd_to_ehci (hcd); unsigned long flags; struct ehci_qh *qh, *tmp; + int eptype = usb_endpoint_type(&ep->desc); /* ASSERT: any requests/urbs are being unlinked */ /* ASSERT: nobody can be submitting urbs for this any more */ @@ -973,17 +974,12 @@ rescan: qh->qh_state = QH_STATE_IDLE; switch (qh->qh_state) { case QH_STATE_LINKED: - case QH_STATE_COMPLETING: - for (tmp = ehci->async->qh_next.qh; - tmp && tmp != qh; - tmp = tmp->qh_next.qh) - continue; - /* periodic qh self-unlinks on empty, and a COMPLETING qh - * may already be unlinked. - */ - if (tmp) + if (eptype != USB_ENDPOINT_XFER_INT) start_unlink_async(ehci, qh); + else + start_unlink_intr(ehci, qh); /* FALL THROUGH */ + case QH_STATE_COMPLETING: /* already in unlinking */ case QH_STATE_UNLINK: /* wait for hw to finish? */ case QH_STATE_UNLINK_WAIT: idle_timeout: >> @@ -632,6 +649,31 @@ static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) >> } >> } >> >> +/* >> + * It is common only one intr URB is scheduled on one qh, and >> + * given complete() is run in tasklet context, introduce a bit >> + * delay to avoid unlink qh too early. >> + */ >> +static void start_unlink_intr_wait(struct ehci_hcd *ehci, >> + struct ehci_qh *qh) >> +{ >> + /* If the QH isn't linked then there's nothing we can do. */ >> + if (qh->qh_state != QH_STATE_LINKED) >> + return; > > This test isn't needed. This routine is called from only one spot, and > there we know that the state is LINKED. The test should be related with your below comment. >> @@ -921,7 +966,7 @@ static void scan_intr(struct ehci_hcd *ehci) >> temp = qh_completions(ehci, qh); >> if (unlikely(temp || (list_empty(&qh->qtd_list) && >> qh->qh_state == QH_STATE_LINKED))) >> - start_unlink_intr(ehci, qh); >> + start_unlink_intr_wait(ehci, qh); > > This is not quite right. If temp is nonzero then we want to unlink the > QH right away (because a fault occurred), so we should call It might depend on the fault type, looks we need to unlink qh immediately on SHUTDOWN and qh->dequeue_during_giveback, and for other non-unlink faults, drivers may not treat it as fault and continue to resubmit URB, such as hub_irq(). So how about the below test? diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c index ed4d2aa..679e704 100644 --- a/drivers/usb/host/ehci-sched.c +++ b/drivers/usb/host/ehci-sched.c @@ -969,8 +969,13 @@ static void scan_intr(struct ehci_hcd *ehci) * in qh_unlink_periodic(). */ temp = qh_completions(ehci, qh); - if (unlikely(temp || (list_empty(&qh->qtd_list) && - qh->qh_state == QH_STATE_LINKED))) + if (unlikely(temp && (qh->dequeue_during_giveback || + ehci->rh_state < EHCI_RH_RUNNING))) + start_unlink_intr(ehci, qh); + else if (unlikely(temp)) + start_unlink_intr_wait(ehci, qh); + else if ((list_empty(&qh->qtd_list) && + qh->qh_state == QH_STATE_LINKED)) start_unlink_intr_wait(ehci, qh); } } > start_unlink_intr(). Otherwise, if the qh->qtd_list is empty and the > state is LINKED then we can call start_unlink_intr_wait(). > >> @@ -98,7 +99,6 @@ static void ehci_enable_event(struct ehci_hcd *ehci, unsigned event, >> } >> } >> >> - >> /* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */ >> static void ehci_poll_ASS(struct ehci_hcd *ehci) >> { > > This blank line should remain. OK. Thanks, -- Ming Lei -- 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