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