[PATCH]: r8169: Expose hardware stats via ethtool

[PATCH]: r8169: Expose hardware stats via ethtool

[Richard Dawe]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

Hi Francois and Jon!

Please find attached a patch that adds the hardware statistics ethtool 
operations to the r8169 driver. It's against 2.6.11-rc5.

Signed-Off-By: Richard Dawe <rich@phekda.gotadsl.co.uk>

Basically it's a port of the 8139cp stats routines to r8169. In 8139cp 
the stats buffer is in the ring buffers' DMA mapping. In this patch for 
r8169 it has its own DMA mapping.

One problem: Bogus stats are generated when I insert the module but 
don't bring it up. E.g.: if I do this on FC3 (eth0 == r8169):

   <--(Using 2.6.11-rc5's r8169 driver here)-->
   service network stop
   rmmod r8169
   insmod /path/to/new/r8169.ko
   ethtool -S eth0

I get this:

NIC statistics:
      tx_ok: 18446604436244066304
      rx_ok: 4096
      tx_err: 0
      rx_err: 0
      rx_fifo: 4
      frame_align: 1
      tx_ok_1col: 488917820
      tx_ok_mcol: 0
      rx_ok_phys: 18446604435732824074
      rx_ok_bcast: 18446744071565939505
      rx_ok_mcast: 0
      tx_abort: 18446604435732824064
      tx_underrun: 18446604436090647520

If I then bring the interface up ("ifconfig eth0 up"), I get valid stats.

Any suggestions on how to fix this? I tried a couple of things:

* Return in get_ethtool_stats if !netif_running(). Made no difference.

* Zero the stats after creating the DMA mapping with 
pci_alloc_consistent(). Made no difference.

I wonder if 8139cp has the same problem?

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

[-- Attachment #2: r8169-ethtool-stats.diff --]
[-- Type: text/plain, Size: 4982 bytes --]

--- linux-2.6.11-rc5/drivers/net/r8169.c.orig	2005-02-24 16:40:30.000000000 +0000
+++ linux-2.6.11-rc5/drivers/net/r8169.c	2005-02-26 14:28:37.000000000 +0000
@@ -128,6 +128,7 @@ static int multicast_filter_limit = 32;
 #define RX_BUF_SIZE	1536	/* Rx Buffer size */
 #define R8169_TX_RING_BYTES	(NUM_TX_DESC * sizeof(struct TxDesc))
 #define R8169_RX_RING_BYTES	(NUM_RX_DESC * sizeof(struct RxDesc))
+#define R8169_STATS_BYTES	64
 
 #define RTL8169_TX_TIMEOUT	(6*HZ)
 #define RTL8169_PHY_TIMEOUT	(10*HZ)
@@ -187,6 +188,8 @@ static int use_dac;
 enum RTL8169_registers {
 	MAC0 = 0,		/* Ethernet hardware address. */
 	MAR0 = 8,		/* Multicast filter. */
+	StatsAddrLow = 0x10,
+	StatsAddrHigh = 0x14,
 	TxDescStartAddrLow = 0x20,
 	TxDescStartAddrHigh = 0x24,
 	TxHDescStartAddrLow = 0x28,
@@ -255,6 +258,9 @@ enum RTL8169_register_content {
 	Cfg9346_Lock = 0x00,
 	Cfg9346_Unlock = 0xC0,
 
+	/* StatsAddr register */
+	DumpStats = (1 << 3),
+
 	/* rx_mode_bits */
 	AcceptErr = 0x20,
 	AcceptRunt = 0x10,
@@ -380,6 +386,22 @@ struct ring_info {
 	u8		__pad[sizeof(void *) - sizeof(u32)];
 };
 
+struct rtl8169_stats {
+	u64 tx_ok;
+	u64 rx_ok;
+	u64 tx_err;
+	u32 rx_err;
+	u16 rx_fifo;
+	u16 frame_align;
+	u32 tx_ok_1col;
+	u32 tx_ok_mcol;
+	u64 rx_ok_phys;
+	u64 rx_ok_bcast;
+	u32 rx_ok_mcast;
+	u16 tx_abort;
+	u16 tx_underrun;
+} __attribute__((packed));
+
 struct rtl8169_private {
 	void __iomem *mmio_addr;	/* memory map physical address */
 	struct pci_dev *pci_dev;	/* Index of PCI device */
@@ -404,6 +426,8 @@ struct rtl8169_private {
 	u16 intr_mask;
 	int phy_auto_nego_reg;
 	int phy_1000_ctrl_reg;
+	struct rtl8169_stats *nic_stats;
+	dma_addr_t nic_stats_addr;
 #ifdef CONFIG_R8169_VLAN
 	struct vlan_group *vlgrp;
 #endif
@@ -871,6 +895,68 @@ static void rtl8169_get_regs(struct net_
         spin_unlock_irqrestore(&tp->lock, flags);
 }
 
+static const char rtl8169_gstrings_stats[][ETH_GSTRING_LEN] = {
+	"tx_ok", "rx_ok", "tx_err", "rx_err",
+	"rx_fifo", "frame_align", "tx_ok_1col", "tx_ok_mcol",
+	"rx_ok_phys", "rx_ok_bcast", "rx_ok_mcast", "tx_abort",
+	"tx_underrun",
+};
+#define RTL8169_STATS_LEN sizeof(rtl8169_gstrings_stats) / ETH_GSTRING_LEN
+
+static int rtl8169_get_stats_count(struct net_device *dev)
+{
+	return RTL8169_STATS_LEN;
+}
+
+static void rtl8169_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
+{
+	switch(stringset) {
+	case ETH_SS_STATS:
+		memcpy(data, *rtl8169_gstrings_stats, sizeof(rtl8169_gstrings_stats));
+		break;
+	}
+}
+
+static void rtl8169_get_ethtool_stats(struct net_device *netdev,
+	struct ethtool_stats *stats, u64 *data)
+{
+	struct rtl8169_private *tp = netdev_priv(netdev);
+	void __iomem *ioaddr = tp->mmio_addr;
+	int work = 100;
+	int i;
+
+	/* begin NIC statistics dump */
+	RTL_W32(StatsAddrHigh, tp->nic_stats_addr >> 32);
+	RTL_W32(StatsAddrLow, (tp->nic_stats_addr & 0xffffffff) | DumpStats);
+	RTL_R32(StatsAddrLow);
+
+	while (work-- > 0) {
+		if ((RTL_R32(StatsAddrLow) & DumpStats) == 0)
+			break;
+		cpu_relax();
+	}
+
+	if (RTL_R32(StatsAddrLow) & DumpStats)
+		return; /* no stats - took too long */
+
+	i = 0;
+	data[i++] = le64_to_cpu(tp->nic_stats->tx_ok);
+	data[i++] = le64_to_cpu(tp->nic_stats->rx_ok);
+	data[i++] = le64_to_cpu(tp->nic_stats->tx_err);
+	data[i++] = le32_to_cpu(tp->nic_stats->rx_err);
+	data[i++] = le16_to_cpu(tp->nic_stats->rx_fifo);
+	data[i++] = le16_to_cpu(tp->nic_stats->frame_align);
+	data[i++] = le32_to_cpu(tp->nic_stats->tx_ok_1col);
+	data[i++] = le32_to_cpu(tp->nic_stats->tx_ok_mcol);
+	data[i++] = le64_to_cpu(tp->nic_stats->rx_ok_phys);
+	data[i++] = le64_to_cpu(tp->nic_stats->rx_ok_bcast);
+	data[i++] = le32_to_cpu(tp->nic_stats->rx_ok_mcast);
+	data[i++] = le16_to_cpu(tp->nic_stats->tx_abort);
+	data[i++] = le16_to_cpu(tp->nic_stats->tx_underrun);
+	if (i != RTL8169_STATS_LEN)
+		BUG();
+}
+
 static struct ethtool_ops rtl8169_ethtool_ops = {
 	.get_drvinfo		= rtl8169_get_drvinfo,
 	.get_regs_len		= rtl8169_get_regs_len,
@@ -886,6 +972,9 @@ static struct ethtool_ops rtl8169_ethtoo
 	.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,
@@ -1531,6 +1620,11 @@ static int rtl8169_open(struct net_devic
 	if (retval < 0)
 		goto err_free_rx;
 
+	tp->nic_stats = pci_alloc_consistent(pdev, R8169_STATS_BYTES,
+					     &tp->nic_stats_addr);
+	if (!tp->nic_stats)
+		goto err_free_nic_stats;
+
 	INIT_WORK(&tp->task, NULL, dev);
 
 	rtl8169_hw_start(dev);
@@ -1541,6 +1635,10 @@ static int rtl8169_open(struct net_devic
 out:
 	return retval;
 
+err_free_nic_stats:
+	pci_free_consistent(pdev, R8169_STATS_BYTES, tp->nic_stats,
+			    tp->nic_stats_addr);
+
 err_free_rx:
 	pci_free_consistent(pdev, R8169_RX_RING_BYTES, tp->RxDescArray,
 			    tp->RxPhyAddr);

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jeff Garzik]

Richard Dawe wrote:
> Hi Francois and Jon!
> 
> Please find attached a patch that adds the hardware statistics ethtool 
> operations to the r8169 driver. It's against 2.6.11-rc5.
> 
> Signed-Off-By: Richard Dawe <rich@phekda.gotadsl.co.uk>
> 
> Basically it's a port of the 8139cp stats routines to r8169. In 8139cp 
> the stats buffer is in the ring buffers' DMA mapping. In this patch for 
> r8169 it has its own DMA mapping.
> 
> One problem: Bogus stats are generated when I insert the module but 
> don't bring it up. E.g.: if I do this on FC3 (eth0 == r8169):
> 
>   <--(Using 2.6.11-rc5's r8169 driver here)-->
>   service network stop
>   rmmod r8169
>   insmod /path/to/new/r8169.ko
>   ethtool -S eth0
> 
> I get this:
> 
> NIC statistics:
>      tx_ok: 18446604436244066304
>      rx_ok: 4096
>      tx_err: 0
>      rx_err: 0
>      rx_fifo: 4
>      frame_align: 1
>      tx_ok_1col: 488917820
>      tx_ok_mcol: 0
>      rx_ok_phys: 18446604435732824074
>      rx_ok_bcast: 18446744071565939505
>      rx_ok_mcast: 0
>      tx_abort: 18446604435732824064
>      tx_underrun: 18446604436090647520
> 
> If I then bring the interface up ("ifconfig eth0 up"), I get valid stats.
> 
> Any suggestions on how to fix this? I tried a couple of things:
> 
> * Return in get_ethtool_stats if !netif_running(). Made no difference.
> 
> * Zero the stats after creating the DMA mapping with 
> pci_alloc_consistent(). Made no difference.
> 
> I wonder if 8139cp has the same problem?

No idea..  Worth checking.



> +static const char rtl8169_gstrings_stats[][ETH_GSTRING_LEN] = {
> +	"tx_ok", "rx_ok", "tx_err", "rx_err",
> +	"rx_fifo", "frame_align", "tx_ok_1col", "tx_ok_mcol",
> +	"rx_ok_phys", "rx_ok_bcast", "rx_ok_mcast", "tx_abort",
> +	"tx_underrun",
> +};

Don't needlessly reformat copied code.  It's one-string-per-line 
intentionally, for ease of maintenance and ease of adding new strings.

Also, I don't see why you renamed this from ethtool_stats_keys[].


> +	/* begin NIC statistics dump */
> +	RTL_W32(StatsAddrHigh, tp->nic_stats_addr >> 32);
> +	RTL_W32(StatsAddrLow, (tp->nic_stats_addr & 0xffffffff) | DumpStats);
> +	RTL_R32(StatsAddrLow);

This last RTL_R32() can be removed [from 8139cp too], because a flush 
immediately follows anyway:

> +	while (work-- > 0) {
> +		if ((RTL_R32(StatsAddrLow) & DumpStats) == 0)
> +			break;
> +		cpu_relax();
> +	}

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jon Mason]

On Saturday 26 February 2005 08:53 am, Richard Dawe wrote:
> Hi Francois and Jon!
> 
> Please find attached a patch that adds the hardware statistics ethtool 
> operations to the r8169 driver. It's against 2.6.11-rc5.

Good Work!  I'll give it a try here in a little bit.

[...]
> If I then bring the interface up ("ifconfig eth0 up"), I get valid stats.
> 
> Any suggestions on how to fix this? I tried a couple of things:
> 
> * Return in get_ethtool_stats if !netif_running(). Made no difference.
> 
> * Zero the stats after creating the DMA mapping with 
> pci_alloc_consistent(). Made no difference.

Can you confirm that the registers are outputting these bogus values?

See comments below. 

<paste from attachment>

--- linux-2.6.11-rc5/drivers/net/r8169.c.orig   2005-02-24 16:40:30.000000000 +0000
+++ linux-2.6.11-rc5/drivers/net/r8169.c        2005-02-26 14:28:37.000000000 +0000
@@ -128,6 +128,7 @@ static int multicast_filter_limit = 32;
 #define RX_BUF_SIZE    1536    /* Rx Buffer size */
 #define R8169_TX_RING_BYTES    (NUM_TX_DESC * sizeof(struct TxDesc))
 #define R8169_RX_RING_BYTES    (NUM_RX_DESC * sizeof(struct RxDesc))
+#define R8169_STATS_BYTES      64
 
 #define RTL8169_TX_TIMEOUT     (6*HZ)
 #define RTL8169_PHY_TIMEOUT    (10*HZ)
@@ -187,6 +188,8 @@ static int use_dac;
 enum RTL8169_registers {
        MAC0 = 0,               /* Ethernet hardware address. */
        MAR0 = 8,               /* Multicast filter. */
+       StatsAddrLow = 0x10,
+       StatsAddrHigh = 0x14,
        TxDescStartAddrLow = 0x20,
        TxDescStartAddrHigh = 0x24,
        TxHDescStartAddrLow = 0x28,
@@ -255,6 +258,9 @@ enum RTL8169_register_content {
        Cfg9346_Lock = 0x00,
        Cfg9346_Unlock = 0xC0,
 
+       /* StatsAddr register */
+       DumpStats = (1 << 3),
+

Wouldn't this be better as "0x08"?  Also, RTL8169_register_content could do with a bit of tidying (values are expressed in decimal and hex, some are aligned and others not, etc).  I'll try and come-up with a patch here in a bit.



        /* rx_mode_bits */
        AcceptErr = 0x20,
        AcceptRunt = 0x10,
@@ -380,6 +386,22 @@ struct ring_info {
        u8              __pad[sizeof(void *) - sizeof(u32)];
 };
 
+struct rtl8169_stats {
+       u64 tx_ok;
+       u64 rx_ok;
+       u64 tx_err;
+       u32 rx_err;
+       u16 rx_fifo;
+       u16 frame_align;
+       u32 tx_ok_1col;
+       u32 tx_ok_mcol;
+       u64 rx_ok_phys;
+       u64 rx_ok_bcast;
+       u32 rx_ok_mcast;
+       u16 tx_abort;
+       u16 tx_underrun;
+} __attribute__((packed));
+


These could all be u64's.  It would take-up more memory (and a bit more code in the register dump), but the values would be more accurate.  Just an idea.



 struct rtl8169_private {
        void __iomem *mmio_addr;        /* memory map physical address */
        struct pci_dev *pci_dev;        /* Index of PCI device */
@@ -404,6 +426,8 @@ struct rtl8169_private {
        u16 intr_mask;
        int phy_auto_nego_reg;
        int phy_1000_ctrl_reg;
+       struct rtl8169_stats *nic_stats;
+       dma_addr_t nic_stats_addr;
 #ifdef CONFIG_R8169_VLAN
        struct vlan_group *vlgrp;
 #endif
@@ -871,6 +895,68 @@ static void rtl8169_get_regs(struct net_
         spin_unlock_irqrestore(&tp->lock, flags);
 }
 
+static const char rtl8169_gstrings_stats[][ETH_GSTRING_LEN] = {
+       "tx_ok", "rx_ok", "tx_err", "rx_err",
+       "rx_fifo", "frame_align", "tx_ok_1col", "tx_ok_mcol",
+       "rx_ok_phys", "rx_ok_bcast", "rx_ok_mcast", "tx_abort",
+       "tx_underrun",
+};
+#define RTL8169_STATS_LEN sizeof(rtl8169_gstrings_stats) / ETH_GSTRING_LEN
+
+static int rtl8169_get_stats_count(struct net_device *dev)
+{
+       return RTL8169_STATS_LEN;
+}
+
+static void rtl8169_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
+{
+       switch(stringset) {
+       case ETH_SS_STATS:
+               memcpy(data, *rtl8169_gstrings_stats, sizeof(rtl8169_gstrings_stats));
+               break;
+       }
+}
+
+static void rtl8169_get_ethtool_stats(struct net_device *netdev,
+       struct ethtool_stats *stats, u64 *data)
+{
+       struct rtl8169_private *tp = netdev_priv(netdev);
+       void __iomem *ioaddr = tp->mmio_addr;
+       int work = 100;
+       int i;
+
+       /* begin NIC statistics dump */
+       RTL_W32(StatsAddrHigh, tp->nic_stats_addr >> 32);
+       RTL_W32(StatsAddrLow, (tp->nic_stats_addr & 0xffffffff) | DumpStats);
+       RTL_R32(StatsAddrLow);
+
+       while (work-- > 0) {
+               if ((RTL_R32(StatsAddrLow) & DumpStats) == 0)
+                       break;
+               cpu_relax();
+       }
+
+       if (RTL_R32(StatsAddrLow) & DumpStats)
+               return; /* no stats - took too long */
+
+       i = 0;
+       data[i++] = le64_to_cpu(tp->nic_stats->tx_ok);
+       data[i++] = le64_to_cpu(tp->nic_stats->rx_ok);
+       data[i++] = le64_to_cpu(tp->nic_stats->tx_err);
+       data[i++] = le32_to_cpu(tp->nic_stats->rx_err);
+       data[i++] = le16_to_cpu(tp->nic_stats->rx_fifo);
+       data[i++] = le16_to_cpu(tp->nic_stats->frame_align);
+       data[i++] = le32_to_cpu(tp->nic_stats->tx_ok_1col);
+       data[i++] = le32_to_cpu(tp->nic_stats->tx_ok_mcol);
+       data[i++] = le64_to_cpu(tp->nic_stats->rx_ok_phys);
+       data[i++] = le64_to_cpu(tp->nic_stats->rx_ok_bcast);
+       data[i++] = le32_to_cpu(tp->nic_stats->rx_ok_mcast);
+       data[i++] = le16_to_cpu(tp->nic_stats->tx_abort);
+       data[i++] = le16_to_cpu(tp->nic_stats->tx_underrun);
+       if (i != RTL8169_STATS_LEN)
+               BUG();
+}
+

It seems to me that 'i' could be re-used, instead of having both 'i' and 'work'.  Also,  'u32' or 'unsigned int' is prefered to int  (see http://www.kroah.com/linux/talks/portable_kernel_code_talk_2001_10_02/mgp00022.html).  Also, changes will need to be made if it is decided to use u64 (see above)



 static struct ethtool_ops rtl8169_ethtool_ops = {
        .get_drvinfo            = rtl8169_get_drvinfo,
        .get_regs_len           = rtl8169_get_regs_len,
@@ -886,6 +972,9 @@ static struct ethtool_ops rtl8169_ethtoo
        .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,
@@ -1531,6 +1620,11 @@ static int rtl8169_open(struct net_devic
        if (retval < 0)
                goto err_free_rx;
 
+       tp->nic_stats = pci_alloc_consistent(pdev, R8169_STATS_BYTES,
+                                            &tp->nic_stats_addr);
+       if (!tp->nic_stats)
+               goto err_free_nic_stats;
+
        INIT_WORK(&tp->task, NULL, dev);
 
        rtl8169_hw_start(dev);
@@ -1541,6 +1635,10 @@ static int rtl8169_open(struct net_devic
 out:
        return retval;
 
+err_free_nic_stats:
+       pci_free_consistent(pdev, R8169_STATS_BYTES, tp->nic_stats,
+                           tp->nic_stats_addr);
+
 err_free_rx:
        pci_free_consistent(pdev, R8169_RX_RING_BYTES, tp->RxDescArray,
                            tp->RxPhyAddr);

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jeff Garzik]

Jon Mason wrote:
> On Saturday 26 February 2005 08:53 am, Richard Dawe wrote:
> 
>>Hi Francois and Jon!
>>
>>Please find attached a patch that adds the hardware statistics ethtool 
>>operations to the r8169 driver. It's against 2.6.11-rc5.
> 
> 
> Good Work!  I'll give it a try here in a little bit.
> 
> [...]
> 
>>If I then bring the interface up ("ifconfig eth0 up"), I get valid stats.
>>
>>Any suggestions on how to fix this? I tried a couple of things:
>>
>>* Return in get_ethtool_stats if !netif_running(). Made no difference.
>>
>>* Zero the stats after creating the DMA mapping with 
>>pci_alloc_consistent(). Made no difference.
> 
> 
> Can you confirm that the registers are outputting these bogus values?
> 
> See comments below. 
> 
> <paste from attachment>
> 
> --- linux-2.6.11-rc5/drivers/net/r8169.c.orig   2005-02-24 16:40:30.000000000 +0000
> +++ linux-2.6.11-rc5/drivers/net/r8169.c        2005-02-26 14:28:37.000000000 +0000
> @@ -128,6 +128,7 @@ static int multicast_filter_limit = 32;
>  #define RX_BUF_SIZE    1536    /* Rx Buffer size */
>  #define R8169_TX_RING_BYTES    (NUM_TX_DESC * sizeof(struct TxDesc))
>  #define R8169_RX_RING_BYTES    (NUM_RX_DESC * sizeof(struct RxDesc))
> +#define R8169_STATS_BYTES      64
>  
>  #define RTL8169_TX_TIMEOUT     (6*HZ)
>  #define RTL8169_PHY_TIMEOUT    (10*HZ)
> @@ -187,6 +188,8 @@ static int use_dac;
>  enum RTL8169_registers {
>         MAC0 = 0,               /* Ethernet hardware address. */
>         MAR0 = 8,               /* Multicast filter. */
> +       StatsAddrLow = 0x10,
> +       StatsAddrHigh = 0x14,
>         TxDescStartAddrLow = 0x20,
>         TxDescStartAddrHigh = 0x24,
>         TxHDescStartAddrLow = 0x28,
> @@ -255,6 +258,9 @@ enum RTL8169_register_content {
>         Cfg9346_Lock = 0x00,
>         Cfg9346_Unlock = 0xC0,
>  
> +       /* StatsAddr register */
> +       DumpStats = (1 << 3),
> +
> 
> Wouldn't this be better as "0x08"?  Also, RTL8169_register_content could do with a bit of tidying (values are expressed in decimal and hex, some are aligned and others not, etc).  I'll try and come-up with a patch here in a bit.

The form "(1 << n)" is preferred, since that form makes plain the bit 
number, with zero neural transformation required.

Use the more readable form.  Cleanup patches accepted.


>         /* rx_mode_bits */
>         AcceptErr = 0x20,
>         AcceptRunt = 0x10,
> @@ -380,6 +386,22 @@ struct ring_info {
>         u8              __pad[sizeof(void *) - sizeof(u32)];
>  };
>  
> +struct rtl8169_stats {
> +       u64 tx_ok;
> +       u64 rx_ok;
> +       u64 tx_err;
> +       u32 rx_err;
> +       u16 rx_fifo;
> +       u16 frame_align;
> +       u32 tx_ok_1col;
> +       u32 tx_ok_mcol;
> +       u64 rx_ok_phys;
> +       u64 rx_ok_bcast;
> +       u32 rx_ok_mcast;
> +       u16 tx_abort;
> +       u16 tx_underrun;
> +} __attribute__((packed));
> +
> 
> 
> These could all be u64's.  It would take-up more memory (and a bit more code in the register dump), but the values would be more accurate.  Just an idea.

No, this is the representation of the hardware DMA structure.

It is defined by the hardware, not the programmer.


> +static void rtl8169_get_ethtool_stats(struct net_device *netdev,
> +       struct ethtool_stats *stats, u64 *data)
> +{
> +       struct rtl8169_private *tp = netdev_priv(netdev);
> +       void __iomem *ioaddr = tp->mmio_addr;
> +       int work = 100;
> +       int i;
> +
> +       /* begin NIC statistics dump */
> +       RTL_W32(StatsAddrHigh, tp->nic_stats_addr >> 32);
> +       RTL_W32(StatsAddrLow, (tp->nic_stats_addr & 0xffffffff) | DumpStats);
> +       RTL_R32(StatsAddrLow);
> +
> +       while (work-- > 0) {
> +               if ((RTL_R32(StatsAddrLow) & DumpStats) == 0)
> +                       break;
> +               cpu_relax();
> +       }
> +
> +       if (RTL_R32(StatsAddrLow) & DumpStats)
> +               return; /* no stats - took too long */
> +
> +       i = 0;
> +       data[i++] = le64_to_cpu(tp->nic_stats->tx_ok);
> +       data[i++] = le64_to_cpu(tp->nic_stats->rx_ok);
> +       data[i++] = le64_to_cpu(tp->nic_stats->tx_err);
> +       data[i++] = le32_to_cpu(tp->nic_stats->rx_err);
> +       data[i++] = le16_to_cpu(tp->nic_stats->rx_fifo);
> +       data[i++] = le16_to_cpu(tp->nic_stats->frame_align);
> +       data[i++] = le32_to_cpu(tp->nic_stats->tx_ok_1col);
> +       data[i++] = le32_to_cpu(tp->nic_stats->tx_ok_mcol);
> +       data[i++] = le64_to_cpu(tp->nic_stats->rx_ok_phys);
> +       data[i++] = le64_to_cpu(tp->nic_stats->rx_ok_bcast);
> +       data[i++] = le32_to_cpu(tp->nic_stats->rx_ok_mcast);
> +       data[i++] = le16_to_cpu(tp->nic_stats->tx_abort);
> +       data[i++] = le16_to_cpu(tp->nic_stats->tx_underrun);
> +       if (i != RTL8169_STATS_LEN)
> +               BUG();
> +}
> +
> 
> It seems to me that 'i' could be re-used, instead of having both 'i' and 'work'.

No.  That's a completely useless pseudo-optimization.  Write readable 
code, and let the compiler do the rest.

Any modern compiler will see where the live range of 'work' ends, and 
'i' begins.


   Also,  'u32' or 'unsigned int' is prefered to int  (see 
http://www.kroah.com/linux/talks/portable_kernel_code_talk_2001_10_02/mgp00022.html). 
  Also, changes will need to be made if it is decided to use u64 (see above)

True, but irrelevant in this case.  The code generated by the compiler 
is the same.

	Jeff

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jeff Garzik]

Also, please turn on word wrap in your mailer.

	Jeff

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Francois Romieu]

Jeff Garzik <jgarzik@pobox.com> :
[...]
> >+static void rtl8169_get_ethtool_stats(struct net_device *netdev,
> >+       struct ethtool_stats *stats, u64 *data)
> >+{
> >+       struct rtl8169_private *tp = netdev_priv(netdev);
> >+       void __iomem *ioaddr = tp->mmio_addr;
> >+       int work = 100;
> >+       int i;
> >+
> >+       /* begin NIC statistics dump */
> >+       RTL_W32(StatsAddrHigh, tp->nic_stats_addr >> 32);
> >+       RTL_W32(StatsAddrLow, (tp->nic_stats_addr & 0xffffffff) | 
> >DumpStats);
> >+       RTL_R32(StatsAddrLow);
> >+
> >+       while (work-- > 0) {
> >+               if ((RTL_R32(StatsAddrLow) & DumpStats) == 0)
> >+                       break;
> >+               cpu_relax();
> >+       }
> >+
> >+       if (RTL_R32(StatsAddrLow) & DumpStats)
> >+               return; /* no stats - took too long */
> >+
> >+       i = 0;
> >+       data[i++] = le64_to_cpu(tp->nic_stats->tx_ok);
[...]
> >+       data[i++] = le16_to_cpu(tp->nic_stats->tx_underrun);
> >+       if (i != RTL8169_STATS_LEN)
> >+               BUG();
> >+}
> >+
> >
> >It seems to me that 'i' could be re-used, instead of having both 'i' and 
> >'work'.
> 
> No.  That's a completely useless pseudo-optimization.  Write readable 
> code, and let the compiler do the rest.

Btw I'd simply remove the 'work' variable and schedule in an interruptible
way until the dump is done.

BUG() is a bit exagerated imho.

--
Ueimor

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Francois Romieu]

Richard Dawe <rich@phekda.gotadsl.co.uk> :
[...]
> @@ -1531,6 +1620,11 @@ static int rtl8169_open(struct net_devic
>  	if (retval < 0)
>  		goto err_free_rx;
>  
> +	tp->nic_stats = pci_alloc_consistent(pdev, R8169_STATS_BYTES,
> +					     &tp->nic_stats_addr);
> +	if (!tp->nic_stats)
> +		goto err_free_nic_stats;
> +
>  	INIT_WORK(&tp->task, NULL, dev);
>  
>  	rtl8169_hw_start(dev);
> @@ -1541,6 +1635,10 @@ static int rtl8169_open(struct net_devic
>  out:
>  	return retval;
>  
> +err_free_nic_stats:
> +	pci_free_consistent(pdev, R8169_STATS_BYTES, tp->nic_stats,
> +			    tp->nic_stats_addr);
> +

You don't want to free it it was not allocated. Please undo the previous
step (init_ring probably) and:
1) use the form "goto err_descriptive_name_for_the_release_work";
2) if you feel it does not protect against wrong ordering as:
   if (...)
 	goto err_foo;
   if (...)
	goto err_bar;
   [...]
   err_foo:
	...
   err_bar:
	...
   then add extra numbering:
   if (...)
	goto err_foo_0;
   if (...)
	goto err_bar_1;
   [...]
   err_bar_1:
	...
   err_foo_0:
	...
   It is not perfect but it is error proof (something the "goto err_1"
   way alone can not claim btw).

--
Ueimor

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jon Mason]

On Saturday 26 February 2005 12:02 pm, Jeff Garzik wrote:
[...]
> > @@ -255,6 +258,9 @@ enum RTL8169_register_content {
> >         Cfg9346_Lock = 0x00,
> >         Cfg9346_Unlock = 0xC0,
> >  
> > +       /* StatsAddr register */
> > +       DumpStats = (1 << 3),
> > +
> > 
> > Wouldn't this be better as "0x08"?  Also, RTL8169_register_content could 
> > do with a bit of tidying (values are expressed in decimal and hex, some 
> > are aligned and others not, etc).  I'll try and come-up with a patch here 
> > in a bit.   
> 
> The form "(1 << n)" is preferred, since that form makes plain the bit 
> number, with zero neural transformation required.
> 
> Use the more readable form.  Cleanup patches accepted.

My suggestion was based on code uniformity (as the rest of the values are 
defined as dex or decimal numbers).  Which takes presidense, uniformity or 
readablity?  If it is the latter, should the rest of thse values be redefined?

> 
> >         /* rx_mode_bits */
> >         AcceptErr = 0x20,
> >         AcceptRunt = 0x10,
> > @@ -380,6 +386,22 @@ struct ring_info {
> >         u8              __pad[sizeof(void *) - sizeof(u32)];
> >  };
> >  
> > +struct rtl8169_stats {
> > +       u64 tx_ok;
> > +       u64 rx_ok;
> > +       u64 tx_err;
> > +       u32 rx_err;
> > +       u16 rx_fifo;
> > +       u16 frame_align;
> > +       u32 tx_ok_1col;
> > +       u32 tx_ok_mcol;
> > +       u64 rx_ok_phys;
> > +       u64 rx_ok_bcast;
> > +       u32 rx_ok_mcast;
> > +       u16 tx_abort;
> > +       u16 tx_underrun;
> > +} __attribute__((packed));
> > +
> > 
> > 
> > These could all be u64's.  It would take-up more memory (and a bit more code in the register dump), but the values would be more accurate.  Just an idea.
> 
> No, this is the representation of the hardware DMA structure.
> 
> It is defined by the hardware, not the programmer.

Sorry, didn't see that on the first pass.  My apologies.
 
Jon

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jon Mason]

On Saturday 26 February 2005 08:53 am, Richard Dawe wrote:
> Hi Francois and Jon!
> 
> Please find attached a patch that adds the hardware statistics ethtool 
> operations to the r8169 driver. It's against 2.6.11-rc5.

I tested it on my amd64 system and it works great.  

I saw the same error if stats were gathered with the interface was down.  As 
a sanity check, I preformed the same test on e1000 and it does not have this 
error.  Not sure the significance of that.

Thanks Richard!

Jon

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Richard Dawe]

Hello.

Francois Romieu wrote:
> Richard Dawe <rich@phekda.gotadsl.co.uk> :
> [...]
> 
>>@@ -1531,6 +1620,11 @@ static int rtl8169_open(struct net_devic
>> 	if (retval < 0)
>> 		goto err_free_rx;
>> 
>>+	tp->nic_stats = pci_alloc_consistent(pdev, R8169_STATS_BYTES,
>>+					     &tp->nic_stats_addr);
>>+	if (!tp->nic_stats)
>>+		goto err_free_nic_stats;
>>+
>> 	INIT_WORK(&tp->task, NULL, dev);
>> 
>> 	rtl8169_hw_start(dev);
>>@@ -1541,6 +1635,10 @@ static int rtl8169_open(struct net_devic
>> out:
>> 	return retval;
>> 
>>+err_free_nic_stats:
>>+	pci_free_consistent(pdev, R8169_STATS_BYTES, tp->nic_stats,
>>+			    tp->nic_stats_addr);
>>+
> 
> 
> You don't want to free it it was not allocated. Please undo the previous
> step (init_ring probably) and:
[snip]

Oops, thanks for spotting that. I'll fix it for the next revision of the 
patch.

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

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Richard Dawe]

Hello.

Jeff Garzik wrote:
> Richard Dawe wrote:
[snip]
>> +static const char rtl8169_gstrings_stats[][ETH_GSTRING_LEN] = {
>> +    "tx_ok", "rx_ok", "tx_err", "rx_err",
>> +    "rx_fifo", "frame_align", "tx_ok_1col", "tx_ok_mcol",
>> +    "rx_ok_phys", "rx_ok_bcast", "rx_ok_mcast", "tx_abort",
>> +    "tx_underrun",
>> +};
> 
> 
> Don't needlessly reformat copied code.  It's one-string-per-line 
> intentionally, for ease of maintenance and ease of adding new strings.

OK, I'll fix that.

> Also, I don't see why you renamed this from ethtool_stats_keys[].

I didn't copy it directly. I started off with something that looked like 
the ethtool stats code from the e100 driver. Then I noticed that 8139cp 
did the right thing for r8169.

I'll rename it.

>> +    /* begin NIC statistics dump */
>> +    RTL_W32(StatsAddrHigh, tp->nic_stats_addr >> 32);
>> +    RTL_W32(StatsAddrLow, (tp->nic_stats_addr & 0xffffffff) | 
>> DumpStats);
>> +    RTL_R32(StatsAddrLow);
> 
> 
> This last RTL_R32() can be removed [from 8139cp too], because a flush 
> immediately follows anyway:
[snip]

OK, will do.

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

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Richard Dawe]

Hello.

Thanks for reviewing, Francois, Jon & Jeff!

Francois Romieu wrote:
[snip]
> Btw I'd simply remove the 'work' variable and schedule in an interruptible
> way until the dump is done.

OK, that will take me a bit longer to code. ;)

> BUG() is a bit exagerated imho.

It seems like a pretty good way of avoiding a buffer overrun to me. 
E.g.: you copy an extra statistic in rtl8169_get_ethtool_stats(), but 
forget to update the stats length. Is it not better to crash early, than 
encounter random behaviour later?

I can put an #ifdef RTL8169_DEBUG / #endif around it, if you'd be happier.

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

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jeff Garzik]

Richard Dawe wrote:
>> BUG() is a bit exagerated imho.
> 
> 
> It seems like a pretty good way of avoiding a buffer overrun to me. 
> E.g.: you copy an extra statistic in rtl8169_get_ethtool_stats(), but 
> forget to update the stats length. Is it not better to crash early, than 
> encounter random behaviour later?


Yeah, that's why the BUG() is present in 8139cp:  force an oops rather 
than continue corrupting memory, if the programmer made an error.

	Jeff

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jon Mason]

I think I've found a (very hackish) way around the bad stats error.  

Tested on amd64, and "solves" the problem.

--- drivers/net/r8169.c 2005-02-27 20:27:48.000000000 -0600
+++ drivers/net/r8169.c.new     2005-02-27 20:29:29.000000000 -0600
@@ -929,8 +929,13 @@ static void rtl8169_get_ethtool_stats(st
                cpu_relax();
        }

-       if (RTL_R32(StatsAddrLow) & DumpStats)
+       if (RTL_R32(StatsAddrLow) & DumpStats) {
+               if (!netif_running(netdev)) {
+                       for (i = 0; i < 14; i++)
+                               data[i] = 0;
+               }
                return; /* no stats - took too long */
+       }

        i = 0;
        data[i++] = le64_to_cpu(tp->nic_stats->tx_ok);

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Jeff Garzik]

Jon Mason wrote:
> I think I've found a (very hackish) way around the bad stats error.  
> 
> Tested on amd64, and "solves" the problem.

Seems to me, we should instead find a way to avoid calling the stats 
function if !netif_running()

	Jeff

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Ben Greear]

Jeff Garzik wrote:
> Jon Mason wrote:
> 
>> I think I've found a (very hackish) way around the bad stats error. 
>> Tested on amd64, and "solves" the problem.
> 
> 
> Seems to me, we should instead find a way to avoid calling the stats 
> function if !netif_running()

Are the stats valid in the hardware if you start it, stop it, and then read them?

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Richard Dawe]

Hello.

Ben Greear wrote:
> Jeff Garzik wrote:
> 
>> Jon Mason wrote:
>>
>>> I think I've found a (very hackish) way around the bad stats error. 
>>> Tested on amd64, and "solves" the problem.
>>
>>
>>
>> Seems to me, we should instead find a way to avoid calling the stats 
>> function if !netif_running()
> 
> 
> Are the stats valid in the hardware if you start it, stop it, and then 
> read them?

They don't seem to be. I get the same kind of garbage before starting as 
  I do after I've started and stopped it.

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