On Tue, 20 Aug 2013, Ming Lei wrote:
> On Mon, Aug 19, 2013 at 10:33 PM, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Why would those events not need to be handled?
>
> For IAA_WATCHDOG event, it needn't to handle when
> IAA interrupt comes.
>
> For START_UNLINK_INTR event, we don't need to unlink intr
> qh any more if intr urb submit request comes.
This could be explained more clearly. The "events" mentioned in the
patch description aren't real events at all; they are timeouts. The
actual events are the IAA interrupt and URB submission. The idea of
the patch is that once the actual event occurs, the corresponding
timeout isn't needed any more and so the timer can be cancelled.
> >> +/* Warning: don't call this function from hrtimer handler context */
> >> +static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
> >> +{
> >> + ehci->enabled_hrtimer_events &= ~(1 << event);
> >> + if (!ehci->enabled_hrtimer_events) {
> >> + ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
> >> + hrtimer_cancel(&ehci->hrtimer);
> >> + } else if (ehci->next_hrtimer_event == event) {
> >> + ehci->next_hrtimer_event =
> >> + ffs(ehci->enabled_hrtimer_events) - 1;
> >
> > Should the timer be rescheduled here? It's hard to say without seeing
> > some test results.
>
> You are right, the timer should be rescheduled here, otherwise there is no
> effect on HCD with need_io_watchdog set.
>
> With below change on ehci_disable_event(), about 7~8 times timer
> expiration can be saved when one asix network NIC is connected
> to EHCI without network traffic(the device responds interrupt poll
> 7~8 times per second constantly for link status query), no matter
> ehci->need_io_watchdog is set or not.
>
> BTW, the timer expiration event is observed via /proc/timer_stats.
>
> /* Warning: don't call this function from hrtimer handler context */
> static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
> {
> ehci->enabled_hrtimer_events &= ~(1 << event);
>
> if (!ehci->enabled_hrtimer_events) {
> ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
> hrtimer_cancel(&ehci->hrtimer);
> } else if (ehci->next_hrtimer_event == event) {
> ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
> enum ehci_hrtimer_event next =
> ffs(ehci->enabled_hrtimer_events) - 1;
Don't declare new variables in the middle of a block.
> ehci_enable_event(ehci, next, 0);
The third argument should be "false", not "0". It is a bool.
Alan Stern
--
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
