| Message ID | 1372066925-23579-4-git-send-email-ming.lei@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 24 June 2013 17:42:04 Ming Lei wrote: > This patch improves this above situation, and the qh will wait for 5 > milliseconds before being unlinked from hardware, if one URB is submitted > during the period, the qh is move out of unlink wait list and the > interrupt transfer can be scheduled immediately, not like before: the > transfer is only linked to hardware until previous unlink is completed. It seems to me that this logic should be used only if the URB completed without error. Regards Oliver
On Mon, Jun 24, 2013 at 6:28 PM, Oliver Neukum <oliver@neukum.org> wrote: > On Monday 24 June 2013 17:42:04 Ming Lei wrote: >> This patch improves this above situation, and the qh will wait for 5 >> milliseconds before being unlinked from hardware, if one URB is submitted >> during the period, the qh is move out of unlink wait list and the >> interrupt transfer can be scheduled immediately, not like before: the >> transfer is only linked to hardware until previous unlink is completed. > > It seems to me that this logic should be used only if the URB completed > without error. The current completed URB with error doesn't mean the later submitted URB will complete with error, so I don't think the logic is related with URB completion error. Thanks, -- Ming Lei
On Monday 24 June 2013 19:16:43 Ming Lei wrote: > On Mon, Jun 24, 2013 at 6:28 PM, Oliver Neukum <oliver@neukum.org> wrote: > > On Monday 24 June 2013 17:42:04 Ming Lei wrote: > >> This patch improves this above situation, and the qh will wait for 5 > >> milliseconds before being unlinked from hardware, if one URB is submitted > >> during the period, the qh is move out of unlink wait list and the > >> interrupt transfer can be scheduled immediately, not like before: the > >> transfer is only linked to hardware until previous unlink is completed. > > > > It seems to me that this logic should be used only if the URB completed > > without error. > > The current completed URB with error doesn't mean the later submitted > URB will complete with error, so I don't think the logic is related with URB > completion error. But a) it is likelier b) the driver will start error handling And what about the bandwidth? At least if you unlink the URB, its bandwidth should be given back immediately. Regards Oliver
On Mon, Jun 24, 2013 at 8:08 PM, Oliver Neukum <oliver@neukum.org> wrote: > On Monday 24 June 2013 19:16:43 Ming Lei wrote: >> On Mon, Jun 24, 2013 at 6:28 PM, Oliver Neukum <oliver@neukum.org> wrote: >> > On Monday 24 June 2013 17:42:04 Ming Lei wrote: >> >> This patch improves this above situation, and the qh will wait for 5 >> >> milliseconds before being unlinked from hardware, if one URB is submitted >> >> during the period, the qh is move out of unlink wait list and the >> >> interrupt transfer can be scheduled immediately, not like before: the >> >> transfer is only linked to hardware until previous unlink is completed. >> > >> > It seems to me that this logic should be used only if the URB completed >> > without error. >> >> The current completed URB with error doesn't mean the later submitted >> URB will complete with error, so I don't think the logic is related with URB >> completion error. > > But > > a) it is likelier > b) the driver will start error handling > > And what about the bandwidth? At least if you unlink the URB, its bandwidth > should be given back immediately. Yes, the patch doesn't affect unlinking intr URB because the behavior of start_unlink_intr() isn't changed basically. Thanks, -- Ming Lei
On Mon, 24 Jun 2013, Ming Lei wrote: > Given interrupt URB will be resubmitted from tasklet context which > is scheduled by ehci hardware interrupt handler, and commonly only > one interrupt URB is scheduled on qh, so the qh may be unlinked > immediately once qh_completions() returns from ehci_irq(), then > the intr URB to be resubmitted in complete() can only be scheduled > and linked to hardware until the qh unlink is completed. > > This patch improves this above situation, and the qh will wait for 5 > milliseconds before being unlinked from hardware, if one URB is submitted > during the period, the qh is move out of unlink wait list and the > interrupt transfer can be scheduled immediately, not like before: the > transfer is only linked to hardware until previous unlink is completed. This description is very hard to understand. I suggest rewriting it, something like this: 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. > diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c > index f80d033..5bf67e2 100644 > --- a/drivers/usb/host/ehci-sched.c > +++ b/drivers/usb/host/ehci-sched.c > @@ -601,12 +601,24 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci, struct ehci_qh *qh) > list_del(&qh->intr_node); > } > > -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) > +/* must be called with holding ehci->lock */ The comment should be: /* caller must hold ehci->lock */ But in fact you can leave it out. Almost all the code in this file runs with the lock held. > +static void cancel_unlink_wait_intr(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) > + if (qh->qh_state != QH_STATE_LINKED || list_empty(&qh->unlink_node)) > return; > > + list_del_init(&qh->unlink_node); > + > + /* avoid unnecessary CPU wakeup */ > + if (list_empty(&ehci->intr_unlink_wait)) > + ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR); If you don't mind, can we leave out ehci_disable_event() for now and add it after the rest of this series is merged? It will keeps things a little simpler, and then we'll be able to use ehci_disable_event() for the IAA watchdog timer event as well as using it here. > +} > + > +static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) > +{ > + /* if the qh is waitting for unlink, cancel it now */ s/waitt/wait/ > + cancel_unlink_wait_intr(ehci, qh); > + > qh_unlink_periodic (ehci, qh); > > /* Make sure the unlinks are visible before starting the timer */ > @@ -632,6 +644,45 @@ 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(struct ehci_hcd *ehci, struct ehci_qh *qh, > + bool wait) > +{ > + /* If the QH isn't linked then there's nothing we can do. */ > + if (qh->qh_state != QH_STATE_LINKED) > + return; > + > + if (!wait) > + return start_do_unlink_intr(ehci, qh); > + > + qh->unlink_cycle = ehci->intr_unlink_wait_cycle; > + > + /* New entries go at the end of the intr_unlink_wait list */ > + list_add_tail(&qh->unlink_node, &ehci->intr_unlink_wait); > + > + if (ehci->rh_state < EHCI_RH_RUNNING) > + ehci_handle_start_intr_unlinks(ehci); > + else if (ehci->intr_unlink_wait.next == &qh->unlink_node) { > + ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true); > + ++ehci->intr_unlink_wait_cycle; > + } > +} > + > +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) > +{ > + __start_unlink_intr(ehci, qh, false); > +} > + > +static void start_unlink_intr_wait(struct ehci_hcd *ehci, > + struct ehci_qh *qh) > +{ > + __start_unlink_intr(ehci, qh, true); > +} This is not what I had in mind. Instead, copy the qh->qh_state test into the beginning of start_unlink_intr() and move the rest of start_do_unlink_intr() there. Then you can rename __start_unlink_intr() to start_unlink_intr_wait() and get rid of the "wait" parameter. > @@ -881,6 +932,9 @@ static int intr_submit ( > goto done; > } > > + /* put back qh from unlink wait list */ > + cancel_unlink_wait_intr(ehci, qh); It might be better to change this line into an "else" clause for the "if (qh->qh_state == QH_STATE_IDLE)" statement after the BUG_ON below. > + > /* then queue the urb's tds to the qh */ > qh = qh_append_tds(ehci, urb, qtd_list, epnum, &urb->ep->hcpriv); > BUG_ON (qh == NULL); > +static void ehci_handle_start_intr_unlinks(struct ehci_hcd *ehci) > +{ > + bool stopped = (ehci->rh_state < EHCI_RH_RUNNING); > + > + /* > + * Process all the QHs on the intr_unlink list that were added > + * before the current unlink cycle began. The list is in > + * temporal order, so stop when we reach the first entry in the > + * current cycle. But if the root hub isn't running then > + * process all the QHs on the list. > + */ > + while (!list_empty(&ehci->intr_unlink_wait)) { > + struct ehci_qh *qh; > + > + qh = list_first_entry(&ehci->intr_unlink_wait, > + struct ehci_qh, unlink_node); As I mentioned before, this line should be indented by two tab stops beyond the preceding line. > + if (!stopped && > + (qh->unlink_cycle == This doesn't need to be on a separate line at all. > + ehci->intr_unlink_wait_cycle)) And this should also be indented by two tab stops beyond the preceding line. > --- a/drivers/usb/host/ehci.h > +++ b/drivers/usb/host/ehci.h > @@ -88,6 +88,7 @@ enum ehci_hrtimer_event { > EHCI_HRTIMER_POLL_DEAD, /* Wait for dead controller to stop */ > EHCI_HRTIMER_UNLINK_INTR, /* Wait for interrupt QH unlink */ > EHCI_HRTIMER_FREE_ITDS, /* Wait for unused iTDs and siTDs */ > + EHCI_HRTIMER_START_UNLINK_INTR, /* wait for new intr schedule */ The comment should be: /* Unlink empty interrupt QHs */ > EHCI_HRTIMER_ASYNC_UNLINKS, /* Unlink empty async QHs */ > EHCI_HRTIMER_IAA_WATCHDOG, /* Handle lost IAA interrupts */ > EHCI_HRTIMER_DISABLE_PERIODIC, /* Wait to disable periodic sched */ > @@ -143,6 +144,8 @@ struct ehci_hcd { /* one per controller */ > unsigned i_thresh; /* uframes HC might cache */ > > union ehci_shadow *pshadow; /* mirror hw periodic table */ > + struct list_head intr_unlink_wait; > + unsigned intr_unlink_wait_cycle; > struct list_head intr_unlink; > unsigned intr_unlink_cycle; > unsigned now_frame; /* frame from HC hardware */ To avoid an alignment gap on 64-bit systems, put intr_unlink_wait_cycle after intr_unlink. Alan Stern
On Tue, Jun 25, 2013 at 2:53 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 24 Jun 2013, Ming Lei wrote: > >> Given interrupt URB will be resubmitted from tasklet context which >> is scheduled by ehci hardware interrupt handler, and commonly only >> one interrupt URB is scheduled on qh, so the qh may be unlinked >> immediately once qh_completions() returns from ehci_irq(), then >> the intr URB to be resubmitted in complete() can only be scheduled >> and linked to hardware until the qh unlink is completed. >> >> This patch improves this above situation, and the qh will wait for 5 >> milliseconds before being unlinked from hardware, if one URB is submitted >> during the period, the qh is move out of unlink wait list and the >> interrupt transfer can be scheduled immediately, not like before: the >> transfer is only linked to hardware until previous unlink is completed. > > This description is very hard to understand. I suggest rewriting it, > something like this: > > 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. Excellent description about the change, and I will add it in V3, also care to add your signed-off in the patch for the change log part? > >> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c >> index f80d033..5bf67e2 100644 >> --- a/drivers/usb/host/ehci-sched.c >> +++ b/drivers/usb/host/ehci-sched.c >> @@ -601,12 +601,24 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci, struct ehci_qh *qh) >> list_del(&qh->intr_node); >> } >> >> -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) >> +/* must be called with holding ehci->lock */ > > The comment should be: > > /* caller must hold ehci->lock */ > > But in fact you can leave it out. Almost all the code in this file > runs with the lock held. OK. > >> +static void cancel_unlink_wait_intr(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) >> + if (qh->qh_state != QH_STATE_LINKED || list_empty(&qh->unlink_node)) >> return; >> >> + list_del_init(&qh->unlink_node); >> + >> + /* avoid unnecessary CPU wakeup */ >> + if (list_empty(&ehci->intr_unlink_wait)) >> + ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR); > > If you don't mind, can we leave out ehci_disable_event() for now and > add it after the rest of this series is merged? It will keeps things a > little simpler, and then we'll be able to use ehci_disable_event() for > the IAA watchdog timer event as well as using it here. I am fine, so this patch will become simpler and has less change. > >> +} >> + >> +static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) >> +{ >> + /* if the qh is waitting for unlink, cancel it now */ > > s/waitt/wait/ > >> + cancel_unlink_wait_intr(ehci, qh); >> + >> qh_unlink_periodic (ehci, qh); >> >> /* Make sure the unlinks are visible before starting the timer */ >> @@ -632,6 +644,45 @@ 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(struct ehci_hcd *ehci, struct ehci_qh *qh, >> + bool wait) >> +{ >> + /* If the QH isn't linked then there's nothing we can do. */ >> + if (qh->qh_state != QH_STATE_LINKED) >> + return; >> + >> + if (!wait) >> + return start_do_unlink_intr(ehci, qh); >> + >> + qh->unlink_cycle = ehci->intr_unlink_wait_cycle; >> + >> + /* New entries go at the end of the intr_unlink_wait list */ >> + list_add_tail(&qh->unlink_node, &ehci->intr_unlink_wait); >> + >> + if (ehci->rh_state < EHCI_RH_RUNNING) >> + ehci_handle_start_intr_unlinks(ehci); >> + else if (ehci->intr_unlink_wait.next == &qh->unlink_node) { >> + ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true); >> + ++ehci->intr_unlink_wait_cycle; >> + } >> +} >> + >> +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) >> +{ >> + __start_unlink_intr(ehci, qh, false); >> +} >> + >> +static void start_unlink_intr_wait(struct ehci_hcd *ehci, >> + struct ehci_qh *qh) >> +{ >> + __start_unlink_intr(ehci, qh, true); >> +} > > This is not what I had in mind. > > Instead, copy the qh->qh_state test into the beginning of > start_unlink_intr() and move the rest of start_do_unlink_intr() there. > Then you can rename __start_unlink_intr() to start_unlink_intr_wait() > and get rid of the "wait" parameter. The current code can do the check centrally, but it isn't big deal, and I will follow your suggestion. > >> @@ -881,6 +932,9 @@ static int intr_submit ( >> goto done; >> } >> >> + /* put back qh from unlink wait list */ >> + cancel_unlink_wait_intr(ehci, qh); > > It might be better to change this line into an "else" clause for the > "if (qh->qh_state == QH_STATE_IDLE)" statement after the BUG_ON below. OK. > >> + >> /* then queue the urb's tds to the qh */ >> qh = qh_append_tds(ehci, urb, qtd_list, epnum, &urb->ep->hcpriv); >> BUG_ON (qh == NULL); > >> +static void ehci_handle_start_intr_unlinks(struct ehci_hcd *ehci) >> +{ >> + bool stopped = (ehci->rh_state < EHCI_RH_RUNNING); >> + >> + /* >> + * Process all the QHs on the intr_unlink list that were added >> + * before the current unlink cycle began. The list is in >> + * temporal order, so stop when we reach the first entry in the >> + * current cycle. But if the root hub isn't running then >> + * process all the QHs on the list. >> + */ >> + while (!list_empty(&ehci->intr_unlink_wait)) { >> + struct ehci_qh *qh; >> + >> + qh = list_first_entry(&ehci->intr_unlink_wait, >> + struct ehci_qh, unlink_node); > > As I mentioned before, this line should be indented by two tab stops > beyond the preceding line. Looks I forget this one. > >> + if (!stopped && >> + (qh->unlink_cycle == > > This doesn't need to be on a separate line at all. > >> + ehci->intr_unlink_wait_cycle)) > > And this should also be indented by two tab stops beyond the preceding > line. OK. BTW, indenting two tab might cause more lines, and looks many subsystems don't do that. But, anyway, I am fine with two tab, :-) > >> --- a/drivers/usb/host/ehci.h >> +++ b/drivers/usb/host/ehci.h >> @@ -88,6 +88,7 @@ enum ehci_hrtimer_event { >> EHCI_HRTIMER_POLL_DEAD, /* Wait for dead controller to stop */ >> EHCI_HRTIMER_UNLINK_INTR, /* Wait for interrupt QH unlink */ >> EHCI_HRTIMER_FREE_ITDS, /* Wait for unused iTDs and siTDs */ >> + EHCI_HRTIMER_START_UNLINK_INTR, /* wait for new intr schedule */ > > The comment should be: /* Unlink empty interrupt QHs */ OK > >> EHCI_HRTIMER_ASYNC_UNLINKS, /* Unlink empty async QHs */ >> EHCI_HRTIMER_IAA_WATCHDOG, /* Handle lost IAA interrupts */ >> EHCI_HRTIMER_DISABLE_PERIODIC, /* Wait to disable periodic sched */ >> @@ -143,6 +144,8 @@ struct ehci_hcd { /* one per controller */ >> unsigned i_thresh; /* uframes HC might cache */ >> >> union ehci_shadow *pshadow; /* mirror hw periodic table */ >> + struct list_head intr_unlink_wait; >> + unsigned intr_unlink_wait_cycle; >> struct list_head intr_unlink; >> unsigned intr_unlink_cycle; >> unsigned now_frame; /* frame from HC hardware */ > > To avoid an alignment gap on 64-bit systems, put intr_unlink_wait_cycle > after intr_unlink. Good catch. Thanks, -- Ming Lei
On Tue, 25 Jun 2013, Ming Lei wrote: > On Tue, Jun 25, 2013 at 2:53 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, 24 Jun 2013, Ming Lei wrote: > > > >> Given interrupt URB will be resubmitted from tasklet context which > >> is scheduled by ehci hardware interrupt handler, and commonly only > >> one interrupt URB is scheduled on qh, so the qh may be unlinked > >> immediately once qh_completions() returns from ehci_irq(), then > >> the intr URB to be resubmitted in complete() can only be scheduled > >> and linked to hardware until the qh unlink is completed. > >> > >> This patch improves this above situation, and the qh will wait for 5 > >> milliseconds before being unlinked from hardware, if one URB is submitted > >> during the period, the qh is move out of unlink wait list and the > >> interrupt transfer can be scheduled immediately, not like before: the > >> transfer is only linked to hardware until previous unlink is completed. > > > > This description is very hard to understand. I suggest rewriting it, > > something like this: > > > > 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. > > Excellent description about the change, and I will add it in V3, > also care to add your signed-off in the patch for the change log part? You have my permission to add it. > >> +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) > >> +{ > >> + __start_unlink_intr(ehci, qh, false); > >> +} > >> + > >> +static void start_unlink_intr_wait(struct ehci_hcd *ehci, > >> + struct ehci_qh *qh) > >> +{ > >> + __start_unlink_intr(ehci, qh, true); > >> +} > > > > This is not what I had in mind. > > > > Instead, copy the qh->qh_state test into the beginning of > > start_unlink_intr() and move the rest of start_do_unlink_intr() there. > > Then you can rename __start_unlink_intr() to start_unlink_intr_wait() > > and get rid of the "wait" parameter. > > The current code can do the check centrally, but it isn't big deal, and I > will follow your suggestion. Copying the check adds two lines of code (plus a comment and a blank line). Your two extra function headers, extra parameter, and extra test add more than that. > OK. > > BTW, indenting two tab might cause more lines, and looks many subsystems > don't do that. But, anyway, I am fine with two tab, :-) I admit, it isn't a perfect system and sometimes I don't use it. But it does have the advantage that changes to the first line don't require the continuation line to be changed as well, just to keep the desired alignment. Furthermore, aligning the continuation with the first parameter of a function call can often require _more_ than two extra tab stops of indentation. Alan Stern
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 7abf1ce..35f1372 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -487,6 +487,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); diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c index 2b70277..6037f84 100644 --- a/drivers/usb/host/ehci-hub.c +++ b/drivers/usb/host/ehci-hub.c @@ -345,6 +345,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd) end_unlink_async(ehci); unlink_empty_async_suspended(ehci); + ehci_handle_start_intr_unlinks(ehci); ehci_handle_intr_unlinks(ehci); end_free_itds(ehci); diff --git a/drivers/usb/host/ehci-mem.c b/drivers/usb/host/ehci-mem.c index ef2c3a1..52a7773 100644 --- a/drivers/usb/host/ehci-mem.c +++ b/drivers/usb/host/ehci-mem.c @@ -93,6 +93,7 @@ static struct ehci_qh *ehci_qh_alloc (struct ehci_hcd *ehci, gfp_t flags) qh->qh_dma = dma; // INIT_LIST_HEAD (&qh->qh_list); INIT_LIST_HEAD (&qh->qtd_list); + INIT_LIST_HEAD(&qh->unlink_node); /* dummy td enables safe urb queuing */ qh->dummy = ehci_qtd_alloc (ehci, flags); diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c index f80d033..5bf67e2 100644 --- a/drivers/usb/host/ehci-sched.c +++ b/drivers/usb/host/ehci-sched.c @@ -601,12 +601,24 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci, struct ehci_qh *qh) list_del(&qh->intr_node); } -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) +/* must be called with holding ehci->lock */ +static void cancel_unlink_wait_intr(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) + if (qh->qh_state != QH_STATE_LINKED || list_empty(&qh->unlink_node)) return; + list_del_init(&qh->unlink_node); + + /* avoid unnecessary CPU wakeup */ + if (list_empty(&ehci->intr_unlink_wait)) + ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR); +} + +static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) +{ + /* if the qh is waitting for unlink, cancel it now */ + cancel_unlink_wait_intr(ehci, qh); + qh_unlink_periodic (ehci, qh); /* Make sure the unlinks are visible before starting the timer */ @@ -632,6 +644,45 @@ 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(struct ehci_hcd *ehci, struct ehci_qh *qh, + bool wait) +{ + /* If the QH isn't linked then there's nothing we can do. */ + if (qh->qh_state != QH_STATE_LINKED) + return; + + if (!wait) + return start_do_unlink_intr(ehci, qh); + + qh->unlink_cycle = ehci->intr_unlink_wait_cycle; + + /* New entries go at the end of the intr_unlink_wait list */ + list_add_tail(&qh->unlink_node, &ehci->intr_unlink_wait); + + if (ehci->rh_state < EHCI_RH_RUNNING) + ehci_handle_start_intr_unlinks(ehci); + else if (ehci->intr_unlink_wait.next == &qh->unlink_node) { + ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true); + ++ehci->intr_unlink_wait_cycle; + } +} + +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) +{ + __start_unlink_intr(ehci, qh, false); +} + +static void start_unlink_intr_wait(struct ehci_hcd *ehci, + struct ehci_qh *qh) +{ + __start_unlink_intr(ehci, qh, true); +} + static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh) { struct ehci_qh_hw *hw = qh->hw; @@ -881,6 +932,9 @@ static int intr_submit ( goto done; } + /* put back qh from unlink wait list */ + cancel_unlink_wait_intr(ehci, qh); + /* then queue the urb's tds to the qh */ qh = qh_append_tds(ehci, urb, qtd_list, epnum, &urb->ep->hcpriv); BUG_ON (qh == NULL); @@ -926,7 +980,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); } } } diff --git a/drivers/usb/host/ehci-timer.c b/drivers/usb/host/ehci-timer.c index 11e5b32..9fef5d4 100644 --- a/drivers/usb/host/ehci-timer.c +++ b/drivers/usb/host/ehci-timer.c @@ -72,6 +72,7 @@ static unsigned event_delays_ns[] = { 1 * NSEC_PER_MSEC, /* EHCI_HRTIMER_POLL_DEAD */ 1125 * NSEC_PER_USEC, /* EHCI_HRTIMER_UNLINK_INTR */ 2 * NSEC_PER_MSEC, /* EHCI_HRTIMER_FREE_ITDS */ + 5 * NSEC_PER_MSEC, /* EHCI_HRTIMER_START_UNLINK_INTR */ 6 * NSEC_PER_MSEC, /* EHCI_HRTIMER_ASYNC_UNLINKS */ 10 * NSEC_PER_MSEC, /* EHCI_HRTIMER_IAA_WATCHDOG */ 10 * NSEC_PER_MSEC, /* EHCI_HRTIMER_DISABLE_PERIODIC */ @@ -98,6 +99,19 @@ static void ehci_enable_event(struct ehci_hcd *ehci, unsigned event, } } +/* Warning: don't call this function from hrtimer handler context */ +static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event) +{ + if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci))) + return; + + ehci->enabled_hrtimer_events &= ~(1 << event); + if (!ehci->enabled_hrtimer_events) { + ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT; + hrtimer_cancel(&ehci->hrtimer); + } +} + /* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */ static void ehci_poll_ASS(struct ehci_hcd *ehci) @@ -215,6 +229,37 @@ static void ehci_handle_controller_death(struct ehci_hcd *ehci) /* Not in process context, so don't try to reset the controller */ } +/* start to unlink interrupt QHs */ +static void ehci_handle_start_intr_unlinks(struct ehci_hcd *ehci) +{ + bool stopped = (ehci->rh_state < EHCI_RH_RUNNING); + + /* + * Process all the QHs on the intr_unlink list that were added + * before the current unlink cycle began. The list is in + * temporal order, so stop when we reach the first entry in the + * current cycle. But if the root hub isn't running then + * process all the QHs on the list. + */ + while (!list_empty(&ehci->intr_unlink_wait)) { + struct ehci_qh *qh; + + qh = list_first_entry(&ehci->intr_unlink_wait, + struct ehci_qh, unlink_node); + if (!stopped && + (qh->unlink_cycle == + ehci->intr_unlink_wait_cycle)) + break; + list_del_init(&qh->unlink_node); + start_unlink_intr(ehci, qh); + } + + /* Handle remaining entries later */ + if (!list_empty(&ehci->intr_unlink_wait)) { + ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true); + ++ehci->intr_unlink_wait_cycle; + } +} /* Handle unlinked interrupt QHs once they are gone from the hardware */ static void ehci_handle_intr_unlinks(struct ehci_hcd *ehci) @@ -236,7 +281,7 @@ static void ehci_handle_intr_unlinks(struct ehci_hcd *ehci) unlink_node); if (!stopped && qh->unlink_cycle == ehci->intr_unlink_cycle) break; - list_del(&qh->unlink_node); + list_del_init(&qh->unlink_node); end_unlink_intr(ehci, qh); } @@ -363,6 +408,7 @@ static void (*event_handlers[])(struct ehci_hcd *) = { ehci_handle_controller_death, /* EHCI_HRTIMER_POLL_DEAD */ ehci_handle_intr_unlinks, /* EHCI_HRTIMER_UNLINK_INTR */ end_free_itds, /* EHCI_HRTIMER_FREE_ITDS */ + ehci_handle_start_intr_unlinks, /* EHCI_HRTIMER_START_UNLINK_INTR */ unlink_empty_async, /* EHCI_HRTIMER_ASYNC_UNLINKS */ ehci_iaa_watchdog, /* EHCI_HRTIMER_IAA_WATCHDOG */ ehci_disable_PSE, /* EHCI_HRTIMER_DISABLE_PERIODIC */ diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index 64f9a08..28dc8d2 100644 --- a/drivers/usb/host/ehci.h +++ b/drivers/usb/host/ehci.h @@ -88,6 +88,7 @@ enum ehci_hrtimer_event { EHCI_HRTIMER_POLL_DEAD, /* Wait for dead controller to stop */ EHCI_HRTIMER_UNLINK_INTR, /* Wait for interrupt QH unlink */ EHCI_HRTIMER_FREE_ITDS, /* Wait for unused iTDs and siTDs */ + EHCI_HRTIMER_START_UNLINK_INTR, /* wait for new intr schedule */ EHCI_HRTIMER_ASYNC_UNLINKS, /* Unlink empty async QHs */ EHCI_HRTIMER_IAA_WATCHDOG, /* Handle lost IAA interrupts */ EHCI_HRTIMER_DISABLE_PERIODIC, /* Wait to disable periodic sched */ @@ -143,6 +144,8 @@ struct ehci_hcd { /* one per controller */ unsigned i_thresh; /* uframes HC might cache */ union ehci_shadow *pshadow; /* mirror hw periodic table */ + struct list_head intr_unlink_wait; + unsigned intr_unlink_wait_cycle; struct list_head intr_unlink; unsigned intr_unlink_cycle; unsigned now_frame; /* frame from HC hardware */
Given interrupt URB will be resubmitted from tasklet context which is scheduled by ehci hardware interrupt handler, and commonly only one interrupt URB is scheduled on qh, so the qh may be unlinked immediately once qh_completions() returns from ehci_irq(), then the intr URB to be resubmitted in complete() can only be scheduled and linked to hardware until the qh unlink is completed. This patch improves this above situation, and the qh will wait for 5 milliseconds before being unlinked from hardware, if one URB is submitted during the period, the qh is move out of unlink wait list and the interrupt transfer can be scheduled immediately, not like before: the transfer is only linked to hardware until previous unlink is completed. Cc: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- drivers/usb/host/ehci-hcd.c | 1 + drivers/usb/host/ehci-hub.c | 1 + drivers/usb/host/ehci-mem.c | 1 + drivers/usb/host/ehci-sched.c | 62 ++++++++++++++++++++++++++++++++++++++--- drivers/usb/host/ehci-timer.c | 48 ++++++++++++++++++++++++++++++- drivers/usb/host/ehci.h | 3 ++ 6 files changed, 111 insertions(+), 5 deletions(-)