linux-kernel - 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] [day] [month] [year] [list]

Message-ID: <2ee6f2eb-9a3f-4e04-a6d5-059c4381cbd8@ti.com>
Date: Mon, 19 Aug 2024 12:45:30 +0530
From: "Anwar, Md Danish" <a0501179@...com>
To: Dan Carpenter <dan.carpenter@...aro.org>,
        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 8/15/2024 4:58 PM, Dan Carpenter wrote:
> 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.

Sure Dan.

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

Sure Dan. It will make more sense to have different array for pa_stats.
I will do this change and update.

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

Correct, the "icssg_all_stats" should be renamed to "icssg_mii_g_rt_stats".

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

It's okay. Thanks for the review. I will address all these comments and
send out a v6.

> regards,
> dan carpenter
> 

-- 
Thanks and Regards,
Md Danish Anwar