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

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

[Tobias Waldekranz]

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

4/8 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).

To support the histogram counters from the "rmon" group, we have to
change mv88e6xxx's configuration of them. Instead of counting rx and
tx, we restrict them to rx-only. 6/8 has the details.

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

Tie it all together with a selftest (8/8).

v2 -> v3:
- Added 6/8
- Added 8/8

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 (8):
  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: Limit histogram counters to ingress traffic
  net: dsa: mv88e6xxx: Add "rmon" counter group support
  selftests: forwarding: ethtool_rmon: Add histogram counter test

 drivers/net/dsa/mv88e6xxx/chip.c              | 390 ++++++++++++------
 drivers/net/dsa/mv88e6xxx/chip.h              |  31 +-
 drivers/net/dsa/mv88e6xxx/global1.c           |   7 +-
 drivers/net/dsa/mv88e6xxx/serdes.c            |  10 +-
 drivers/net/dsa/mv88e6xxx/serdes.h            |   8 +-
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../selftests/net/forwarding/ethtool_rmon.sh  | 106 +++++
 tools/testing/selftests/net/forwarding/lib.sh |   9 +
 8 files changed, 397 insertions(+), 165 deletions(-)
 create mode 100755 tools/testing/selftests/net/forwarding/ethtool_rmon.sh

-- 
2.34.1

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

[Tobias Waldekranz]

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

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
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 07a22c74fe81..4bd3ceffde17 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -943,10 +943,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[] = {
@@ -1284,16 +1290,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 v3 net-next 1/8] net: dsa: mv88e6xxx: Push locking into stats snapshotting

[Florian Fainelli]

On 12/11/23 14:33, Tobias Waldekranz wrote:
> This is more consistent with the driver's general structure.
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH v3 net-next 2/8] 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.).

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
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 4bd3ceffde17..d0cce23c98ff 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1018,7 +1018,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)
 {
@@ -1201,59 +1201,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,
@@ -1269,10 +1292,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) {
@@ -3988,7 +4010,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,
@@ -4026,7 +4048,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,
@@ -4067,7 +4089,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,
@@ -4109,7 +4131,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,
@@ -4152,7 +4174,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,
@@ -4201,7 +4223,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,
@@ -4256,7 +4278,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,
@@ -4294,7 +4316,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,
@@ -4342,7 +4364,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,
@@ -4391,7 +4413,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,
@@ -4442,7 +4464,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,
@@ -4491,7 +4513,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,
@@ -4536,7 +4558,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,
@@ -4585,7 +4607,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,
@@ -4643,7 +4665,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,
@@ -4699,7 +4721,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,
@@ -4758,7 +4780,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,
@@ -4811,7 +4833,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,
@@ -4858,7 +4880,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,
@@ -4917,7 +4939,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,
@@ -4964,7 +4986,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,
@@ -5013,7 +5035,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,
@@ -5071,7 +5093,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,
@@ -5117,7 +5139,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,
@@ -5168,7 +5190,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,
@@ -5230,7 +5252,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,
@@ -5292,7 +5314,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,
@@ -5354,7 +5376,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 v3 net-next 2/8] net: dsa: mv88e6xxx: Create API to read a single stat counter

[Florian Fainelli]

On 12/11/23 14:33, 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.).
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.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 4bd3ceffde17..d0cce23c98ff 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1018,7 +1018,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)
>   {
> @@ -1201,59 +1201,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,
> @@ -1269,10 +1292,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) {
> @@ -3988,7 +4010,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,
> @@ -4026,7 +4048,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,
> @@ -4067,7 +4089,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,
> @@ -4109,7 +4131,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,
> @@ -4152,7 +4174,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,
> @@ -4201,7 +4223,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,
> @@ -4256,7 +4278,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,
> @@ -4294,7 +4316,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,
> @@ -4342,7 +4364,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,
> @@ -4391,7 +4413,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,
> @@ -4442,7 +4464,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,
> @@ -4491,7 +4513,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,
> @@ -4536,7 +4558,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,
> @@ -4585,7 +4607,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,
> @@ -4643,7 +4665,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,
> @@ -4699,7 +4721,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,
> @@ -4758,7 +4780,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,
> @@ -4811,7 +4833,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,
> @@ -4858,7 +4880,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,
> @@ -4917,7 +4939,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,
> @@ -4964,7 +4986,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,
> @@ -5013,7 +5035,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,
> @@ -5071,7 +5093,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,
> @@ -5117,7 +5139,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,
> @@ -5168,7 +5190,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,
> @@ -5230,7 +5252,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,
> @@ -5292,7 +5314,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,
> @@ -5354,7 +5376,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 &&

-- 
Florian

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

[Vladimir Oltean]

On Mon, Dec 11, 2023 at 11:33:40PM +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.).
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

