netdev - Re: [PATCH net-next 1/4] net: dsa: mv88e6xxx: Create API to read a single stat counter

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: <20231204161823.eyac4mwjakizygmz@skbuf>
Date: Mon, 4 Dec 2023 18:18:23 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
	f.fainelli@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/4] net: dsa: mv88e6xxx: Create API to read a
 single stat counter

On Fri, Dec 01, 2023 at 01:58:09PM +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.).
> 
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---

I think I like this patch set. Some nitpicks below.

> -static int mv88e6320_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
> -				     uint64_t *data)
> +static int 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);
> +	}

There is no chip->info->ops which does not provide stats_get_stat().
As a separate change, I suppose you could drop the "if".

> +
> +	return ret;
>  }
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index 44383a03ef2f..b0dfbcae0be9 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -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);
> +	int (*stats_get_stat)(struct mv88e6xxx_chip *chip,  int port,
> +			      const struct mv88e6xxx_hw_stat *stat,
> +			      uint64_t *data);

Since you are touching this function prototype anyway, I guess you could
change its return type. Int assumes it can also return a negative error
code, which it doesn't. Maybe size_t to denote that it returns an unsigned
count of statistics?

Also, there is an extraneous space before the "int port" argument which
you could fix up now.

>  	int (*set_cpu_port)(struct mv88e6xxx_chip *chip, int port);
>  	int (*set_egress_port)(struct mv88e6xxx_chip *chip,
>  			       enum mv88e6xxx_egress_direction direction,