Re: [PATCH net-next v2] net: dlink: add support for reporting stats via `ethtool -S`

[Jakub Kicinski <kuba@kernel.org>]

On Fri,  8 Nov 2024 00:19:33 +0900 Moon Yeounsu wrote:
> +}, mac_stats[] = {
> +	DEFINE_MAC_STATS(tx_packets, FramesXmtOk,
> +			 u32, FramesTransmittedOK),
> +	DEFINE_MAC_STATS(rx_packets, FramesRcvOk,
> +			 u32, FramesReceivedOK),
> +	DEFINE_MAC_STATS(tx_bytes, OctetXmtOk,
> +			 u32, OctetsTransmittedOK),
> +	DEFINE_MAC_STATS(rx_bytes, OctetRcvOk,
> +			 u32, OctetsReceivedOK),
> +	DEFINE_MAC_STATS(single_collisions, SingleColFrames,
> +			 u32, SingleCollisionFrames),
> +	DEFINE_MAC_STATS(multi_collisions, MultiColFrames,
> +			 u32, MultipleCollisionFrames),
> +	DEFINE_MAC_STATS(late_collisions, LateCollisions,
> +			 u32, LateCollisions),
> +	DEFINE_MAC_STATS(rx_frames_too_long_errors, FrameTooLongErrors,
> +			 u16, FrameTooLongErrors),
> +	DEFINE_MAC_STATS(rx_in_range_length_errors, InRangeLengthErrors,
> +			 u16, InRangeLengthErrors),
> +	DEFINE_MAC_STATS(rx_frames_check_seq_errors, FramesCheckSeqErrors,
> +			 u16, FrameCheckSequenceErrors),
> +	DEFINE_MAC_STATS(rx_frames_lost_errors, FramesLostRxErrors,
> +			 u16, FramesLostDueToIntMACRcvError),
> +	DEFINE_MAC_STATS(tx_frames_abort, FramesAbortXSColls,
> +			 u16, FramesAbortedDueToXSColls),
> +	DEFINE_MAC_STATS(tx_carrier_sense_errors, CarrierSenseErrors,
> +			 u16, CarrierSenseErrors),
> +	DEFINE_MAC_STATS(tx_multicast_frames, McstFramesXmtdOk,
> +			 u32, MulticastFramesXmittedOK),
> +	DEFINE_MAC_STATS(rx_multicast_frames, McstFramesRcvdOk,
> +			 u32, MulticastFramesReceivedOK),
> +	DEFINE_MAC_STATS(tx_broadcast_frames, BcstFramesXmtdOk,
> +			 u16, BroadcastFramesXmittedOK),
> +	DEFINE_MAC_STATS(rx_broadcast_frames, BcstFramesRcvdOk,
> +			 u16, BroadcastFramesReceivedOK),
> +	DEFINE_MAC_STATS(tx_frames_deferred, FramesWDeferredXmt,
> +			 u32, FramesWithDeferredXmissions),
> +	DEFINE_MAC_STATS(tx_frames_excessive_deferral, FramesWEXDeferal,
> +			 u16, FramesWithExcessiveDeferral),
> +}, rmon_stats[] = {
> +	DEFINE_RMON_STATS(rmon_under_size_packets,
> +			  EtherStatsUndersizePkts, undersize_pkts),
> +	DEFINE_RMON_STATS(rmon_fragments,
> +			  EtherStatsFragments, fragments),
> +	DEFINE_RMON_STATS(rmon_jabbers,
> +			  EtherStatsJabbers, jabbers),
> +	DEFINE_RMON_STATS(rmon_tx_byte_64,
> +			  EtherStatsPkts64OctetTransmit, hist_tx[0]),
> +	DEFINE_RMON_STATS(rmon_rx_byte_64,
> +			  EtherStats64Octets, hist[0]),
> +	DEFINE_RMON_STATS(rmon_tx_byte_65to127,
> +			  EtherStats65to127OctetsTransmit, hist_tx[1]),
> +	DEFINE_RMON_STATS(rmon_rx_byte_64to127,
> +			  EtherStatsPkts65to127Octets, hist[1]),
> +	DEFINE_RMON_STATS(rmon_tx_byte_128to255,
> +			  EtherStatsPkts128to255OctetsTransmit, hist_tx[2]),
> +	DEFINE_RMON_STATS(rmon_rx_byte_128to255,
> +			  EtherStatsPkts128to255Octets, hist[2]),
> +	DEFINE_RMON_STATS(rmon_tx_byte_256to511,
> +			  EtherStatsPkts256to511OctetsTransmit, hist_tx[3]),
> +	DEFINE_RMON_STATS(rmon_rx_byte_256to511,
> +			  EtherStatsPkts256to511Octets, hist[3]),
> +	DEFINE_RMON_STATS(rmon_tx_byte_512to1023,
> +			  EtherStatsPkts512to1023OctetsTransmit, hist_tx[4]),
> +	DEFINE_RMON_STATS(rmon_rx_byte_512to1203,
> +			  EtherStatsPkts512to1023Octets, hist[4]),
> +	DEFINE_RMON_STATS(rmon_tx_byte_1204to1518,
> +			  EtherStatsPkts1024to1518OctetsTransmit, hist_tx[5]),
> +	DEFINE_RMON_STATS(rmon_rx_byte_1204to1518,
> +			  EtherStatsPkts1024to1518Octets, hist[5])
> +};

Do these macro wrappers really buy you anything?
They make the code a lot harder to follow :(

> +static void get_ethtool_mac_stats(struct net_device *dev,
> +				  struct ethtool_eth_mac_stats *mac_base)
> +{
> +	struct netdev_private *np = netdev_priv(dev);
> +
> +	get_stats(dev);
> +
> +	if (mac_base->src != ETHTOOL_MAC_STATS_SRC_AGGREGATE)
> +		return;
> +
> +	for (int i = 0; i < MAC_STATS_SIZE; i++)
> +		READ_DATA(mac_stats, mac_base, i) = READ_STAT(mac_stats, np, i);
> +}
> +
> +

nit: multiple empty lines, checkpatch --strict should catch this

> +static void get_strings(struct net_device *dev, u32 stringset, u8 *data)
> +{
> +	switch (stringset) {
> +	case ETH_SS_STATS:
> +		for (int i = 0; i < STATS_SIZE; i++) {
> +			memcpy(data, stats[i].string, ETH_GSTRING_LEN);
> +			data += ETH_GSTRING_LEN;

We've been getting conversion patches replacing such code with
ethtool_puts() lately, let's use it from the start.
-- 
pw-bot: cr