[PATCH v2 net-next 0/6] net: dsa: mv88e6xxx: Add "eth-mac" and "rmon" counter group support

[PATCH v2 net-next 0/6] net: dsa: mv88e6xxx: Add "eth-mac" and "rmon" counter group support

[Tobias Waldekranz]

The majority of the changes (2/6) are about refactoring the existing
ethtool statistics support to make it possible to read individual
counters, rather than the whole set.

4/6 tries to collect all information about a stat in a single place
using a mapper macro, which is then used to generate the original list
of stats, along with a matching enum. checkpatch is less than amused
with this construct, but prior art exists (__BPF_FUNC_MAPPER in
include/uapi/linux/bpf.h, for example).

With that in place, adding the actual counter groups is pretty
straight forward (5-6/6).

v1 -> v2:
- Added 1/6
- Added 3/6
- Changed prototype of stats operation to reflect the fact that the
  number of read stats are returned, no errors
- Moved comma into MV88E6XXX_HW_STAT_MAPPER definition
- Avoid the construction of mapping table iteration which relied on
  struct layouts outside of mv88e6xxx's control

Tobias Waldekranz (6):
  net: dsa: mv88e6xxx: Push locking into stats snapshotting
  net: dsa: mv88e6xxx: Create API to read a single stat counter
  net: dsa: mv88e6xxx: Fix mv88e6352_serdes_get_stats error path
  net: dsa: mv88e6xxx: Give each hw stat an ID
  net: dsa: mv88e6xxx: Add "eth-mac" counter group support
  net: dsa: mv88e6xxx: Add "rmon" counter group support

 drivers/net/dsa/mv88e6xxx/chip.c   | 390 +++++++++++++++++++----------
 drivers/net/dsa/mv88e6xxx/chip.h   |  31 +--
 drivers/net/dsa/mv88e6xxx/serdes.c |  10 +-
 drivers/net/dsa/mv88e6xxx/serdes.h |   8 +-
 4 files changed, 278 insertions(+), 161 deletions(-)

-- 
2.34.1

[PATCH v2 net-next 1/6] net: dsa: mv88e6xxx: Push locking into stats snapshotting

[Tobias Waldekranz]

This is more consistent with the driver's general structure.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 42b1acaca33a..95a9dbd60aa6 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -931,10 +931,16 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
 
 static int mv88e6xxx_stats_snapshot(struct mv88e6xxx_chip *chip, int port)
 {
+	int err;
+
 	if (!chip->info->ops->stats_snapshot)
 		return -EOPNOTSUPP;
 
-	return chip->info->ops->stats_snapshot(chip, port);
+	mv88e6xxx_reg_lock(chip);
+	err = chip->info->ops->stats_snapshot(chip, port);
+	mv88e6xxx_reg_unlock(chip);
+
+	return err;
 }
 
 static struct mv88e6xxx_hw_stat mv88e6xxx_hw_stats[] = {
@@ -1272,16 +1278,11 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int ret;
 
-	mv88e6xxx_reg_lock(chip);
-
 	ret = mv88e6xxx_stats_snapshot(chip, port);
-	mv88e6xxx_reg_unlock(chip);
-
 	if (ret < 0)
 		return;
 
 	mv88e6xxx_get_stats(chip, port, data);
-
 }
 
 static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
-- 
2.34.1

Re: [PATCH v2 net-next 1/6] net: dsa: mv88e6xxx: Push locking into stats snapshotting

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 05:04:13PM +0100, Tobias Waldekranz wrote:
> This is more consistent with the driver's general structure.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH v2 net-next 2/6] net: dsa: mv88e6xxx: Create API to read a single stat counter

[Tobias Waldekranz]

This change contains no functional change. We simply push the hardware
specific stats logic to a function reading a single counter, rather
than the whole set.

This is a preparatory change for the upcoming standard ethtool
statistics support (i.e. "eth-mac", "eth-ctrl" etc.).

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 160 ++++++++++++++++++-------------
 drivers/net/dsa/mv88e6xxx/chip.h |  27 +++---
 2 files changed, 105 insertions(+), 82 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 95a9dbd60aa6..bb18b47de49c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1006,7 +1006,7 @@ static struct mv88e6xxx_hw_stat mv88e6xxx_hw_stats[] = {
 };
 
 static uint64_t _mv88e6xxx_get_ethtool_stat(struct mv88e6xxx_chip *chip,
-					    struct mv88e6xxx_hw_stat *s,
+					    const struct mv88e6xxx_hw_stat *s,
 					    int port, u16 bank1_select,
 					    u16 histogram)
 {
@@ -1189,59 +1189,82 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port, int sset)
 	return count;
 }
 
-static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
-				     uint64_t *data, int types,
-				     u16 bank1_select, u16 histogram)
+static size_t mv88e6095_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
+				       const struct mv88e6xxx_hw_stat *stat,
+				       uint64_t *data)
 {
-	struct mv88e6xxx_hw_stat *stat;
-	int i, j;
+	if (!(stat->type & (STATS_TYPE_BANK0 | STATS_TYPE_PORT)))
+		return 0;
 
-	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
-		stat = &mv88e6xxx_hw_stats[i];
-		if (stat->type & types) {
-			mv88e6xxx_reg_lock(chip);
-			data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
-							      bank1_select,
-							      histogram);
-			mv88e6xxx_reg_unlock(chip);
+	*data = _mv88e6xxx_get_ethtool_stat(chip, stat, port, 0,
+					    MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+	return 1;
+}
 
-			j++;
-		}
-	}
-	return j;
+static size_t mv88e6250_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
+				       const struct mv88e6xxx_hw_stat *stat,
+				       uint64_t *data)
+{
+	if (!(stat->type & STATS_TYPE_BANK0))
+		return 0;
+
+	*data = _mv88e6xxx_get_ethtool_stat(chip, stat, port, 0,
+					    MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+	return 1;
 }
 
-static int mv88e6095_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
-				     uint64_t *data)
+static size_t mv88e6320_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
+				       const struct mv88e6xxx_hw_stat *stat,
+				       uint64_t *data)
 {
-	return mv88e6xxx_stats_get_stats(chip, port, data,
-					 STATS_TYPE_BANK0 | STATS_TYPE_PORT,
-					 0, MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+	if (!(stat->type & (STATS_TYPE_BANK0 | STATS_TYPE_BANK1)))
+		return 0;
+
+	*data = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
+					    MV88E6XXX_G1_STATS_OP_BANK_1_BIT_9,
+					    MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+	return 1;
 }
 
-static int mv88e6250_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
-				     uint64_t *data)
+static size_t mv88e6390_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
+				       const struct mv88e6xxx_hw_stat *stat,
+				       uint64_t *data)
 {
-	return mv88e6xxx_stats_get_stats(chip, port, data, STATS_TYPE_BANK0,
-					 0, MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+	if (!(stat->type & (STATS_TYPE_BANK0 | STATS_TYPE_BANK1)))
+		return 0;
+
+	*data = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
+					    MV88E6XXX_G1_STATS_OP_BANK_1_BIT_10,
+					    0);
+	return 1;
 }
 
