lkml.org 
[lkml]   [2008]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management
From
Date
On Thu, 2008-10-02 at 16:56 -0700, Andrew Morton wrote:
> On Thu, 25 Sep 2008 17:25:44 +0800
> Yi Yang <yi.y.yang@intel.com> wrote:
>
> > ehci_watchdog will wake up CPU very frequently so that CPU
> > stays at C3 very short, average residence time is about 50
> > ms on Aspire One, but we expect it should be about 1 second
> > or more, so this kind of periodic timer is very bad for power
> > saving.
> >
> > We can't remove this timer because of some bad USB controller
> > chipset, but at least we should reduce its side effect to as
> > possible as low.
> >
> > This patch can make CPU stay at C3 longer, average residence time
> > is about twice as long as original.
> >
> > Please consider to apply it, thanks
> >
> > Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> > ---
> > ehci.h | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> > index 5799298..9d530d9 100644
> > --- a/drivers/usb/host/ehci.h
> > +++ b/drivers/usb/host/ehci.h
> > @@ -181,14 +181,16 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
> > * the async ring; just the I/O watchdog. Note that if a
> > * SHRINK were pending, OFF would never be requested.
> > */
> > - if (timer_pending(&ehci->watchdog)
> > - && ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> > - & ehci->actions))
> > - return;
> > + enum ehci_timer_action oldactions = ehci->actions;
> >
> > if (!test_and_set_bit (action, &ehci->actions)) {
> > unsigned long t;
> >
> > + if (timer_pending(&ehci->watchdog)
> > + && ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> > + & oldactions))
> > + return;
> > +
> > switch (action) {
> > case TIMER_IO_WATCHDOG:
> > t = EHCI_IO_JIFFIES;
> > @@ -204,7 +206,7 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
> > t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
> > break;
> > }
> > - mod_timer(&ehci->watchdog, t + jiffies);
> > + mod_timer(&ehci->watchdog, round_jiffies(t + jiffies));
> > }
> > }
> >
>
> <looks>
>
> <regrets it>
>
>
> Why does this:
>
> t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
>
> add "1000" to a jiffies value when it doesn't know what HZ is? It'll
> be adding anywhere from one second up to ten seconds to the timeout
> interval depending upon compile-time options.
I don't know why original author did so.

>
> I suspect s/1000/HZ/ would improve things here. Or just delete it -
> doesn't the subsequent round_jiffies() do the same thing, only better?
> This code needs help, I suspect.
If 1000 is substituted with HZ, this computation is meaningless, we can
invoke EHCI_SHRINK_FRAMES directly.

I think the best way to do with this bad case is to remove such a
watchdog completely unless we detect a bad usb chipset which will lost
data frame if we don't start watchdog timer.
>
>
> Also, do we really need to inline this large function into at least
> five callsites?
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2008-10-08 04:43    [W:0.076 / U:6.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site