On Wed, Jul 11, 2012 at 11:21 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> This patch (as1572) begins the conversion of ehci-hcd over to using
> high-resolution timers rather than old-fashioned low-resolution kernel
> timers. This reduces overhead caused by timer roundoff on systems
> where HZ is smaller than 1000. Also, the new timer framework
> introduced here is much more logical and easily extended than the
> ad-hoc approach ehci-hcd currently uses for timers.
>
> An hrtimer structure is added to ehci_hcd, along with a bitflag array
> and an array of ktime_t values, to keep track of which timing events
> are pending and what their expiration times are.
>
> Only the infrastructure for the timing operations is added in this
> patch. Later patches will add routines for handling each of the
> various timing events the driver needs. In some cases the new hrtimer
> handlers will replace the existing handlers for ehci-hcd's kernel
> timers; as this happens the old timers will be removed. In other
> cases the new timing events will replace busy-wait loops.
>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>
> ---
>
> drivers/usb/host/ehci-hcd.c | 15 +++++
> drivers/usb/host/ehci-hub.c | 3 +
> drivers/usb/host/ehci-timer.c | 106 ++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/ehci.h | 16 ++++++
> 4 files changed, 138 insertions(+), 2 deletions(-)
>
> Index: usb-3.4/drivers/usb/host/ehci.h
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/ehci.h
> +++ usb-3.4/drivers/usb/host/ehci.h
> @@ -73,7 +73,23 @@ enum ehci_rh_state {
> EHCI_RH_STOPPING
> };
>
> +/*
> + * Timer events, ordered by increasing delay length.
> + * Always update event_delays_ns[] and event_handlers[] (defined in
> + * ehci-timer.c) in parallel with this list.
> + */
> +enum ehci_hrtimer_event {
> + EHCI_HRTIMER_NUM_EVENTS /* Must come last */
> +};
> +#define EHCI_HRTIMER_NO_EVENT 99
> +
> struct ehci_hcd { /* one per controller */
> + /* timing support */
> + enum ehci_hrtimer_event next_hrtimer_event;
> + unsigned enabled_hrtimer_events;
> + ktime_t hr_timeouts[EHCI_HRTIMER_NUM_EVENTS];
> + struct hrtimer hrtimer;
> +
> /* glue to PCI and HCD framework */
> struct ehci_caps __iomem *caps;
> struct ehci_regs __iomem *regs;
> Index: usb-3.4/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/ehci-hcd.c
> +++ usb-3.4/drivers/usb/host/ehci-hcd.c
> @@ -30,8 +30,7 @@
> #include <linux/vmalloc.h>
> #include <linux/errno.h>
> #include <linux/init.h>
> -#include <linux/timer.h>
> -#include <linux/ktime.h>
> +#include <linux/hrtimer.h>
> #include <linux/list.h>
> #include <linux/interrupt.h>
> #include <linux/usb.h>
> @@ -380,6 +379,7 @@ static void ehci_quiesce (struct ehci_hc
> static void end_unlink_async(struct ehci_hcd *ehci);
> static void ehci_work(struct ehci_hcd *ehci);
>
> +#include "ehci-timer.c"
> #include "ehci-hub.c"
> #include "ehci-lpm.c"
> #include "ehci-mem.c"
> @@ -494,7 +494,10 @@ static void ehci_shutdown(struct usb_hcd
> spin_lock_irq(&ehci->lock);
> ehci->rh_state = EHCI_RH_STOPPING;
> ehci_silence_controller(ehci);
> + ehci->enabled_hrtimer_events = 0;
> spin_unlock_irq(&ehci->lock);
> +
> + hrtimer_cancel(&ehci->hrtimer);
> }
>
> static void ehci_port_power (struct ehci_hcd *ehci, int is_on)
> @@ -561,12 +564,14 @@ static void ehci_stop (struct usb_hcd *h
> del_timer_sync(&ehci->iaa_watchdog);
>
> spin_lock_irq(&ehci->lock);
> + ehci->enabled_hrtimer_events = 0;
> ehci_quiesce(ehci);
>
> ehci_silence_controller(ehci);
> ehci_reset (ehci);
> spin_unlock_irq(&ehci->lock);
>
> + hrtimer_cancel(&ehci->hrtimer);
> remove_sysfs_files(ehci);
> remove_debug_files (ehci);
>
> @@ -615,6 +620,10 @@ static int ehci_init(struct usb_hcd *hcd
> ehci->iaa_watchdog.function = ehci_iaa_watchdog;
> ehci->iaa_watchdog.data = (unsigned long) ehci;
>
> + hrtimer_init(&ehci->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> + ehci->hrtimer.function = ehci_hrtimer_func;
> + ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
> +
> hcc_params = ehci_readl(ehci, &ehci->caps->hcc_params);
>
> /*
> @@ -954,6 +963,8 @@ static irqreturn_t ehci_irq (struct usb_
> dbg_status(ehci, "fatal", status);
> ehci_halt(ehci);
> dead:
> + ehci->enabled_hrtimer_events = 0;
> + hrtimer_try_to_cancel(&ehci->hrtimer);
> ehci_reset(ehci);
> ehci_writel(ehci, 0, &ehci->regs->configured_flag);
> usb_hc_died(hcd);
> Index: usb-3.4/drivers/usb/host/ehci-hub.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/ehci-hub.c
> +++ usb-3.4/drivers/usb/host/ehci-hub.c
> @@ -311,12 +311,15 @@ static int ehci_bus_suspend (struct usb_
> ehci_readl(ehci, &ehci->regs->intr_enable);
>
> ehci->next_statechange = jiffies + msecs_to_jiffies(10);
> + ehci->enabled_hrtimer_events = 0;
> + ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
> spin_unlock_irq (&ehci->lock);
>
> /* ehci_work() may have re-enabled the watchdog timer, which we do not
> * want, and so we must delete any pending watchdog timer events.
> */
> del_timer_sync(&ehci->watchdog);
> + hrtimer_cancel(&ehci->hrtimer);
> return 0;
> }
>
> Index: usb-3.4/drivers/usb/host/ehci-timer.c
> ===================================================================
> --- /dev/null
> +++ usb-3.4/drivers/usb/host/ehci-timer.c
> @@ -0,0 +1,106 @@
> +/*
> + * Copyright (C) 2012 by Alan Stern
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +/* This file is part of ehci-hcd.c */
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/*
> + * EHCI timer support... Now using hrtimers.
> + *
> + * Lots of different events are triggered from ehci->hrtimer. Whenever
> + * the timer routine runs, it checks each possible event; events that are
> + * currently enabled and whose expiration time has passed get handled.
> + * The set of enabled events is stored as a collection of bitflags in
> + * ehci->enabled_hrtimer_events, and they are numbered in order of
> + * increasing delay values (ranging between 1 ms and 100 ms).
> + *
> + * Rather than implementing a sorted list or tree of all pending events,
Another candidate may be to use one standalone hrtimer for each event.
> + * we keep track only of the lowest-numbered pending event, in
> + * ehci->next_hrtimer_event. Whenever ehci->hrtimer gets restarted, its
> + * expiration time is set to the timeout value for this event.
> + *
> + * As a result, events might not get handled right away; the actual delay
> + * could be anywhere up to twice the requested delay. This doesn't
> + * matter, because none of the events are especially time-critical. The
> + * ones that matter most all have a delay of 1 ms, so they will be
> + * handled after 2 ms at most, which is okay. In addition to this, we
> + * allow for an expiration range of 1 ms.
> + */
> +
> +/*
> + * Delay lengths for the hrtimer event types.
> + * Keep this list sorted by delay length, in the same order as
> + * the event types indexed by enum ehci_hrtimer_event in ehci.h.
> + */
> +static unsigned event_delays_ns[] = {
According to your design, the time in the above array can't be changed,
so it should be marked as 'const' to reflect the design principle.
> +};
> +
> +/* Enable a pending hrtimer event */
> +static void ehci_enable_event(struct ehci_hcd *ehci, unsigned event,
> + bool resched)
> +{
> + ktime_t *timeout = &ehci->hr_timeouts[event];
> +
> + if (resched)
> + *timeout = ktime_add(ktime_get(),
> + ktime_set(0, event_delays_ns[event]));
> + ehci->enabled_hrtimer_events |= (1 << event);
> +
> + /* Track only the lowest-numbered pending event */
> + if (event < ehci->next_hrtimer_event) {
> + ehci->next_hrtimer_event = event;
> + hrtimer_start_range_ns(&ehci->hrtimer, *timeout,
> + NSEC_PER_MSEC, HRTIMER_MODE_ABS);
The *timeout may have been expired for some time, so can the timer handler
still be called after return from hrtimer_start_range_ns? If not, it
may be a problem.
> + }
> +}
> +
> +
> +/*
> + * Handler functions for the hrtimer event types.
> + * Keep this array in the same order as the event types indexed by
> + * enum ehci_hrtimer_event in ehci.h.
> + */
> +static void (*event_handlers[])(struct ehci_hcd *) = {
> +};
> +
> +static enum hrtimer_restart ehci_hrtimer_func(struct hrtimer *t)
> +{
> + struct ehci_hcd *ehci = container_of(t, struct ehci_hcd, hrtimer);
> + ktime_t now;
> + unsigned long events;
> + unsigned long flags;
> + unsigned e;
> +
> + spin_lock_irqsave(&ehci->lock, flags);
> +
> + events = ehci->enabled_hrtimer_events;
> + ehci->enabled_hrtimer_events = 0;
> + ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
> +
> + /*
> + * Check each pending event. If its time has expired, handle
> + * the event; otherwise re-enable it.
> + */
> + now = ktime_get();
It is better to move the above line after the next line since the event_hander
may consume some timer.
> + for_each_set_bit(e, &events, EHCI_HRTIMER_NUM_EVENTS) {
> + if (now.tv64 >= ehci->hr_timeouts[e].tv64)
> + event_handlers[e](ehci);
> + else
> + ehci_enable_event(ehci, e, false);
> + }
> +
> + spin_unlock_irqrestore(&ehci->lock, flags);
> + return HRTIMER_NORESTART;
> +}
>
>
> --
> 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
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