-static int mv88e6320_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
-				     uint64_t *data)
+static size_t mv88e6xxx_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
+				       const struct mv88e6xxx_hw_stat *stat,
+				       uint64_t *data)
 {
-	return mv88e6xxx_stats_get_stats(chip, port, data,
-					 STATS_TYPE_BANK0 | STATS_TYPE_BANK1,
-					 MV88E6XXX_G1_STATS_OP_BANK_1_BIT_9,
-					 MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+	int ret = 0;
+
+	if (chip->info->ops->stats_get_stat) {
+		mv88e6xxx_reg_lock(chip);
+		ret = chip->info->ops->stats_get_stat(chip, port, stat, data);
+		mv88e6xxx_reg_unlock(chip);
+	}
+
+	return ret;
 }
 
-static int mv88e6390_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
+static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 				     uint64_t *data)
 {
-	return mv88e6xxx_stats_get_stats(chip, port, data,
-					 STATS_TYPE_BANK0 | STATS_TYPE_BANK1,
-					 MV88E6XXX_G1_STATS_OP_BANK_1_BIT_10,
-					 0);
+	struct mv88e6xxx_hw_stat *stat;
+	size_t i, j;
+
+	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
+		stat = &mv88e6xxx_hw_stats[i];
+		j += mv88e6xxx_stats_get_stat(chip, port, stat, &data[j]);
+	}
+	return j;
 }
 
 static void mv88e6xxx_atu_vtu_get_stats(struct mv88e6xxx_chip *chip, int port,
@@ -1257,10 +1280,9 @@ static void mv88e6xxx_atu_vtu_get_stats(struct mv88e6xxx_chip *chip, int port,
 static void mv88e6xxx_get_stats(struct mv88e6xxx_chip *chip, int port,
 				uint64_t *data)
 {
-	int count = 0;
+	size_t count;
 
-	if (chip->info->ops->stats_get_stats)
-		count = chip->info->ops->stats_get_stats(chip, port, data);
+	count = mv88e6xxx_stats_get_stats(chip, port, data);
 
 	mv88e6xxx_reg_lock(chip);
 	if (chip->info->ops->serdes_get_stats) {
@@ -3974,7 +3996,7 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4012,7 +4034,7 @@ static const struct mv88e6xxx_ops mv88e6095_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.mgmt_rsvd2cpu = mv88e6185_g2_mgmt_rsvd2cpu,
 	.ppu_enable = mv88e6185_g1_ppu_enable,
 	.ppu_disable = mv88e6185_g1_ppu_disable,
@@ -4053,7 +4075,7 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4095,7 +4117,7 @@ static const struct mv88e6xxx_ops mv88e6123_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4138,7 +4160,7 @@ static const struct mv88e6xxx_ops mv88e6131_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4187,7 +4209,7 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	.set_cpu_port = mv88e6390_g1_set_cpu_port,
 	.set_egress_port = mv88e6390_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -4242,7 +4264,7 @@ static const struct mv88e6xxx_ops mv88e6161_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4280,7 +4302,7 @@ static const struct mv88e6xxx_ops mv88e6165_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4328,7 +4350,7 @@ static const struct mv88e6xxx_ops mv88e6171_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4377,7 +4399,7 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4428,7 +4450,7 @@ static const struct mv88e6xxx_ops mv88e6175_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4477,7 +4499,7 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4522,7 +4544,7 @@ static const struct mv88e6xxx_ops mv88e6185_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4571,7 +4593,7 @@ static const struct mv88e6xxx_ops mv88e6190_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	.set_cpu_port = mv88e6390_g1_set_cpu_port,
 	.set_egress_port = mv88e6390_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -4629,7 +4651,7 @@ static const struct mv88e6xxx_ops mv88e6190x_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	.set_cpu_port = mv88e6390_g1_set_cpu_port,
 	.set_egress_port = mv88e6390_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -4685,7 +4707,7 @@ static const struct mv88e6xxx_ops mv88e6191_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	.set_cpu_port = mv88e6390_g1_set_cpu_port,
 	.set_egress_port = mv88e6390_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -4744,7 +4766,7 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -4797,7 +4819,7 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6250_stats_get_sset_count,
 	.stats_get_strings = mv88e6250_stats_get_strings,
-	.stats_get_stats = mv88e6250_stats_get_stats,
+	.stats_get_stat = mv88e6250_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6250_watchdog_ops,
@@ -4844,7 +4866,7 @@ static const struct mv88e6xxx_ops mv88e6290_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	.set_cpu_port = mv88e6390_g1_set_cpu_port,
 	.set_egress_port = mv88e6390_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -4903,7 +4925,7 @@ static const struct mv88e6xxx_ops mv88e6320_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6320_stats_get_stats,
+	.stats_get_stat = mv88e6320_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -4950,7 +4972,7 @@ static const struct mv88e6xxx_ops mv88e6321_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6320_stats_get_stats,
+	.stats_get_stat = mv88e6320_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -4999,7 +5021,7 @@ static const struct mv88e6xxx_ops mv88e6341_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	.set_cpu_port = mv88e6390_g1_set_cpu_port,
 	.set_egress_port = mv88e6390_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -5057,7 +5079,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -5103,7 +5125,7 @@ static const struct mv88e6xxx_ops mv88e6351_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -5154,7 +5176,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
 	.stats_get_strings = mv88e6095_stats_get_strings,
-	.stats_get_stats = mv88e6095_stats_get_stats,
+	.stats_get_stat = mv88e6095_stats_get_stat,
 	.set_cpu_port = mv88e6095_g1_set_cpu_port,
 	.set_egress_port = mv88e6095_g1_set_egress_port,
 	.watchdog_ops = &mv88e6097_watchdog_ops,
@@ -5216,7 +5238,7 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	.set_cpu_port = mv88e6390_g1_set_cpu_port,
 	.set_egress_port = mv88e6390_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -5278,7 +5300,7 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	.set_cpu_port = mv88e6390_g1_set_cpu_port,
 	.set_egress_port = mv88e6390_g1_set_egress_port,
 	.watchdog_ops = &mv88e6390_watchdog_ops,
@@ -5340,7 +5362,7 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 	.stats_set_histogram = mv88e6390_g1_stats_set_histogram,
 	.stats_get_sset_count = mv88e6320_stats_get_sset_count,
 	.stats_get_strings = mv88e6320_stats_get_strings,
-	.stats_get_stats = mv88e6390_stats_get_stats,
+	.stats_get_stat = mv88e6390_stats_get_stat,
 	/* .set_cpu_port is missing because this family does not support a global
 	 * CPU port, only per port CPU port which is set via
 	 * .port_set_upstream_port method.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 44383a03ef2f..c3c53ef543e5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -318,6 +318,17 @@ struct mv88e6xxx_mst {
 	struct mv88e6xxx_stu_entry stu;
 };
 
+#define STATS_TYPE_PORT		BIT(0)
+#define STATS_TYPE_BANK0	BIT(1)
+#define STATS_TYPE_BANK1	BIT(2)
+
+struct mv88e6xxx_hw_stat {
+	char string[ETH_GSTRING_LEN];
+	size_t size;
+	int reg;
+	int type;
+};
+
 struct mv88e6xxx_chip {
 	const struct mv88e6xxx_info *info;
 
@@ -574,8 +585,9 @@ struct mv88e6xxx_ops {
 	/* Return the number of strings describing statistics */
 	int (*stats_get_sset_count)(struct mv88e6xxx_chip *chip);
 	int (*stats_get_strings)(struct mv88e6xxx_chip *chip,  uint8_t *data);
-	int (*stats_get_stats)(struct mv88e6xxx_chip *chip,  int port,
-			       uint64_t *data);
+	size_t (*stats_get_stat)(struct mv88e6xxx_chip *chip, int port,
+				 const struct mv88e6xxx_hw_stat *stat,
+				 uint64_t *data);
 	int (*set_cpu_port)(struct mv88e6xxx_chip *chip, int port);
 	int (*set_egress_port)(struct mv88e6xxx_chip *chip,
 			       enum mv88e6xxx_egress_direction direction,
@@ -727,17 +739,6 @@ struct mv88e6xxx_pcs_ops {
 
 };
 
-#define STATS_TYPE_PORT		BIT(0)
-#define STATS_TYPE_BANK0	BIT(1)
-#define STATS_TYPE_BANK1	BIT(2)
-
-struct mv88e6xxx_hw_stat {
-	char string[ETH_GSTRING_LEN];
-	size_t size;
-	int reg;
-	int type;
-};
-
 static inline bool mv88e6xxx_has_stu(struct mv88e6xxx_chip *chip)
 {
 	return chip->info->max_sid > 0 &&
-- 
2.34.1

Re: [PATCH v2 net-next 2/6] net: dsa: mv88e6xxx: Create API to read a single stat counter

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 05:04:14PM +0100, Tobias Waldekranz wrote:
> This change contains no functional change. We simply push the hardware
> specific stats logic to a function reading a single counter, rather
> than the whole set.
> 
> This is a preparatory change for the upcoming standard ethtool
> statistics support (i.e. "eth-mac", "eth-ctrl" etc.).
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH v2 net-next 3/6] net: dsa: mv88e6xxx: Fix mv88e6352_serdes_get_stats error path

[Tobias Waldekranz]

mv88e6xxx_get_stats, which collects stats from various sources,
expects all callees to return the number of stats read. If an error
occurs, 0 should be returned.

Prevent future mishaps of this kind by updating the return type to
reflect this contract.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.h   |  4 ++--
 drivers/net/dsa/mv88e6xxx/serdes.c | 10 +++++-----
 drivers/net/dsa/mv88e6xxx/serdes.h |  8 ++++----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index c3c53ef543e5..85eb293381a7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -613,8 +613,8 @@ struct mv88e6xxx_ops {
 	int (*serdes_get_sset_count)(struct mv88e6xxx_chip *chip, int port);
 	int (*serdes_get_strings)(struct mv88e6xxx_chip *chip,  int port,
 				  uint8_t *data);
-	int (*serdes_get_stats)(struct mv88e6xxx_chip *chip,  int port,
-				uint64_t *data);
+	size_t (*serdes_get_stats)(struct mv88e6xxx_chip *chip, int port,
+				   uint64_t *data);
 
 	/* SERDES registers for ethtool */
 	int (*serdes_get_regs_len)(struct mv88e6xxx_chip *chip,  int port);
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 3b4b42651fa3..01ea53940786 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -177,8 +177,8 @@ static uint64_t mv88e6352_serdes_get_stat(struct mv88e6xxx_chip *chip,
 	return val;
 }
 
-int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
-			       uint64_t *data)
+size_t mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
+				  uint64_t *data)
 {
 	struct mv88e6xxx_port *mv88e6xxx_port = &chip->ports[port];
 	struct mv88e6352_serdes_hw_stat *stat;
@@ -187,7 +187,7 @@ int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
 
 	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
 	if (err <= 0)
-		return err;
+		return 0;
 
 	BUILD_BUG_ON(ARRAY_SIZE(mv88e6352_serdes_hw_stats) >
 		     ARRAY_SIZE(mv88e6xxx_port->serdes_stats));
@@ -429,8 +429,8 @@ static uint64_t mv88e6390_serdes_get_stat(struct mv88e6xxx_chip *chip, int lane,
 	return reg[0] | ((u64)reg[1] << 16) | ((u64)reg[2] << 32);
 }
 
-int mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
-			       uint64_t *data)
+size_t mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
+				  uint64_t *data)
 {
 	struct mv88e6390_serdes_hw_stat *stat;
 	int lane;
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index aac95cab46e3..ff5c3ab31e15 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -127,13 +127,13 @@ unsigned int mv88e6390_serdes_irq_mapping(struct mv88e6xxx_chip *chip,
 int mv88e6352_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port);
 int mv88e6352_serdes_get_strings(struct mv88e6xxx_chip *chip,
 				 int port, uint8_t *data);
-int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
-			       uint64_t *data);
+size_t mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
+				  uint64_t *data);
 int mv88e6390_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port);
 int mv88e6390_serdes_get_strings(struct mv88e6xxx_chip *chip,
 				 int port, uint8_t *data);
-int mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
-			       uint64_t *data);
+size_t mv88e6390_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
+				  uint64_t *data);
 
 int mv88e6352_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port);
 void mv88e6352_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p);