You left this function prototype as returning int rather than size_t.

static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
				     uint64_t *data)

[PATCH v3 net-next 3/8] 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.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
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 v3 net-next 3/8] net: dsa: mv88e6xxx: Fix mv88e6352_serdes_get_stats error path

[Florian Fainelli]

On 12/11/23 14:33, 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.
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH v3 net-next 4/8] 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.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
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 d0cce23c98ff..cbbaf393ed28 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -955,66 +955,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,
@@ -1061,7 +1073,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++) {
@@ -1142,7 +1154,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++) {
@@ -1269,7 +1281,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 v3 net-next 4/8] net: dsa: mv88e6xxx: Give each hw stat an ID

[Florian Fainelli]

On 12/11/23 14:33, 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.
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH v3 net-next 5/8] 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.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
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 cbbaf393ed28..6f1f71cb0de5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1331,6 +1331,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;
@@ -6852,6 +6890,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 v3 net-next 5/8] net: dsa: mv88e6xxx: Add "eth-mac" counter group support

[Florian Fainelli]

On 12/11/23 14:33, Tobias Waldekranz wrote:
> Report the applicable subset of an mv88e6xxx port's counters using
> ethtool's standardized "eth-mac" counter group.
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Not usually fond of defining a macro with a local scope, but no strong 
objection either:

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH v3 net-next 6/8] net: dsa: mv88e6xxx: Limit histogram counters to ingress traffic

[Tobias Waldekranz]

Chips in this family only has one set of histogram counters, which can
be used to count ingressing and/or egressing traffic. mv88e6xxx has,
up until this point, kept the hardware default of counting both
directions.

In the mean time, standard counter group support has been added to
ethtool. Via that interface, drivers may report ingress-only and
egress-only histograms separately - but not combined.

In order for mv88e6xxx to maximalize amount of diagnostic information
that can be exported via standard interfaces, we opt to limit the
histogram counters to ingress traffic only. Which will allow us to
export them via the standard "rmon" group in an upcoming commit.

The reason for choosing ingress-only over egress-only, is to be
compatible with RFC2819 (RMON MIB).

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

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 6f1f71cb0de5..a3bd12ceba7b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1221,7 +1221,7 @@ static size_t mv88e6095_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
 		return 0;
 
 	*data = _mv88e6xxx_get_ethtool_stat(chip, stat, port, 0,
-					    MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+					    MV88E6XXX_G1_STATS_OP_HIST_RX);
 	return 1;
 }
 
@@ -1233,7 +1233,7 @@ static size_t mv88e6250_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
 		return 0;
 
 	*data = _mv88e6xxx_get_ethtool_stat(chip, stat, port, 0,
-					    MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+					    MV88E6XXX_G1_STATS_OP_HIST_RX);
 	return 1;
 }
 
@@ -1246,7 +1246,7 @@ static size_t mv88e6320_stats_get_stat(struct mv88e6xxx_chip *chip, int port,
 
 	*data = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
 					    MV88E6XXX_G1_STATS_OP_BANK_1_BIT_9,
-					    MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
+					    MV88E6XXX_G1_STATS_OP_HIST_RX);
 	return 1;
 }
 
diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
index 174c773b38c2..49444a72ff09 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -462,8 +462,7 @@ int mv88e6390_g1_rmu_disable(struct mv88e6xxx_chip *chip)
 int mv88e6390_g1_stats_set_histogram(struct mv88e6xxx_chip *chip)
 {
 	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6390_G1_CTL2_HIST_MODE_MASK,
-				      MV88E6390_G1_CTL2_HIST_MODE_RX |
-				      MV88E6390_G1_CTL2_HIST_MODE_TX);
+				      MV88E6390_G1_CTL2_HIST_MODE_RX);
 }
 
 int mv88e6xxx_g1_set_device_number(struct mv88e6xxx_chip *chip, int index)
@@ -491,7 +490,7 @@ int mv88e6095_g1_stats_set_histogram(struct mv88e6xxx_chip *chip)
 	if (err)
 		return err;
 
-	val |= MV88E6XXX_G1_STATS_OP_HIST_RX_TX;
+	val |= MV88E6XXX_G1_STATS_OP_HIST_RX;
 
 	err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_STATS_OP, val);
 
@@ -506,7 +505,7 @@ int mv88e6xxx_g1_stats_snapshot(struct mv88e6xxx_chip *chip, int port)
 	err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_STATS_OP,
 				 MV88E6XXX_G1_STATS_OP_BUSY |
 				 MV88E6XXX_G1_STATS_OP_CAPTURE_PORT |
-				 MV88E6XXX_G1_STATS_OP_HIST_RX_TX | port);
+				 MV88E6XXX_G1_STATS_OP_HIST_RX | port);
 	if (err)
 		return err;
 
-- 
2.34.1

Re: [PATCH v3 net-next 6/8] net: dsa: mv88e6xxx: Limit histogram counters to ingress traffic

[Florian Fainelli]

On 12/11/23 14:33, Tobias Waldekranz wrote:
> Chips in this family only has one set of histogram counters, which can
> be used to count ingressing and/or egressing traffic. mv88e6xxx has,
> up until this point, kept the hardware default of counting both
> directions.

s/has/have/

> 
> In the mean time, standard counter group support has been added to
> ethtool. Via that interface, drivers may report ingress-only and
> egress-only histograms separately - but not combined.
> 
> In order for mv88e6xxx to maximalize amount of diagnostic information
> that can be exported via standard interfaces, we opt to limit the
> histogram counters to ingress traffic only. Which will allow us to
> export them via the standard "rmon" group in an upcoming commit.

s/maximalize/maximize/

> 
> The reason for choosing ingress-only over egress-only, is to be
> compatible with RFC2819 (RMON MIB).
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Out of curiosity: does this commit and the next one need to be swapped 
in order to surprises if someone happened to be bisecting across this 
patch series?

Unless there is something else that needs to be addressed, please 
address the two typos above, regardless:

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v3 net-next 6/8] net: dsa: mv88e6xxx: Limit histogram counters to ingress traffic

[Tobias Waldekranz]

On mon, dec 11, 2023 at 15:03, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 12/11/23 14:33, Tobias Waldekranz wrote:
>> Chips in this family only has one set of histogram counters, which can
>> be used to count ingressing and/or egressing traffic. mv88e6xxx has,
>> up until this point, kept the hardware default of counting both
>> directions.
>
> s/has/have/
>
>> 
>> In the mean time, standard counter group support has been added to
>> ethtool. Via that interface, drivers may report ingress-only and
>> egress-only histograms separately - but not combined.
>> 
>> In order for mv88e6xxx to maximalize amount of diagnostic information
>> that can be exported via standard interfaces, we opt to limit the
>> histogram counters to ingress traffic only. Which will allow us to
>> export them via the standard "rmon" group in an upcoming commit.
>
> s/maximalize/maximize/
>
>> 
>> The reason for choosing ingress-only over egress-only, is to be
>> compatible with RFC2819 (RMON MIB).
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>
> Out of curiosity: does this commit and the next one need to be swapped 
> in order to surprises if someone happened to be bisecting across this 
> patch series?

