Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751558AbcCTUxB (ORCPT ); Sun, 20 Mar 2016 16:53:01 -0400 Received: from shards.monkeyblade.net ([149.20.54.216]:60296 "EHLO shards.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751131AbcCTUwy (ORCPT ); Sun, 20 Mar 2016 16:52:54 -0400 Date: Sun, 20 Mar 2016 16:52:51 -0400 (EDT) Message-Id: <20160320.165251.836830192768797071.davem@davemloft.net> To: eric.dumazet@gmail.com Cc: geert@linux-m68k.org, woojung.huh@microchip.com, UNGLinuxDriver@microchip.com, linux@roeck-us.net, rjw@rjwysocki.net, netdev@vger.kernel.org, linux-usb@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM From: David Miller In-Reply-To: <1458491752.10868.4.camel@edumazet-glaptop3.roam.corp.google.com> References: <1458470636-18986-1-git-send-email-geert@linux-m68k.org> <1458491752.10868.4.camel@edumazet-glaptop3.roam.corp.google.com> X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO) Mime-Version: 1.0 Content-Type: Text/Plain; charset=iso-8859-7 X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.12 (shards.monkeyblade.net [149.20.54.216]); Sun, 20 Mar 2016 13:52:53 -0700 (PDT) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u2KKr5Oh002264 Content-Length: 2318 Lines: 61 From: Eric Dumazet Date: Sun, 20 Mar 2016 09:35:52 -0700 > On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote: >> If CONFIG_PM=n: >> >> drivers/net/usb/lan78xx.c: In function ?lan78xx_get_stats64?: >> drivers/net/usb/lan78xx.c:3274: error: ?struct dev_pm_info? has no member named ?runtime_auto? >> >> If PM is disabled, the runtime_auto flag is not available, but auto >> suspend is not enabled anyway. Hence protect the check for runtime_auto >> by #ifdef CONFIG_PM to fix this. >> >> Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64") >> Reported-by: Guenter Roeck >> Signed-off-by: Geert Uytterhoeven >> --- >> Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper to >> include/linux/pm.h, which always return false if CONFIG_PM is disabled. >> >> The only other user in non-core code (drivers/usb/core/sysfs.c) has a >> big #ifdef CONFIG_PM check around all PM-related code. >> >> Thoughts? >> --- >> drivers/net/usb/lan78xx.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c >> index d36d5ebf37f355f2..7b9ac47b2ecf9905 100644 >> --- a/drivers/net/usb/lan78xx.c >> +++ b/drivers/net/usb/lan78xx.c >> @@ -3271,7 +3271,9 @@ struct rtnl_link_stats64 *lan78xx_get_stats64(struct net_device *netdev, >> * periodic reading from HW will prevent from entering USB auto suspend. >> * if autosuspend is disabled, read from HW. >> */ >> +#ifdef CONFIG_PM >> if (!dev->udev->dev.power.runtime_auto) >> +#endif >> lan78xx_update_stats(dev); >> >> mutex_lock(&dev->stats.access_lock); > > Note that a ndo_get_stat64() handler is not allowed to sleep, > so the mutex_lock() is not wise... > > Historically /proc/net/dev handler but also bonding ndo_get_stats() used > RCU or a rwlock or a spinlock. > > So a complete fix would need to get rid of this mutex as well. This function is also buggy for another reason. The driver needs to capture the statistics when the runtime suspend happens. Since it's much easier to find things wrong rather than right with this new code, I've decided to completely revert the commit that added lan78xx_get_stats64(). Once a correct version is implemented we can add it back. Thanks.