From patchwork Wed Jul 21 07:12:00 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: florian@mickler.org X-Patchwork-Id: 59404 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id A20FAB6F14 for ; Wed, 21 Jul 2010 17:12:34 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933578Ab0GUHMN (ORCPT ); Wed, 21 Jul 2010 03:12:13 -0400 Received: from ist.d-labs.de ([213.239.218.44]:48085 "EHLO mx01.d-labs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933332Ab0GUHML convert rfc822-to-8bit (ORCPT ); Wed, 21 Jul 2010 03:12:11 -0400 Received: from schatten.dmk.lab (f053215167.adsl.alicedsl.de [78.53.215.167]) by mx01.d-labs.de (Postfix) with ESMTP id BE5007F906; Wed, 21 Jul 2010 09:12:04 +0200 (CEST) Date: Wed, 21 Jul 2010 09:12:00 +0200 From: Florian Mickler To: Andrew Morton Newsgroups: gmane.linux.drivers.e1000.devel, gmane.linux.network, gmane.linux.kernel Cc: Valdis.Kletnieks@vt.edu, "Rafael J. Wysocki" , mark gross , e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, James Bottomley , Thomas Gleixner , "David S. Miller" Subject: [PATCH] Re: mmotm 2010-07-19 - e1000e vs. pm_qos_update_request issues Message-ID: <20100721091200.40c43158@schatten.dmk.lab> In-Reply-To: <20100720140751.71ee83a8.akpm@linux-foundation.org> References: <201007200007.o6K07Xbg028863@imap1.linux-foundation.org> <6182.1279658125@localhost> <20100720140751.71ee83a8.akpm@linux-foundation.org> X-Newsreader: Claws Mail 3.7.6 (GTK+ 2.18.9; x86_64-unknown-linux-gnu) Mime-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tue, 20 Jul 2010 14:07:51 -0700 Andrew Morton wrote: > On Tue, 20 Jul 2010 16:35:25 -0400 > Valdis.Kletnieks@vt.edu wrote: > > > On Mon, 19 Jul 2010 16:38:09 PDT, akpm@linux-foundation.org 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] [] warn_slowpath_common+0x80/0x98 > > [ 1.792205] [] warn_slowpath_fmt+0x41/0x43 > > [ 1.793279] [] pm_qos_update_request+0x28/0x54 > > [ 1.794347] [] e1000_configure+0x421/0x459 > > [ 1.795393] [] e1000_open+0xbd/0x37c > > [ 1.796436] [] ? raw_notifier_call_chain+0xf/0x11 > > [ 1.797491] [] __dev_open+0xae/0xe2 > > [ 1.798547] [] dev_open+0x1b/0x49 > > [ 1.799612] [] netpoll_setup+0x84/0x259 > > [ 1.800685] [] init_netconsole+0xbc/0x21f > > [ 1.801744] [] ? sir_wq_init+0x0/0x35 > > [ 1.802793] [] ? init_netconsole+0x0/0x21f > > [ 1.803845] [] do_one_initcall+0x7a/0x12f > > [ 1.804885] [] kernel_init+0x138/0x1c2 > > [ 1.805915] [] kernel_thread_helper+0x4/0x10 > > [ 1.806937] [] ? restore_args+0x0/0x30 > > [ 1.807955] [] ? kernel_init+0x0/0x1c2 > > [ 1.808958] [] ? 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 > : AuthorDate: Mon Jul 5 22:53:06 2010 +0200 > : Commit: Rafael J. Wysocki > : 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 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] [] warn_slowpath_common+0x80/0x98 [ 1.792205] [] warn_slowpath_fmt+0x41/0x43 [ 1.793279] [] pm_qos_update_request+0x28/0x54 [ 1.794347] [] e1000_configure+0x421/0x459 [ 1.795393] [] e1000_open+0xbd/0x37c [ 1.796436] [] ? raw_notifier_call_chain+0xf/0x11 [ 1.797491] [] __dev_open+0xae/0xe2 [ 1.798547] [] dev_open+0x1b/0x49 [ 1.799612] [] netpoll_setup+0x84/0x259 [ 1.800685] [] init_netconsole+0xbc/0x21f [ 1.801744] [] ? sir_wq_init+0x0/0x35 [ 1.802793] [] ? init_netconsole+0x0/0x21f [ 1.803845] [] do_one_initcall+0x7a/0x12f [ 1.804885] [] kernel_init+0x138/0x1c2 [ 1.805915] [] kernel_thread_helper+0x4/0x10 [ 1.806937] [] ? restore_args+0x0/0x30 [ 1.807955] [] ? kernel_init+0x0/0x1c2 [ 1.808958] [] ? kernel_thread_helper+0x0/0x10 [ 1.809958] ---[ end trace 84b562a00a60539e ]--- Signed-off-by: Florian Mickler Tested-by: Valdis Kletnieks --- drivers/net/e1000e/netdev.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c index 8ba366a..1bd9054 100644 --- a/drivers/net/e1000e/netdev.c +++ b/drivers/net/e1000e/netdev.c @@ -3218,12 +3218,6 @@ int e1000e_up(struct e1000_adapter *adapter) { struct e1000_hw *hw = &adapter->hw; - /* DMA latency requirement to workaround early-receive/jumbo issue */ - if (adapter->flags & FLAG_HAS_ERT) - pm_qos_add_request(&adapter->netdev->pm_qos_req, - PM_QOS_CPU_DMA_LATENCY, - PM_QOS_DEFAULT_VALUE); - /* hardware has been reset, we need to reload some things */ e1000_configure(adapter); @@ -3287,9 +3281,6 @@ void e1000e_down(struct e1000_adapter *adapter) e1000_clean_tx_ring(adapter); e1000_clean_rx_ring(adapter); - if (adapter->flags & FLAG_HAS_ERT) - pm_qos_remove_request(&adapter->netdev->pm_qos_req); - /* * TODO: for power management, we could drop the link and * pci_disable_device here. @@ -3524,6 +3515,12 @@ static int e1000_open(struct net_device *netdev) E1000_MNG_DHCP_COOKIE_STATUS_VLAN)) e1000_update_mng_vlan(adapter); + /* DMA latency requirement to workaround early-receive/jumbo issue */ + if (adapter->flags & FLAG_HAS_ERT) + pm_qos_add_request(&adapter->netdev->pm_qos_req, + PM_QOS_CPU_DMA_LATENCY, + PM_QOS_DEFAULT_VALUE); + /* * before we allocate an interrupt, we must be ready to handle it. * Setting DEBUG_SHIRQ in the kernel makes it fire an interrupt @@ -3628,6 +3625,9 @@ static int e1000_close(struct net_device *netdev) if (adapter->flags & FLAG_HAS_AMT) e1000_release_hw_control(adapter); + if (adapter->flags & FLAG_HAS_ERT) + pm_qos_remove_request(&adapter->netdev->pm_qos_req); + pm_runtime_put_sync(&pdev->dev); return 0;