I'm not sure I follow. This commit only changes the behavior of the
existing counters (ethtool -S). If it was swapped with the next one,
then there would be one commit in the history in which the "rmon"
histogram counters would report the wrong values (the bug pointed out by
Vladimir on v2)

> Unless there is something else that needs to be addressed, please 
> address the two typos above, regardless:

s/Unless/If/?

> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

Thanks for the review!

Re: [PATCH v3 net-next 6/8] net: dsa: mv88e6xxx: Limit histogram counters to ingress traffic

[Florian Fainelli]

On 12/11/23 15:35, Tobias Waldekranz wrote:
> On mon, dec 11, 2023 at 15:03, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 12/11/23 14:33, Tobias Waldekranz wrote:
>>> Chips in this family only has one set of histogram counters, which can
>>> be used to count ingressing and/or egressing traffic. mv88e6xxx has,
>>> up until this point, kept the hardware default of counting both
>>> directions.
>>
>> s/has/have/
>>
>>>
>>> In the mean time, standard counter group support has been added to
>>> ethtool. Via that interface, drivers may report ingress-only and
>>> egress-only histograms separately - but not combined.
>>>
>>> In order for mv88e6xxx to maximalize amount of diagnostic information
>>> that can be exported via standard interfaces, we opt to limit the
>>> histogram counters to ingress traffic only. Which will allow us to
>>> export them via the standard "rmon" group in an upcoming commit.
>>
>> s/maximalize/maximize/
>>
>>>
>>> The reason for choosing ingress-only over egress-only, is to be
>>> compatible with RFC2819 (RMON MIB).
>>>
>>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>>
>> Out of curiosity: does this commit and the next one need to be swapped
>> in order to surprises if someone happened to be bisecting across this
>> patch series?
> 
> I'm not sure I follow. This commit only changes the behavior of the
> existing counters (ethtool -S). If it was swapped with the next one,
> then there would be one commit in the history in which the "rmon"
> histogram counters would report the wrong values (the bug pointed out by
> Vladimir on v2)

Right, somehow I thought there was provision for reporting the TX 
histograms through the rmon interface which your subsequent patch would 
do, but it does not appear so, never mind then.

> 
>> Unless there is something else that needs to be addressed, please
>> address the two typos above, regardless:
> 
> s/Unless/If/?

Yes, the latter!
-- 
Florian

Re: [PATCH v3 net-next 6/8] net: dsa: mv88e6xxx: Limit histogram counters to ingress traffic

[Vladimir Oltean]

On Mon, Dec 11, 2023 at 11:33:44PM +0100, Tobias Waldekranz wrote:
> Chips in this family only has one set of histogram counters, which can
> be used to count ingressing and/or egressing traffic. mv88e6xxx has,
> up until this point, kept the hardware default of counting both
> directions.
> 
> In the mean time, standard counter group support has been added to
> ethtool. Via that interface, drivers may report ingress-only and
> egress-only histograms separately - but not combined.
> 
> In order for mv88e6xxx to maximalize amount of diagnostic information
> that can be exported via standard interfaces, we opt to limit the
> histogram counters to ingress traffic only. Which will allow us to
> export them via the standard "rmon" group in an upcoming commit.
> 
> The reason for choosing ingress-only over egress-only, is to be
> compatible with RFC2819 (RMON MIB).
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

I guess this needs an ack from Andrew.

Re: [PATCH v3 net-next 6/8] net: dsa: mv88e6xxx: Limit histogram counters to ingress traffic

[Andrew Lunn]

