[PATCH 5/5] r8169: ethtool hardware statistics support

[PATCH 5/5] r8169: ethtool hardware statistics support

[Stephen Hemminger]

Add ethtool support for dumping the chip statistics. There aren't lots
of statistics available, but this is what is available according to the RealTek
documentation.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

diff -Nru a/drivers/net/r8169.c b/drivers/net/r8169.c
--- a/drivers/net/r8169.c	2005-03-09 11:26:04 -08:00
+++ b/drivers/net/r8169.c	2005-03-09 11:26:04 -08:00
@@ -195,6 +195,8 @@
 enum RTL8169_registers {
 	MAC0 = 0,		/* Ethernet hardware address. */
 	MAR0 = 8,		/* Multicast filter. */
+	CounterAddrLow = 0x10,
+	CounterAddrHigh = 0x14,
 	TxDescStartAddrLow = 0x20,
 	TxDescStartAddrHigh = 0x24,
 	TxHDescStartAddrLow = 0x28,
@@ -336,6 +338,9 @@
 
 	/* _TBICSRBit */
 	TBILinkOK = 0x02000000,
+
+	/* DumpCounterCommand */
+	CounterDump = 0x8,
 };
 
 enum _DescStatusBit {
@@ -897,6 +902,100 @@
 	tp->msg_enable = value;
 }
 
