[PATCH v2 0/5] alx: add statistics

[PATCH v2 0/5] alx: add statistics

[Sabrina Dubroca]

Currently, the alx driver doesn't support statistics [1,2]. The
original alx driver [3] that Johannes Berg modified provided
statistics. This patch is an adaptation of the statistics code from
the original driver to the alx driver included in the kernel.


v2:
 - use u64 instead of unsigned long  (Ben Hutchings)
 - implement ndo_get_stats64 instead of ndo_get_stats (Ben Hutchings)
 - use EINVAL instead of ENOTSUPP  (Ben Hutchings)
 - add BUILD_BUG_ON to check the size of the stats (Johannes Berg, Ben
   Hutchings)
 - add a comment regarding persistence of the stats (Stephen Hemminger)
 - align assignments in __alx_update_hw_stats


The original driver has a different version of the
__alx_update_hw_stats function (patch 3). I rewrote it because I
thought the code was not as clear, and it could be cause bugs if the
stats structure was modified. Here is the original version:

void __alx_update_hw_stats(struct alx_hw *hw)
{
	u16 reg;
	u32 data;
	unsigned long *p;

	/* RX stats */
	reg = ALX_RX_STATS_BIN;
	p = &hw->stats.rx_ok;
	while (reg <=  ALX_RX_STATS_END) {
		data = alx_read_mem32(hw, reg);
		*p++ += data;
		reg += 4;
	}

	/* TX stats */
	reg = ALX_TX_STATS_BIN;
	p = &hw->stats.tx_ok;
	while (reg <= ALX_TX_STATS_END) {
		data = alx_read_mem32(hw, reg);
		*p++ += data;
		reg += 4;
	}
}

If you prefer this version, I can resend the patches with this code
instead of the one in patch 3.
Patch 2 removes the constants used in the original version of the
update function and adds the ones necessary for patch 3, so it shouldn't
be used with the original version (above).


[1] https://bugzilla.kernel.org/show_bug.cgi?id=63401
[2] http://www.spinics.net/lists/netdev/msg245544.html
[3] https://github.com/mcgrof/alx


Sabrina Dubroca (5):
  alx: add a hardware stats structure
  alx: add constants for the stats fields
  alx: add stats update function
  alx: add alx_get_stats64 operation
  alx: add stats to ethtool

 drivers/net/ethernet/atheros/alx/alx.h     |   3 +
 drivers/net/ethernet/atheros/alx/ethtool.c | 101 +++++++++++++++++++++++++++++
 drivers/net/ethernet/atheros/alx/hw.c      |  58 +++++++++++++++++
 drivers/net/ethernet/atheros/alx/hw.h      |  66 +++++++++++++++++++
 drivers/net/ethernet/atheros/alx/main.c    |  44 +++++++++++++
 drivers/net/ethernet/atheros/alx/reg.h     |  52 +++++++++++++--
 6 files changed, 320 insertions(+), 4 deletions(-)

-- 
1.8.5.2

[PATCH v2 1/5] alx: add a hardware stats structure

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/ethernet/atheros/alx/hw.h | 58 +++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
index 96f3b43..1c1a0ee 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -381,6 +381,64 @@ struct alx_rrd {
 				 ALX_ISR_RX_Q6 | \
 				 ALX_ISR_RX_Q7)
 
+/* Statistics counters collected by the MAC */
+struct alx_hw_stats {
+	/* rx */
+	u64 rx_ok;
+	u64 rx_bcast;
+	u64 rx_mcast;
+	u64 rx_pause;
+	u64 rx_ctrl;
+	u64 rx_fcs_err;
+	u64 rx_len_err;
+	u64 rx_byte_cnt;
+	u64 rx_runt;
+	u64 rx_frag;
+	u64 rx_sz_64B;
+	u64 rx_sz_127B;
+	u64 rx_sz_255B;
+	u64 rx_sz_511B;
+	u64 rx_sz_1023B;
+	u64 rx_sz_1518B;
+	u64 rx_sz_max;
+	u64 rx_ov_sz;
+	u64 rx_ov_rxf;
+	u64 rx_ov_rrd;
+	u64 rx_align_err;
+	u64 rx_bc_byte_cnt;
+	u64 rx_mc_byte_cnt;
+	u64 rx_err_addr;
+
+	/* tx */
+	u64 tx_ok;
+	u64 tx_bcast;
+	u64 tx_mcast;
+	u64 tx_pause;
+	u64 tx_exc_defer;
+	u64 tx_ctrl;
+	u64 tx_defer;
+	u64 tx_byte_cnt;
+	u64 tx_sz_64B;
+	u64 tx_sz_127B;
+	u64 tx_sz_255B;
+	u64 tx_sz_511B;
+	u64 tx_sz_1023B;
+	u64 tx_sz_1518B;
+	u64 tx_sz_max;
+	u64 tx_single_col;
+	u64 tx_multi_col;
+	u64 tx_late_col;
+	u64 tx_abort_col;
+	u64 tx_underrun;
+	u64 tx_trd_eop;
+	u64 tx_len_err;
+	u64 tx_trunc;
+	u64 tx_bc_byte_cnt;
+	u64 tx_mc_byte_cnt;
+	u64 update;
+};
+
+
 /* maximum interrupt vectors for msix */
 #define ALX_MAX_MSIX_INTRS	16
 
-- 
1.8.5.2

[PATCH v2 2/5] alx: add constants for the stats fields

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/ethernet/atheros/alx/reg.h | 52 +++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/atheros/alx/reg.h b/drivers/net/ethernet/atheros/alx/reg.h
index e4358c9..af006b4 100644
--- a/drivers/net/ethernet/atheros/alx/reg.h
+++ b/drivers/net/ethernet/atheros/alx/reg.h
@@ -404,15 +404,59 @@
 
 /* MIB */
 #define ALX_MIB_BASE					0x1700
+
 #define ALX_MIB_RX_OK					(ALX_MIB_BASE + 0)
+#define ALX_MIB_RX_BCAST				(ALX_MIB_BASE + 4)
+#define ALX_MIB_RX_MCAST				(ALX_MIB_BASE + 8)
+#define ALX_MIB_RX_PAUSE				(ALX_MIB_BASE + 12)
+#define ALX_MIB_RX_CTRL					(ALX_MIB_BASE + 16)
+#define ALX_MIB_RX_FCS_ERR				(ALX_MIB_BASE + 20)
+#define ALX_MIB_RX_LEN_ERR				(ALX_MIB_BASE + 24)
+#define ALX_MIB_RX_BYTE_CNT				(ALX_MIB_BASE + 28)
+#define ALX_MIB_RX_RUNT					(ALX_MIB_BASE + 32)
+#define ALX_MIB_RX_FRAG					(ALX_MIB_BASE + 36)
+#define ALX_MIB_RX_SZ_64B				(ALX_MIB_BASE + 40)
+#define ALX_MIB_RX_SZ_127B				(ALX_MIB_BASE + 44)
+#define ALX_MIB_RX_SZ_255B				(ALX_MIB_BASE + 48)
+#define ALX_MIB_RX_SZ_511B				(ALX_MIB_BASE + 52)
+#define ALX_MIB_RX_SZ_1023B				(ALX_MIB_BASE + 56)
+#define ALX_MIB_RX_SZ_1518B				(ALX_MIB_BASE + 60)
+#define ALX_MIB_RX_SZ_MAX				(ALX_MIB_BASE + 64)
+#define ALX_MIB_RX_OV_SZ				(ALX_MIB_BASE + 68)
+#define ALX_MIB_RX_OV_RXF				(ALX_MIB_BASE + 72)
+#define ALX_MIB_RX_OV_RRD				(ALX_MIB_BASE + 76)
+#define ALX_MIB_RX_ALIGN_ERR				(ALX_MIB_BASE + 80)
+#define ALX_MIB_RX_BCCNT				(ALX_MIB_BASE + 84)
+#define ALX_MIB_RX_MCCNT				(ALX_MIB_BASE + 88)
 #define ALX_MIB_RX_ERRADDR				(ALX_MIB_BASE + 92)
+
 #define ALX_MIB_TX_OK					(ALX_MIB_BASE + 96)
