Re: [RESEND PATCH net-next v2 1/3] net: libwx: support hardware statistics

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 27 Sep 2023 14:14:55 +0800 Jiawen Wu wrote:
> +static const struct wx_stats wx_gstrings_stats[] = {
> +	WX_NETDEV_STAT("rx_packets", rx_packets),
> +	WX_NETDEV_STAT("tx_packets", tx_packets),
> +	WX_NETDEV_STAT("rx_bytes", rx_bytes),
> +	WX_NETDEV_STAT("tx_bytes", tx_bytes),
> +	WX_NETDEV_STAT("multicast", multicast),
> +	WX_NETDEV_STAT("rx_errors", rx_errors),
> +	WX_NETDEV_STAT("rx_length_errors", rx_length_errors),
> +	WX_NETDEV_STAT("rx_crc_errors", rx_crc_errors),

Please don't report standard netdev statistics in ethtool -S.
Users can get them thru any of the (too) many standard APIs.

> +	WX_STAT("rx_pkts_nic", stats.gprc),
> +	WX_STAT("tx_pkts_nic", stats.gptc),
> +	WX_STAT("rx_bytes_nic", stats.gorc),
> +	WX_STAT("tx_bytes_nic", stats.gotc),
> +	WX_STAT("rx_total_pkts", stats.tpr),
> +	WX_STAT("tx_total_pkts", stats.tpt),
> +	WX_STAT("rx_broadcast", stats.bprc),
> +	WX_STAT("tx_broadcast", stats.bptc),
> +	WX_STAT("rx_multicast", stats.mprc),
> +	WX_STAT("tx_multicast", stats.mptc),
> +	WX_STAT("rx_long_length_count", stats.roc),
> +	WX_STAT("rx_short_length_count", stats.ruc),
> +	WX_STAT("rx_flow_control_xon_xoff", stats.lxonoffrxc),
> +	WX_STAT("tx_flow_control_xon", stats.lxontxc),
> +	WX_STAT("tx_flow_control_xoff", stats.lxofftxc),

Please take a look at the statistics defined in ethtool.h
Look for callbacks in struct ethtool_ops with "stats" in the name.
Anything that matches one of the callbacks should go into those
APIs no to ethtool -S.

> +	WX_STAT("os2bmc_rx_by_bmc", stats.o2bgptc),
> +	WX_STAT("os2bmc_tx_by_bmc", stats.b2ospc),
> +	WX_STAT("os2bmc_tx_by_host", stats.o2bspc),
> +	WX_STAT("os2bmc_rx_by_host", stats.b2ogprc),
> +	WX_STAT("rx_no_dma_resources", stats.rdmdrop),
> +	WX_STAT("tx_busy", tx_busy),
> +	WX_STAT("non_eop_descs", non_eop_descs),
> +	WX_STAT("tx_restart_queue", restart_queue),
> +	WX_STAT("rx_csum_offload_good_count", hw_csum_rx_good),
> +	WX_STAT("rx_csum_offload_errors", hw_csum_rx_error),
> +	WX_STAT("alloc_rx_buff_failed", alloc_rx_buff_failed),
> +};
> +
> +static const char wx_gstrings_test[][ETH_GSTRING_LEN] = {
> +	"Register test  (offline)", "Eeprom test    (offline)",
> +	"Interrupt test (offline)", "Loopback test  (offline)",
> +	"Link test   (on/offline)"
> +};

You said this adds stats. Why are strings for tests here?
-- 
pw-bot: cr