Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux