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

["Anwar, Md Danish" <a0501179@ti.com>]

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