+#define ALX_MIB_TX_BCAST				(ALX_MIB_BASE + 100)
+#define ALX_MIB_TX_MCAST				(ALX_MIB_BASE + 104)
+#define ALX_MIB_TX_PAUSE				(ALX_MIB_BASE + 108)
+#define ALX_MIB_TX_EXC_DEFER				(ALX_MIB_BASE + 112)
+#define ALX_MIB_TX_CTRL					(ALX_MIB_BASE + 116)
+#define ALX_MIB_TX_DEFER				(ALX_MIB_BASE + 120)
+#define ALX_MIB_TX_BYTE_CNT				(ALX_MIB_BASE + 124)
+#define ALX_MIB_TX_SZ_64B				(ALX_MIB_BASE + 128)
+#define ALX_MIB_TX_SZ_127B				(ALX_MIB_BASE + 132)
+#define ALX_MIB_TX_SZ_255B				(ALX_MIB_BASE + 136)
+#define ALX_MIB_TX_SZ_511B				(ALX_MIB_BASE + 140)
+#define ALX_MIB_TX_SZ_1023B				(ALX_MIB_BASE + 144)
+#define ALX_MIB_TX_SZ_1518B				(ALX_MIB_BASE + 148)
+#define ALX_MIB_TX_SZ_MAX				(ALX_MIB_BASE + 152)
+#define ALX_MIB_TX_SINGLE_COL				(ALX_MIB_BASE + 156)
+#define ALX_MIB_TX_MULTI_COL				(ALX_MIB_BASE + 160)
+#define ALX_MIB_TX_LATE_COL				(ALX_MIB_BASE + 164)
+#define ALX_MIB_TX_ABORT_COL				(ALX_MIB_BASE + 168)
+#define ALX_MIB_TX_UNDERRUN				(ALX_MIB_BASE + 172)
+#define ALX_MIB_TX_TRD_EOP				(ALX_MIB_BASE + 176)
+#define ALX_MIB_TX_LEN_ERR				(ALX_MIB_BASE + 180)
+#define ALX_MIB_TX_TRUNC				(ALX_MIB_BASE + 184)
+#define ALX_MIB_TX_BCCNT				(ALX_MIB_BASE + 188)
 #define ALX_MIB_TX_MCCNT				(ALX_MIB_BASE + 192)
+#define ALX_MIB_UPDATE					(ALX_MIB_BASE + 196)
 
-#define ALX_RX_STATS_BIN				ALX_MIB_RX_OK
-#define ALX_RX_STATS_END				ALX_MIB_RX_ERRADDR
-#define ALX_TX_STATS_BIN				ALX_MIB_TX_OK
-#define ALX_TX_STATS_END				ALX_MIB_TX_MCCNT
 
 #define ALX_ISR						0x1600
 #define ALX_ISR_DIS					BIT(31)
-- 
1.8.5.2

[PATCH v2 3/5] alx: add stats update function

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/ethernet/atheros/alx/hw.c | 58 +++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/atheros/alx/hw.h |  4 +++
 2 files changed, 62 insertions(+)

diff --git a/drivers/net/ethernet/atheros/alx/hw.c b/drivers/net/ethernet/atheros/alx/hw.c
index 1e8c24a..7d4fef7 100644
--- a/drivers/net/ethernet/atheros/alx/hw.c
+++ b/drivers/net/ethernet/atheros/alx/hw.c
@@ -1050,3 +1050,61 @@ bool alx_get_phy_info(struct alx_hw *hw)
 
 	return true;
 }