-- 
2.34.1

Re: [PATCH v2 net-next 3/6] net: dsa: mv88e6xxx: Fix mv88e6352_serdes_get_stats error path

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 05:04:15PM +0100, Tobias Waldekranz wrote:
> mv88e6xxx_get_stats, which collects stats from various sources,
> expects all callees to return the number of stats read. If an error
> occurs, 0 should be returned.
> 
> Prevent future mishaps of this kind by updating the return type to
> reflect this contract.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
> diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
> index 3b4b42651fa3..01ea53940786 100644
> --- a/drivers/net/dsa/mv88e6xxx/serdes.c
> +++ b/drivers/net/dsa/mv88e6xxx/serdes.c
> @@ -187,7 +187,7 @@ int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
>  
>  	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
>  	if (err <= 0)
> -		return err;
> +		return 0;

Ok, you're saying we don't care enough about handling the catastrophic
event where an MDIO access error takes place in mv88e6xxx_g2_scratch_read()
to submit this to "stable".

I guess the impact in such a case is that the error (interpreted as negative
count) makes us go back by -EIO (5) entries or whatever into the "data"
array provided to user space, overwriting some previous stats and making
everything after the failed counter minus the error code be reported in
the wrong place relative to its string. I don't think that the error
codes are high enough to overcome the ~60 port stats and cause memory
accesses behind the "data" array.

Anyway, for the patch content:

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH v2 net-next 3/6] net: dsa: mv88e6xxx: Fix mv88e6352_serdes_get_stats error path

[Tobias Waldekranz]

On tis, dec 05, 2023 at 19:50, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Tue, Dec 05, 2023 at 05:04:15PM +0100, Tobias Waldekranz wrote:
>> mv88e6xxx_get_stats, which collects stats from various sources,
>> expects all callees to return the number of stats read. If an error
>> occurs, 0 should be returned.
>> 
>> Prevent future mishaps of this kind by updating the return type to
>> reflect this contract.
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> ---
>> diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
>> index 3b4b42651fa3..01ea53940786 100644
>> --- a/drivers/net/dsa/mv88e6xxx/serdes.c
>> +++ b/drivers/net/dsa/mv88e6xxx/serdes.c
>> @@ -187,7 +187,7 @@ int mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
>>  
>>  	err = mv88e6352_g2_scratch_port_has_serdes(chip, port);
>>  	if (err <= 0)
>> -		return err;
>> +		return 0;
>
> Ok, you're saying we don't care enough about handling the catastrophic
> event where an MDIO access error takes place in mv88e6xxx_g2_scratch_read()
> to submit this to "stable".

It just felt like one of those theoretical bugs that, if you were to hit
it, you most likely have way bigger issues than not getting at your
SERDES counters; and since, as you say...

> I guess the impact in such a case is that the error (interpreted as negative
> count) makes us go back by -EIO (5) entries or whatever into the "data"
> array provided to user space, overwriting some previous stats and making
> everything after the failed counter minus the error code be reported in
> the wrong place relative to its string. I don't think that the error
> codes are high enough to overcome the ~60 port stats and cause memory
> accesses behind the "data" array.

...the potential for data corruption seems low. But I could send a v3
and split this into one change that only fixes the return value (which
could go into -net), and another one that changes the type. Do you think
it's worth it?

Re: [PATCH v2 net-next 3/6] net: dsa: mv88e6xxx: Fix mv88e6352_serdes_get_stats error path

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 10:13:12PM +0100, Tobias Waldekranz wrote:
> > Ok, you're saying we don't care enough about handling the catastrophic
> > event where an MDIO access error takes place in mv88e6xxx_g2_scratch_read()
> > to submit this to "stable".
> 
> It just felt like one of those theoretical bugs that, if you were to hit
> it, you most likely have way bigger issues than not getting at your
> SERDES counters; and since, as you say...
> 
> > I guess the impact in such a case is that the error (interpreted as negative
> > count) makes us go back by -EIO (5) entries or whatever into the "data"
> > array provided to user space, overwriting some previous stats and making
> > everything after the failed counter minus the error code be reported in
> > the wrong place relative to its string. I don't think that the error
> > codes are high enough to overcome the ~60 port stats and cause memory
> > accesses behind the "data" array.
> 
> ...the potential for data corruption seems low. But I could send a v3
> and split this into one change that only fixes the return value (which
> could go into -net), and another one that changes the type. Do you think
> it's worth it?

Reading Documentation/process/stable-kernel-rules.rst, I think that
consistent error checking for register access on a non-hotpluggable bus
is the type of bug fix that is exceedingly unlikely to have any measurable
impact on end users, so it might not even qualify for "net".

To me, this is good enough. Let's spend our time doing meaningful things,
while also keeping the material for "net.git" meaningful.

[PATCH v2 net-next 4/6] net: dsa: mv88e6xxx: Give each hw stat an ID

[Tobias Waldekranz]

With the upcoming standard counter group support, we are no longer
reading out the whole set of counters, but rather mapping a subset to
the requested group.

Therefore, create an enum with an ID for each stat, such that
mv88e6xxx_hw_stats[] can be subscripted with a human-readable ID
corresponding to the counter's name.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 138 +++++++++++++++++--------------
 1 file changed, 75 insertions(+), 63 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index bb18b47de49c..473f31761b26 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -943,66 +943,78 @@ static int mv88e6xxx_stats_snapshot(struct mv88e6xxx_chip *chip, int port)
 	return err;
 }
 
