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

[Jon Mason <jdmason@us.ibm.com>]

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);