+
+void __alx_update_hw_stats(struct alx_hw *hw)
+{
+	/* RX stats */
+	hw->stats.rx_ok          += alx_read_mem32(hw, ALX_MIB_RX_OK);
+	hw->stats.rx_bcast       += alx_read_mem32(hw, ALX_MIB_RX_BCAST);
+	hw->stats.rx_mcast       += alx_read_mem32(hw, ALX_MIB_RX_MCAST);
+	hw->stats.rx_pause       += alx_read_mem32(hw, ALX_MIB_RX_PAUSE);
+	hw->stats.rx_ctrl        += alx_read_mem32(hw, ALX_MIB_RX_CTRL);
+	hw->stats.rx_fcs_err     += alx_read_mem32(hw, ALX_MIB_RX_FCS_ERR);
+	hw->stats.rx_len_err     += alx_read_mem32(hw, ALX_MIB_RX_LEN_ERR);
+	hw->stats.rx_byte_cnt    += alx_read_mem32(hw, ALX_MIB_RX_BYTE_CNT);
+	hw->stats.rx_runt        += alx_read_mem32(hw, ALX_MIB_RX_RUNT);
+	hw->stats.rx_frag        += alx_read_mem32(hw, ALX_MIB_RX_FRAG);
+	hw->stats.rx_sz_64B      += alx_read_mem32(hw, ALX_MIB_RX_SZ_64B);
+	hw->stats.rx_sz_127B     += alx_read_mem32(hw, ALX_MIB_RX_SZ_127B);
+	hw->stats.rx_sz_255B     += alx_read_mem32(hw, ALX_MIB_RX_SZ_255B);
+	hw->stats.rx_sz_511B     += alx_read_mem32(hw, ALX_MIB_RX_SZ_511B);
+	hw->stats.rx_sz_1023B    += alx_read_mem32(hw, ALX_MIB_RX_SZ_1023B);
+	hw->stats.rx_sz_1518B    += alx_read_mem32(hw, ALX_MIB_RX_SZ_1518B);
+	hw->stats.rx_sz_max      += alx_read_mem32(hw, ALX_MIB_RX_SZ_MAX);
+	hw->stats.rx_ov_sz       += alx_read_mem32(hw, ALX_MIB_RX_OV_SZ);
+	hw->stats.rx_ov_rxf      += alx_read_mem32(hw, ALX_MIB_RX_OV_RXF);
+	hw->stats.rx_ov_rrd      += alx_read_mem32(hw, ALX_MIB_RX_OV_RRD);
+	hw->stats.rx_align_err   += alx_read_mem32(hw, ALX_MIB_RX_ALIGN_ERR);
+	hw->stats.rx_bc_byte_cnt += alx_read_mem32(hw, ALX_MIB_RX_BCCNT);
+	hw->stats.rx_mc_byte_cnt += alx_read_mem32(hw, ALX_MIB_RX_MCCNT);
+	hw->stats.rx_err_addr    += alx_read_mem32(hw, ALX_MIB_RX_ERRADDR);
+
+	/* TX stats */
+	hw->stats.tx_ok          += alx_read_mem32(hw, ALX_MIB_TX_OK);
+	hw->stats.tx_bcast       += alx_read_mem32(hw, ALX_MIB_TX_BCAST);
+	hw->stats.tx_mcast       += alx_read_mem32(hw, ALX_MIB_TX_MCAST);
+	hw->stats.tx_pause       += alx_read_mem32(hw, ALX_MIB_TX_PAUSE);
+	hw->stats.tx_exc_defer   += alx_read_mem32(hw, ALX_MIB_TX_EXC_DEFER);
+	hw->stats.tx_ctrl        += alx_read_mem32(hw, ALX_MIB_TX_CTRL);
+	hw->stats.tx_defer       += alx_read_mem32(hw, ALX_MIB_TX_DEFER);
+	hw->stats.tx_byte_cnt    += alx_read_mem32(hw, ALX_MIB_TX_BYTE_CNT);
+	hw->stats.tx_sz_64B      += alx_read_mem32(hw, ALX_MIB_TX_SZ_64B);
+	hw->stats.tx_sz_127B     += alx_read_mem32(hw, ALX_MIB_TX_SZ_127B);
+	hw->stats.tx_sz_255B     += alx_read_mem32(hw, ALX_MIB_TX_SZ_255B);
+	hw->stats.tx_sz_511B     += alx_read_mem32(hw, ALX_MIB_TX_SZ_511B);
+	hw->stats.tx_sz_1023B    += alx_read_mem32(hw, ALX_MIB_TX_SZ_1023B);
+	hw->stats.tx_sz_1518B    += alx_read_mem32(hw, ALX_MIB_TX_SZ_1518B);
+	hw->stats.tx_sz_max      += alx_read_mem32(hw, ALX_MIB_TX_SZ_MAX);
+	hw->stats.tx_single_col  += alx_read_mem32(hw, ALX_MIB_TX_SINGLE_COL);
+	hw->stats.tx_multi_col   += alx_read_mem32(hw, ALX_MIB_TX_MULTI_COL);
+	hw->stats.tx_late_col    += alx_read_mem32(hw, ALX_MIB_TX_LATE_COL);
+	hw->stats.tx_abort_col   += alx_read_mem32(hw, ALX_MIB_TX_ABORT_COL);
+	hw->stats.tx_underrun    += alx_read_mem32(hw, ALX_MIB_TX_UNDERRUN);
+	hw->stats.tx_trd_eop     += alx_read_mem32(hw, ALX_MIB_TX_TRD_EOP);
+	hw->stats.tx_len_err     += alx_read_mem32(hw, ALX_MIB_TX_LEN_ERR);
+	hw->stats.tx_trunc       += alx_read_mem32(hw, ALX_MIB_TX_TRUNC);
+	hw->stats.tx_bc_byte_cnt += alx_read_mem32(hw, ALX_MIB_TX_BCCNT);
+	hw->stats.tx_mc_byte_cnt += alx_read_mem32(hw, ALX_MIB_TX_MCCNT);
+
+	hw->stats.update         += alx_read_mem32(hw, ALX_MIB_UPDATE);
+}
diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
index 1c1a0ee..3780773 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -482,6 +482,9 @@ struct alx_hw {
 
 	/* PHY link patch flag */
 	bool lnk_patch;
+
+	/* cumulated stats from the hardware (registers are cleared on read) */
+	struct alx_hw_stats stats;
 };
 
 static inline int alx_hw_revision(struct alx_hw *hw)
