Re: [RFC v2 PATCH 05/10] net: ti: prueth: Adds ethtool support for ICSSM PRUETH Driver

[Simon Horman <horms@kernel.org>]

On Fri, Jan 24, 2025 at 07:10:51PM +0530, Basharath Hussain Khaja wrote:
> From: Roger Quadros <rogerq@ti.com>
> 
> Changes for enabling ethtool support for the newly added PRU Ethernet
> interfaces. Extends the support for statistics collection from PRU internal
> memory and displays it in the user space. Along with statistics,
> enable/disable of features, configuring link speed etc.are now supported.
> 
> The firmware running on PRU maintains statistics in internal data memory.
> When requested ethtool collects all the statistics for the specified
> interface and displays it in the user space.
> 
> Makefile is updated to include ethtool support into PRUETH driver.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> Signed-off-by: Parvathi Pudi <parvathi@couthit.com>
> Signed-off-by: Basharath Hussain Khaja <basharath@couthit.com>

...

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_ethtool.c b/drivers/net/ethernet/ti/icssm/icssm_ethtool.c

...

> +static const struct {
> +	char string[ETH_GSTRING_LEN];
> +	u32 offset;
> +} prueth_ethtool_stats[] = {
> +	{"txBcast", PRUETH_STAT_OFFSET(tx_bcast)},
> +	{"txMcast", PRUETH_STAT_OFFSET(tx_mcast)},
> +	{"txUcast", PRUETH_STAT_OFFSET(tx_ucast)},
> +	{"txOctets", PRUETH_STAT_OFFSET(tx_octets)},
> +	{"rxBcast", PRUETH_STAT_OFFSET(rx_bcast)},
> +	{"rxMcast", PRUETH_STAT_OFFSET(rx_mcast)},
> +	{"rxUcast", PRUETH_STAT_OFFSET(rx_ucast)},
> +	{"rxOctets", PRUETH_STAT_OFFSET(rx_octets)},

Hi Roger, Basharath, all,

There seems to be some overlap between the above and struct rtnl_link_stats64.

Please implement those stats which are present in struct rtnl_link_stats64
using ndo_get_stats64 and omit them from your implementation of
get_ethtool_stats.

IOW, get_ethtool_stats() is for extended stats, whereas is for standard
stats ndo_get_stats64().  And standard stats should not be presented to the
user as extended stats.

Link: https://docs.kernel.org/networking/statistics.html#notes-for-driver-authors

> +	{"tx64byte", PRUETH_STAT_OFFSET(tx64byte)},
> +	{"tx65_127byte", PRUETH_STAT_OFFSET(tx65_127byte)},
> +	{"tx128_255byte", PRUETH_STAT_OFFSET(tx128_255byte)},
> +	{"tx256_511byte", PRUETH_STAT_OFFSET(tx256_511byte)},
> +	{"tx512_1023byte", PRUETH_STAT_OFFSET(tx512_1023byte)},
> +	{"tx1024byte", PRUETH_STAT_OFFSET(tx1024byte)},
> +	{"rx64byte", PRUETH_STAT_OFFSET(rx64byte)},
> +	{"rx65_127byte", PRUETH_STAT_OFFSET(rx65_127byte)},
> +	{"rx128_255byte", PRUETH_STAT_OFFSET(rx128_255byte)},
> +	{"rx256_511byte", PRUETH_STAT_OFFSET(rx256_511byte)},
> +	{"rx512_1023byte", PRUETH_STAT_OFFSET(rx512_1023byte)},
> +	{"rx1024byte", PRUETH_STAT_OFFSET(rx1024byte)},

Similarly, the above, along with rxOverSizedFrames and rxUnderSizedFrames
below seem to be RMON (RFC 2819) statistics. So I think they should
be handled by implementing get_rmon_stats().

> +
> +	{"lateColl", PRUETH_STAT_OFFSET(late_coll)},
> +	{"singleColl", PRUETH_STAT_OFFSET(single_coll)},
> +	{"multiColl", PRUETH_STAT_OFFSET(multi_coll)},
> +	{"excessColl", PRUETH_STAT_OFFSET(excess_coll)},

And likewise, the section above and below seem to overlap
with Basic IEEE 802.3 MAC statistics which I believe
should be handled by implementing get_eth_mac_stats()

> +
> +	{"rxMisAlignmentFrames", PRUETH_STAT_OFFSET(rx_misalignment_frames)},
> +	{"stormPrevCounterBC", PRUETH_STAT_OFFSET(stormprev_counter_bc)},
> +	{"stormPrevCounterMC", PRUETH_STAT_OFFSET(stormprev_counter_mc)},
> +	{"stormPrevCounterUC", PRUETH_STAT_OFFSET(stormprev_counter_uc)},
> +	{"macRxError", PRUETH_STAT_OFFSET(mac_rxerror)},
> +	{"SFDError", PRUETH_STAT_OFFSET(sfd_error)},
> +	{"defTx", PRUETH_STAT_OFFSET(def_tx)},
> +	{"macTxError", PRUETH_STAT_OFFSET(mac_txerror)},
> +	{"rxOverSizedFrames", PRUETH_STAT_OFFSET(rx_oversized_frames)},
> +	{"rxUnderSizedFrames", PRUETH_STAT_OFFSET(rx_undersized_frames)},
> +	{"rxCRCFrames", PRUETH_STAT_OFFSET(rx_crc_frames)},
> +	{"droppedPackets", PRUETH_STAT_OFFSET(dropped_packets)},
> +
> +	{"txHWQOverFlow", PRUETH_STAT_OFFSET(tx_hwq_overflow)},
> +	{"txHWQUnderFlow", PRUETH_STAT_OFFSET(tx_hwq_underflow)},
> +	{"vlanDropped", PRUETH_STAT_OFFSET(vlan_dropped)},
> +	{"multicastDropped", PRUETH_STAT_OFFSET(multicast_dropped)},
> +};

...