diff mbox

[v4,net-next] r8169: Add values missing in @get_stats64 from HW counters

Message ID 1440413559-9354-1-git-send-email-vinschen@redhat.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Corinna Vinschen Aug. 24, 2015, 10:52 a.m. UTC
The r8169 driver collects statistical information returned by
@get_stats64 by counting them in the driver itself, even though many
(but not all) of the values are already collected by tally counters
(TCs) in the NIC.  Some of these TC values are not returned by
@get_stats64.  Especially the received multicast packages are missing
from /proc/net/dev.

Rectify this by fetching the TCs and returning them from
rtl8169_get_stats64.

The counters collected in the driver obviously disappear as soon as the
driver is unloaded so after a driver is loaded the counters always start
at 0. The TCs on the other hand are only reset by a power cycle.  Without
further considerations the values collected by the driver would not match
up against the TC values.

This patch introduces a new function rtl8169_reset_counters which
resets the TCs.  Also, since rtl8169_reset_counters shares most of
its code with rtl8169_update_counters, refactor the shared code into
two new functions  rtl8169_map_counters and rtl8169_unmap_counters.

Unfortunately chip versions prior to RTL_GIGA_MAC_VER_19 don't allow
to reset the TCs programatically.  Therefore introduce an addition to
the rtl8169_private struct and a function rtl8169_init_counter_offsets
to store the TCs at first rtl_open.  Use these values as offsets in
rtl8169_get_stats64.  Propagate a failure to reset *and* update the
counters up to rtl_open and emit a warning message, if so.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---

[v3 -> v4]: Remove useless rtl8169_close chunk changing case of comment


 drivers/net/ethernet/realtek/r8169.c | 163 ++++++++++++++++++++++++++++++++---
 1 file changed, 149 insertions(+), 14 deletions(-)

Comments

David Miller Aug. 25, 2015, 9:15 p.m. UTC | #1
From: Corinna Vinschen <vinschen@redhat.com>
Date: Mon, 24 Aug 2015 12:52:39 +0200

> The r8169 driver collects statistical information returned by
> @get_stats64 by counting them in the driver itself, even though many
> (but not all) of the values are already collected by tally counters
> (TCs) in the NIC.  Some of these TC values are not returned by
> @get_stats64.  Especially the received multicast packages are missing
> from /proc/net/dev.
> 
> Rectify this by fetching the TCs and returning them from
> rtl8169_get_stats64.
> 
> The counters collected in the driver obviously disappear as soon as the
> driver is unloaded so after a driver is loaded the counters always start
> at 0. The TCs on the other hand are only reset by a power cycle.  Without
> further considerations the values collected by the driver would not match
> up against the TC values.
> 
> This patch introduces a new function rtl8169_reset_counters which
> resets the TCs.  Also, since rtl8169_reset_counters shares most of
> its code with rtl8169_update_counters, refactor the shared code into
> two new functions  rtl8169_map_counters and rtl8169_unmap_counters.
> 
> Unfortunately chip versions prior to RTL_GIGA_MAC_VER_19 don't allow
> to reset the TCs programatically.  Therefore introduce an addition to
> the rtl8169_private struct and a function rtl8169_init_counter_offsets
> to store the TCs at first rtl_open.  Use these values as offsets in
> rtl8169_get_stats64.  Propagate a failure to reset *and* update the
> counters up to rtl_open and emit a warning message, if so.
> 
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>

Your counter offsets should be read at probe time, not open time.

Bringing the interface is brought down/up should not reset the
counters.  Look at how the software counters behave in such a
situation.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francois Romieu Aug. 25, 2015, 10:54 p.m. UTC | #2
David Miller <davem@davemloft.net> :
[...]
> Your counter offsets should be read at probe time, not open time.

It can be done but the "CmdRxEnb / rx traffic must be enabled" constraint
will make it a major pita. 

Reading counter offsets at the end of open() naturally solves this
constraint (retentive error unwinding in opne() stops being completely
trivial though :o/ ).

> Bringing the interface is brought down/up should not reset the
> counters.

Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
takes care of it: it's set during the first open() after probe().

Looking at it again, the patch directly stores 16 and 32 bit values
in rtnl_link_stats64. Nobody should care about exact exceedingly high
error count but rx_multicast ought to be accumulated.
David Miller Aug. 25, 2015, 10:59 p.m. UTC | #3
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Wed, 26 Aug 2015 00:54:06 +0200

> David Miller <davem@davemloft.net> :
> [...]
>> Your counter offsets should be read at probe time, not open time.
> 
> It can be done but the "CmdRxEnb / rx traffic must be enabled" constraint
> will make it a major pita. 
> 
> Reading counter offsets at the end of open() naturally solves this
> constraint (retentive error unwinding in opne() stops being completely
> trivial though :o/ ).

So you can manage whether you've done the "once per device probe"
counter reset, and act upon it at the end of ->open().