@@ -549,6 +552,7 @@ bool alx_phy_configured(struct alx_hw *hw);
 void alx_configure_basic(struct alx_hw *hw);
 void alx_disable_rss(struct alx_hw *hw);
 bool alx_get_phy_info(struct alx_hw *hw);
+void __alx_update_hw_stats(struct alx_hw *hw);
 
 static inline u32 alx_speed_to_ethadv(int speed, u8 duplex)
 {
-- 
1.8.5.2

Re: [PATCH v2 3/5] alx: add stats update function

[Stephen Hemminger]

On Sat,  4 Jan 2014 17:47:09 +0100
Sabrina Dubroca <sd@queasysnail.net> wrote:

>  }
> +
> +void __alx_update_hw_stats(struct alx_hw *hw)

Minor NIT:
Why __ prefix? that is usually reserved for functions only
called from one file (or when wrapping an existing API)

Re: [PATCH v2 3/5] alx: add stats update function

[Sabrina Dubroca]

[2014-01-04] Stephen Hemminger wrote:
> On Sat,  4 Jan 2014 17:47:09 +0100
> Sabrina Dubroca <sd@queasysnail.net> wrote:
> 
> >  }
> > +
> > +void __alx_update_hw_stats(struct alx_hw *hw)
> 
> Minor NIT:
> Why __ prefix? that is usually reserved for functions only
> called from one file (or when wrapping an existing API)

In the original code, that's the name it had, so I just kept it. There
were two additional wrappers, but in Johannes's rewrite of the driver,
they became unnecessary.
I'll rename it to alx_update_hw_stats in v3.


I didn't know about that convention. I can't find a reference in
Documentation/CodingStyle, or from a quick look at a grep in the rest
of the files there: maybe it should be added to the documentation?


Thanks,

-- 
Sabrina

[PATCH v2 4/5] alx: add alx_get_stats64 operation

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/ethernet/atheros/alx/alx.h  |  3 +++
 drivers/net/ethernet/atheros/alx/main.c | 44 +++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/net/ethernet/atheros/alx/alx.h b/drivers/net/ethernet/atheros/alx/alx.h
index d71103d..8fc93c5 100644
--- a/drivers/net/ethernet/atheros/alx/alx.h
+++ b/drivers/net/ethernet/atheros/alx/alx.h
@@ -106,6 +106,9 @@ struct alx_priv {
 	u16 msg_enable;
 
 	bool msi;
+
+	/* protects hw.stats */
+	spinlock_t stats_lock;
 };
 
 extern const struct ethtool_ops alx_ethtool_ops;
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index c3c4c26..a191be7 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -1166,10 +1166,54 @@ static void alx_poll_controller(struct net_device *netdev)
 }
 #endif
 