On Mon, Dec 11, 2023 at 11:33:44PM +0100, Tobias Waldekranz wrote:
> Chips in this family only has one set of histogram counters, which can
> be used to count ingressing and/or egressing traffic. mv88e6xxx has,
> up until this point, kept the hardware default of counting both
> directions.
> 
> In the mean time, standard counter group support has been added to
> ethtool. Via that interface, drivers may report ingress-only and
> egress-only histograms separately - but not combined.
> 
> In order for mv88e6xxx to maximalize amount of diagnostic information
> that can be exported via standard interfaces, we opt to limit the
> histogram counters to ingress traffic only. Which will allow us to
> export them via the standard "rmon" group in an upcoming commit.
> 
> The reason for choosing ingress-only over egress-only, is to be
> compatible with RFC2819 (RMON MIB).
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Keeping the hardware defaults is pretty arbitrary. So i don't see why
we cannot change it to fulfil standard RMON.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 6/8] net: dsa: mv88e6xxx: Limit histogram counters to ingress traffic

[Vladimir Oltean]

On Mon, Dec 11, 2023 at 11:33:44PM +0100, Tobias Waldekranz wrote:
> Chips in this family only has one set of histogram counters, which can
> be used to count ingressing and/or egressing traffic. mv88e6xxx has,
> up until this point, kept the hardware default of counting both
> directions.
> 
> In the mean time, standard counter group support has been added to
> ethtool. Via that interface, drivers may report ingress-only and
> egress-only histograms separately - but not combined.
> 
> In order for mv88e6xxx to maximalize amount of diagnostic information
> that can be exported via standard interfaces, we opt to limit the
> histogram counters to ingress traffic only. Which will allow us to
> export them via the standard "rmon" group in an upcoming commit.
> 
> The reason for choosing ingress-only over egress-only, is to be
> compatible with RFC2819 (RMON MIB).
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

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

[PATCH v3 net-next 7/8] 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.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
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 a3bd12ceba7b..a237f132c163 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1369,6 +1369,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;
@@ -6891,6 +6932,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 v3 net-next 7/8] net: dsa: mv88e6xxx: Add "rmon" counter group support

[Florian Fainelli]

On 12/11/23 14:33, Tobias Waldekranz wrote:
> Report the applicable subset of an mv88e6xxx port's counters using
> ethtool's standardized "rmon" counter group.
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH v3 net-next 8/8] selftests: forwarding: ethtool_rmon: Add histogram counter test

[Tobias Waldekranz]

Validate the operation of rx and tx histogram counters, if supported
by the interface, by sending batches of packets targeted for each
bucket.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../selftests/net/forwarding/ethtool_rmon.sh  | 106 ++++++++++++++++++
 tools/testing/selftests/net/forwarding/lib.sh |   9 ++
 3 files changed, 116 insertions(+)
 create mode 100755 tools/testing/selftests/net/forwarding/ethtool_rmon.sh

diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index df593b7b3e6b..452693514be4 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -17,6 +17,7 @@ TEST_PROGS = bridge_fdb_learning_limit.sh \
 	dual_vxlan_bridge.sh \
 	ethtool_extended_state.sh \
 	ethtool_mm.sh \
+	ethtool_rmon.sh \
 	ethtool.sh \
 	gre_custom_multipath_hash.sh \
 	gre_inner_v4_multipath.sh \
