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

[Tobias Waldekranz <tobias@waldekranz.com>]

On tis, dec 05, 2023 at 20:07, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 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.

Fair enough. I was trying to come up with something that would make it
easy to audit the chosen mapping between "native" and "standard" counter
names, since I imagine that is what future readers of this are going to
be interested in.

>> +
>> +	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?

That sounds like a good idea.

> 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>

Thanks for the review!