On Sat, Jun 29, 2013 at 1:36 AM, Alan Stern <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