diff --git a/tools/testing/selftests/net/forwarding/ethtool_rmon.sh b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
new file mode 100755
index 000000000000..73e3fbe28f37
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
@@ -0,0 +1,106 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="
+	rmon_rx_histogram
+	rmon_tx_histogram
+"
+
+NUM_NETIFS=2
+source lib.sh
+
+bucket_test()
+{
+	local set=$1; shift
+	local bucket=$1; shift
+	local len=$1; shift
+	local num_rx=10000
+	local num_tx=20000
+	local expected=
+	local before=
+	local after=
+	local delta=
+
+	# Mausezahn does not include FCS bytes in its length - but the
+	# histogram counters do
+	len=$((len - 4))
+
+	before=$(ethtool --json -S $h2 --groups rmon | \
+		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][$bucket].val")
+
+	# Send 10k one way and 20k in the other, to detect counters
+	# mapped to the wrong direction
+	$MZ $h1 -q -c $num_rx -p $len -a own -b bcast -d 10us
+	$MZ $h2 -q -c $num_tx -p $len -a own -b bcast -d 10us
+
+	after=$(ethtool --json -S $h2 --groups rmon | \
+		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][$bucket].val")
+
+	delta=$((after - before))
+
+	expected=$([ $set = rx ] && echo $num_rx || echo $num_tx)
+
+	# Allow some extra tolerance for other packets sent by the stack
+	[ $delta -ge $expected ] && [ $delta -le $((expected + 100)) ]
+}
+
+rmon_histogram()
+{
+	local set=$1; shift
+	local nbuckets=0
+
+	RET=0
+
+	while read -r -a bucket; do
+		bucket_test $set $nbuckets ${bucket[0]}
+		check_err "$?" "Verification failed for bucket ${bucket[0]}-${bucket[1]}"
+		nbuckets=$((nbuckets + 1))
+	done < <(ethtool --json -S $h2 --groups rmon | \
+		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][]|[.low, .high, .val]|@tsv" 2>/dev/null)
+
+	if [ $nbuckets -eq 0 ]; then
+		log_test_skip "$h2 does not support $set histogram counters"
+		return
+	fi
+
+	log_test "$set histogram counters"
+}
+
+rmon_rx_histogram()
+{
+	rmon_histogram rx
+}
+
+rmon_tx_histogram()
+{
+	rmon_histogram tx
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	h2=${NETIFS[p2]}
+
+	for iface in $h1 $h2; do
+		ip link set dev $iface up
+	done
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	for iface in $h2 $h1; do
+		ip link set dev $iface down
+	done
+}
+
+check_ethtool_counter_group_support
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 8f6ca458af9a..e3740163c384 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -146,6 +146,15 @@ check_ethtool_mm_support()
 	fi
 }
 
+check_ethtool_counter_group_support()
+{
+	ethtool --help 2>&1| grep -- '--all-groups' &> /dev/null
+	if [[ $? -ne 0 ]]; then
+		echo "SKIP: ethtool too old; it is missing standard counter group support"
+		exit $ksft_skip
+	fi
+}
+
 check_locked_port_support()
 {
 	if ! bridge -d link show | grep -q " locked"; then
-- 
2.34.1

Re: [PATCH v3 net-next 8/8] selftests: forwarding: ethtool_rmon: Add histogram counter test

[Florian Fainelli]

On 12/11/23 14:33, Tobias Waldekranz wrote:
> Validate the operation of rx and tx histogram counters, if supported
> by the interface, by sending batches of packets targeted for each
> bucket.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v3 net-next 8/8] selftests: forwarding: ethtool_rmon: Add histogram counter test

[Vladimir Oltean]

Hi Tobias,

On Mon, Dec 11, 2023 at 11:33:46PM +0100, Tobias Waldekranz wrote:
> Validate the operation of rx and tx histogram counters, if supported
> by the interface, by sending batches of packets targeted for each
> bucket.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Thank you so much for writing down this test.

I tested it on enetc and ocelot/felix, and I can report back that I
already found 2 bugs. One in ocelot, for which I've sent this patch:
https://lore.kernel.org/netdev/20231214000902.545625-1-vladimir.oltean@nxp.com/

and one in this selftest. I hope the logs below make it quite clear what
is going on.

Before the change:

root@debian:~/selftests/net/forwarding# ./ethtool_rmon.sh eno0 swp0
[   37.359447] fsl_enetc 0000:00:00.0 eno0: PHY [0000:00:00.3:02] driver [Qualcomm Atheros AR8031/AR8033] (irq=POLL)
[   37.370906] fsl_enetc 0000:00:00.0 eno0: configuring for inband/sgmii link mode
[   37.387399] mscc_felix 0000:00:00.5 swp0: configuring for inband/qsgmii link mode
[   41.478974] fsl_enetc 0000:00:00.0 eno0: Link is Up - 1Gbps/Full - flow control rx/tx
[   41.479119] mscc_felix 0000:00:00.5 swp0: Link is Up - 1Gbps/Full - flow control rx/tx
TEST: rx histogram counters for bucket 64-64                        [ OK ]
TEST: rx histogram counters for bucket 65-127                       [ OK ]
TEST: rx histogram counters for bucket 128-255                      [ OK ]
TEST: rx histogram counters for bucket 256-511                      [ OK ]
TEST: rx histogram counters for bucket 512-1023                     [ OK ]
TEST: rx histogram counters for bucket 1024-1526                    [ OK ]
TEST: rx histogram counters for bucket 1527-65535                   [FAIL]
        Verification failed for swp0 bucket 1527-65535
