From patchwork Mon Sep 7 14:44:12 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Corinna Vinschen X-Patchwork-Id: 515168 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 9C73F1401AF for ; Tue, 8 Sep 2015 00:44:21 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752216AbbIGOoS (ORCPT ); Mon, 7 Sep 2015 10:44:18 -0400 Received: from mail-n.franken.de ([193.175.24.27]:40835 "EHLO mail-n.franken.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750977AbbIGOoQ (ORCPT ); Mon, 7 Sep 2015 10:44:16 -0400 Received: from aqua.hirmke.de (aquarius.franken.de [193.175.24.89]) by mail-n.franken.de (Postfix) with ESMTP id 9E7271C0B460E; Mon, 7 Sep 2015 16:44:13 +0200 (CEST) Received: from calimero.vinschen.de (calimero.vinschen.de [192.168.129.6]) by aqua.hirmke.de (Postfix) with ESMTP id 05AE35E0377; Mon, 7 Sep 2015 16:44:13 +0200 (CEST) Received: by calimero.vinschen.de (Postfix, from userid 500) id F0508A807DA; Mon, 7 Sep 2015 16:44:12 +0200 (CEST) Date: Mon, 7 Sep 2015 16:44:12 +0200 From: Corinna Vinschen To: romieu@fr.zoreil.com, netdev@vger.kernel.org, David Miller , pomidorabelisima@gmail.com Subject: Re: [PATCH net 3/3] r8169: increase the lifespan of the hardware counters dump area. Message-ID: <20150907144412.GA26524@calimero.vinschen.de> Mail-Followup-To: romieu@fr.zoreil.com, netdev@vger.kernel.org, David Miller , pomidorabelisima@gmail.com References: <20150906103732.GF30539@calimero.vinschen.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150906103732.GF30539@calimero.vinschen.de> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sep 6 12:37, Corinna Vinschen wrote: > On Sep 5 14:18, romieu@fr.zoreil.com wrote: > > From: Francois Romieu > > > > net/core/net-sysfs.c::netstat_show retrieves stats with spinlock held. > > > > This change avoids sleepable allocation and performs some housekeeping: > > - receive ring, transmit ring and counters dump area allocation failures > > are all considered fatal during open() > > - netif_warn is now redundant with rtl_reset_counters_cond built-in > > failure message. > > - rtl_reset_counters_cond induced failures in open() are also considered > > fatal: it takes acceptable work to unwind comfortably. > > Why? The counter reset is not a fatal condition for the operation of > the NIC. Even if the counters show incorrect values, the normal > operation of the NIC is not affected. And to top that off, even if > resetting the counters actually doesn't work, the driver keeps the > offset values, so a failed reset won't even harm the statistics. It > just doesn't make sense to break the NIC operation for such a minor > problem. And worse: > > > -static bool rtl8169_reset_counters(struct net_device *dev) > > +static int rtl8169_reset_counters(struct net_device *dev) > > { > > struct rtl8169_private *tp = netdev_priv(dev); > > - struct rtl8169_counters *counters; > > - dma_addr_t paddr; > > - bool ret = true; > > > > /* > > * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the > > * tally counters. > > */ > > if (tp->mac_version < RTL_GIGA_MAC_VER_19) > > - return true; > > - > > - counters = rtl8169_map_counters(dev, &paddr, CounterReset); > > - if (!counters) > > - return false; > > - > > - if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000)) > > - ret = false; > > - > > - rtl8169_unmap_counters(dev, paddr, counters); > > + return -EINVAL; > > This returns -EINVAL even for older chip versions which are not capable > of resetting the counters. The result is, this driver will not work at > all on chip versions prior to RTL_GIGA_MAC_VER_19 anymore, because > rtl_open will always fail. > > Other then that, I can test the patch probably tomorrow. I have a bit of a problem with this patch. It crashes when loading the driver manually w/ modprobe. For some reason dev_get_stats is called during rtl_init_one and at that time the counters pointer is NULL, so the kernel gets a SEGV. After I worked around that I got a SEGV in rtl_remove_one, which I still have to find out why. I didn't test with the latest kernel, though, so I still have to check if that's the reason for the crashes. That takes a bit longer, I just wanted to let you know. There's also a problem with rtl8169_cmd_counters: It always calls rtl_udelay_loop_wait_low w/ rtl_reset_counters_cond, even when called from rtl8169_update_counters, where it should call rtl_udelay_loop_wait_low w/ rtl_counters_cond to check for the CounterDump flag, rather than for the CounterReset flag. For now I applied the below patch, but I think it's a bit ugly to conditionalize the rtl_cond struct by the incoming flag value. Corinna diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index ae07567..cd7adbf 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -2200,6 +2200,13 @@ DECLARE_RTL_COND(rtl_reset_counters_cond) return RTL_R32(CounterAddrLow) & CounterReset; } +DECLARE_RTL_COND(rtl_counters_cond) +{ + void __iomem *ioaddr = tp->mmio_addr; + + return RTL_R32(CounterAddrLow) & CounterDump; +} + static int rtl8169_cmd_counters(struct net_device *dev, u32 counter_cmd) { struct rtl8169_private *tp = netdev_priv(dev); @@ -2212,7 +2219,10 @@ static int rtl8169_cmd_counters(struct net_device *dev, u32 counter_cmd) RTL_W32(CounterAddrLow, cmd); RTL_W32(CounterAddrLow, cmd | counter_cmd); - return rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000); + return rtl_udelay_loop_wait_low(tp, + counter_cmd == CounterDump ? &rtl_counters_cond : + &rtl_reset_counters_cond, + 10, 1000); } static int rtl8169_reset_counters(struct net_device *dev) @@ -2229,13 +2239,6 @@ static int rtl8169_reset_counters(struct net_device *dev) return rtl8169_cmd_counters(dev, CounterReset); } -DECLARE_RTL_COND(rtl_counters_cond) -{ - void __iomem *ioaddr = tp->mmio_addr; - - return RTL_R32(CounterAddrLow) & CounterDump; -} - static int rtl8169_update_counters(struct net_device *dev) { struct rtl8169_private *tp = netdev_priv(dev); @@ -7800,8 +7803,11 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats) /* * Fetch additonal counter values missing in stats collected by driver - * from tally counters. + * from tally counters, but only if we're already initialized. */ + if (!counters) + return stats; + rtl8169_update_counters(dev); /*