-static struct mv88e6xxx_hw_stat mv88e6xxx_hw_stats[] = {
-	{ "in_good_octets",		8, 0x00, STATS_TYPE_BANK0, },
-	{ "in_bad_octets",		4, 0x02, STATS_TYPE_BANK0, },
-	{ "in_unicast",			4, 0x04, STATS_TYPE_BANK0, },
-	{ "in_broadcasts",		4, 0x06, STATS_TYPE_BANK0, },
-	{ "in_multicasts",		4, 0x07, STATS_TYPE_BANK0, },
-	{ "in_pause",			4, 0x16, STATS_TYPE_BANK0, },
-	{ "in_undersize",		4, 0x18, STATS_TYPE_BANK0, },
-	{ "in_fragments",		4, 0x19, STATS_TYPE_BANK0, },
-	{ "in_oversize",		4, 0x1a, STATS_TYPE_BANK0, },
-	{ "in_jabber",			4, 0x1b, STATS_TYPE_BANK0, },
-	{ "in_rx_error",		4, 0x1c, STATS_TYPE_BANK0, },
-	{ "in_fcs_error",		4, 0x1d, STATS_TYPE_BANK0, },
-	{ "out_octets",			8, 0x0e, STATS_TYPE_BANK0, },
-	{ "out_unicast",		4, 0x10, STATS_TYPE_BANK0, },
-	{ "out_broadcasts",		4, 0x13, STATS_TYPE_BANK0, },
-	{ "out_multicasts",		4, 0x12, STATS_TYPE_BANK0, },
-	{ "out_pause",			4, 0x15, STATS_TYPE_BANK0, },
-	{ "excessive",			4, 0x11, STATS_TYPE_BANK0, },
-	{ "collisions",			4, 0x1e, STATS_TYPE_BANK0, },
-	{ "deferred",			4, 0x05, STATS_TYPE_BANK0, },
-	{ "single",			4, 0x14, STATS_TYPE_BANK0, },
-	{ "multiple",			4, 0x17, STATS_TYPE_BANK0, },
-	{ "out_fcs_error",		4, 0x03, STATS_TYPE_BANK0, },
-	{ "late",			4, 0x1f, STATS_TYPE_BANK0, },
-	{ "hist_64bytes",		4, 0x08, STATS_TYPE_BANK0, },
-	{ "hist_65_127bytes",		4, 0x09, STATS_TYPE_BANK0, },
-	{ "hist_128_255bytes",		4, 0x0a, STATS_TYPE_BANK0, },
-	{ "hist_256_511bytes",		4, 0x0b, STATS_TYPE_BANK0, },
-	{ "hist_512_1023bytes",		4, 0x0c, STATS_TYPE_BANK0, },
-	{ "hist_1024_max_bytes",	4, 0x0d, STATS_TYPE_BANK0, },
-	{ "sw_in_discards",		4, 0x10, STATS_TYPE_PORT, },
-	{ "sw_in_filtered",		2, 0x12, STATS_TYPE_PORT, },
-	{ "sw_out_filtered",		2, 0x13, STATS_TYPE_PORT, },
-	{ "in_discards",		4, 0x00, STATS_TYPE_BANK1, },
-	{ "in_filtered",		4, 0x01, STATS_TYPE_BANK1, },
-	{ "in_accepted",		4, 0x02, STATS_TYPE_BANK1, },
-	{ "in_bad_accepted",		4, 0x03, STATS_TYPE_BANK1, },
-	{ "in_good_avb_class_a",	4, 0x04, STATS_TYPE_BANK1, },
-	{ "in_good_avb_class_b",	4, 0x05, STATS_TYPE_BANK1, },
-	{ "in_bad_avb_class_a",		4, 0x06, STATS_TYPE_BANK1, },
-	{ "in_bad_avb_class_b",		4, 0x07, STATS_TYPE_BANK1, },
-	{ "tcam_counter_0",		4, 0x08, STATS_TYPE_BANK1, },
-	{ "tcam_counter_1",		4, 0x09, STATS_TYPE_BANK1, },
-	{ "tcam_counter_2",		4, 0x0a, STATS_TYPE_BANK1, },
-	{ "tcam_counter_3",		4, 0x0b, STATS_TYPE_BANK1, },
-	{ "in_da_unknown",		4, 0x0e, STATS_TYPE_BANK1, },
-	{ "in_management",		4, 0x0f, STATS_TYPE_BANK1, },
-	{ "out_queue_0",		4, 0x10, STATS_TYPE_BANK1, },
-	{ "out_queue_1",		4, 0x11, STATS_TYPE_BANK1, },
-	{ "out_queue_2",		4, 0x12, STATS_TYPE_BANK1, },
-	{ "out_queue_3",		4, 0x13, STATS_TYPE_BANK1, },
-	{ "out_queue_4",		4, 0x14, STATS_TYPE_BANK1, },
-	{ "out_queue_5",		4, 0x15, STATS_TYPE_BANK1, },
-	{ "out_queue_6",		4, 0x16, STATS_TYPE_BANK1, },
-	{ "out_queue_7",		4, 0x17, STATS_TYPE_BANK1, },
-	{ "out_cut_through",		4, 0x18, STATS_TYPE_BANK1, },
-	{ "out_octets_a",		4, 0x1a, STATS_TYPE_BANK1, },
-	{ "out_octets_b",		4, 0x1b, STATS_TYPE_BANK1, },
-	{ "out_management",		4, 0x1f, STATS_TYPE_BANK1, },
+#define MV88E6XXX_HW_STAT_MAPPER(_fn)				    \
+	_fn(in_good_octets,		8, 0x00, STATS_TYPE_BANK0), \
+	_fn(in_bad_octets,		4, 0x02, STATS_TYPE_BANK0), \
+	_fn(in_unicast,			4, 0x04, STATS_TYPE_BANK0), \
+	_fn(in_broadcasts,		4, 0x06, STATS_TYPE_BANK0), \
+	_fn(in_multicasts,		4, 0x07, STATS_TYPE_BANK0), \
+	_fn(in_pause,			4, 0x16, STATS_TYPE_BANK0), \
+	_fn(in_undersize,		4, 0x18, STATS_TYPE_BANK0), \
+	_fn(in_fragments,		4, 0x19, STATS_TYPE_BANK0), \
+	_fn(in_oversize,		4, 0x1a, STATS_TYPE_BANK0), \
+	_fn(in_jabber,			4, 0x1b, STATS_TYPE_BANK0), \
+	_fn(in_rx_error,		4, 0x1c, STATS_TYPE_BANK0), \
+	_fn(in_fcs_error,		4, 0x1d, STATS_TYPE_BANK0), \
+	_fn(out_octets,			8, 0x0e, STATS_TYPE_BANK0), \
+	_fn(out_unicast,		4, 0x10, STATS_TYPE_BANK0), \
+	_fn(out_broadcasts,		4, 0x13, STATS_TYPE_BANK0), \
+	_fn(out_multicasts,		4, 0x12, STATS_TYPE_BANK0), \
+	_fn(out_pause,			4, 0x15, STATS_TYPE_BANK0), \
+	_fn(excessive,			4, 0x11, STATS_TYPE_BANK0), \
+	_fn(collisions,			4, 0x1e, STATS_TYPE_BANK0), \
+	_fn(deferred,			4, 0x05, STATS_TYPE_BANK0), \
+	_fn(single,			4, 0x14, STATS_TYPE_BANK0), \
+	_fn(multiple,			4, 0x17, STATS_TYPE_BANK0), \
+	_fn(out_fcs_error,		4, 0x03, STATS_TYPE_BANK0), \
+	_fn(late,			4, 0x1f, STATS_TYPE_BANK0), \
+	_fn(hist_64bytes,		4, 0x08, STATS_TYPE_BANK0), \
+	_fn(hist_65_127bytes,		4, 0x09, STATS_TYPE_BANK0), \
+	_fn(hist_128_255bytes,		4, 0x0a, STATS_TYPE_BANK0), \
+	_fn(hist_256_511bytes,		4, 0x0b, STATS_TYPE_BANK0), \
+	_fn(hist_512_1023bytes,		4, 0x0c, STATS_TYPE_BANK0), \
+	_fn(hist_1024_max_bytes,	4, 0x0d, STATS_TYPE_BANK0), \
+	_fn(sw_in_discards,		4, 0x10, STATS_TYPE_PORT), \
+	_fn(sw_in_filtered,		2, 0x12, STATS_TYPE_PORT), \
+	_fn(sw_out_filtered,		2, 0x13, STATS_TYPE_PORT), \
+	_fn(in_discards,		4, 0x00, STATS_TYPE_BANK1), \
+	_fn(in_filtered,		4, 0x01, STATS_TYPE_BANK1), \
+	_fn(in_accepted,		4, 0x02, STATS_TYPE_BANK1), \
+	_fn(in_bad_accepted,		4, 0x03, STATS_TYPE_BANK1), \
+	_fn(in_good_avb_class_a,	4, 0x04, STATS_TYPE_BANK1), \
+	_fn(in_good_avb_class_b,	4, 0x05, STATS_TYPE_BANK1), \
+	_fn(in_bad_avb_class_a,		4, 0x06, STATS_TYPE_BANK1), \
+	_fn(in_bad_avb_class_b,		4, 0x07, STATS_TYPE_BANK1), \
+	_fn(tcam_counter_0,		4, 0x08, STATS_TYPE_BANK1), \
+	_fn(tcam_counter_1,		4, 0x09, STATS_TYPE_BANK1), \
+	_fn(tcam_counter_2,		4, 0x0a, STATS_TYPE_BANK1), \
+	_fn(tcam_counter_3,		4, 0x0b, STATS_TYPE_BANK1), \
+	_fn(in_da_unknown,		4, 0x0e, STATS_TYPE_BANK1), \
+	_fn(in_management,		4, 0x0f, STATS_TYPE_BANK1), \
+	_fn(out_queue_0,		4, 0x10, STATS_TYPE_BANK1), \
+	_fn(out_queue_1,		4, 0x11, STATS_TYPE_BANK1), \
+	_fn(out_queue_2,		4, 0x12, STATS_TYPE_BANK1), \
+	_fn(out_queue_3,		4, 0x13, STATS_TYPE_BANK1), \
+	_fn(out_queue_4,		4, 0x14, STATS_TYPE_BANK1), \
+	_fn(out_queue_5,		4, 0x15, STATS_TYPE_BANK1), \
+	_fn(out_queue_6,		4, 0x16, STATS_TYPE_BANK1), \
+	_fn(out_queue_7,		4, 0x17, STATS_TYPE_BANK1), \
+	_fn(out_cut_through,		4, 0x18, STATS_TYPE_BANK1), \
+	_fn(out_octets_a,		4, 0x1a, STATS_TYPE_BANK1), \
+	_fn(out_octets_b,		4, 0x1b, STATS_TYPE_BANK1), \
+	_fn(out_management,		4, 0x1f, STATS_TYPE_BANK1), \
+	/*  */
+
+#define MV88E6XXX_HW_STAT_ENTRY(_string, _size, _reg, _type) \
+	{ #_string, _size, _reg, _type }
+static const struct mv88e6xxx_hw_stat mv88e6xxx_hw_stats[] = {
+	MV88E6XXX_HW_STAT_MAPPER(MV88E6XXX_HW_STAT_ENTRY)
+};
+
+#define MV88E6XXX_HW_STAT_ENUM(_string, _size, _reg, _type) \
+	MV88E6XXX_HW_STAT_ID_ ## _string
+enum mv88e6xxx_hw_stat_id {
+	MV88E6XXX_HW_STAT_MAPPER(MV88E6XXX_HW_STAT_ENUM)
 };
 
 static uint64_t _mv88e6xxx_get_ethtool_stat(struct mv88e6xxx_chip *chip,
@@ -1049,7 +1061,7 @@ static uint64_t _mv88e6xxx_get_ethtool_stat(struct mv88e6xxx_chip *chip,
 static int mv88e6xxx_stats_get_strings(struct mv88e6xxx_chip *chip,
 				       uint8_t *data, int types)
 {
-	struct mv88e6xxx_hw_stat *stat;
+	const struct mv88e6xxx_hw_stat *stat;
 	int i, j;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
@@ -1130,7 +1142,7 @@ static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
 static int mv88e6xxx_stats_get_sset_count(struct mv88e6xxx_chip *chip,
 					  int types)
 {
-	struct mv88e6xxx_hw_stat *stat;
+	const struct mv88e6xxx_hw_stat *stat;
 	int i, j;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
@@ -1257,7 +1269,7 @@ static size_t mv88e6xxx_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
 static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 				     uint64_t *data)
 {
-	struct mv88e6xxx_hw_stat *stat;
+	const struct mv88e6xxx_hw_stat *stat;
 	size_t i, j;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
-- 
2.34.1

Re: [PATCH v2 net-next 4/6] net: dsa: mv88e6xxx: Give each hw stat an ID

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 05:04:16PM +0100, Tobias Waldekranz wrote:
> With the upcoming standard counter group support, we are no longer
> reading out the whole set of counters, but rather mapping a subset to
> the requested group.
> 
> Therefore, create an enum with an ID for each stat, such that
> mv88e6xxx_hw_stats[] can be subscripted with a human-readable ID
> corresponding to the counter's name.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH v2 net-next 5/6] net: dsa: mv88e6xxx: Add "eth-mac" counter group support

[Tobias Waldekranz]

Report the applicable subset of an mv88e6xxx port's counters using
ethtool's standardized "eth-mac" counter group.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 39 ++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 473f31761b26..1a16698181fb 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1319,6 +1319,44 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
 	mv88e6xxx_get_stats(chip, port, data);
 }
 
+static void mv88e6xxx_get_eth_mac_stats(struct dsa_switch *ds, int port,
+					struct ethtool_eth_mac_stats *mac_stats)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int ret;
+
+	ret = mv88e6xxx_stats_snapshot(chip, port);
+	if (ret < 0)
+		return;
+
+#define MV88E6XXX_ETH_MAC_STAT_MAP(_id, _member)			\
+	mv88e6xxx_stats_get_stat(chip, port,				\
+				 &mv88e6xxx_hw_stats[MV88E6XXX_HW_STAT_ID_ ## _id], \
+				 &mac_stats->stats._member)
+
+	MV88E6XXX_ETH_MAC_STAT_MAP(out_unicast, FramesTransmittedOK);
+	MV88E6XXX_ETH_MAC_STAT_MAP(single, SingleCollisionFrames);
+	MV88E6XXX_ETH_MAC_STAT_MAP(multiple, MultipleCollisionFrames);
+	MV88E6XXX_ETH_MAC_STAT_MAP(in_unicast, FramesReceivedOK);
+	MV88E6XXX_ETH_MAC_STAT_MAP(in_fcs_error, FrameCheckSequenceErrors);
+	MV88E6XXX_ETH_MAC_STAT_MAP(out_octets, OctetsTransmittedOK);
+	MV88E6XXX_ETH_MAC_STAT_MAP(deferred, FramesWithDeferredXmissions);
+	MV88E6XXX_ETH_MAC_STAT_MAP(late, LateCollisions);
+	MV88E6XXX_ETH_MAC_STAT_MAP(in_good_octets, OctetsReceivedOK);
+	MV88E6XXX_ETH_MAC_STAT_MAP(out_multicasts, MulticastFramesXmittedOK);
+	MV88E6XXX_ETH_MAC_STAT_MAP(out_broadcasts, BroadcastFramesXmittedOK);
+	MV88E6XXX_ETH_MAC_STAT_MAP(excessive, FramesWithExcessiveDeferral);
+	MV88E6XXX_ETH_MAC_STAT_MAP(in_multicasts, MulticastFramesReceivedOK);
+	MV88E6XXX_ETH_MAC_STAT_MAP(in_broadcasts, BroadcastFramesReceivedOK);
+
+#undef MV88E6XXX_ETH_MAC_STAT_MAP
+
+	mac_stats->stats.FramesTransmittedOK += mac_stats->stats.MulticastFramesXmittedOK;
+	mac_stats->stats.FramesTransmittedOK += mac_stats->stats.BroadcastFramesXmittedOK;
+	mac_stats->stats.FramesReceivedOK += mac_stats->stats.MulticastFramesReceivedOK;
+	mac_stats->stats.FramesReceivedOK += mac_stats->stats.BroadcastFramesReceivedOK;
+}
+
 static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
@@ -6838,6 +6876,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.phylink_mac_link_up	= mv88e6xxx_mac_link_up,
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
+	.get_eth_mac_stats	= mv88e6xxx_get_eth_mac_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
 	.port_max_mtu		= mv88e6xxx_get_max_mtu,
 	.port_change_mtu	= mv88e6xxx_change_mtu,
-- 
2.34.1

Re: [PATCH v2 net-next 5/6] net: dsa: mv88e6xxx: Add "eth-mac" counter group support

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 05:04:17PM +0100, Tobias Waldekranz wrote:
> Report the applicable subset of an mv88e6xxx port's counters using
> ethtool's standardized "eth-mac" counter group.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 39 ++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 473f31761b26..1a16698181fb 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1319,6 +1319,44 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>  	mv88e6xxx_get_stats(chip, port, data);
>  }
>  
> +static void mv88e6xxx_get_eth_mac_stats(struct dsa_switch *ds, int port,
> +					struct ethtool_eth_mac_stats *mac_stats)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int ret;
> +
> +	ret = mv88e6xxx_stats_snapshot(chip, port);
> +	if (ret < 0)
> +		return;
> +
> +#define MV88E6XXX_ETH_MAC_STAT_MAP(_id, _member)			\
> +	mv88e6xxx_stats_get_stat(chip, port,				\
> +				 &mv88e6xxx_hw_stats[MV88E6XXX_HW_STAT_ID_ ## _id], \
> +				 &mac_stats->stats._member)
> +
> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_unicast, FramesTransmittedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(single, SingleCollisionFrames);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(multiple, MultipleCollisionFrames);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_unicast, FramesReceivedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_fcs_error, FrameCheckSequenceErrors);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_octets, OctetsTransmittedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(deferred, FramesWithDeferredXmissions);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(late, LateCollisions);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_good_octets, OctetsReceivedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_multicasts, MulticastFramesXmittedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_broadcasts, BroadcastFramesXmittedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(excessive, FramesWithExcessiveDeferral);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_multicasts, MulticastFramesReceivedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_broadcasts, BroadcastFramesReceivedOK);
> +
> +#undef MV88E6XXX_ETH_MAC_STAT_MAP

I don't exactly enjoy this use (and placement) of the C preprocessor macro
when spelling out code would have worked just fine, but to each his own.
At least it is consistent in that we can jump to the other occurrences
of the statistics counter.

> +
> +	mac_stats->stats.FramesTransmittedOK += mac_stats->stats.MulticastFramesXmittedOK;
> +	mac_stats->stats.FramesTransmittedOK += mac_stats->stats.BroadcastFramesXmittedOK;
> +	mac_stats->stats.FramesReceivedOK += mac_stats->stats.MulticastFramesReceivedOK;
> +	mac_stats->stats.FramesReceivedOK += mac_stats->stats.BroadcastFramesReceivedOK;
> +}

Not sure if there's a "best thing to do" in case a previous mv88e6xxx_stats_get_stat()
call fails. In net/ethtool/stats.c we have ethtool_stats_sum(), and that's the
core saying that U64_MAX means one of the sum terms was not reported by
the driver, and it makes that transparent by simply returning the other.

Here, "not reported by the driver" is due to a bus I/O error, and using
ethtool_stats_sum() as-is would hide that error away completely, and
report only the other sum term. Sounds like a failure that would be too
silent. Whereas your proposal would just report a wildly incorrect
number - but at high data rates (for offloaded traffic, too), maybe that
wouldn't be exactly trivial to notice, either.

Maybe we need a variant of ethtool_stats_sum() that requires both terms,
otherwise returns ETHTOOL_STAT_NOT_SET?

Anyway, this is not a blocker for the current patch set, which is a bit
too large to resend for trivial matters.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH v2 net-next 5/6] net: dsa: mv88e6xxx: Add "eth-mac" counter group support

[Tobias Waldekranz]

On tis, dec 05, 2023 at 20:07, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Tue, Dec 05, 2023 at 05:04:17PM +0100, Tobias Waldekranz wrote:
>> Report the applicable subset of an mv88e6xxx port's counters using
>> ethtool's standardized "eth-mac" counter group.
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c | 39 ++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 473f31761b26..1a16698181fb 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1319,6 +1319,44 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>>  	mv88e6xxx_get_stats(chip, port, data);
>>  }
>>  
>> +static void mv88e6xxx_get_eth_mac_stats(struct dsa_switch *ds, int port,
>> +					struct ethtool_eth_mac_stats *mac_stats)
>> +{
>> +	struct mv88e6xxx_chip *chip = ds->priv;
>> +	int ret;
>> +
>> +	ret = mv88e6xxx_stats_snapshot(chip, port);
>> +	if (ret < 0)
>> +		return;
>> +
>> +#define MV88E6XXX_ETH_MAC_STAT_MAP(_id, _member)			\
>> +	mv88e6xxx_stats_get_stat(chip, port,				\
>> +				 &mv88e6xxx_hw_stats[MV88E6XXX_HW_STAT_ID_ ## _id], \
>> +				 &mac_stats->stats._member)
>> +
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_unicast, FramesTransmittedOK);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(single, SingleCollisionFrames);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(multiple, MultipleCollisionFrames);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_unicast, FramesReceivedOK);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_fcs_error, FrameCheckSequenceErrors);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_octets, OctetsTransmittedOK);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(deferred, FramesWithDeferredXmissions);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(late, LateCollisions);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_good_octets, OctetsReceivedOK);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_multicasts, MulticastFramesXmittedOK);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_broadcasts, BroadcastFramesXmittedOK);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(excessive, FramesWithExcessiveDeferral);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_multicasts, MulticastFramesReceivedOK);
>> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_broadcasts, BroadcastFramesReceivedOK);
>> +
>> +#undef MV88E6XXX_ETH_MAC_STAT_MAP
>
> I don't exactly enjoy this use (and placement) of the C preprocessor macro
> when spelling out code would have worked just fine, but to each his own.
> At least it is consistent in that we can jump to the other occurrences
> of the statistics counter.

