[PATCH 0/5] alx: add statistics

[PATCH 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.

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_stats operation
  alx: add stats to ethtool

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

-- 
1.8.5.2

[PATCH 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..207bcc6 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 */
+	unsigned long rx_ok;
+	unsigned long rx_bcast;
+	unsigned long rx_mcast;
+	unsigned long rx_pause;
+	unsigned long rx_ctrl;
+	unsigned long rx_fcs_err;
+	unsigned long rx_len_err;
+	unsigned long rx_byte_cnt;
+	unsigned long rx_runt;
+	unsigned long rx_frag;
+	unsigned long rx_sz_64B;
+	unsigned long rx_sz_127B;
+	unsigned long rx_sz_255B;
+	unsigned long rx_sz_511B;
+	unsigned long rx_sz_1023B;
+	unsigned long rx_sz_1518B;
+	unsigned long rx_sz_max;
+	unsigned long rx_ov_sz;
+	unsigned long rx_ov_rxf;
+	unsigned long rx_ov_rrd;
+	unsigned long rx_align_err;
+	unsigned long rx_bc_byte_cnt;
+	unsigned long rx_mc_byte_cnt;
+	unsigned long rx_err_addr;
+
+	/* tx */
+	unsigned long tx_ok;
+	unsigned long tx_bcast;
+	unsigned long tx_mcast;
+	unsigned long tx_pause;
+	unsigned long tx_exc_defer;
+	unsigned long tx_ctrl;
+	unsigned long tx_defer;
+	unsigned long tx_byte_cnt;
+	unsigned long tx_sz_64B;
+	unsigned long tx_sz_127B;
+	unsigned long tx_sz_255B;
+	unsigned long tx_sz_511B;
+	unsigned long tx_sz_1023B;
+	unsigned long tx_sz_1518B;
+	unsigned long tx_sz_max;
+	unsigned long tx_single_col;
+	unsigned long tx_multi_col;
+	unsigned long tx_late_col;
+	unsigned long tx_abort_col;
+	unsigned long tx_underrun;
+	unsigned long tx_trd_eop;
+	unsigned long tx_len_err;
+	unsigned long tx_trunc;
+	unsigned long tx_bc_byte_cnt;
+	unsigned long tx_mc_byte_cnt;
+	unsigned long update;
+};
+
+
 /* maximum interrupt vectors for msix */
 #define ALX_MAX_MSIX_INTRS	16
 
-- 
1.8.5.2

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

[Ben Hutchings]

On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote:
> 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..207bcc6 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 */
> +	unsigned long rx_ok;
[...]

Use u64 for all of these.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH 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 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 |  3 ++
 2 files changed, 61 insertions(+)

diff --git a/drivers/net/ethernet/atheros/alx/hw.c b/drivers/net/ethernet/atheros/alx/hw.c
index 1e8c24a..37ab921 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 207bcc6..97e8731 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -482,6 +482,8 @@ struct alx_hw {
 
 	/* PHY link patch flag */
 	bool lnk_patch;
+
+	struct alx_hw_stats stats;
 };
 
 static inline int alx_hw_revision(struct alx_hw *hw)
@@ -549,6 +551,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 3/5] alx: add stats update function

[Stephen Hemminger]

On Thu,  2 Jan 2014 00:40:26 +0100
Sabrina Dubroca <sd@queasysnail.net> wrote:

> diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
> index 207bcc6..97e8731 100644
> --- a/drivers/net/ethernet/atheros/alx/hw.h
> +++ b/drivers/net/ethernet/atheros/alx/hw.h
> @@ -482,6 +482,8 @@ struct alx_hw {
>  
>  	/* PHY link patch flag */
>  	bool lnk_patch;
> +
> +	struct alx_hw_stats stats;
>  };

Why do you need this to be part of the hw struct?
It doesn't need to be persistent.
couldn't the callers just pass it on the stack.

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

[Ben Hutchings]

On Wed, 2014-01-01 at 21:55 -0800, Stephen Hemminger wrote:
> On Thu,  2 Jan 2014 00:40:26 +0100
> Sabrina Dubroca <sd@queasysnail.net> wrote:
> 
> > diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
> > index 207bcc6..97e8731 100644
> > --- a/drivers/net/ethernet/atheros/alx/hw.h
> > +++ b/drivers/net/ethernet/atheros/alx/hw.h
> > @@ -482,6 +482,8 @@ struct alx_hw {
> >  
> >  	/* PHY link patch flag */
> >  	bool lnk_patch;
> > +
> > +	struct alx_hw_stats stats;
> >  };
> 
> Why do you need this to be part of the hw struct?
> It doesn't need to be persistent.
> couldn't the callers just pass it on the stack.

