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@xxxxxx>
>> ---
>> 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
