Re: [PATCH v2 net-next 5/6] net: dsa: mv88e6xxx: Add "eth-mac" counter group support

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Dec 05, 2023 at 05:04:17PM +0100, Tobias Waldekranz wrote:
> Report the applicable subset of an mv88e6xxx port's counters using
> ethtool's standardized "eth-mac" counter group.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 39 ++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 473f31761b26..1a16698181fb 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1319,6 +1319,44 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>  	mv88e6xxx_get_stats(chip, port, data);
>  }
>  
> +static void mv88e6xxx_get_eth_mac_stats(struct dsa_switch *ds, int port,
> +					struct ethtool_eth_mac_stats *mac_stats)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int ret;
> +
> +	ret = mv88e6xxx_stats_snapshot(chip, port);
> +	if (ret < 0)
> +		return;
> +
> +#define MV88E6XXX_ETH_MAC_STAT_MAP(_id, _member)			\
> +	mv88e6xxx_stats_get_stat(chip, port,				\
> +				 &mv88e6xxx_hw_stats[MV88E6XXX_HW_STAT_ID_ ## _id], \
> +				 &mac_stats->stats._member)
> +
> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_unicast, FramesTransmittedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(single, SingleCollisionFrames);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(multiple, MultipleCollisionFrames);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_unicast, FramesReceivedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_fcs_error, FrameCheckSequenceErrors);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_octets, OctetsTransmittedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(deferred, FramesWithDeferredXmissions);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(late, LateCollisions);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_good_octets, OctetsReceivedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_multicasts, MulticastFramesXmittedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(out_broadcasts, BroadcastFramesXmittedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(excessive, FramesWithExcessiveDeferral);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_multicasts, MulticastFramesReceivedOK);
> +	MV88E6XXX_ETH_MAC_STAT_MAP(in_broadcasts, BroadcastFramesReceivedOK);
> +
> +#undef MV88E6XXX_ETH_MAC_STAT_MAP

I don't exactly enjoy this use (and placement) of the C preprocessor macro
when spelling out code would have worked just fine, but to each his own.
At least it is consistent in that we can jump to the other occurrences
of the statistics counter.

> +
> +	mac_stats->stats.FramesTransmittedOK += mac_stats->stats.MulticastFramesXmittedOK;
> +	mac_stats->stats.FramesTransmittedOK += mac_stats->stats.BroadcastFramesXmittedOK;
> +	mac_stats->stats.FramesReceivedOK += mac_stats->stats.MulticastFramesReceivedOK;
> +	mac_stats->stats.FramesReceivedOK += mac_stats->stats.BroadcastFramesReceivedOK;
> +}

Not sure if there's a "best thing to do" in case a previous mv88e6xxx_stats_get_stat()
call fails. In net/ethtool/stats.c we have ethtool_stats_sum(), and that's the
core saying that U64_MAX means one of the sum terms was not reported by
the driver, and it makes that transparent by simply returning the other.

Here, "not reported by the driver" is due to a bus I/O error, and using
ethtool_stats_sum() as-is would hide that error away completely, and
report only the other sum term. Sounds like a failure that would be too
silent. Whereas your proposal would just report a wildly incorrect
number - but at high data rates (for offloaded traffic, too), maybe that
wouldn't be exactly trivial to notice, either.

Maybe we need a variant of ethtool_stats_sum() that requires both terms,
otherwise returns ETHTOOL_STAT_NOT_SET?

Anyway, this is not a blocker for the current patch set, which is a bit
too large to resend for trivial matters.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>