[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
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 applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh)
state to improve 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 state and the
interrupt transfer can be scheduled immediately, not like before: the
transfer is only linked to hardware until previous unlink is completed.
Only enable the improvement in case that HCD supports to run
giveback of URB in tasklet context.
Cc: Alan Stern
Signed-off-by: Ming Lei
---
drivers/usb/host/ehci-hcd.c | 16 ---
drivers/usb/host/ehci-hub.c |1 +
drivers/usb/host/ehci-sched.c | 60 ++---
drivers/usb/host/ehci-timer.c | 43 +
drivers/usb/host/ehci.h |3 +++
5 files changed, 115 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 246e124..0ab82de 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -304,7 +304,9 @@ static void end_unlink_async(struct ehci_hcd *ehci);
static void unlink_empty_async(struct ehci_hcd *ehci);
static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
static void ehci_work(struct ehci_hcd *ehci);
-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
+static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
+ bool wait);
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
#include "ehci-timer.c"
@@ -484,6 +486,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);
@@ -908,15 +911,20 @@ static int ehci_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
switch (qh->qh_state) {
case QH_STATE_LINKED:
if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT)
- start_unlink_intr(ehci, qh);
+ start_unlink_intr(ehci, qh, false);
else
start_unlink_async(ehci, qh);
break;
case QH_STATE_COMPLETING:
qh->dequeue_during_giveback = 1;
break;
- case QH_STATE_UNLINK:
case QH_STATE_UNLINK_WAIT:
+ if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) {
+ cancel_unlink_wait_intr(ehci, qh);
+ start_unlink_intr(ehci, qh, false);
+ }
+ break;
+ case QH_STATE_UNLINK:
/* already started */
break;
case QH_STATE_IDLE:
@@ -1042,7 +1050,7 @@ ehci_endpoint_reset(struct usb_hcd *hcd, struct
usb_host_endpoint *ep)
if (eptype == USB_ENDPOINT_XFER_BULK)
start_unlink_async(ehci, qh);
else
- start_unlink_intr(ehci, qh);
+ start_unlink_intr(ehci, qh, false);
}
}
spin_unlock_irqrestore(&ehci->lock, flags);
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index b2f6450..4104c66 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-sched.c b/drivers/usb/host/ehci-sched.c
index f80d033..5b9ca31 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -601,10 +601,10 @@ 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)
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
- /* If the QH isn't linked then there's nothing we can do. */
Re: [RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
On Tue, 18 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 applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh) > state to improve 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 state and the > interrupt transfer can be scheduled immediately, not like before: the > transfer is only linked to hardware until previous unlink is completed. > > Only enable the improvement in case that HCD supports to run > giveback of URB in tasklet context. The approach used in this patch is wrong. You shouldn't start the unlink, then wait, then finish the unlink. Consider what would happen if an URB were submitted during the delay: It would have to wait until the QH was completely unlinked. Instead, you should wait first, then do the entire unlink. For example, scan_async() in ehci-q.c doesn't immediately begin to unlink empty async QHs. It merely marks them as being empty and starts a timer to check them later. It they are still empty at that point, then they are unlinked. Also, it's silly to check whether or not giveback uses a tasklet. We know that following the 6/6 patch it will. Even if it doesn't, there's no harm in waiting a little while before unlinking an empty interrupt QH. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
On Wed, Jun 19, 2013 at 12:51 AM, Alan Stern wrote: > On Tue, 18 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 applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh) >> state to improve 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 state and the >> interrupt transfer can be scheduled immediately, not like before: the >> transfer is only linked to hardware until previous unlink is completed. >> >> Only enable the improvement in case that HCD supports to run >> giveback of URB in tasklet context. > > The approach used in this patch is wrong. You shouldn't start the > unlink, then wait, then finish the unlink. Consider what would happen What you mentioned above is just what the patch is doing, :-) start_unlink_intr() only sets the qh as QH_STATE_UNLINK_WAIT, puts the qh into one wait list and starts a timer, if it is expired the qh will be started to unlink, otherwise the qh can be recovered to QH_STATE_LINKED immediately if there is one URB submitted. So unlinking intr qh becomes a 3-stage process: - wait(qh return to linked state if URB is submitted during the period, otherwise go to start unlink) - start unlink(do unlink, and wait for end of unlink) - end unlink > if an URB were submitted during the delay: It would have to wait until If an URB were submitted during the delay, the previous wait is canceled immediately, and the qh state is recovered to linked state, please see cancel_unlink_wait_intr() called in intr_submit(). > the QH was completely unlinked. Instead, you should wait first, then > do the entire unlink. Yes, it is just what the patch is doing, :-) > > For example, scan_async() in ehci-q.c doesn't immediately begin to > unlink empty async QHs. It merely marks them as being empty and starts > a timer to check them later. It they are still empty at that point, > then they are unlinked. Yes, the patch starts to use QH_STATE_UNLINK_WAIT state for intr qh, and previously the state is only used by async qh. > > Also, it's silly to check whether or not giveback uses a tasklet. We > know that following the 6/6 patch it will. Even if it doesn't, there's > no harm in waiting a little while before unlinking an empty interrupt > QH. It is still for the test reason, since the patch may cause recursion if HCD_BH isn't set. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
On Wed, 19 Jun 2013, Ming Lei wrote: > > The approach used in this patch is wrong. You shouldn't start the > > unlink, then wait, then finish the unlink. Consider what would happen > > What you mentioned above is just what the patch is doing, :-) > > start_unlink_intr() only sets the qh as QH_STATE_UNLINK_WAIT, puts > the qh into one wait list and starts a timer, if it is expired the qh will be > started to unlink, otherwise the qh can be recovered to QH_STATE_LINKED > immediately if there is one URB submitted. Okay, maybe I was fooled by your use of QH_STATE_UNLINK_WAIT. Setting the state to that value means that the QH is going to be unlinked after a time delay. However, that's not what you mean; you mean that after a time delay you will decide whether or not to unlink the QH. I think you should copy the approach used for the async QHs. > So unlinking intr qh becomes a 3-stage process: > >- wait(qh return to linked state if URB is submitted during the period, > otherwise go to start unlink) >- start unlink(do unlink, and wait for end of unlink) >- end unlink > > > if an URB were submitted during the delay: It would have to wait until > > If an URB were submitted during the delay, the previous wait is canceled > immediately, and the qh state is recovered to linked state, please see > cancel_unlink_wait_intr() called in intr_submit(). Right. But you're not allowed to do that after changing qh->state. One of the invariants of the driver is that once qh->state takes on any value other than QH_STATE_LINKED (or, temporarily, QH_STATE_COMPLETING), the QH must be unlinked. The state can't change back to QH_STATE_LINKED without first passing through QH_STATE_IDLE. > > Also, it's silly to check whether or not giveback uses a tasklet. We > > know that following the 6/6 patch it will. Even if it doesn't, there's > > no harm in waiting a little while before unlinking an empty interrupt > > QH. > > It is still for the test reason, since the patch may cause recursion if > HCD_BH isn't set. How can it cause recursion? The async unlink code doesn't. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
On Wed, Jun 19, 2013 at 11:44 PM, Alan Stern wrote: > On Wed, 19 Jun 2013, Ming Lei wrote: > >> > The approach used in this patch is wrong. You shouldn't start the >> > unlink, then wait, then finish the unlink. Consider what would happen >> >> What you mentioned above is just what the patch is doing, :-) >> >> start_unlink_intr() only sets the qh as QH_STATE_UNLINK_WAIT, puts >> the qh into one wait list and starts a timer, if it is expired the qh will be >> started to unlink, otherwise the qh can be recovered to QH_STATE_LINKED >> immediately if there is one URB submitted. > > Okay, maybe I was fooled by your use of QH_STATE_UNLINK_WAIT. Setting > the state to that value means that the QH is going to be unlinked after Here the meaning of intr qh's QH_STATE_UNLINK_WAIT is a bit different with async qh's. > a time delay. However, that's not what you mean; you mean that after a > time delay you will decide whether or not to unlink the QH. Yes, that is the difference with async QH. > > I think you should copy the approach used for the async QHs. Yes, we can copy, but current async unlinking has some disadvantages, see below. >> So unlinking intr qh becomes a 3-stage process: >> >>- wait(qh return to linked state if URB is submitted during the >> period, >> otherwise go to start unlink) >>- start unlink(do unlink, and wait for end of unlink) >>- end unlink >> >> > if an URB were submitted during the delay: It would have to wait until >> >> If an URB were submitted during the delay, the previous wait is canceled >> immediately, and the qh state is recovered to linked state, please see >> cancel_unlink_wait_intr() called in intr_submit(). > > Right. But you're not allowed to do that after changing qh->state. > One of the invariants of the driver is that once qh->state takes on any > value other than QH_STATE_LINKED (or, temporarily, > QH_STATE_COMPLETING), the QH must be unlinked. The state can't change > back to QH_STATE_LINKED without first passing through QH_STATE_IDLE. IMO, there is no any side effect when we change the state to QH_STATE_UNLINK_WAIT and later change back to QH_STATE_LINKED later under this situation. The reason why I use QH_STATE_UNLINK_WAIT here is for avoiding unnecessary CPU wakeup. Without the state, the unlink wait timer is still triggered to check if there are intr QHs to be unlinked, but in most of situations, there aren't QHs to be unlinked since tasklet is surely run before the wait timer is expired. So generally, without knowing the wait state, CPU wakeup events may be introduced unnecessarily. Considered that the interval of interrupt endpoint might be very small(e.g. 125us, 1ms) and some devices have very frequent intr event, I think we need to pay attention to the problem. Even on async QH situation, we might need to consider the problem too since some application(eg. bulk only mass storage on xhci) may have thousands of interrupts per seconds during data transfer, so CPU wakeup events could be increased much by letting wait timer expire unnecessarily. Also the async QH unlink approach has another disadvantage: - if there are several QHs which are become empty during one wait period, the hrtimer has to be scheduled several times for starting unlink one qh each time. And after introducing the unlink wait list like the patch, we only need schedule the hrtimer one time for unlinking all these empty QHs. Finally, unlink wait for intr qh is only needed when the qh is completed right now, and other cases(eg. unlink) needn't the wait. The attached patch removes the QH_STATE_UNLINK_WAIT, and can avoid the above disadvantages of async QH unlink, could you comment on the new patch? --- drivers/usb/host/ehci-hcd.c |8 +++--- drivers/usb/host/ehci-hub.c |1 + drivers/usb/host/ehci-sched.c | 54 ++--- drivers/usb/host/ehci-timer.c | 45 +- drivers/usb/host/ehci.h |3 +++ 5 files changed, 103 insertions(+), 8 deletions(-) diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 246e124..35b4148 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -304,7 +304,8 @@ static void end_unlink_async(struct ehci_hcd *ehci); static void unlink_empty_async(struct ehci_hcd *ehci); static void unlink_empty_async_suspended(struct ehci_hcd *ehci); static void ehci_work(struct ehci_hcd *ehci); -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh); +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh, + bool wait); static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh); #include "ehci-timer.c" @@ -484,6 +485,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->i
Re: [RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
On Thu, 20 Jun 2013, Ming Lei wrote:
> IMO, there is no any side effect when we change the state to
> QH_STATE_UNLINK_WAIT and later change back to QH_STATE_LINKED
> later under this situation.
I don't like the idea of changing an invariant.
> The reason why I use QH_STATE_UNLINK_WAIT here is for avoiding
> unnecessary CPU wakeup. Without the state, the unlink wait timer
> is still triggered to check if there are intr QHs to be unlinked, but in most
> of
> situations, there aren't QHs to be unlinked since tasklet is surely run
> before the wait timer is expired. So generally, without knowing the wait
> state,
> CPU wakeup events may be introduced unnecessarily.
Avoiding unnecessary timer events is a good thing, but I don't
understand how it is connected with QH_STATE_UNLINK_WAIT. Doesn't this
revised patch avoid the events without using this state?
Besides, don't you already know the wait state by checking whether
qh->unlink_node is empty?
> Considered that the interval of interrupt endpoint might be very
> small(e.g. 125us,
> 1ms) and some devices have very frequent intr event, I think we
> need to pay attention to the problem.
Yes, we do. The hrtimer code in ehci-timer is written specifically to
handle that sort of situation.
> Even on async QH situation, we might need to consider the problem too
> since some application(eg. bulk only mass storage on xhci) may have
> thousands of interrupts per seconds during data transfer, so CPU wakeup
> events could be increased much by letting wait timer expire unnecessarily.
I suspect it's the other way around. Letting the timer expire
_reduces_ the amount of work, because we don't have to start and stop
the timer as often.
It's a tradeoff. One approach starts and cancels the timer N times
(where N can be fairly large); the other approach starts the timer
once and lets it expire, and then the handler routine does almost no
work. Which approach uses more CPU time? I don't know; I haven't
measured it.
> Also the async QH unlink approach has another disadvantage:
>
> - if there are several QHs which are become empty during one wait period,
> the hrtimer has to be scheduled several times for starting unlink one qh
> each time.
That's because the driver unlinks only one async QH at a time. It is
unavoidable. In earlier kernels the driver would unlink multiple async
QHs simultaneously, and it needed to schedule the timer only once.
For some reason (I still don't understand why), this did not work on
some systems.
> And after introducing the unlink wait list like the patch, we only
> need schedule the hrtimer one time for unlinking all these empty QHs.
The async code used to work that way, before I changed it to unlink
only one async QH at a time.
> Finally, unlink wait for intr qh is only needed when the qh is completed
> right now, and other cases(eg. unlink) needn't the wait.
Right.
> The attached patch removes the QH_STATE_UNLINK_WAIT, and can
> avoid the above disadvantages of async QH unlink, could you comment
> on the new patch?
Okay...
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 246e124..35b4148 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -304,7 +304,8 @@ static void end_unlink_async(struct ehci_hcd *ehci);
> static void unlink_empty_async(struct ehci_hcd *ehci);
> static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
> static void ehci_work(struct ehci_hcd *ehci);
> -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
> +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
> + bool wait);
Adding a "wait" argument is a bad approach. You should create a new
function: start_unlink_intr_wait() or something like that. After all,
it is used in only one place.
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index acff5b8..5dfda56 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -548,6 +548,9 @@ static void qh_link_periodic(struct ehci_hcd
> *ehci, struct ehci_qh *qh)
>
> list_add(&qh->intr_node, &ehci->intr_qh_list);
>
> + /* unlink need this node to be initialized */
> + INIT_LIST_HEAD(&qh->unlink_node);
This should be done only once, when the QH is created. And the comment
is unnecessary.
> @@ -98,6 +99,17 @@ 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;
This test looks really ugly, and it won't be needed after the driver
switches over to tasklets. Of course, there's a problem before we
switch over: this routine will be called by an interrupt URB
submission, which could occur during a giveback in the timer handle
Re: [RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
On Sat, Jun 22, 2013 at 4:16 AM, Alan Stern wrote:
> On Thu, 20 Jun 2013, Ming Lei wrote:
>
>> IMO, there is no any side effect when we change the state to
>> QH_STATE_UNLINK_WAIT and later change back to QH_STATE_LINKED
>> later under this situation.
>
> I don't like the idea of changing an invariant.
>
>> The reason why I use QH_STATE_UNLINK_WAIT here is for avoiding
>> unnecessary CPU wakeup. Without the state, the unlink wait timer
>> is still triggered to check if there are intr QHs to be unlinked, but in
>> most of
>> situations, there aren't QHs to be unlinked since tasklet is surely run
>> before the wait timer is expired. So generally, without knowing the wait
>> state,
>> CPU wakeup events may be introduced unnecessarily.
>
> Avoiding unnecessary timer events is a good thing, but I don't
> understand how it is connected with QH_STATE_UNLINK_WAIT. Doesn't this
> revised patch avoid the events without using this state?
>
> Besides, don't you already know the wait state by checking whether
> qh->unlink_node is empty?
>
>> Considered that the interval of interrupt endpoint might be very
>> small(e.g. 125us,
>> 1ms) and some devices have very frequent intr event, I think we
>> need to pay attention to the problem.
>
> Yes, we do. The hrtimer code in ehci-timer is written specifically to
> handle that sort of situation.
>
>> Even on async QH situation, we might need to consider the problem too
>> since some application(eg. bulk only mass storage on xhci) may have
>> thousands of interrupts per seconds during data transfer, so CPU wakeup
>> events could be increased much by letting wait timer expire unnecessarily.
>
> I suspect it's the other way around. Letting the timer expire
> _reduces_ the amount of work, because we don't have to start and stop
> the timer as often.
>
> It's a tradeoff. One approach starts and cancels the timer N times
> (where N can be fairly large); the other approach starts the timer
> once and lets it expire, and then the handler routine does almost no
> work. Which approach uses more CPU time? I don't know; I haven't
> measured it.
>
>> Also the async QH unlink approach has another disadvantage:
>>
>> - if there are several QHs which are become empty during one wait period,
>> the hrtimer has to be scheduled several times for starting unlink one qh
>> each time.
>
> That's because the driver unlinks only one async QH at a time. It is
> unavoidable. In earlier kernels the driver would unlink multiple async
> QHs simultaneously, and it needed to schedule the timer only once.
> For some reason (I still don't understand why), this did not work on
> some systems.
>
>> And after introducing the unlink wait list like the patch, we only
>> need schedule the hrtimer one time for unlinking all these empty QHs.
>
> The async code used to work that way, before I changed it to unlink
> only one async QH at a time.
>
>> Finally, unlink wait for intr qh is only needed when the qh is completed
>> right now, and other cases(eg. unlink) needn't the wait.
>
> Right.
>
>> The attached patch removes the QH_STATE_UNLINK_WAIT, and can
>> avoid the above disadvantages of async QH unlink, could you comment
>> on the new patch?
>
> Okay...
Thanks for your review.
>
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index 246e124..35b4148 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -304,7 +304,8 @@ static void end_unlink_async(struct ehci_hcd *ehci);
>> static void unlink_empty_async(struct ehci_hcd *ehci);
>> static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
>> static void ehci_work(struct ehci_hcd *ehci);
>> -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
>> +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
>> + bool wait);
>
> Adding a "wait" argument is a bad approach. You should create a new
> function: start_unlink_intr_wait() or something like that. After all,
> it is used in only one place.
OK.
>
>> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
>> index acff5b8..5dfda56 100644
>> --- a/drivers/usb/host/ehci-sched.c
>> +++ b/drivers/usb/host/ehci-sched.c
>> @@ -548,6 +548,9 @@ static void qh_link_periodic(struct ehci_hcd
>> *ehci, struct ehci_qh *qh)
>>
>> list_add(&qh->intr_node, &ehci->intr_qh_list);
>>
>> + /* unlink need this node to be initialized */
>> + INIT_LIST_HEAD(&qh->unlink_node);
>
> This should be done only once, when the QH is created. And the comment
> is unnecessary.
OK, I will move it into ehci_qh_alloc().
>
>> @@ -98,6 +99,17 @@ 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;
>
