diff mbox

[net-next,2/2] bgmac: Add support for ethtool statistics

Message ID 1464973621-14794-3-git-send-email-f.fainelli@gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Florian Fainelli June 3, 2016, 5:07 p.m. UTC 
Read the statistics from the BGMAC's builtin MAC and return them to
user-space using the standard ethtool helpers.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bgmac.c | 125 ++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bgmac.h |   4 +-
 2 files changed, 127 insertions(+), 2 deletions(-)

Comments

Ben Hutchings June 3, 2016, 5:57 p.m. UTC | #1 
On Fri, 2016-06-03 at 10:07 -0700, Florian Fainelli wrote:
[...]
> +static void bgmac_get_strings(struct net_device *dev, u32 stringset,
> +			      u8 *data)
> +{
> +	int i;
> +
> +	if (stringset != ETH_SS_STATS)
> +		return;
> +
> +	for (i = 0; i < BGMAC_STATS_LEN; i++)
> +		memcpy(data + i * ETH_GSTRING_LEN,
> +		       bgmac_get_strings_stats[i].name,
> +		       ETH_GSTRING_LEN);

These strings are null-terminated, not padded to ETH_GSTRING_LEN.  So
here you should be using strlcpy() instead of memcpy().

> +}
> +
> +static void bgmac_get_ethtool_stats(struct net_device *dev,
> +				    struct ethtool_stats *ss, uint64_t *data)
> +{
> +	struct bgmac *bgmac = netdev_priv(dev);
> +	const struct bgmac_stat *s;
> +	unsigned int i;
> +	u64 val;
> +
> +	if (!netif_running(dev))
> +		return;
> +
> +	for (i = 0; i < BGMAC_STATS_LEN; i++) {
> +		s = &bgmac_get_strings_stats[i];
> +		val = 0;
> +		if (s->size == 8)
> +			val = (u64)bgmac_read(bgmac, s->offset + 4);

Isn't this missing a << 32?

Does reading the high 32 bits latch the value of the low 32 bits?  If
not, you need to read the high bits again after the low bits and retry
if they changed.

> +		val |= bgmac_read(bgmac, s->offset);
> +		data[i] = (u64)val;

Redundant cast.

Ben.

> +	}
> +}
[...]
Florian Fainelli June 3, 2016, 6:10 p.m. UTC | #2 
On 06/03/2016 10:57 AM, Ben Hutchings wrote:
> On Fri, 2016-06-03 at 10:07 -0700, Florian Fainelli wrote:
> [...]
>> +static void bgmac_get_strings(struct net_device *dev, u32 stringset,
>> +			      u8 *data)
>> +{
>> +	int i;
>> +
>> +	if (stringset != ETH_SS_STATS)
>> +		return;
>> +
>> +	for (i = 0; i < BGMAC_STATS_LEN; i++)
>> +		memcpy(data + i * ETH_GSTRING_LEN,
>> +		       bgmac_get_strings_stats[i].name,
>> +		       ETH_GSTRING_LEN);
> 
> These strings are null-terminated, not padded to ETH_GSTRING_LEN.  So
> here you should be using strlcpy() instead of memcpy().
> 
>> +}
>> +
>> +static void bgmac_get_ethtool_stats(struct net_device *dev,
>> +				    struct ethtool_stats *ss, uint64_t *data)
>> +{
>> +	struct bgmac *bgmac = netdev_priv(dev);
>> +	const struct bgmac_stat *s;
>> +	unsigned int i;
>> +	u64 val;
>> +
>> +	if (!netif_running(dev))
>> +		return;
>> +
>> +	for (i = 0; i < BGMAC_STATS_LEN; i++) {
>> +		s = &bgmac_get_strings_stats[i];
>> +		val = 0;
>> +		if (s->size == 8)
>> +			val = (u64)bgmac_read(bgmac, s->offset + 4);
> 
> Isn't this missing a << 32?

It is, guess I should have made sure there was 4GB+ worth of traffic to
make sure this seemed reasonable.
> 
> Does reading the high 32 bits latch the value of the low 32 bits?  If
> not, you need to read the high bits again after the low bits and retry
> if they changed.

Yes these registers are latched.

> 
>> +		val |= bgmac_read(bgmac, s->offset);
>> +		data[i] = (u64)val;
> 
> Redundant cast.

Indeed, thanks.
diff mbox

Patch

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 156fa6323745..d5877365d81a 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1382,6 +1382,128 @@  static const struct net_device_ops bgmac_netdev_ops = {
  * ethtool_ops
  **************************************************/
 
+struct bgmac_stat {
+	u8 size;
+	u32 offset;
+	const char *name;
+};
+
+static struct bgmac_stat bgmac_get_strings_stats[] = {
+	{ 8, BGMAC_TX_GOOD_OCTETS, "tx_good_octets" },
+	{ 4, BGMAC_TX_GOOD_PKTS, "tx_good" },
+	{ 8, BGMAC_TX_OCTETS, "tx_octets" },
+	{ 4, BGMAC_TX_PKTS, "tx_pkts" },
+	{ 4, BGMAC_TX_BROADCAST_PKTS, "tx_broadcast" },
+	{ 4, BGMAC_TX_MULTICAST_PKTS, "tx_multicast" },
+	{ 4, BGMAC_TX_LEN_64, "tx_64" },
+	{ 4, BGMAC_TX_LEN_65_TO_127, "tx_65_127" },
+	{ 4, BGMAC_TX_LEN_128_TO_255, "tx_128_255" },
+	{ 4, BGMAC_TX_LEN_256_TO_511, "tx_256_511" },
+	{ 4, BGMAC_TX_LEN_512_TO_1023, "tx_512_1023" },
+	{ 4, BGMAC_TX_LEN_1024_TO_1522, "tx_1024_1522" },
+	{ 4, BGMAC_TX_LEN_1523_TO_2047, "tx_1523_2047" },
+	{ 4, BGMAC_TX_LEN_2048_TO_4095, "tx_2048_4095" },
+	{ 4, BGMAC_TX_LEN_4096_TO_8191, "tx_4096_8191" },
+	{ 4, BGMAC_TX_LEN_8192_TO_MAX, "tx_8192_max" },
+	{ 4, BGMAC_TX_JABBER_PKTS, "tx_jabber" },
+	{ 4, BGMAC_TX_OVERSIZE_PKTS, "tx_oversize" },
+	{ 4, BGMAC_TX_FRAGMENT_PKTS, "tx_fragment" },
+	{ 4, BGMAC_TX_UNDERRUNS, "tx_underruns" },
+	{ 4, BGMAC_TX_TOTAL_COLS, "tx_total_cols" },
+	{ 4, BGMAC_TX_SINGLE_COLS, "tx_single_cols" },
+	{ 4, BGMAC_TX_MULTIPLE_COLS, "tx_multiple_cols" },
+	{ 4, BGMAC_TX_EXCESSIVE_COLS, "tx_excessive_cols" },
+	{ 4, BGMAC_TX_LATE_COLS, "tx_late_cols" },
+	{ 4, BGMAC_TX_DEFERED, "tx_defered" },
+	{ 4, BGMAC_TX_CARRIER_LOST, "tx_carrier_lost" },
+	{ 4, BGMAC_TX_PAUSE_PKTS, "tx_pause" },
+	{ 4, BGMAC_TX_UNI_PKTS, "tx_unicast" },
+	{ 4, BGMAC_TX_Q0_PKTS, "tx_q0" },
+	{ 8, BGMAC_TX_Q0_OCTETS, "tx_q0_octets" },
+	{ 4, BGMAC_TX_Q1_PKTS, "tx_q1" },
+	{ 8, BGMAC_TX_Q1_OCTETS, "tx_q1_octets" },
+	{ 4, BGMAC_TX_Q2_PKTS, "tx_q2" },
+	{ 8, BGMAC_TX_Q2_OCTETS, "tx_q2_octets" },
+	{ 4, BGMAC_TX_Q3_PKTS, "tx_q3" },
+	{ 8, BGMAC_TX_Q3_OCTETS, "tx_q3_octets" },
+	{ 8, BGMAC_RX_GOOD_OCTETS, "rx_good_octets" },
+	{ 4, BGMAC_RX_GOOD_PKTS, "rx_good" },
+	{ 8, BGMAC_RX_OCTETS, "rx_octets" },
+	{ 4, BGMAC_RX_PKTS, "rx_pkts" },
+	{ 4, BGMAC_RX_BROADCAST_PKTS, "rx_broadcast" },
+	{ 4, BGMAC_RX_MULTICAST_PKTS, "rx_multicast" },
+	{ 4, BGMAC_RX_LEN_64, "rx_64" },
+	{ 4, BGMAC_RX_LEN_65_TO_127, "rx_65_127" },
+	{ 4, BGMAC_RX_LEN_128_TO_255, "rx_128_255" },
+	{ 4, BGMAC_RX_LEN_256_TO_511, "rx_256_511" },
+	{ 4, BGMAC_RX_LEN_512_TO_1023, "rx_512_1023" },
+	{ 4, BGMAC_RX_LEN_1024_TO_1522, "rx_1024_1522" },
+	{ 4, BGMAC_RX_LEN_1523_TO_2047, "rx_1523_2047" },
+	{ 4, BGMAC_RX_LEN_2048_TO_4095, "rx_2048_4095" },
+	{ 4, BGMAC_RX_LEN_4096_TO_8191, "rx_4096_8191" },
+	{ 4, BGMAC_RX_LEN_8192_TO_MAX, "rx_8192_max" },
+	{ 4, BGMAC_RX_JABBER_PKTS, "rx_jabber" },
+	{ 4, BGMAC_RX_OVERSIZE_PKTS, "rx_oversize" },
+	{ 4, BGMAC_RX_FRAGMENT_PKTS, "rx_fragment" },
+	{ 4, BGMAC_RX_MISSED_PKTS, "rx_missed" },
+	{ 4, BGMAC_RX_CRC_ALIGN_ERRS, "rx_crc_align" },
+	{ 4, BGMAC_RX_UNDERSIZE, "rx_undersize" },
+	{ 4, BGMAC_RX_CRC_ERRS, "rx_crc" },
+	{ 4, BGMAC_RX_ALIGN_ERRS, "rx_align" },
+	{ 4, BGMAC_RX_SYMBOL_ERRS, "rx_symbol" },
+	{ 4, BGMAC_RX_PAUSE_PKTS, "rx_pause" },
+	{ 4, BGMAC_RX_NONPAUSE_PKTS, "rx_nonpause" },
+	{ 4, BGMAC_RX_SACHANGES, "rx_sa_changes" },
+	{ 4, BGMAC_RX_UNI_PKTS, "rx_unicast" },
+};
+
+#define BGMAC_STATS_LEN	ARRAY_SIZE(bgmac_get_strings_stats)
+
+static int bgmac_get_sset_count(struct net_device *dev, int string_set)
+{
+	switch (string_set) {
+	case ETH_SS_STATS:
+		return BGMAC_STATS_LEN;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static void bgmac_get_strings(struct net_device *dev, u32 stringset,
+			      u8 *data)
+{
+	int i;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	for (i = 0; i < BGMAC_STATS_LEN; i++)
+		memcpy(data + i * ETH_GSTRING_LEN,
+		       bgmac_get_strings_stats[i].name,
+		       ETH_GSTRING_LEN);
+}
+
+static void bgmac_get_ethtool_stats(struct net_device *dev,
+				    struct ethtool_stats *ss, uint64_t *data)
+{
+	struct bgmac *bgmac = netdev_priv(dev);
+	const struct bgmac_stat *s;
+	unsigned int i;
+	u64 val;
+
+	if (!netif_running(dev))
+		return;
+
+	for (i = 0; i < BGMAC_STATS_LEN; i++) {
+		s = &bgmac_get_strings_stats[i];
+		val = 0;
+		if (s->size == 8)
+			val = (u64)bgmac_read(bgmac, s->offset + 4);
+		val |= bgmac_read(bgmac, s->offset);
+		data[i] = (u64)val;
+	}
+}
+
 static int bgmac_get_settings(struct net_device *net_dev,
 			      struct ethtool_cmd *cmd)
 {
@@ -1406,6 +1528,9 @@  static void bgmac_get_drvinfo(struct net_device *net_dev,
 }
 
 static const struct ethtool_ops bgmac_ethtool_ops = {
+	.get_strings		= bgmac_get_strings,
+	.get_sset_count		= bgmac_get_sset_count,
+	.get_ethtool_stats	= bgmac_get_ethtool_stats,
 	.get_settings		= bgmac_get_settings,
 	.set_settings		= bgmac_set_settings,
 	.get_drvinfo		= bgmac_get_drvinfo,
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 9a03c142b742..853d72b132d1 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -123,7 +123,7 @@ 
 #define BGMAC_TX_LEN_1024_TO_1522		0x334
 #define BGMAC_TX_LEN_1523_TO_2047		0x338
 #define BGMAC_TX_LEN_2048_TO_4095		0x33c
-#define BGMAC_TX_LEN_4095_TO_8191		0x340
+#define BGMAC_TX_LEN_4096_TO_8191		0x340
 #define BGMAC_TX_LEN_8192_TO_MAX		0x344
 #define BGMAC_TX_JABBER_PKTS			0x348		/* Error */
 #define BGMAC_TX_OVERSIZE_PKTS			0x34c		/* Error */
@@ -166,7 +166,7 @@ 
 #define BGMAC_RX_LEN_1024_TO_1522		0x3e4
 #define BGMAC_RX_LEN_1523_TO_2047		0x3e8
 #define BGMAC_RX_LEN_2048_TO_4095		0x3ec
-#define BGMAC_RX_LEN_4095_TO_8191		0x3f0
+#define BGMAC_RX_LEN_4096_TO_8191		0x3f0
 #define BGMAC_RX_LEN_8192_TO_MAX		0x3f4
 #define BGMAC_RX_JABBER_PKTS			0x3f8		/* Error */
 #define BGMAC_RX_OVERSIZE_PKTS			0x3fc		/* Error */