Fair enough. I was trying to come up with something that would make it
easy to audit the chosen mapping between "native" and "standard" counter
names, since I imagine that is what future readers of this are going to
be interested in.

>> +
>> +	mac_stats->stats.FramesTransmittedOK += mac_stats->stats.MulticastFramesXmittedOK;
>> +	mac_stats->stats.FramesTransmittedOK += mac_stats->stats.BroadcastFramesXmittedOK;
>> +	mac_stats->stats.FramesReceivedOK += mac_stats->stats.MulticastFramesReceivedOK;
>> +	mac_stats->stats.FramesReceivedOK += mac_stats->stats.BroadcastFramesReceivedOK;
>> +}
>
> Not sure if there's a "best thing to do" in case a previous mv88e6xxx_stats_get_stat()
> call fails. In net/ethtool/stats.c we have ethtool_stats_sum(), and that's the
> core saying that U64_MAX means one of the sum terms was not reported by
> the driver, and it makes that transparent by simply returning the other.
>
> Here, "not reported by the driver" is due to a bus I/O error, and using
> ethtool_stats_sum() as-is would hide that error away completely, and
> report only the other sum term. Sounds like a failure that would be too
> silent. Whereas your proposal would just report a wildly incorrect
> number - but at high data rates (for offloaded traffic, too), maybe that
> wouldn't be exactly trivial to notice, either.
>
> Maybe we need a variant of ethtool_stats_sum() that requires both terms,
> otherwise returns ETHTOOL_STAT_NOT_SET?

