On Wed, 21 Aug 2013, Ming Lei wrote:
> This patch introduces ehci_disable_event(), which is applied on
> IAA_WATCHDOG and START_UNLINK_INTR timeouts if the two corresponding
> events(IAA and intr URB submission) happened, so that we may avoid
> unnecessary CPU wakeup by canceling the timer.
>
> One simple test indicates about 7~8 timer expirations/sec
> (20% ~ 30% of totoal timer expirations/sec) 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).
>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> ---
> drivers/usb/host/ehci-hcd.c | 12 +-----------
> drivers/usb/host/ehci-sched.c | 4 +++-
> drivers/usb/host/ehci-timer.c | 16 ++++++++++++++++
> 3 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 73c7299..549da36 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -738,17 +738,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
> if (status & STS_IAA) {
>
> /* Turn off the IAA watchdog */
> - ehci->enabled_hrtimer_events &= ~BIT(EHCI_HRTIMER_IAA_WATCHDOG);
> -
> - /*
> - * Mild optimization: Allow another IAAD to reset the
> - * hrtimer, if one occurs before the next expiration.
> - * In theory we could always cancel the hrtimer, but
> - * tests show that about half the time it will be reset
> - * for some other event anyway.
> - */
> - if (ehci->next_hrtimer_event == EHCI_HRTIMER_IAA_WATCHDOG)
> - ++ehci->next_hrtimer_event;
> + ehci_disable_event(ehci, EHCI_HRTIMER_IAA_WATCHDOG);
>
> /* guard against (alleged) silicon errata */
> if (cmd & CMD_IAAD)
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index 6631089..83be03f 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -610,9 +610,11 @@ static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
> list_del_init(&qh->unlink_node);
>
> /*
> - * TODO: disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
> + * disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
> * avoiding unnecessary CPU wakeup
> */
> + if (list_empty(&ehci->intr_unlink_wait))
> + ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR);
> }
>
> static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
> diff --git a/drivers/usb/host/ehci-timer.c b/drivers/usb/host/ehci-timer.c
> index 424ac5d..983d063 100644
> --- a/drivers/usb/host/ehci-timer.c
> +++ b/drivers/usb/host/ehci-timer.c
> @@ -99,6 +99,22 @@ 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)
> +{
> + 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;
> + ehci_enable_event(ehci,
> + ffs(ehci->enabled_hrtimer_events) - 1,
> + false);
> + }
> +}
> +
>
> /* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */
> static void ehci_poll_ASS(struct ehci_hcd *ehci)
Okay. You can add
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
and submit it as a version-2 patch.
By the way, even though it's a little late to ask this... Why did you
decide to move only the giveback routine into a tasklet, instead of
moving the entire interrupt handler?
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