+static const char rtl8169_gstrings[][ETH_GSTRING_LEN] = {
+	"tx_packets",
+	"rx_packets",
+	"tx_errors",
+	"rx_errors",
+	"rx_missed",
+	"align_errors",
+	"tx_single_collisions",
+	"tx_multi_collisions",
+	"unicast",
+	"broadcast",
+	"multicast",
+	"tx_aborted",
+	"tx_underrun",
+};
+
+struct rtl8169_counters {
+	u64	tx_packets;
+	u64	rx_packets;
+	u64	tx_errors;
+	u32	rx_errors;
+	u16	rx_missed;
+	u16	align_errors;
+	u32	tx_one_collision;
+	u32	tx_multi_collision;
+	u64	rx_unicast;
+	u64	rx_broadcast;
+	u32	rx_multicast;
+	u16	tx_aborted;
+	u16	tx_underun;
+};
+
+static int rtl8169_get_stats_count(struct net_device *dev)
+{
+	return 13;
+}
+
+static void rtl8169_get_ethtool_stats(struct net_device *dev,
+				      struct ethtool_stats *stats, u64 *data)
+{
+        struct rtl8169_private *tp = netdev_priv(dev);
+	struct rtl8169_counters *counters;
+	void __iomem *ioaddr = tp->mmio_addr;
+	int i;
+	dma_addr_t paddr;
+	u32 cmd;
+
+	ASSERT_RTNL();
+
+	counters = pci_alloc_consistent(tp->pci_dev, sizeof(*counters),
+					&paddr);
+	if (!counters)
+		return;
+	
+	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
+	cmd = (u64) paddr & DMA_32BIT_MASK;
+	RTL_W32(CounterAddrLow, cmd);
+	RTL_W32(CounterAddrLow, cmd | CounterDump);
+
+	for (i = 0; i < 1000; i++) {
+		if (!RTL_R32(CounterAddrLow) & CounterDump)
+			break;
+		udelay(10);
+	}
+	RTL_W32(CounterAddrLow, 0);
+	RTL_W32(CounterAddrHigh, 0);
+
+	data[0]	= le64_to_cpu(counters->tx_packets);
+	data[1] = le64_to_cpu(counters->rx_packets);
+	data[2] = le64_to_cpu(counters->tx_errors);
+	data[3] = le32_to_cpu(counters->rx_errors);
+	data[4] = le16_to_cpu(counters->rx_missed);
+	data[5] = le16_to_cpu(counters->align_errors);
+	data[6] = le32_to_cpu(counters->tx_one_collision);
+	data[7] = le32_to_cpu(counters->tx_multi_collision);
+	data[8] = le64_to_cpu(counters->rx_unicast);
+	data[9] = le64_to_cpu(counters->rx_broadcast);
+	data[10] = le32_to_cpu(counters->rx_multicast);
+	data[11] = le16_to_cpu(counters->tx_aborted);
+	data[12] = le16_to_cpu(counters->tx_underun);
+
+	pci_free_consistent(tp->pci_dev, sizeof(*counters), counters, paddr);
+}
+
+static void rtl8169_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	switch(stringset) {
+	case ETH_SS_STATS:
+		memcpy(data, *rtl8169_gstrings, sizeof(rtl8169_gstrings));
+		break;
+	}
+}
+
+
 static struct ethtool_ops rtl8169_ethtool_ops = {
 	.get_drvinfo		= rtl8169_get_drvinfo,
 	.get_regs_len		= rtl8169_get_regs_len,
@@ -914,6 +1013,9 @@
 	.get_tso		= ethtool_op_get_tso,
 	.set_tso		= ethtool_op_set_tso,
 	.get_regs		= rtl8169_get_regs,
+	.get_strings		= rtl8169_get_strings,
+	.get_stats_count	= rtl8169_get_stats_count,
+	.get_ethtool_stats	= rtl8169_get_ethtool_stats,
 };
 
 static void rtl8169_write_gmii_reg_bit(void __iomem *ioaddr, int reg, int bitnum,

Re: [PATCH 5/5] r8169: ethtool hardware statistics support

[Jeff Garzik]

Stephen Hemminger wrote:
> Add ethtool support for dumping the chip statistics. There aren't lots
> of statistics available, but this is what is available according to the RealTek
> documentation.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> 
> diff -Nru a/drivers/net/r8169.c b/drivers/net/r8169.c
> --- a/drivers/net/r8169.c	2005-03-09 11:26:04 -08:00
> +++ b/drivers/net/r8169.c	2005-03-09 11:26:04 -08:00
> @@ -195,6 +195,8 @@
>  enum RTL8169_registers {
>  	MAC0 = 0,		/* Ethernet hardware address. */
>  	MAR0 = 8,		/* Multicast filter. */
> +	CounterAddrLow = 0x10,
> +	CounterAddrHigh = 0x14,
>  	TxDescStartAddrLow = 0x20,
>  	TxDescStartAddrHigh = 0x24,
>  	TxHDescStartAddrLow = 0x28,
> @@ -336,6 +338,9 @@
>  
>  	/* _TBICSRBit */
>  	TBILinkOK = 0x02000000,
> +
> +	/* DumpCounterCommand */
> +	CounterDump = 0x8,
>  };
>  
>  enum _DescStatusBit {
> @@ -897,6 +902,100 @@
>  	tp->msg_enable = value;
>  }
>  
> +static const char rtl8169_gstrings[][ETH_GSTRING_LEN] = {
> +	"tx_packets",
> +	"rx_packets",
> +	"tx_errors",
> +	"rx_errors",
> +	"rx_missed",
> +	"align_errors",
> +	"tx_single_collisions",
> +	"tx_multi_collisions",
> +	"unicast",
> +	"broadcast",
> +	"multicast",
> +	"tx_aborted",
> +	"tx_underrun",
> +};
> +
> +struct rtl8169_counters {
> +	u64	tx_packets;
> +	u64	rx_packets;
> +	u64	tx_errors;
> +	u32	rx_errors;
> +	u16	rx_missed;
> +	u16	align_errors;
> +	u32	tx_one_collision;
> +	u32	tx_multi_collision;
> +	u64	rx_unicast;
> +	u64	rx_broadcast;
> +	u32	rx_multicast;
> +	u16	tx_aborted;
> +	u16	tx_underun;
> +};
> +
> +static int rtl8169_get_stats_count(struct net_device *dev)
> +{
> +	return 13;
> +}

maintenance:  use a constant, not a magic number.


> +static void rtl8169_get_ethtool_stats(struct net_device *dev,
> +				      struct ethtool_stats *stats, u64 *data)
> +{
> +        struct rtl8169_private *tp = netdev_priv(dev);

whitespace breakage


> +	struct rtl8169_counters *counters;
> +	void __iomem *ioaddr = tp->mmio_addr;
> +	int i;
> +	dma_addr_t paddr;
> +	u32 cmd;
> +
> +	ASSERT_RTNL();
> +
> +	counters = pci_alloc_consistent(tp->pci_dev, sizeof(*counters),
> +					&paddr);
> +	if (!counters)
> +		return;
> +	
> +	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
> +	cmd = (u64) paddr & DMA_32BIT_MASK;
> +	RTL_W32(CounterAddrLow, cmd);
> +	RTL_W32(CounterAddrLow, cmd | CounterDump);
> +
> +	for (i = 0; i < 1000; i++) {
> +		if (!RTL_R32(CounterAddrLow) & CounterDump)
> +			break;
> +		udelay(10);
> +	}
> +	RTL_W32(CounterAddrLow, 0);
> +	RTL_W32(CounterAddrHigh, 0);
> +
> +	data[0]	= le64_to_cpu(counters->tx_packets);
> +	data[1] = le64_to_cpu(counters->rx_packets);
> +	data[2] = le64_to_cpu(counters->tx_errors);
> +	data[3] = le32_to_cpu(counters->rx_errors);
> +	data[4] = le16_to_cpu(counters->rx_missed);
> +	data[5] = le16_to_cpu(counters->align_errors);
> +	data[6] = le32_to_cpu(counters->tx_one_collision);
> +	data[7] = le32_to_cpu(counters->tx_multi_collision);
> +	data[8] = le64_to_cpu(counters->rx_unicast);
> +	data[9] = le64_to_cpu(counters->rx_broadcast);
> +	data[10] = le32_to_cpu(counters->rx_multicast);
> +	data[11] = le16_to_cpu(counters->tx_aborted);
> +	data[12] = le16_to_cpu(counters->tx_underun);

use the "i++" indexing method found in other drivers.  The above code 
becomes an immediate PITA when you want to insert a new stat near the 
top (such as a software-generate stat).

Also, it's nice to add a BUG (or WARN_ON) check like the check found in 
other drivers.  That helps you ensure that the R8169_STATS_COUNT equals 
the number of entries you added to the data output.

	Jeff

Re: [PATCH 5/5] r8169: ethtool hardware statistics support

[Jon Mason]

Does this patch fix the problem of bogus statistics if the interface is down?

Re: [PATCH 5/5] r8169: ethtool hardware statistics support

[Stephen Hemminger]

On Wed, 9 Mar 2005 15:37:30 -0600
Jon Mason <jdmason@us.ibm.com> wrote:

> Does this patch fix the problem of bogus statistics if the interface is down
I see no problem when interface is down. It returns all zero's because
the device is reset on probe (and on shutdown).

Re: [PATCH 5/5] r8169: ethtool hardware statistics support

[Jon Mason]

On Wednesday 09 March 2005 03:53 pm, Stephen Hemminger wrote:
> On Wed, 9 Mar 2005 15:37:30 -0600
>
> Jon Mason <jdmason@us.ibm.com> wrote:
> > Does this patch fix the problem of bogus statistics if the interface is
> > down
>
> I see no problem when interface is down. It returns all zero's because
> the device is reset on probe (and on shutdown).

I can confirm that I see no statistic errors either. 

I believe the faulty code in Richard's patch was:
+       if (RTL_R32(StatsAddrLow) & DumpStats)
+               return; /* no stats - took too long */

Which returned before the stats were populated (leaving garbage).  Since your 
patch lacks this, we see no problem.  If this is true, there is probably a 
problem on 8139cp, which has a similar error path in its cp_get_ethtool_stats 
function.   

Thanks,
Jon

Re: [PATCH 5/5] r8169: ethtool hardware statistics support

[Stephen Hemminger]

On Wed, 9 Mar 2005 22:21:48 -0600
Jon Mason <jdmason@us.ibm.com> wrote:

> On Wednesday 09 March 2005 03:53 pm, Stephen Hemminger wrote:
> > On Wed, 9 Mar 2005 15:37:30 -0600
> >
> > Jon Mason <jdmason@us.ibm.com> wrote:
> > > Does this patch fix the problem of bogus statistics if the interface is
> > > down
> >
> > I see no problem when interface is down. It returns all zero's because
> > the device is reset on probe (and on shutdown).
> 
> I can confirm that I see no statistic errors either. 
> 
> I believe the faulty code in Richard's patch was:
> +       if (RTL_R32(StatsAddrLow) & DumpStats)
> +               return; /* no stats - took too long */
> 
> Which returned before the stats were populated (leaving garbage).  Since your 
> patch lacks this, we see no problem.  If this is true, there is probably a 
> problem on 8139cp, which has a similar error path in its cp_get_ethtool_stats 
> function.   

The problem with the 8139cp is that it allocates the area to hold
the statistics as part of the ring structure. The ring structure doesn't
exist until device is up.

I figured that there was no point in holding the extra space unless needed.
One more brief pci allocation was easier.  Also, my code waits longer
(up to 10ms) and is CPU speed independent.

Should I go back and fix the 8139cp?

Re: [PATCH 5/5] r8169: ethtool hardware statistics support

[Jeff Garzik]

Stephen Hemminger wrote:
> On Wed, 9 Mar 2005 22:21:48 -0600
> Jon Mason <jdmason@us.ibm.com> wrote:
> 
> 
>>On Wednesday 09 March 2005 03:53 pm, Stephen Hemminger wrote:
>>
>>>On Wed, 9 Mar 2005 15:37:30 -0600
>>>
>>>Jon Mason <jdmason@us.ibm.com> wrote:
>>>
>>>>Does this patch fix the problem of bogus statistics if the interface is
>>>>down
>>>
>>>I see no problem when interface is down. It returns all zero's because
>>>the device is reset on probe (and on shutdown).
>>
>>I can confirm that I see no statistic errors either. 
>>
>>I believe the faulty code in Richard's patch was:
>>+       if (RTL_R32(StatsAddrLow) & DumpStats)
>>+               return; /* no stats - took too long */
>>
>>Which returned before the stats were populated (leaving garbage).  Since your 
>>patch lacks this, we see no problem.  If this is true, there is probably a 
>>problem on 8139cp, which has a similar error path in its cp_get_ethtool_stats 
>>function.   
> 
> 
> The problem with the 8139cp is that it allocates the area to hold
> the statistics as part of the ring structure. The ring structure doesn't
> exist until device is up.
> 
> I figured that there was no point in holding the extra space unless needed.
> One more brief pci allocation was easier.  Also, my code waits longer
> (up to 10ms) and is CPU speed independent.
> 
> Should I go back and fix the 8139cp?

8139cp fixes are welcome :)

	Jeff

Re: [PATCH 5/5] r8169: ethtool hardware statistics support

[Richard Dawe]

Hello.

Stephen Hemminger wrote:
> Add ethtool support for dumping the chip statistics. There aren't lots
> of statistics available, but this is what is available according to the RealTek
> documentation.
[snip]

Works fine for me on my Athlon64 laptop with a 8169 with built-in PHY.

Did some rsync'ing and some broadcast pings. Statistics looked sane. 
Also tried stopping networking ("/etc/init.d/network stop") and checked 
that all the stats had been reset to zero.

Thanks, bye, Rich =]

-- 
Richard Dawe [ http://homepages.nildram.co.uk/~phekda/richdawe/ ]

"You can't evaluate a man by logic alone."
   -- McCoy, "I, Mudd", Star Trek