* [PATCH v3 1/5] alx: add a hardware stats structure
2014-01-06 16:33 [PATCH v2 0/5] alx: add statistics Sabrina Dubroca
@ 2014-01-06 16:33 ` Sabrina Dubroca
2014-01-06 16:33 ` [PATCH v3 2/5] alx: add constants for the stats fields Sabrina Dubroca
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Sabrina Dubroca @ 2014-01-06 16:33 UTC (permalink / raw)
To: davem; +Cc: bhutchings, johannes, stephen, netdev, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 2/5] alx: add constants for the stats fields
2014-01-06 16:33 [PATCH v2 0/5] alx: add statistics Sabrina Dubroca
2014-01-06 16:33 ` [PATCH v3 1/5] alx: add a hardware stats structure Sabrina Dubroca
@ 2014-01-06 16:33 ` Sabrina Dubroca
2014-01-06 16:33 ` [PATCH v3 3/5] alx: add stats update function Sabrina Dubroca
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Sabrina Dubroca @ 2014-01-06 16:33 UTC (permalink / raw)
To: davem; +Cc: bhutchings, johannes, stephen, netdev, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 3/5] alx: add stats update function
2014-01-06 16:33 [PATCH v2 0/5] alx: add statistics Sabrina Dubroca
2014-01-06 16:33 ` [PATCH v3 1/5] alx: add a hardware stats structure Sabrina Dubroca
2014-01-06 16:33 ` [PATCH v3 2/5] alx: add constants for the stats fields Sabrina Dubroca
@ 2014-01-06 16:33 ` Sabrina Dubroca
2014-01-06 16:33 ` [PATCH v3 4/5] alx: add alx_get_stats64 operation Sabrina Dubroca
2014-01-06 16:33 ` [PATCH v3 5/5] alx: add stats to ethtool Sabrina Dubroca
4 siblings, 0 replies; 9+ messages in thread
From: Sabrina Dubroca @ 2014-01-06 16:33 UTC (permalink / raw)
To: davem; +Cc: bhutchings, johannes, stephen, netdev, 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..7712f06 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..a97edfb 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 4/5] alx: add alx_get_stats64 operation
2014-01-06 16:33 [PATCH v2 0/5] alx: add statistics Sabrina Dubroca
` (2 preceding siblings ...)
2014-01-06 16:33 ` [PATCH v3 3/5] alx: add stats update function Sabrina Dubroca
@ 2014-01-06 16:33 ` Sabrina Dubroca
2014-01-06 19:07 ` Ben Hutchings
2014-01-06 16:33 ` [PATCH v3 5/5] alx: add stats to ethtool Sabrina Dubroca
4 siblings, 1 reply; 9+ messages in thread
From: Sabrina Dubroca @ 2014-01-06 16:33 UTC (permalink / raw)
To: davem; +Cc: bhutchings, johannes, stephen, netdev, 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..912d732 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 4/5] alx: add alx_get_stats64 operation
2014-01-06 16:33 ` [PATCH v3 4/5] alx: add alx_get_stats64 operation Sabrina Dubroca
@ 2014-01-06 19:07 ` Ben Hutchings
2014-01-07 10:02 ` Sabrina Dubroca
0 siblings, 1 reply; 9+ messages in thread
From: Ben Hutchings @ 2014-01-06 19:07 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: davem, johannes, stephen, netdev
On Mon, 2014-01-06 at 17:33 +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 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;
I think this should be set to hw_stats->tx_ok + net_stats->tx_errors
(after you set tx_errors).
> + net_stats->tx_bytes = hw_stats->tx_byte_cnt;
> + net_stats->rx_packets = hw_stats->rx_ok;
Similarly, I think this should be hw_stats->rx_ok +
net_stats->rx_errors.
> + 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 +
I would expect this to count the number of packets that had collisions
rather than total number of collisions (which you're only guessing at by
using * 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;
rx_over_errors is commented as 'receiver ring buff overflow' and
ifconfig includes it in the 'frame' error count. I think it is intended
to count frames which are too large for on-chip RX buffers and should
always be 0 for devices that do RX DMA.
Each error should contribute to at most one specific error stat, so
don't count rx_ov_rxf in both rx_fifo_errors and rx_over_errors. I
would guess rx_fifo_errors is the right counter.
I don't know what rx_ov_rrd represents, but if it's the number of
packets dropped because the RX descriptor ring was empty then it should
be counted in rx_dropped not rx_over_errors.
> + net_stats->rx_missed_errors = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf;
[...]
This counter is commented as 'receiver missed packet'. I think this
means the MAC detected SOF but was somehow too busy to receive the
packet, but I'm not sure. I don't think these hardware counters match
the description, and certainly they shouldn't be counted here as well as
the other specific error 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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 4/5] alx: add alx_get_stats64 operation
2014-01-06 19:07 ` Ben Hutchings
@ 2014-01-07 10:02 ` Sabrina Dubroca
0 siblings, 0 replies; 9+ messages in thread
From: Sabrina Dubroca @ 2014-01-07 10:02 UTC (permalink / raw)
To: Ben Hutchings; +Cc: davem, johannes, stephen, netdev
Then the stats in all atheros drivers are a bit broken? They all use
the same formulas for ndo_get_stats.
Other atheros drivers (atl1e/atl1e.h and atl1c/atl1c.h) have the same
stats structure with a comment for each field. I will copy the
comments to alx.h.
2014-01-06, 19:07:33 +0000, Ben Hutchings wrote:
> On Mon, 2014-01-06 at 17:33 +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 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;
>
> I think this should be set to hw_stats->tx_ok + net_stats->tx_errors
> (after you set tx_errors).
>
> > + net_stats->tx_bytes = hw_stats->tx_byte_cnt;
> > + net_stats->rx_packets = hw_stats->rx_ok;
>
> Similarly, I think this should be hw_stats->rx_ok +
> net_stats->rx_errors.
>
> > + 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 +
>
> I would expect this to count the number of packets that had collisions
> rather than total number of collisions (which you're only guessing at by
> using * 2).
Okay, all changed.
> > + 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;
>
> rx_over_errors is commented as 'receiver ring buff overflow' and
> ifconfig includes it in the 'frame' error count. I think it is intended
> to count frames which are too large for on-chip RX buffers and should
> always be 0 for devices that do RX DMA.
>
> Each error should contribute to at most one specific error stat, so
> don't count rx_ov_rxf in both rx_fifo_errors and rx_over_errors. I
> would guess rx_fifo_errors is the right counter.
>
> I don't know what rx_ov_rrd represents, but if it's the number of
> packets dropped because the RX descriptor ring was empty then it should
> be counted in rx_dropped not rx_over_errors.
in atl1e/atl1e.h:
rx_ov_rxf: The number of frame dropped due to occurrence of RX FIFO overflow.
rx_ov_rrd: The number of frame dropped due to occurrence of RRD overflow.
I'm not sure which counter fits these best.
> > + net_stats->rx_missed_errors = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf;
> [...]
>
> This counter is commented as 'receiver missed packet'. I think this
> means the MAC detected SOF but was somehow too busy to receive the
> packet, but I'm not sure. I don't think these hardware counters match
> the description, and certainly they shouldn't be counted here as well as
> the other specific error stats.
Okay.
Thanks,
--
Sabrina
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 5/5] alx: add stats to ethtool
2014-01-06 16:33 [PATCH v2 0/5] alx: add statistics Sabrina Dubroca
` (3 preceding siblings ...)
2014-01-06 16:33 ` [PATCH v3 4/5] alx: add alx_get_stats64 operation Sabrina Dubroca
@ 2014-01-06 16:33 ` Sabrina Dubroca
2014-01-06 19:08 ` Ben Hutchings
4 siblings, 1 reply; 9+ messages in thread
From: Sabrina Dubroca @ 2014-01-06 16:33 UTC (permalink / raw)
To: davem; +Cc: bhutchings, johannes, stephen, netdev, 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 | 7 +-
2 files changed, 107 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 45b3650..08e22df 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 a97edfb..5978253 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -381,7 +381,12 @@ 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
+ * All stats fields should be u64
+ * See ethtool.c
+ */
struct alx_hw_stats {
/* rx */
u64 rx_ok;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 5/5] alx: add stats to ethtool
2014-01-06 16:33 ` [PATCH v3 5/5] alx: add stats to ethtool Sabrina Dubroca
@ 2014-01-06 19:08 ` Ben Hutchings
0 siblings, 0 replies; 9+ messages in thread
From: Ben Hutchings @ 2014-01-06 19:08 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: davem, johannes, stephen, netdev
On Mon, 2014-01-06 at 17:33 +0100, Sabrina Dubroca wrote:
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> drivers/net/ethernet/atheros/alx/ethtool.c | 101 +++++++++++++++++++++++++++++
> drivers/net/ethernet/atheros/alx/hw.h | 7 +-
> 2 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
> index 45b3650..08e22df 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 a97edfb..5978253 100644
> --- a/drivers/net/ethernet/atheros/alx/hw.h
> +++ b/drivers/net/ethernet/atheros/alx/hw.h
> @@ -381,7 +381,12 @@ 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
> + * All stats fields should be u64
> + * See ethtool.c
> + */
> struct alx_hw_stats {
> /* rx */
> u64 rx_ok;
--
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.
^ permalink raw reply [flat|nested] 9+ messages in thread