netdev - Re: [PATCH net-next v12 5/6] net: dsa: mv88e6xxx: rmon: Use RMU for reading RMON data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YyONb429ODhmiu6I@lunn.ch>
Date:   Thu, 15 Sep 2022 22:39:11 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
Cc:     netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux@...linux.org.uk,
        ansuelsmth@...il.com
Subject: Re: [PATCH net-next v12 5/6] net: dsa: mv88e6xxx: rmon: Use RMU for
 reading RMON data

> +	{ "sw_in_discards",		4, 0x10, 0x81, STATS_TYPE_PORT, },
> +	{ "sw_in_filtered",		2, 0x12, 0x85, STATS_TYPE_PORT, },
> +	{ "sw_out_filtered",		2, 0x13, 0x89, STATS_TYPE_PORT, },

I agree with Florian here. Maybe add a table just for these three
values? Or even a switch statement.

> -static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
> -				     uint64_t *data, int types,
> -				     u16 bank1_select, u16 histogram)
> +static int mv88e6xxx_state_get_stats_rmu(struct mv88e6xxx_chip *chip, int port,
> +					 uint64_t *data, int types,
> +					 u16 bank1_select, u16 histogram)
> +{
> +	const u64 *stats = chip->ports[port].rmu_raw_stats;
> +	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) {
> +			if (stat->type & STATS_TYPE_PORT) {
> +				data[j] = stats[stat->rmu_reg];
> +			} else {
> +				if (stat->type & STATS_TYPE_BANK1)
> +					offset = 32;
> +
> +				data[j] = stats[stat->reg + offset];
> +				if (stat->size == 8) {
> +					high = stats[stat->reg + offset + 1];
> +					data[j] += (high << 32);
> +				}
> +			}
> +
> +			j++;
> +		}
> +	}
> +	return j;
> +}

This is definitely getting better, closer to what i was thinking. But...

> +static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
> +				     uint64_t *data, int types,
> +				     u16 bank1_select, u16 histogram)
> +{
> +	if (mv88e6xxx_rmu_available(chip))
> +		return mv88e6xxx_state_get_stats_rmu(chip, port, data, types,
> +						     bank1_select, histogram);
> +	else
> +		return mv88e6xxx_stats_get_stats_mdio(chip, port, data, types,
> +						      bank1_select, histogram);
> +}

I would still like to get rid of this.

What i think the problem here is, you are trying to be 100%
compatible. So you only show bank1 statistics, if they where available
via MDIO. However, it seems like all the RMU implementations make all
statistics available, both banks, and counters in the port space.

So long as you keep the names consistent, you can return more
statistics via RMU than via MDIO.

   Andrew