TEST: tx histogram counters for bucket 64-64                        [ OK ]
TEST: tx histogram counters for bucket 65-127                       [ OK ]
TEST: tx histogram counters for bucket 128-255                      [ OK ]
TEST: tx histogram counters for bucket 256-511                      [ OK ]
TEST: tx histogram counters for bucket 512-1023                     [ OK ]
TEST: tx histogram counters for bucket 1024-1526                    [ OK ]
TEST: tx histogram counters for bucket 1527-65535                   [FAIL]
        Verification failed for swp0 bucket 1527-65535

The change itself:

root@debian:~/selftests/net/forwarding# ip link set swp0 mtu 9000
root@debian:~/selftests/net/forwarding# ip link set eno0 mtu 9000

After the change:

root@debian:~/selftests/net/forwarding# ./ethtool_rmon.sh eno0 swp0
TEST: rx histogram counters for bucket 64-64                        [ OK ]
TEST: rx histogram counters for bucket 65-127                       [ OK ]
TEST: rx histogram counters for bucket 128-255                      [ OK ]
TEST: rx histogram counters for bucket 256-511                      [ OK ]
TEST: rx histogram counters for bucket 512-1023                     [ OK ]
TEST: rx histogram counters for bucket 1024-1526                    [ OK ]
TEST: rx histogram counters for bucket 1527-65535                   [ OK ]
TEST: tx histogram counters for bucket 64-64                        [ OK ]
TEST: tx histogram counters for bucket 65-127                       [ OK ]
TEST: tx histogram counters for bucket 128-255                      [ OK ]
TEST: tx histogram counters for bucket 256-511                      [ OK ]
TEST: tx histogram counters for bucket 512-1023                     [ OK ]
TEST: tx histogram counters for bucket 1024-1526                    [ OK ]
TEST: tx histogram counters for bucket 1527-65535                   [ OK ]

We'd need to raise the MTU on both $h1 and $h2 to $len - ETH_HLEN.
Note that $h1 - the device whose counters we are not looking at - may
not have the same histograms, and even the same MTU. It means we may not
be able to test all of $h2's histograms if we can't set the MTU to the
appropriate value, and that should just mean a skipped test.

The initial MTU of the interfaces should be restored at cleanup() time,
and only modified during each test if necessary, I suppose.

I noticed that the test is asymmetric, so I ran it a second time with
the argument order "swp0 eno0" and that passed as well. It's probably
all too easy to miss that it leaves $h1's counters untested, though.

The test also passes on mv88e6390, because all buckets start with a
value smaller than 1518, so the MTU never needs to be increased:

TEST: rx histogram counters for bucket 64-64                        [ OK ]
TEST: rx histogram counters for bucket 65-127                       [ OK ]
TEST: rx histogram counters for bucket 128-255                      [ OK ]
TEST: rx histogram counters for bucket 256-511                      [ OK ]
TEST: rx histogram counters for bucket 512-1023                     [ OK ]
TEST: rx histogram counters for bucket 1024-65535                   [ OK ]
TEST: lan2 does not support tx histogram counters                   [SKIP]

