Re: [PATCH net-next v16 1/2] hinic3: Add ethtool statistic ops part1

[Dimitri Daskalakis <dimitri.daskalakis1@gmail.com>]

On 10/16/26 4:13 AM, Fan Gong wrote:
> This commit adds per-queue TX/RX software counters and firmware-backed
> vport and physical port statistics to the hinic3 driver. It implements
> the .get_strings, .get_sset_count and .get_ethtool_stats callbacks so
> these can be read with ethtool -S, and adds packet/byte accounting to
> the RX and TX hot paths.
> 
> Co-developed-by: Wu Di <wudi234@huawei.com>
> Signed-off-by: Wu Di <wudi234@huawei.com>
> Co-developed-by: Teng Peisen <tengpeisen@huawei.com>
> Signed-off-by: Teng Peisen <tengpeisen@huawei.com>
> Signed-off-by: Fan Gong <gongfan1@huawei.com>
> ---
>  .../ethernet/huawei/hinic3/hinic3_ethtool.c   | 294 ++++++++++++++++++
>  .../ethernet/huawei/hinic3/hinic3_hw_intf.h   |  11 +
>  .../huawei/hinic3/hinic3_mgmt_interface.h     |  37 +++
>  .../ethernet/huawei/hinic3/hinic3_nic_cfg.c   |  64 ++++
>  .../ethernet/huawei/hinic3/hinic3_nic_cfg.h   | 108 +++++++
>  .../net/ethernet/huawei/hinic3/hinic3_rx.c    |  64 +++-
>  .../net/ethernet/huawei/hinic3/hinic3_rx.h    |   8 +-
>  .../net/ethernet/huawei/hinic3/hinic3_tx.c    |  70 ++++-
>  .../net/ethernet/huawei/hinic3/hinic3_tx.h    |   5 +-
>  9 files changed, 649 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> index 90fc16288de9..bb6094d61196 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
> @@ -13,8 +13,11 @@

[...]
> +static u16 hinic3_get_hw_stats_strings(struct hinic3_nic_dev *nic_dev, char *p)
> +{
> +	u16 i, cnt = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(hinic3_function_stats); i++) {
> +		memcpy(p, hinic3_function_stats[i].name, ETH_GSTRING_LEN);
> +		p += ETH_GSTRING_LEN;
> +		cnt++;
> +	}

Do you need to manually add/track the offset? Couldn't you let
ethtool_puts()/ethtool_sprintf() do the hard work?

> +
> +	if (!HINIC3_IS_VF(nic_dev->hwdev)) {
> +		for (i = 0; i < ARRAY_SIZE(hinic3_port_stats); i++) {
> +			memcpy(p, hinic3_port_stats[i].name, ETH_GSTRING_LEN);
> +			p += ETH_GSTRING_LEN;
> +			cnt++;
> +		}
> +	}
> +
> +	return cnt;
> +}
> +
> +static void hinic3_get_qp_stats_strings(struct hinic3_nic_dev *nic_dev, char *p)
> +{
> +	u8 *data = p;
> +	u16 i, j;
> +
> +	for (i = 0; i < nic_dev->q_params.num_qps; i++) {
> +		for (j = 0; j < ARRAY_SIZE(hinic3_tx_queue_stats); j++)
> +			ethtool_sprintf(&data,
> +					hinic3_tx_queue_stats[j].name, i);
> +	}
> +
> +	for (i = 0; i < nic_dev->q_params.num_qps; i++) {
> +		for (j = 0; j < ARRAY_SIZE(hinic3_rx_queue_stats); j++)
> +			ethtool_sprintf(&data,
> +					hinic3_rx_queue_stats[j].name, i);
> +	}
> +}
> +
> +static void hinic3_get_strings(struct net_device *netdev,
> +			       u32 stringset, u8 *data)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	char *p = (char *)data;

What's the purpose of casting here and throughout the patch?

> +	u16 offset;
> +
> +	switch (stringset) {
> +	case ETH_SS_STATS:
> +		offset = hinic3_get_hw_stats_strings(nic_dev, p);
> +		hinic3_get_qp_stats_strings(nic_dev,
> +					    p + offset * ETH_GSTRING_LEN);
> +
> +		break;
[...]