+static struct rtnl_link_stats64 *alx_get_stats64(struct net_device *dev,
+					struct rtnl_link_stats64 *net_stats)
+{
+	struct alx_priv *alx = netdev_priv(dev);
+	struct alx_hw_stats *hw_stats = &alx->hw.stats;
+
+	spin_lock(&alx->stats_lock);
+
+	__alx_update_hw_stats(&alx->hw);
+
+	net_stats->tx_packets = hw_stats->tx_ok;
+	net_stats->tx_bytes   = hw_stats->tx_byte_cnt;
+	net_stats->rx_packets = hw_stats->rx_ok;
+	net_stats->rx_bytes   = hw_stats->rx_byte_cnt;
+	net_stats->multicast  = hw_stats->rx_mcast;
+	net_stats->collisions = hw_stats->tx_single_col +
+				hw_stats->tx_multi_col * 2 +
+				hw_stats->tx_late_col + hw_stats->tx_abort_col;
+
+	net_stats->rx_errors  = hw_stats->rx_frag + hw_stats->rx_fcs_err +
+				hw_stats->rx_len_err + hw_stats->rx_ov_sz +
+				hw_stats->rx_ov_rrd + hw_stats->rx_align_err;
+
+	net_stats->rx_fifo_errors   = hw_stats->rx_ov_rxf;
+	net_stats->rx_length_errors = hw_stats->rx_len_err;
+	net_stats->rx_crc_errors    = hw_stats->rx_fcs_err;
+	net_stats->rx_frame_errors  = hw_stats->rx_align_err;
+	net_stats->rx_over_errors   = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf;
+
+	net_stats->rx_missed_errors = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf;
+
+	net_stats->tx_errors = hw_stats->tx_late_col + hw_stats->tx_abort_col +
+			       hw_stats->tx_underrun + hw_stats->tx_trunc;
+
+	net_stats->tx_aborted_errors = hw_stats->tx_abort_col;
+	net_stats->tx_fifo_errors    = hw_stats->tx_underrun;
+	net_stats->tx_window_errors  = hw_stats->tx_late_col;
+
+	spin_unlock(&alx->stats_lock);
+
+	return net_stats;
+}
+
 static const struct net_device_ops alx_netdev_ops = {
 	.ndo_open               = alx_open,
 	.ndo_stop               = alx_stop,
 	.ndo_start_xmit         = alx_start_xmit,
+	.ndo_get_stats64        = alx_get_stats64,
 	.ndo_set_rx_mode        = alx_set_rx_mode,
 	.ndo_validate_addr      = eth_validate_addr,
 	.ndo_set_mac_address    = alx_set_mac_address,
-- 
1.8.5.2

[PATCH v2 5/5] alx: add stats to ethtool

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/ethernet/atheros/alx/ethtool.c | 101 +++++++++++++++++++++++++++++
 drivers/net/ethernet/atheros/alx/hw.h      |   6 +-
 2 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 45b3650..62aea08 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -46,6 +46,66 @@
 #include "reg.h"
 #include "hw.h"
 
+/* The order of these strings must match the order of the fields in
+ * struct alx_hw_stats
+ * See hw.h
+ */
+static const char alx_gstrings_stats[][ETH_GSTRING_LEN] = {
+	"rx_packets",
+	"rx_bcast_packets",
+	"rx_mcast_packets",
+	"rx_pause_packets",
+	"rx_ctrl_packets",
+	"rx_fcs_errors",
+	"rx_length_errors",
+	"rx_bytes",
+	"rx_runt_packets",
+	"rx_fragments",
+	"rx_64B_or_less_packets",
+	"rx_65B_to_127B_packets",
+	"rx_128B_to_255B_packets",
+	"rx_256B_to_511B_packets",
+	"rx_512B_to_1023B_packets",
+	"rx_1024B_to_1518B_packets",
+	"rx_1519B_to_mtu_packets",
+	"rx_oversize_packets",
+	"rx_rxf_ov_drop_packets",
+	"rx_rrd_ov_drop_packets",
+	"rx_align_errors",
+	"rx_bcast_bytes",
+	"rx_mcast_bytes",
+	"rx_address_errors",
+	"tx_packets",
+	"tx_bcast_packets",
+	"tx_mcast_packets",
+	"tx_pause_packets",
+	"tx_exc_defer_packets",
+	"tx_ctrl_packets",
+	"tx_defer_packets",
+	"tx_bytes",
+	"tx_64B_or_less_packets",
+	"tx_65B_to_127B_packets",
+	"tx_128B_to_255B_packets",
+	"tx_256B_to_511B_packets",
+	"tx_512B_to_1023B_packets",
+	"tx_1024B_to_1518B_packets",
+	"tx_1519B_to_mtu_packets",
+	"tx_single_collision",
+	"tx_multiple_collisions",
+	"tx_late_collision",
+	"tx_abort_collision",
+	"tx_underrun",
+	"tx_trd_eop",
+	"tx_length_errors",
+	"tx_trunc_packets",
+	"tx_bcast_bytes",
+	"tx_mcast_bytes",
+	"tx_update",
+};
+
+#define ALX_NUM_STATS ARRAY_SIZE(alx_gstrings_stats)
+
+
 static u32 alx_get_supported_speeds(struct alx_hw *hw)
 {
 	u32 supported = SUPPORTED_10baseT_Half |
@@ -201,6 +261,44 @@ static void alx_set_msglevel(struct net_device *netdev, u32 data)
 	alx->msg_enable = data;
 }
 
+static void alx_get_ethtool_stats(struct net_device *netdev,
+				  struct ethtool_stats *estats, u64 *data)
+{
+	struct alx_priv *alx = netdev_priv(netdev);
+	struct alx_hw *hw = &alx->hw;
+
+	spin_lock(&alx->stats_lock);
+
+	__alx_update_hw_stats(hw);
+	BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) <
+		     ALX_NUM_STATS * sizeof(u64));
+	memcpy(data, &hw->stats.rx_ok, ALX_NUM_STATS * sizeof(u64));
+
+	spin_unlock(&alx->stats_lock);
+}
+
+static void alx_get_strings(struct net_device *netdev, u32 stringset, u8 *buf)
+{
+	switch (stringset) {
+	case ETH_SS_STATS:
+		memcpy(buf, &alx_gstrings_stats, sizeof(alx_gstrings_stats));
+		break;
+	default:
+		WARN_ON(1);
+		break;
+	}
+}
+
+static int alx_get_sset_count(struct net_device *netdev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS:
+		return ALX_NUM_STATS;
+	default:
+		return -EINVAL;
+	}
+}
+
 const struct ethtool_ops alx_ethtool_ops = {
 	.get_settings	= alx_get_settings,
 	.set_settings	= alx_set_settings,
@@ -209,4 +307,7 @@ const struct ethtool_ops alx_ethtool_ops = {
 	.get_msglevel	= alx_get_msglevel,
 	.set_msglevel	= alx_set_msglevel,
 	.get_link	= ethtool_op_get_link,
+	.get_strings	= alx_get_strings,
+	.get_sset_count	= alx_get_sset_count,
+	.get_ethtool_stats	= alx_get_ethtool_stats,
 };
diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
index 3780773..de0bdaf 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -381,7 +381,11 @@ struct alx_rrd {
 				 ALX_ISR_RX_Q6 | \
 				 ALX_ISR_RX_Q7)
 
-/* Statistics counters collected by the MAC */
+/* Statistics counters collected by the MAC
+ *
+ * The order of the fields must match the strings in alx_gstrings_stats
+ * See ethtool.c
+ */
 struct alx_hw_stats {
 	/* rx */
 	u64 rx_ok;
-- 
1.8.5.2

Re: [PATCH v2 5/5] alx: add stats to ethtool

[Johannes Berg]

On Sat, 2014-01-04 at 17:47 +0100, Sabrina Dubroca wrote:

> +	__alx_update_hw_stats(hw);
> +	BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) <
> +		     ALX_NUM_STATS * sizeof(u64));