>> Bringing the interface is brought down/up should not reset the
>> counters.
> 
> Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
> takes care of it: it's set during the first open() after probe().

Ok, then it's fine.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Aug. 25, 2015, 11:03 p.m. UTC | #4
From: David Miller <davem@davemloft.net>
Date: Tue, 25 Aug 2015 15:59:21 -0700 (PDT)

> From: Francois Romieu <romieu@fr.zoreil.com>
> Date: Wed, 26 Aug 2015 00:54:06 +0200
> 
>>> Bringing the interface is brought down/up should not reset the
>>> counters.
>> 
>> Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
>> takes care of it: it's set during the first open() after probe().
> 
> Ok, then it's fine.

And as such I've applied this patch, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Corinna Vinschen Aug. 26, 2015, 8:54 a.m. UTC | #5
On Aug 26 00:54, Francois Romieu wrote:
> David Miller <davem@davemloft.net> :
> [...]
> > Your counter offsets should be read at probe time, not open time.
> 
> It can be done but the "CmdRxEnb / rx traffic must be enabled" constraint
> will make it a major pita. 
> 
> Reading counter offsets at the end of open() naturally solves this
> constraint (retentive error unwinding in opne() stops being completely
> trivial though :o/ ).
> 
> > Bringing the interface is brought down/up should not reset the
> > counters.
> 
> Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
> takes care of it: it's set during the first open() after probe().
> 
> Looking at it again, the patch directly stores 16 and 32 bit values
> in rtnl_link_stats64. Nobody should care about exact exceedingly high
> error count but rx_multicast ought to be accumulated.

I'll have a look into that for a followup patch.


Thanks,
Corinna
Corinna Vinschen Aug. 26, 2015, 8:55 a.m. UTC | #6
On Aug 25 16:03, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Tue, 25 Aug 2015 15:59:21 -0700 (PDT)
> 
> > From: Francois Romieu <romieu@fr.zoreil.com>
> > Date: Wed, 26 Aug 2015 00:54:06 +0200
> > 
> >>> Bringing the interface is brought down/up should not reset the
> >>> counters.
> >> 
> >> Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
> >> takes care of it: it's set during the first open() after probe().
> > 
> > Ok, then it's fine.
> 
> And as such I've applied this patch, thanks.

Thanks,
Corinna
diff mbox

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f790f61..d6d39df 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -637,6 +637,9 @@  enum rtl_register_content {
 	/* _TBICSRBit */
 	TBILinkOK	= 0x02000000,
 
+	/* ResetCounterCommand */
+	CounterReset	= 0x1,
+
 	/* DumpCounterCommand */
 	CounterDump	= 0x8,
 
@@ -747,6 +750,14 @@  struct rtl8169_counters {
 	__le16	tx_underun;
 };
 
+struct rtl8169_tc_offsets {
+	bool	inited;
+	__le64	tx_errors;
+	__le32	tx_multi_collision;
+	__le32	rx_multicast;
+	__le16	tx_aborted;
+};
+
 enum rtl_flag {
 	RTL_FLAG_TASK_ENABLED,
 	RTL_FLAG_TASK_SLOW_PENDING,
@@ -824,6 +835,7 @@  struct rtl8169_private {
 
 	struct mii_if_info mii;
 	struct rtl8169_counters counters;
+	struct rtl8169_tc_offsets tc_offset;
 	u32 saved_wolopts;
 	u32 opts1_mask;
 
@@ -2179,6 +2191,73 @@  static int rtl8169_get_sset_count(struct net_device *dev, int sset)
 	}
 }
 
+static struct rtl8169_counters *rtl8169_map_counters(struct net_device *dev,
+						     dma_addr_t *paddr,
+						     u32 counter_cmd)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	void __iomem *ioaddr = tp->mmio_addr;
+	struct device *d = &tp->pci_dev->dev;
+	struct rtl8169_counters *counters;
+	u32 cmd;
+
+	counters = dma_alloc_coherent(d, sizeof(*counters), paddr, GFP_KERNEL);
+	if (counters) {
+		RTL_W32(CounterAddrHigh, (u64)*paddr >> 32);
+		cmd = (u64)*paddr & DMA_BIT_MASK(32);
+		RTL_W32(CounterAddrLow, cmd);
+		RTL_W32(CounterAddrLow, cmd | counter_cmd);
+	}
+	return counters;
+}
+
+static void rtl8169_unmap_counters (struct net_device *dev,
+				    dma_addr_t paddr,
+				    struct rtl8169_counters *counters)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	void __iomem *ioaddr = tp->mmio_addr;
+	struct device *d = &tp->pci_dev->dev;
+
+	RTL_W32(CounterAddrLow, 0);
+	RTL_W32(CounterAddrHigh, 0);
+
+	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+}
+
+DECLARE_RTL_COND(rtl_reset_counters_cond)
+{
+	void __iomem *ioaddr = tp->mmio_addr;
+
+	return RTL_R32(CounterAddrLow) & CounterReset;
+}
+
+static bool 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 ret;
+}
+
 DECLARE_RTL_COND(rtl_counters_cond)
 {
 	void __iomem *ioaddr = tp->mmio_addr;
@@ -2186,38 +2265,72 @@  DECLARE_RTL_COND(rtl_counters_cond)
 	return RTL_R32(CounterAddrLow) & CounterDump;
 }
 
