netdev - Re: [PATCH net-next v5 2/2] net: ti: icssg-prueth: Add support for PA Stats

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: <cd15268f-f6d3-4fca-bd7f-c94011f55996@stanley.mountain>
Date: Thu, 15 Aug 2024 14:28:56 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: MD Danish Anwar <danishanwar@...com>
Cc: Suman Anna <s-anna@...com>, Sai Krishna <saikrishnag@...vell.com>,
	Jan Kiszka <jan.kiszka@...mens.com>, Andrew Lunn <andrew@...n.ch>,
	Diogo Ivo <diogo.ivo@...mens.com>,
	Kory Maincent <kory.maincent@...tlin.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Simon Horman <horms@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Roger Quadros <rogerq@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Santosh Shilimkar <ssantosh@...nel.org>, Nishanth Menon <nm@...com>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	srk@...com, Vignesh Raghavendra <vigneshr@...com>
Subject: Re: [PATCH net-next v5 2/2] net: ti: icssg-prueth: Add support for
 PA Stats

On Wed, Aug 14, 2024 at 02:50:33PM +0530, MD Danish Anwar wrote:
> Add support for dumping PA stats registers via ethtool.
> Firmware maintained stats are stored at PA Stats registers.
> Also modify emac_get_strings() API to use ethtool_puts().
> 
> Signed-off-by: MD Danish Anwar <danishanwar@...com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 17 +++++-----
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  |  6 ++++
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  5 ++-
>  drivers/net/ethernet/ti/icssg/icssg_stats.c   | 19 +++++++++--
>  drivers/net/ethernet/ti/icssg/icssg_stats.h   | 32 +++++++++++++++++++
>  5 files changed, 67 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> index 5688f054cec5..51bb509d37c7 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> @@ -83,13 +83,11 @@ static void emac_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
>  
>  	switch (stringset) {
>  	case ETH_SS_STATS:
> -		for (i = 0; i < ARRAY_SIZE(icssg_all_stats); i++) {
> -			if (!icssg_all_stats[i].standard_stats) {
> -				memcpy(p, icssg_all_stats[i].name,
> -				       ETH_GSTRING_LEN);
> -				p += ETH_GSTRING_LEN;
> -			}
> -		}
> +		for (i = 0; i < ARRAY_SIZE(icssg_all_stats); i++)
> +			if (!icssg_all_stats[i].standard_stats)
> +				ethtool_puts(&p, icssg_all_stats[i].name);
> +		for (i = 0; i < ICSSG_NUM_PA_STATS; i++)

It would probably be better to use ARRAY_SIZE(icssg_all_pa_stats) so that it's
consistent with the loop right before.

> +			ethtool_puts(&p, icssg_all_pa_stats[i].name);
>  		break;
>  	default:
>  		break;
> @@ -100,13 +98,16 @@ static void emac_get_ethtool_stats(struct net_device *ndev,
>  				   struct ethtool_stats *stats, u64 *data)
>  {
>  	struct prueth_emac *emac = netdev_priv(ndev);
> -	int i;
> +	int i, j;
>  
>  	emac_update_hardware_stats(emac);
>  
>  	for (i = 0; i < ARRAY_SIZE(icssg_all_stats); i++)
>  		if (!icssg_all_stats[i].standard_stats)
>  			*(data++) = emac->stats[i];
> +
> +	for (j = 0; j < ICSSG_NUM_PA_STATS; j++)
> +		*(data++) = emac->stats[i + j];

Here i is not an iterator.  It's a stand in for ARRAY_SIZE(icssg_all_stats).
It would be more readable to do that directly.

	for (i = 0; i < ICSSG_NUM_PA_STATS; i++)
		*(data++) = emac->stats[ARRAY_SIZE(icssg_all_stats) + i];

To be honest, putting the pa_stats at the end of ->stats would have made sense
if we could have looped over the whole array, but since they have to be treated
differently we should probably just put them into their own ->pa_stats array.

I haven't tested this so maybe I've missed something obvious.

The "all" in "icssg_all_stats" doesn't really make sense anymore btw...

Sorry for being so nit picky on a v5 patch. :(

regards,
dan carpenter