Oh come on, how is that going to work when the hardware counters are
read-to-clear?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH 4/5] alx: add alx_get_stats 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..d825e38 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 net_device_stats *alx_get_stats(struct net_device *netdev)
+{
+	struct alx_priv *alx = netdev_priv(netdev);
+	struct net_device_stats *net_stats = &netdev->stats;
+	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_stats          = alx_get_stats,
 	.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

Re: [PATCH 4/5] alx: add alx_get_stats operation

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Thu,  2 Jan 2014 00:40:27 +0100

> +	spin_lock(&alx->stats_lock);
> +
> +	__alx_update_hw_stats(&alx->hw);

Please use something like the linux/u64_stats_sync.h stuff rather
than spin locking.

Thank you.

Re: [PATCH 4/5] alx: add alx_get_stats operation

[Ben Hutchings]

On Wed, 2014-01-01 at 22:27 -0500, David Miller wrote:
> From: Sabrina Dubroca <sd@queasysnail.net>
> Date: Thu,  2 Jan 2014 00:40:27 +0100
> 
> > +	spin_lock(&alx->stats_lock);
> > +
> > +	__alx_update_hw_stats(&alx->hw);
> 
> Please use something like the linux/u64_stats_sync.h stuff rather
> than spin locking.

I don't think that'sa useful, as we can have multiple writers
(concurrent calls to get_stats).  And there is really no harm in using a
spinlock to serialise get_stats calls.  The u64_stats API is good for
statistics updated from the data path.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 4/5] alx: add alx_get_stats operation

[Sabrina Dubroca]

[2014-01-02, 11:56:53] Ben Hutchings wrote:
> On Wed, 2014-01-01 at 22:27 -0500, David Miller wrote:
> > From: Sabrina Dubroca <sd@queasysnail.net>
> > Date: Thu,  2 Jan 2014 00:40:27 +0100
> > 
> > > +	spin_lock(&alx->stats_lock);
> > > +
> > > +	__alx_update_hw_stats(&alx->hw);
> > 
> > Please use something like the linux/u64_stats_sync.h stuff rather
> > than spin locking.
> 
> I don't think that'sa useful, as we can have multiple writers
> (concurrent calls to get_stats).  And there is really no harm in using a
> spinlock to serialise get_stats calls.  The u64_stats API is good for
> statistics updated from the data path.

I've read the comments in linux/u64_stats_sync.h, which mentions the
need for exclusive access to the data. I've looked at other drivers
(broadcom/b44.c, nvidia/forcedeth.c) that use the u64_stats functions
to get stats from hardware, and they use a spin_lock around the update
code. The other drivers that I've looked at and that use u64_stats
have software stats that they update on rx/tx, so I think the
situation is a bit different.


For now I've added u64_stats and modified the functions this way:

static struct net_device_stats *alx_get_stats(struct net_device *netdev)
{
	struct alx_priv *alx = netdev_priv(netdev);
	struct net_device_stats *net_stats = &netdev->stats;
	struct alx_hw_stats *hw_stats = &alx->hw.stats;

	unsigned int start;

	spin_lock(&alx->stats_lock);

	__alx_update_hw_stats(&alx->hw);

	spin_unlock(&alx->stats_lock);

	do {
		start = u64_stats_fetch_begin(&hw_stats->syncp);
		/* fill net_stat... */
	} while (u64_stats_fetch_retry(&hw_stats->syncp, start));

	return net_stats;
}


void __alx_update_hw_stats(struct alx_hw *hw)
{
	u64_stats_update_begin(&hw->stats.syncp);

	/* fill hw->stats */

	u64_stats_update_end(&hw->stats.syncp);
}


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;

	unsigned int start;

	spin_lock(&alx->stats_lock);
	__alx_update_hw_stats(hw);
	spin_unlock(&alx->stats_lock);

	do {
		start = u64_stats_fetch_begin(&hw->stats.syncp);
		memcpy(data, &hw->stats, sizeof(hw->stats));
	} while (u64_stats_fetch_retry(&hw->stats.syncp, start));
}

Thanks,

-- 
Sabrina Dubroca

Re: [PATCH 4/5] alx: add alx_get_stats operation

[Ben Hutchings]

On Thu, 2014-01-02 at 17:05 +0100, Sabrina Dubroca wrote:
> [2014-01-02, 11:56:53] Ben Hutchings wrote:
> > On Wed, 2014-01-01 at 22:27 -0500, David Miller wrote:
> > > From: Sabrina Dubroca <sd@queasysnail.net>
> > > Date: Thu,  2 Jan 2014 00:40:27 +0100
> > > 
> > > > +	spin_lock(&alx->stats_lock);
> > > > +
> > > > +	__alx_update_hw_stats(&alx->hw);
> > > 
> > > Please use something like the linux/u64_stats_sync.h stuff rather
> > > than spin locking.
> > 
> > I don't think that'sa useful, as we can have multiple writers
> > (concurrent calls to get_stats).  And there is really no harm in using a
> > spinlock to serialise get_stats calls.  The u64_stats API is good for
> > statistics updated from the data path.
> 
> I've read the comments in linux/u64_stats_sync.h, which mentions the
> need for exclusive access to the data. I've looked at other drivers
> (broadcom/b44.c, nvidia/forcedeth.c) that use the u64_stats functions
> to get stats from hardware, and they use a spin_lock around the update
> code. The other drivers that I've looked at and that use u64_stats
> have software stats that they update on rx/tx, so I think the
> situation is a bit different.
> 
> 
> For now I've added u64_stats and modified the functions this way:
[...]

