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

Powered by blists - more mailing lists