> diff --git a/tools/testing/selftests/net/forwarding/ethtool_rmon.sh b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
> new file mode 100755
> index 000000000000..73e3fbe28f37
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
> @@ -0,0 +1,106 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ALL_TESTS="
> +	rmon_rx_histogram
> +	rmon_tx_histogram
> +"
> +
> +NUM_NETIFS=2
> +source lib.sh
> +
> +bucket_test()
> +{
> +	local set=$1; shift
> +	local bucket=$1; shift
> +	local len=$1; shift
> +	local num_rx=10000
> +	local num_tx=20000
> +	local expected=
> +	local before=
> +	local after=
> +	local delta=
> +
> +	# Mausezahn does not include FCS bytes in its length - but the
> +	# histogram counters do
> +	len=$((len - 4))
> +
> +	before=$(ethtool --json -S $h2 --groups rmon | \
> +		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][$bucket].val")
> +
> +	# Send 10k one way and 20k in the other, to detect counters
> +	# mapped to the wrong direction
> +	$MZ $h1 -q -c $num_rx -p $len -a own -b bcast -d 10us
> +	$MZ $h2 -q -c $num_tx -p $len -a own -b bcast -d 10us
> +
> +	after=$(ethtool --json -S $h2 --groups rmon | \
> +		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][$bucket].val")
> +
> +	delta=$((after - before))
> +
> +	expected=$([ $set = rx ] && echo $num_rx || echo $num_tx)
> +
> +	# Allow some extra tolerance for other packets sent by the stack
> +	[ $delta -ge $expected ] && [ $delta -le $((expected + 100)) ]
> +}
> +
> +rmon_histogram()
> +{
> +	local set=$1; shift
> +	local nbuckets=0
> +
> +	RET=0
> +
> +	while read -r -a bucket; do
> +		bucket_test $set $nbuckets ${bucket[0]}
> +		check_err "$?" "Verification failed for bucket ${bucket[0]}-${bucket[1]}"
> +		nbuckets=$((nbuckets + 1))
> +	done < <(ethtool --json -S $h2 --groups rmon | \
> +		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][]|[.low, .high, .val]|@tsv" 2>/dev/null)
> +
> +	if [ $nbuckets -eq 0 ]; then
> +		log_test_skip "$h2 does not support $set histogram counters"
> +		return
> +	fi
> +
> +	log_test "$set histogram counters"

I'm aware this was probably done on purpose, but I felt the test was not
very interactive (it took over 10 seconds to get some output back), so I
took the liberty to log individual buckets as their own tests. And also
to stop at the first failure, rather than continue the iteration which
got me confused during debugging.

diff --git a/tools/testing/selftests/net/forwarding/ethtool_rmon.sh b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
index 73e3fbe28f37..b0f701063822 100755
--- a/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
+++ b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
@@ -53,7 +53,13 @@ rmon_histogram()
 
 	while read -r -a bucket; do
 		bucket_test $set $nbuckets ${bucket[0]}
-		check_err "$?" "Verification failed for bucket ${bucket[0]}-${bucket[1]}"
+		rc="$?"
+		check_err "$rc" "Verification failed for $h2 bucket ${bucket[0]}-${bucket[1]}"
+		log_test "$set histogram counters for bucket ${bucket[0]}-${bucket[1]}"
+		if [ $rc -ne 0 ]; then
+			return 1
+		fi
+
 		nbuckets=$((nbuckets + 1))
 	done < <(ethtool --json -S $h2 --groups rmon | \
 		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][]|[.low, .high, .val]|@tsv" 2>/dev/null)
@@ -62,8 +68,6 @@ rmon_histogram()
 		log_test_skip "$h2 does not support $set histogram counters"
 		return
 	fi
-
-	log_test "$set histogram counters"
 }
 
 rmon_rx_histogram()

> +}

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

[Vladimir Oltean]

On Mon, Dec 11, 2023 at 11:33:38PM +0100, Tobias Waldekranz wrote:
> The majority of the changes (2/8) are about refactoring the existing
> ethtool statistics support to make it possible to read individual
> counters, rather than the whole set.
> 
> 4/8 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).
> 
> To support the histogram counters from the "rmon" group, we have to
> change mv88e6xxx's configuration of them. Instead of counting rx and
> tx, we restrict them to rx-only. 6/8 has the details.
> 
> With that in place, adding the actual counter groups is pretty
> straight forward (5,7/8).
> 
> Tie it all together with a selftest (8/8).

I plan to test and review this. Please do not merge it yet.