Re: [PATCH net-next v2 3/3] rmon: Use RMU if available

[Vladimir Oltean <olteanv@gmail.com>]

Hi Forsblad,

On Fri, Aug 26, 2022 at 08:38:16AM +0200, Mattias Forsblad wrote:
> 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.

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.

> 
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  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?

> +				}
> +			} 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.

> +		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
>