I think you should make that != instead of <, otherwise you won't catch
all possible differences.

johannes

Re: [PATCH v2 5/5] alx: add stats to ethtool

[Sabrina Dubroca]

[2014-01-06, 10:03:10] Johannes Berg wrote:
> On Sat, 2014-01-04 at 17:47 +0100, Sabrina Dubroca wrote:
> 
> > +	__alx_update_hw_stats(hw);
> > +	BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) <
> > +		     ALX_NUM_STATS * sizeof(u64));
> 
> I think you should make that != instead of <, otherwise you won't catch
> all possible differences.

With a !=, BUILD_BUG_ON is triggered if a new field is added at the
end of the structure. But adding a field doesn't break the code,
though I'm not sure allowing this is useful. The offsetof (and the
source in memcpy) also allows to add new fields at the beginning.

And the way alx_update_hw_stats is written already includes a kind of
check that all fields are present.

-- 
Sabrina

Re: [PATCH v2 5/5] alx: add stats to ethtool

[Johannes Berg]

On Mon, 2014-01-06 at 11:57 +0100, Sabrina Dubroca wrote:
> [2014-01-06, 10:03:10] Johannes Berg wrote:
> > On Sat, 2014-01-04 at 17:47 +0100, Sabrina Dubroca wrote:
> > 
> > > +	__alx_update_hw_stats(hw);
> > > +	BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) <
> > > +		     ALX_NUM_STATS * sizeof(u64));
> > 
> > I think you should make that != instead of <, otherwise you won't catch
> > all possible differences.
> 
> With a !=, BUILD_BUG_ON is triggered if a new field is added at the
> end of the structure. 

