Re: [PATCH] Re: mmotm 2010-07-19 - e1000e vs. pm_qos_update_request issues

[Jeff Kirsher]

On Wed, Jul 21, 2010 at 00:12, Florian Mickler <florian@xxxxxxxxxxx> wrote:
> On Tue, 20 Jul 2010 14:07:51 -0700
> Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>> On Tue, 20 Jul 2010 16:35:25 -0400
>> Valdis.Kletnieks@xxxxxx wrote:
>>
>> > On Mon, 19 Jul 2010 16:38:09 PDT, akpm@xxxxxxxxxxxxxxxxxxxx said:
>> > > The mm-of-the-moment snapshot 2010-07-19-16-37 has been uploaded to
>> > >
>> > > Â Âhttp://userweb.kernel.org/~akpm/mmotm/
>> >
>> > Throws a warning at boot:
>> >
>> > [ Â Â1.786060] WARNING: at kernel/pm_qos_params.c:264 pm_qos_update_request+0x28/0x54()
>> > [ Â Â1.786088] Hardware name: Latitude E6500
>> > [ Â Â1.787045] pm_qos_update_request() called for unknown object
>> > [ Â Â1.787966] Modules linked in:
>> > [ Â Â1.788940] Pid: 1, comm: swapper Not tainted 2.6.35-rc5-mmotm0719 #1
>> > [ Â Â1.790035] Call Trace:
>> > [ Â Â1.791121] Â[<ffffffff81037335>] warn_slowpath_common+0x80/0x98
>> > [ Â Â1.792205] Â[<ffffffff810373e1>] warn_slowpath_fmt+0x41/0x43
>> > [ Â Â1.793279] Â[<ffffffff81057c14>] pm_qos_update_request+0x28/0x54
>> > [ Â Â1.794347] Â[<ffffffff8134889e>] e1000_configure+0x421/0x459
>> > [ Â Â1.795393] Â[<ffffffff8134afbd>] e1000_open+0xbd/0x37c
>> > [ Â Â1.796436] Â[<ffffffff8105743a>] ? raw_notifier_call_chain+0xf/0x11
>> > [ Â Â1.797491] Â[<ffffffff8145f948>] __dev_open+0xae/0xe2
>> > [ Â Â1.798547] Â[<ffffffff8145f997>] dev_open+0x1b/0x49
>> > [ Â Â1.799612] Â[<ffffffff8146e36e>] netpoll_setup+0x84/0x259
>> > [ Â Â1.800685] Â[<ffffffff81b5037c>] init_netconsole+0xbc/0x21f
>> > [ Â Â1.801744] Â[<ffffffff81b5026c>] ? sir_wq_init+0x0/0x35
>> > [ Â Â1.802793] Â[<ffffffff81b502c0>] ? init_netconsole+0x0/0x21f
>> > [ Â Â1.803845] Â[<ffffffff810002ff>] do_one_initcall+0x7a/0x12f
>> > [ Â Â1.804885] Â[<ffffffff81b2ccae>] kernel_init+0x138/0x1c2
>> > [ Â Â1.805915] Â[<ffffffff81003554>] kernel_thread_helper+0x4/0x10
>> > [ Â Â1.806937] Â[<ffffffff81590e00>] ? restore_args+0x0/0x30
>> > [ Â Â1.807955] Â[<ffffffff81b2cb76>] ? kernel_init+0x0/0x1c2
>> > [ Â Â1.808958] Â[<ffffffff81003550>] ? kernel_thread_helper+0x0/0x10
>> > [ Â Â1.809958] ---[ end trace 84b562a00a60539e ]---
>> >
>> > Looks like a repeat of something I reported against -mmotm 2010-05-11, though a
>> > WARNING rather than an outright crash - the traceback is pretty much identical.
>> > ÂI have *no* idea why -rc3-mmotm0701 doesn't whinge similarly.
>> >
>>
>> I don't recall you reporting that, sorry.
>>
>> The warning was added by
>>
>> : commit 82f682514a5df89ffb3890627eebf0897b7a84ec
>> : Author: Â Â James Bottomley <James.Bottomley@xxxxxxx>
>> : AuthorDate: Mon Jul 5 22:53:06 2010 +0200
>> : Commit: Â Â Rafael J. Wysocki <rjw@xxxxxxx>
>> : CommitDate: Mon Jul 19 02:00:34 2010 +0200
>> :
>> : Â Â pm_qos: Get rid of the allocation in pm_qos_add_request()
>>
>>
>> It's a pretty crappy warning too. ÂNeither the warning nor the code
>> comments provide developers with any hint as to what they have done
>> wrong, nor what they must do to fix things. ÂAnd the patch changelog
>> doesn't mention the new warnings *at all*.
>>
>> So one must assume that the people who stuck this thing in the tree
>> have volunteered to fix e1000e. ÂLet's cc 'em.
>>
>
> e1000 calls update_request before registering said request with pm_qos.
> This was silently ignored before but now emits a warning. The warning
> is sound, because it means, that the constraint-request didn't take
> effect.
>
> The right thing is probably to register the request before
> calling update_request.
>
> Attached patch moves the registering from e1000_up to e1000_open and
> the unregistering from e1000_down to e1000_close.
> It is only compile-tested as I don't have the hardware.
>
> Cheers,
> Flo
>
> p.s.: sorry if this get's mangled or is wrongly formatted, i'm just using
> Âthe "insert file" option of my mailclient and crossing my fingers...
>
>
> From 693c71b911ff0845c872261d5704a1d40960722d Mon Sep 17 00:00:00 2001
> From: Florian Mickler <florian@xxxxxxxxxxx>
> Date: Wed, 21 Jul 2010 08:44:21 +0200
> Subject: [PATCH] e1000e: register pm_qos request on hardware activation
>
> The pm_qos_add_request call has to register the pm_qos request with the pm_qos
> susbsystem before first use of the pm_qos request via
> pm_qos_update_request.
>
> As pm_qos changed to use plists there is no benefit in registering and
> unregistering the pm_qos request on ifup/ifdown and thus we move the
> registering into e1000_open and the unregistering in e1000_close.
>
> This fixes the following warning:
>
> [ Â Â1.786060] WARNING: at kernel/pm_qos_params.c:264
> pm_qos_update_request+0x28/0x54()
> [ Â Â1.786088] Hardware name: Latitude E6500
> [ Â Â1.787045] pm_qos_update_request() called for unknown object
> [ Â Â1.787966] Modules linked in:
> [ Â Â1.788940] Pid: 1, comm: swapper Not tainted 2.6.35-rc5-mmotm0719 #1
> [ Â Â1.790035] Call Trace:
> [ Â Â1.791121] Â[<ffffffff81037335>] warn_slowpath_common+0x80/0x98
> [ Â Â1.792205] Â[<ffffffff810373e1>] warn_slowpath_fmt+0x41/0x43
> [ Â Â1.793279] Â[<ffffffff81057c14>] pm_qos_update_request+0x28/0x54
> [ Â Â1.794347] Â[<ffffffff8134889e>] e1000_configure+0x421/0x459
> [ Â Â1.795393] Â[<ffffffff8134afbd>] e1000_open+0xbd/0x37c
> [ Â Â1.796436] Â[<ffffffff8105743a>] ? raw_notifier_call_chain+0xf/0x11
> [ Â Â1.797491] Â[<ffffffff8145f948>] __dev_open+0xae/0xe2
> [ Â Â1.798547] Â[<ffffffff8145f997>] dev_open+0x1b/0x49
> [ Â Â1.799612] Â[<ffffffff8146e36e>] netpoll_setup+0x84/0x259
> [ Â Â1.800685] Â[<ffffffff81b5037c>] init_netconsole+0xbc/0x21f
> [ Â Â1.801744] Â[<ffffffff81b5026c>] ? sir_wq_init+0x0/0x35
> [ Â Â1.802793] Â[<ffffffff81b502c0>] ? init_netconsole+0x0/0x21f
> [ Â Â1.803845] Â[<ffffffff810002ff>] do_one_initcall+0x7a/0x12f
> [ Â Â1.804885] Â[<ffffffff81b2ccae>] kernel_init+0x138/0x1c2
> [ Â Â1.805915] Â[<ffffffff81003554>] kernel_thread_helper+0x4/0x10
> [ Â Â1.806937] Â[<ffffffff81590e00>] ? restore_args+0x0/0x30
> [ Â Â1.807955] Â[<ffffffff81b2cb76>] ? kernel_init+0x0/0x1c2
> [ Â Â1.808958] Â[<ffffffff81003550>] ? kernel_thread_helper+0x0/0x10
> [ Â Â1.809958] ---[ end trace 84b562a00a60539e ]---
>
> Signed-off-by: Florian Mickler <florian@xxxxxxxxxxx>
> ---
> Âdrivers/net/e1000e/netdev.c | Â 18 +++++++++---------
> Â1 files changed, 9 insertions(+), 9 deletions(-)
>

Thanks I have added the patch to my queue.

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