On 2022-08-30 16:20, Vladimir Oltean wrote:

Hi Forsblad,

On Fri, Aug 26, 2022 at 08:38:16AM +0200, Mattias Forsblad wrote:

quoted

If RMU is supported, use that interface to
collect rmon data.

A more adequate commit message would be:

net: dsa: mv88e6xxx: use RMU to collect RMON stats if available

But then, I don't think the splitting of patches is good. I think
mv88e6xxx_inband_rcv(), the producer of rmu_raw_stats[], should be
introduced along with its consumer. Otherwise I have to jump between one
patch and another to be able to comment and see things.

I'll have that in mind for the next round. The next version will
look way different after Andrews suggestion.

Also, it would be good if you could consider actually reporting the RMON
stats through the standardized interface (ds->ops->get_rmon_stats) and
ethtool -S lan0 --groups rmon, rather than just unstructured ethtool -S.

Cool, I didn't know it existed. I'll look into that.
 

quoted

Signed-off-by: Mattias Forsblad <redacted>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 4c0510abd875..0d0241ace708 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1226,16 +1226,30 @@ static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 				     u16 bank1_select, u16 histogram)
 {
 	struct mv88e6xxx_hw_stat *stat;
+	int offset = 0;
+	u64 high;
 	int i, j;
 
 	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);
+			if (chip->rmu.ops && chip->rmu.ops->get_rmon &&
+			    !(stat->type & STATS_TYPE_PORT)) {
+				if (stat->type & STATS_TYPE_BANK1)
+					offset = 32;
+
+				data[j] = chip->ports[port].rmu_raw_stats[stat->reg + offset];
+				if (stat->size == 8) {
+					high = chip->ports[port].rmu_raw_stats[stat->reg + offset
+							+ 1];
+					data[j] += (high << 32);

What exactly protects ethtool -S, a reader of rmu_raw_stats[], from
reading an array that is concurrently overwritten by mv88e6xxx_inband_rcv?

So for the Marvell SOHO the RMU is purely a request/response protocol. The switchcore
will not send a frame unless requested, thus no barrier is needed. For other switchcores
which may have send frames spontaneous additional care may be needed.

quoted

+				}
+			} else {
+				mv88e6xxx_reg_lock(chip);
+				data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
+								      bank1_select, histogram);
+				mv88e6xxx_reg_unlock(chip);
+			}
 
 			j++;
 		}

@@ -1304,8 +1318,8 @@ static void mv88e6xxx_get_stats(struct mv88e6xxx_chip *chip, int port,
 	mv88e6xxx_reg_unlock(chip);
 }
 
-static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
-					uint64_t *data)
+static void mv88e6xxx_get_ethtool_stats_mdio(struct dsa_switch *ds, int port,
+					     uint64_t *data)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int ret;

@@ -1319,7 +1333,20 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
 		return;
 
 	mv88e6xxx_get_stats(chip, port, data);
+}
 
+static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
+					uint64_t *data)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	/* If initialization of RMU isn't available
+	 * fall back to MDIO access.
+	 */
+	if (chip->rmu.ops && chip->rmu.ops->get_rmon)

I would create a helper

static bool mv88e6xxx_rmu_available(struct mv88e6xxx_chip *chip)

and use it here and everywhere, for clarity. Testing the presence of
chip->rmu.ops is not wrong, but confusing.

Also, testing chip->rmu.ops->get_rmon gains exactly nothing, since it is
never NULL when chip->rmu.ops isn't NULL.

Agreed. The next version will draw inspiration from qca8k.

quoted

+		chip->rmu.ops->get_rmon(chip, port, data);
+	else
+		mv88e6xxx_get_ethtool_stats_mdio(ds, port, data);
 }
 
 static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
-- 
2.25.1