That seems reasonable, you'd want to export that field as well? Fields
that shouldn't be exported could be added before rx_ok.

> But adding a field doesn't break the code,
> though I'm not sure allowing this is useful. The offsetof (and the
> source in memcpy) also allows to add new fields at the beginning.
> 
> And the way alx_update_hw_stats is written already includes a kind of
> check that all fields are present.

I was more worried about type mismatches. That's not really a concern
with u64 since that's the largest type that really makes sense here, but
if the type of some variables changed vs. the ethtool type u64...

Maybe I'm overly worried. It seems likely nobody will ever touch this
code again :)

johannes

Re: [PATCH v2 5/5] alx: add stats to ethtool

[Sabrina Dubroca]

2014-01-06, 15:11:36 +0100, Johannes Berg wrote:
> On Mon, 2014-01-06 at 11:57 +0100, Sabrina Dubroca wrote:
> > [2014-01-06, 10:03:10] Johannes Berg wrote:
> > > On Sat, 2014-01-04 at 17:47 +0100, Sabrina Dubroca wrote:
> > > 
> > > > +	__alx_update_hw_stats(hw);
> > > > +	BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) <
> > > > +		     ALX_NUM_STATS * sizeof(u64));
> > > 
> > > I think you should make that != instead of <, otherwise you won't catch
> > > all possible differences.
> > 
> > With a !=, BUILD_BUG_ON is triggered if a new field is added at the
> > end of the structure. 
> 
> That seems reasonable, you'd want to export that field as well? Fields
> that shouldn't be exported could be added before rx_ok.

I don't have anything else to add here, I'm just considering what
others might want to do. Maybe overthinking. But, yeah, that's the idea.
(and fields added to the end of the struct but that don't have a
description in alx_gstrings_stats are ignored)

> > But adding a field doesn't break the code,
> > though I'm not sure allowing this is useful. The offsetof (and the
> > source in memcpy) also allows to add new fields at the beginning.
> > 
> > And the way alx_update_hw_stats is written already includes a kind of
> > check that all fields are present.
> 
> I was more worried about type mismatches. That's not really a concern
> with u64 since that's the largest type that really makes sense here, but
> if the type of some variables changed vs. the ethtool type u64...

Add "all stats fields must be u64" to the comment before the struct?


> Maybe I'm overly worried. It seems likely nobody will ever touch this
> code again :)

:)

-- 
Sabrina

Re: [PATCH v2 5/5] alx: add stats to ethtool

[Johannes Berg]

On Mon, 2014-01-06 at 15:56 +0100, Sabrina Dubroca wrote:

> > I was more worried about type mismatches. That's not really a concern
> > with u64 since that's the largest type that really makes sense here, but
> > if the type of some variables changed vs. the ethtool type u64...
> 
> Add "all stats fields must be u64" to the comment before the struct?

I guess that's OK. I can live with either of these solutions really, I
have no plans to change this driver ever again (though I might need WOL
soon ... *sigh*)

johannes