-static void rtl8169_update_counters(struct net_device *dev)
+static bool rtl8169_update_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
-	struct device *d = &tp->pci_dev->dev;
 	struct rtl8169_counters *counters;
 	dma_addr_t paddr;
-	u32 cmd;
+	bool ret = true;
 
 	/*
 	 * Some chips are unable to dump tally counters when the receiver
 	 * is disabled.
 	 */
 	if ((RTL_R8(ChipCmd) & CmdRxEnb) == 0)
-		return;
+		return true;
 
-	counters = dma_alloc_coherent(d, sizeof(*counters), &paddr, GFP_KERNEL);
+	counters = rtl8169_map_counters(dev, &paddr, CounterDump);
 	if (!counters)
-		return;
-
-	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
-	cmd = (u64)paddr & DMA_BIT_MASK(32);
-	RTL_W32(CounterAddrLow, cmd);
-	RTL_W32(CounterAddrLow, cmd | CounterDump);
+		return false;
 
 	if (rtl_udelay_loop_wait_low(tp, &rtl_counters_cond, 10, 1000))
 		memcpy(&tp->counters, counters, sizeof(*counters));
+	else
+		ret = false;
 
-	RTL_W32(CounterAddrLow, 0);
-	RTL_W32(CounterAddrHigh, 0);
+	rtl8169_unmap_counters(dev, paddr, counters);
 
-	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+	return ret;
+}
+
+static bool rtl8169_init_counter_offsets(struct net_device *dev)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	bool ret = false;
+
+	/*
+	 * rtl8169_init_counter_offsets is called from rtl_open.  On chip
+	 * versions prior to RTL_GIGA_MAC_VER_19 the tally counters are only
+	 * reset by a power cycle, while the counter values collected by the
+	 * driver are reset at every driver unload/load cycle.
+	 *
+	 * To make sure the HW values returned by @get_stats64 match the SW
+	 * values, we collect the initial values at first open(*) and use them
+	 * as offsets to normalize the values returned by @get_stats64.
+	 *
+	 * (*) We can't call rtl8169_init_counter_offsets from rtl_init_one
+	 * for the reason stated in rtl8169_update_counters; CmdRxEnb is only
+	 * set at open time by rtl_hw_start.
+	 */
+
+	if (tp->tc_offset.inited)
+		return true;
+
+	/* If both, reset and update fail, propagate to caller. */
+	if (rtl8169_reset_counters(dev))
+		ret = true;
+
+	if (rtl8169_update_counters(dev))
+		ret = true;
+
+	tp->tc_offset.tx_errors = tp->counters.tx_errors;
+	tp->tc_offset.tx_multi_collision = tp->counters.tx_multi_collision;
+	tp->tc_offset.rx_multicast = tp->counters.rx_multicast;
+	tp->tc_offset.tx_aborted = tp->counters.tx_aborted;
+	tp->tc_offset.inited = true;
+
+	return ret;
 }
 
 static void rtl8169_get_ethtool_stats(struct net_device *dev,
@@ -7631,6 +7744,9 @@  static int rtl_open(struct net_device *dev)
 
 	rtl_hw_start(dev);
 
+	if (!rtl8169_init_counter_offsets(dev))
+		netif_warn(tp, hw, dev, "counter reset/update failed\n");
+
 	netif_start_queue(dev);
 
 	rtl_unlock_work(tp);
@@ -7689,6 +7805,25 @@  rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	stats->rx_fifo_errors	= dev->stats.rx_fifo_errors;
 	stats->rx_missed_errors = dev->stats.rx_missed_errors;
 
+	/*
+	 * Fetch additonal counter values missing in stats collected by driver
+	 * from tally counters.
+	 */
+	rtl8169_update_counters(dev);
+
+	/*
+	 * Subtract values fetched during initalization.
+	 * See rtl8169_init_counter_offsets for a description why we do that.
+	 */
+	stats->tx_errors = le64_to_cpu(tp->counters.tx_errors) -
+		le64_to_cpu(tp->tc_offset.tx_errors);
+	stats->collisions = le32_to_cpu(tp->counters.tx_multi_collision) -
+		le32_to_cpu(tp->tc_offset.tx_multi_collision);
+	stats->multicast = le32_to_cpu(tp->counters.rx_multicast) -
+		le32_to_cpu(tp->tc_offset.rx_multicast);
+	stats->tx_aborted_errors = le16_to_cpu(tp->counters.tx_aborted) -
+		le16_to_cpu(tp->tc_offset.tx_aborted);
+
 	return stats;
 }