That sounds like a good idea.

> Anyway, this is not a blocker for the current patch set, which is a bit
> too large to resend for trivial matters.
>
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Thanks for the review!

[PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter group support

[Tobias Waldekranz]

Report the applicable subset of an mv88e6xxx port's counters using
ethtool's standardized "rmon" counter group.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 42 ++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 1a16698181fb..2e74109196f4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1357,6 +1357,47 @@ static void mv88e6xxx_get_eth_mac_stats(struct dsa_switch *ds, int port,
 	mac_stats->stats.FramesReceivedOK += mac_stats->stats.BroadcastFramesReceivedOK;
 }
 
+static void mv88e6xxx_get_rmon_stats(struct dsa_switch *ds, int port,
+				     struct ethtool_rmon_stats *rmon_stats,
+				     const struct ethtool_rmon_hist_range **ranges)
+{
+	static const struct ethtool_rmon_hist_range rmon_ranges[] = {
+		{   64,    64 },
+		{   65,   127 },
+		{  128,   255 },
+		{  256,   511 },
+		{  512,  1023 },
+		{ 1024, 65535 },
+		{}
+	};
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int ret;
+
+	ret = mv88e6xxx_stats_snapshot(chip, port);
+	if (ret < 0)
+		return;
+
+#define MV88E6XXX_RMON_STAT_MAP(_id, _member)				\
+	mv88e6xxx_stats_get_stat(chip, port,				\
+				 &mv88e6xxx_hw_stats[MV88E6XXX_HW_STAT_ID_ ## _id], \
+				 &rmon_stats->stats._member)
+
+	MV88E6XXX_RMON_STAT_MAP(in_undersize, undersize_pkts);
+	MV88E6XXX_RMON_STAT_MAP(in_oversize, oversize_pkts);
+	MV88E6XXX_RMON_STAT_MAP(in_fragments, fragments);
+	MV88E6XXX_RMON_STAT_MAP(in_jabber, jabbers);
+	MV88E6XXX_RMON_STAT_MAP(hist_64bytes, hist[0]);
+	MV88E6XXX_RMON_STAT_MAP(hist_65_127bytes, hist[1]);
+	MV88E6XXX_RMON_STAT_MAP(hist_128_255bytes, hist[2]);
+	MV88E6XXX_RMON_STAT_MAP(hist_256_511bytes, hist[3]);
+	MV88E6XXX_RMON_STAT_MAP(hist_512_1023bytes, hist[4]);
+	MV88E6XXX_RMON_STAT_MAP(hist_1024_max_bytes, hist[5]);
+
+#undef MV88E6XXX_RMON_STAT_MAP
+
+	*ranges = rmon_ranges;
+}
+
 static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
@@ -6877,6 +6918,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_eth_mac_stats	= mv88e6xxx_get_eth_mac_stats,
+	.get_rmon_stats		= mv88e6xxx_get_rmon_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
 	.port_max_mtu		= mv88e6xxx_get_max_mtu,
 	.port_change_mtu	= mv88e6xxx_change_mtu,
-- 
2.34.1

Re: [PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter group support

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 05:04:18PM +0100, Tobias Waldekranz wrote:
> Report the applicable subset of an mv88e6xxx port's counters using
> ethtool's standardized "rmon" counter group.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 42 ++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 1a16698181fb..2e74109196f4 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1357,6 +1357,47 @@ static void mv88e6xxx_get_eth_mac_stats(struct dsa_switch *ds, int port,
> +	MV88E6XXX_RMON_STAT_MAP(in_undersize, undersize_pkts);
> +	MV88E6XXX_RMON_STAT_MAP(in_oversize, oversize_pkts);
> +	MV88E6XXX_RMON_STAT_MAP(in_fragments, fragments);
> +	MV88E6XXX_RMON_STAT_MAP(in_jabber, jabbers);
> +	MV88E6XXX_RMON_STAT_MAP(hist_64bytes, hist[0]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_65_127bytes, hist[1]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_128_255bytes, hist[2]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_256_511bytes, hist[3]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_512_1023bytes, hist[4]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_1024_max_bytes, hist[5]);

I see that these are in STATS_TYPE_BANK0 and that every switch provides
that. Good.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter group support

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 05:04:18PM +0100, Tobias Waldekranz wrote:
> +static void mv88e6xxx_get_rmon_stats(struct dsa_switch *ds, int port,
> +				     struct ethtool_rmon_stats *rmon_stats,
> +				     const struct ethtool_rmon_hist_range **ranges)
> +{
> +	static const struct ethtool_rmon_hist_range rmon_ranges[] = {
> +		{   64,    64 },
> +		{   65,   127 },
> +		{  128,   255 },
> +		{  256,   511 },
> +		{  512,  1023 },
> +		{ 1024, 65535 },
> +		{}
> +	};
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int ret;
> +
> +	ret = mv88e6xxx_stats_snapshot(chip, port);
> +	if (ret < 0)
> +		return;
> +
> +#define MV88E6XXX_RMON_STAT_MAP(_id, _member)				\
> +	mv88e6xxx_stats_get_stat(chip, port,				\
> +				 &mv88e6xxx_hw_stats[MV88E6XXX_HW_STAT_ID_ ## _id], \
> +				 &rmon_stats->stats._member)
> +
> +	MV88E6XXX_RMON_STAT_MAP(in_undersize, undersize_pkts);
> +	MV88E6XXX_RMON_STAT_MAP(in_oversize, oversize_pkts);
> +	MV88E6XXX_RMON_STAT_MAP(in_fragments, fragments);
> +	MV88E6XXX_RMON_STAT_MAP(in_jabber, jabbers);
> +	MV88E6XXX_RMON_STAT_MAP(hist_64bytes, hist[0]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_65_127bytes, hist[1]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_128_255bytes, hist[2]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_256_511bytes, hist[3]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_512_1023bytes, hist[4]);
> +	MV88E6XXX_RMON_STAT_MAP(hist_1024_max_bytes, hist[5]);
> +
> +#undef MV88E6XXX_RMON_STAT_MAP
> +
> +	*ranges = rmon_ranges;
> +}

I just noticed that this doesn't populate the TX counters, just RX.

I haven't tried it, but I think the Histogram Mode bits (11:10) of the
Stats Operation Register might be able to control what gets reported for
the Set 4 of counters. Currently AFAICS, the driver always sets it to
MV88E6XXX_G1_STATS_OP_HIST_RX_TX, aka what gets reported to
"rx-rmon-etherStatsPkts64to64Octets" is actually an RX+TX counter.

What's the story behind this?

Re: [PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter group support

[Tobias Waldekranz]

On ons, dec 06, 2023 at 02:22, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, Dec 05, 2023 at 05:04:18PM +0100, Tobias Waldekranz wrote:
>> +static void mv88e6xxx_get_rmon_stats(struct dsa_switch *ds, int port,
>> +				     struct ethtool_rmon_stats *rmon_stats,
>> +				     const struct ethtool_rmon_hist_range **ranges)
>> +{
>> +	static const struct ethtool_rmon_hist_range rmon_ranges[] = {
>> +		{   64,    64 },
>> +		{   65,   127 },
>> +		{  128,   255 },
>> +		{  256,   511 },
>> +		{  512,  1023 },
>> +		{ 1024, 65535 },
>> +		{}
>> +	};
>> +	struct mv88e6xxx_chip *chip = ds->priv;
>> +	int ret;
>> +
>> +	ret = mv88e6xxx_stats_snapshot(chip, port);
>> +	if (ret < 0)
>> +		return;
>> +
>> +#define MV88E6XXX_RMON_STAT_MAP(_id, _member)				\
>> +	mv88e6xxx_stats_get_stat(chip, port,				\
>> +				 &mv88e6xxx_hw_stats[MV88E6XXX_HW_STAT_ID_ ## _id], \
>> +				 &rmon_stats->stats._member)
>> +
>> +	MV88E6XXX_RMON_STAT_MAP(in_undersize, undersize_pkts);
>> +	MV88E6XXX_RMON_STAT_MAP(in_oversize, oversize_pkts);
>> +	MV88E6XXX_RMON_STAT_MAP(in_fragments, fragments);
>> +	MV88E6XXX_RMON_STAT_MAP(in_jabber, jabbers);
>> +	MV88E6XXX_RMON_STAT_MAP(hist_64bytes, hist[0]);
>> +	MV88E6XXX_RMON_STAT_MAP(hist_65_127bytes, hist[1]);
>> +	MV88E6XXX_RMON_STAT_MAP(hist_128_255bytes, hist[2]);
>> +	MV88E6XXX_RMON_STAT_MAP(hist_256_511bytes, hist[3]);
>> +	MV88E6XXX_RMON_STAT_MAP(hist_512_1023bytes, hist[4]);
>> +	MV88E6XXX_RMON_STAT_MAP(hist_1024_max_bytes, hist[5]);
>> +
>> +#undef MV88E6XXX_RMON_STAT_MAP
>> +
>> +	*ranges = rmon_ranges;
>> +}
>
> I just noticed that this doesn't populate the TX counters, just RX.
>
> I haven't tried it, but I think the Histogram Mode bits (11:10) of the
> Stats Operation Register might be able to control what gets reported for
> the Set 4 of counters. Currently AFAICS, the driver always sets it to
> MV88E6XXX_G1_STATS_OP_HIST_RX_TX, aka what gets reported to
> "rx-rmon-etherStatsPkts64to64Octets" is actually an RX+TX counter.

You have a keen eye! Yes, that is what's happening.

> What's the story behind this?

I think the story starts, and ends, with this value being the hardware
default.

Seeing as the hardware only has a single set of histogram counters, it
seems to me like we have to prioritize between:

1. Keeping Rx+Tx: Backwards-compatible, but we can't export any histogram via
   the standard RMON group.

2. Move to Rx-only: We can export them via the RMON group, but we change
   the behavior of the "native" counters.

3. Move to Tx-only: We can export them via the RMON group, but we change
   the behavior of the "native" counters.

Looking at RFC2819, which lays out the original RMON MIB, we find this
description:

    etherStatsPkts64Octets OBJECT-TYPE
        SYNTAX     Counter32
        UNITS      "Packets"
        MAX-ACCESS read-only
        STATUS     current
        DESCRIPTION
            "The total number of packets (including bad
            packets) received that were 64 octets in length
            (excluding framing bits but including FCS octets)."
        ::= { etherStatsEntry 14 }

In my opinion, this gives (2) a clear edge over (3), so we're down to
choosing between (1) and (2). Personally, I lean towards (2), as I think
it is more useful because:

- Most people will tend to assume that the histogram counters refers to
  those defined in RFC2819 anyway

- It means we can deliver _something_ rather than nothing to someone
  building an operating system, who is looking for a hardware
  independent way of providing diagnostics

Re: [PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter group support

[Vladimir Oltean]

On Wed, Dec 06, 2023 at 09:27:29AM +0100, Tobias Waldekranz wrote:
> > I just noticed that this doesn't populate the TX counters, just RX.
> >
> > I haven't tried it, but I think the Histogram Mode bits (11:10) of the
> > Stats Operation Register might be able to control what gets reported for
> > the Set 4 of counters. Currently AFAICS, the driver always sets it to
> > MV88E6XXX_G1_STATS_OP_HIST_RX_TX, aka what gets reported to
> > "rx-rmon-etherStatsPkts64to64Octets" is actually an RX+TX counter.
> 
> You have a keen eye! Yes, that is what's happening.

It would be nice if my failure-prone keen eye had the safety net of a
selftest that catches this kind of stuff. After all, the ethtool
counters were standardized in order for us to be able to expect standard
behavior out of them, and for nonconformities to stand out easily.

Do you think (bearing in mind that the questions below might make the
rest irrelevant) that you could look into creating a minimal test in
tools/testing/selftests/net/forwarding and symlinking it to
tools/testing/selftests/drivers/net/dsa? You can start from
ethtool_std_stats_get() and take inspiration from the way in which it is
used by ethtool_mm.sh.

> > What's the story behind this?
> 
> I think the story starts, and ends, with this value being the hardware
> default.

I do hope that is where the story actually ends.

But the 88E6097 documentation I have suggests that the Histogram Mode
bits are reserved to the value of 3 (RX+TX), which suggests that this
cannot be written to any other value.

> Seeing as the hardware only has a single set of histogram counters,

"Seeing" means you tested this? calling chip->info->ops->stats_set_histogram()
at runtime, and seeing if the previously hidden histogram counters are
reset to zero, or if they show retroactively counted packets?

I searched through the git logs, but it's not exactly clear that this
was tried and doesn't work.

> it seems to me like we have to prioritize between:
> 
> 1. Keeping Rx+Tx: Backwards-compatible, but we can't export any histogram via
>    the standard RMON group.
> 
> 2. Move to Rx-only: We can export them via the RMON group, but we change
>    the behavior of the "native" counters.
> 
> 3. Move to Tx-only: We can export them via the RMON group, but we change
>    the behavior of the "native" counters.
> 
> Looking at RFC2819, which lays out the original RMON MIB, we find this
> description:
> 
>     etherStatsPkts64Octets OBJECT-TYPE
>         SYNTAX     Counter32
>         UNITS      "Packets"
>         MAX-ACCESS read-only
>         STATUS     current
>         DESCRIPTION
>             "The total number of packets (including bad
>             packets) received that were 64 octets in length
>             (excluding framing bits but including FCS octets)."
>         ::= { etherStatsEntry 14 }
> 
> In my opinion, this gives (2) a clear edge over (3), so we're down to
> choosing between (1) and (2). Personally, I lean towards (2), as I think
> it is more useful because:
> 
> - Most people will tend to assume that the histogram counters refers to
>   those defined in RFC2819 anyway
> 
> - It means we can deliver _something_ rather than nothing to someone
>   building an operating system, who is looking for a hardware
>   independent way of providing diagnostics

If the "reserved to 3" thing is true, then both (2) and (3) become
practically non-options, at least for 88E6097. The waters would be
further muddied if the driver were to make some chips use one
Histogram Mode, and other chips a different one. It implies that as
a user, you would need to know what switch family you have, before
you know what "ethtool -S lan0 | grep hist_64bytes" is counting!

Re: [PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter group support

[Tobias Waldekranz]

On ons, dec 06, 2023 at 21:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Dec 06, 2023 at 09:27:29AM +0100, Tobias Waldekranz wrote:
>> > I just noticed that this doesn't populate the TX counters, just RX.
>> >
>> > I haven't tried it, but I think the Histogram Mode bits (11:10) of the
>> > Stats Operation Register might be able to control what gets reported for
>> > the Set 4 of counters. Currently AFAICS, the driver always sets it to
>> > MV88E6XXX_G1_STATS_OP_HIST_RX_TX, aka what gets reported to
>> > "rx-rmon-etherStatsPkts64to64Octets" is actually an RX+TX counter.
>> 
>> You have a keen eye! Yes, that is what's happening.
>
> It would be nice if my failure-prone keen eye had the safety net of a
> selftest that catches this kind of stuff. After all, the ethtool
> counters were standardized in order for us to be able to expect standard
> behavior out of them, and for nonconformities to stand out easily.
>
> Do you think (bearing in mind that the questions below might make the
> rest irrelevant) that you could look into creating a minimal test in
> tools/testing/selftests/net/forwarding and symlinking it to
> tools/testing/selftests/drivers/net/dsa? You can start from
> ethtool_std_stats_get() and take inspiration from the way in which it is
> used by ethtool_mm.sh.

I'll give it the old college try.

>> > What's the story behind this?
>> 
>> I think the story starts, and ends, with this value being the hardware
>> default.
>
> I do hope that is where the story actually ends.
>
> But the 88E6097 documentation I have suggests that the Histogram Mode
> bits are reserved to the value of 3 (RX+TX), which suggests that this
> cannot be written to any other value.

I'm pretty sure that is just typo in the documentation. Every other
field in the documentation marked "RES" has the description "Reserved
for future use" - except for this one. My guess is that the author meant
to type "RWS to 0x3", since that is the convetion for all other
multi-bit fields with a non-zero reset value. W is right next to E on
most keyboards, which makes it even more likely.

In section 4.3.8, we find this:

    The counters are designed to support:
    - RFC 2819 – RMON MIB (this RFC obsoletes 1757 which obsoletes 1271)
    ....

At the bottom of that page there is this note for "Set 4" (which is the
histogram group):

    **Note**
    The Set 4 counters can be configured to be ingress only, egress
    only, or both.

These sentences have remained unchanged since at least 6095, up to and
including 6393X.

>> Seeing as the hardware only has a single set of histogram counters,
>
> "Seeing" means you tested this? calling chip->info->ops->stats_set_histogram()
> at runtime, and seeing if the previously hidden histogram counters are
> reset to zero, or if they show retroactively counted packets?

I've now tested it on a 6393X (the only chip I can get my hands on at
the moment). On this device, the bits have moved to Global1:0x1c, but
the description is the same. It behaves like the documentation would
suggest: There is a single set of histogram counters - the user gets to
choose if they collect stats from rx, tx, or both:

We start out with the default of counting rx+tx, we've seen 400 packets
before the test starts:

    root@infix-06-01-00:~# mdio f212a2* mvls 0 g1:0x1c
    0x07c0
    root@infix-06-01-00:~# ethtool -S x10 | grep hist_512
         hist_512_1023bytes: 400

Send 10 pings to our neighbor with a large payload, both rx and tx are
counted (20):

    root@infix-06-01-00:~# ping -L ff02::1%x10 -s 512 -c 10 -i 0.1 -q
    PING ff02::1%x10(ff02::1%x10) 512 data bytes

    --- ff02::1%x10 ping statistics ---
    10 packets transmitted, 10 received, 0% packet loss, time 934ms
    rtt min/avg/max/mdev = 0.150/0.172/0.313/0.046 ms
    root@infix-06-01-00:~# ethtool -S x10 | grep hist_512
         hist_512_1023bytes: 420

Limit to tx only:

    root@infix-06-01-00:~# mdio f212a2* mvls 0 g1:0x1c 0x0740

Same test again now only increments the (same) counter by 10:

    root@infix-06-01-00:~# ping -L ff02::1%x10 -s 512 -c 10 -i 0.1 -q
    PING ff02::1%x10(ff02::1%x10) 512 data bytes

    --- ff02::1%x10 ping statistics ---
    10 packets transmitted, 10 received, 0% packet loss, time 934ms
    rtt min/avg/max/mdev = 0.148/0.173/0.323/0.050 ms
    root@infix-06-01-00:~# ethtool -S x10 | grep hist_512
         hist_512_1023bytes: 430


> I searched through the git logs, but it's not exactly clear that this
> was tried and doesn't work.
>
>> it seems to me like we have to prioritize between:
>> 
>> 1. Keeping Rx+Tx: Backwards-compatible, but we can't export any histogram via
>>    the standard RMON group.
>> 
>> 2. Move to Rx-only: We can export them via the RMON group, but we change
>>    the behavior of the "native" counters.
>> 
>> 3. Move to Tx-only: We can export them via the RMON group, but we change
>>    the behavior of the "native" counters.
>> 
>> Looking at RFC2819, which lays out the original RMON MIB, we find this
>> description:
>> 
>>     etherStatsPkts64Octets OBJECT-TYPE
>>         SYNTAX     Counter32
>>         UNITS      "Packets"
>>         MAX-ACCESS read-only
>>         STATUS     current
>>         DESCRIPTION
>>             "The total number of packets (including bad
>>             packets) received that were 64 octets in length
>>             (excluding framing bits but including FCS octets)."
>>         ::= { etherStatsEntry 14 }
>> 
>> In my opinion, this gives (2) a clear edge over (3), so we're down to
>> choosing between (1) and (2). Personally, I lean towards (2), as I think
>> it is more useful because:
>> 
>> - Most people will tend to assume that the histogram counters refers to
>>   those defined in RFC2819 anyway
>> 
>> - It means we can deliver _something_ rather than nothing to someone
>>   building an operating system, who is looking for a hardware
>>   independent way of providing diagnostics
>
> If the "reserved to 3" thing is true, then both (2) and (3) become
> practically non-options, at least for 88E6097. The waters would be
> further muddied if the driver were to make some chips use one
> Histogram Mode, and other chips a different one. It implies that as
> a user, you would need to know what switch family you have, before
> you know what "ethtool -S lan0 | grep hist_64bytes" is counting!

I agree that having different chips work differently would be a
nightmare. Especially since this could mean that "lan1" might behave
differently than "lan0" if they are attached to different chips on the
same system.

Fortunately though, it looks like (2) is still on the table. Do you
agree with that assessment? If yes, do you think is the right way
forward?

Andrew, what's your opinion?