You could do that but I think your original version was fine.  Of course
it is David's decision in the end.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 4/5] alx: add alx_get_stats operation

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 2 Jan 2014 11:56:53 +0000

> On Wed, 2014-01-01 at 22:27 -0500, David Miller wrote:
>> From: Sabrina Dubroca <sd@queasysnail.net>
>> Date: Thu,  2 Jan 2014 00:40:27 +0100
>> 
>> > +	spin_lock(&alx->stats_lock);
>> > +
>> > +	__alx_update_hw_stats(&alx->hw);
>> 
>> Please use something like the linux/u64_stats_sync.h stuff rather
>> than spin locking.
> 
> I don't think that'sa useful, as we can have multiple writers
> (concurrent calls to get_stats).  And there is really no harm in using a
> spinlock to serialise get_stats calls.  The u64_stats API is good for
> statistics updated from the data path.

Fair enough.

Re: [PATCH 4/5] alx: add alx_get_stats operation

[Ben Hutchings]

On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote:
[...]
> --- 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 net_device_stats *alx_get_stats(struct net_device *netdev)
[...]
>  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_stats          = alx_get_stats,
[...]

You should implement ndo_get_stats64 rather than ndo_get_stats.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[Sabrina Dubroca]

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

diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 45b3650..259056f 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -46,6 +46,62 @@
 #include "reg.h"
 #include "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 +257,42 @@ 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);
+	memcpy(data, &hw->stats, sizeof(hw->stats));
+
+	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 -ENOTSUPP;
+	}
+}
+
 const struct ethtool_ops alx_ethtool_ops = {
 	.get_settings	= alx_get_settings,
 	.set_settings	= alx_set_settings,
@@ -209,4 +301,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,
 };
-- 
1.8.5.2

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

[Ben Hutchings]

On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote:
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  drivers/net/ethernet/atheros/alx/ethtool.c | 95 ++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
> index 45b3650..259056f 100644
> --- a/drivers/net/ethernet/atheros/alx/ethtool.c
> +++ b/drivers/net/ethernet/atheros/alx/ethtool.c
[...]
> +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);
> +	memcpy(data, &hw->stats, sizeof(hw->stats));

This definitely doesn't work if the members of hw->stats are typed as
unsigned long...

> +	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 -ENOTSUPP;
[...]

Never return error code ENOTSUPP; it's *not* the same thing as ENOTSUP
in userland and is not part of the userland ABI.  I would use EINVAL
here.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[Ben Hutchings]

On Thu, 2014-01-02 at 12:01 +0000, Ben Hutchings wrote:
> On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote:
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> >  drivers/net/ethernet/atheros/alx/ethtool.c | 95 ++++++++++++++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
> > index 45b3650..259056f 100644
> > --- a/drivers/net/ethernet/atheros/alx/ethtool.c
> > +++ b/drivers/net/ethernet/atheros/alx/ethtool.c
> [...]
> > +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);
> > +	memcpy(data, &hw->stats, sizeof(hw->stats));
> 
> This definitely doesn't work if the members of hw->stats are typed as
> unsigned long...
[...]

Also it would be a good idea to assert the sizes match here:

	BUILD_BUG_ON(sizeof(hw->stats) != ALX_NUM_STATS * sizeof(u64));

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[Johannes Berg]

On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote:

> +static const char alx_gstrings_stats[][ETH_GSTRING_LEN] = {

You should probably have a comment here and on the stats struct
declaration that they must absolutely match in order/size/etc.

Maybe try to put in some BUILD_BUG_ON() as well, at least checking the
sizeof() vs. ARRAY_SIZE*sizeof(u64) - that might already have caught the
bug that Ben pointed out.

> +	"rx_packets",

Is it useful to provide stats that are already elsewhere? Then again, it
doesn't really hurt and simplifies the code ...

johannes

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

[Sabrina Dubroca]

[2014-01-02, 14:09:14] Johannes Berg wrote:
> On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote:
> 
> > +static const char alx_gstrings_stats[][ETH_GSTRING_LEN] = {
> 
> You should probably have a comment here and on the stats struct
> declaration that they must absolutely match in order/size/etc.


With these comments, and something similar before
__alx_update_hw_stats, should I use the code for __alx_update_hw_stats
mentionned in mail 0?


/* 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 {
	/* ... */
}

/* 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] = {
	/* ... */
}



> Maybe try to put in some BUILD_BUG_ON() as well, at least checking the
> sizeof() vs. ARRAY_SIZE*sizeof(u64) - that might already have caught the
> bug that Ben pointed out.


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

This way, rx_ok doesn't need to come first in struct alx_hw_stats, and
the size of the structure is checked as you said.



> > +	"rx_packets",
> 
> Is it useful to provide stats that are already elsewhere? Then again, it
> doesn't really hurt and simplifies the code ...

You're right, rx_packets is the sum of all the rx_SIZE_RANGE_packets
and is redundant information. The values split by size range are only
for ethtool. ndo_get_stats(64) uses only the sum. I think this
information is more relevant to the user, and anyway, it's available,
so we might as well provide it.


Thanks,

-- 
Sabrina Dubroca