| Message ID | 20231205160418.3770042-6-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Add "eth-mac" and "rmon" counter group support | expand |
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>
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!
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 + + 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; +} + static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port) { struct mv88e6xxx_chip *chip = ds->priv; @@ -6838,6 +6876,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = { .phylink_mac_link_up = mv88e6xxx_mac_link_up, .get_strings = mv88e6xxx_get_strings, .get_ethtool_stats = mv88e6xxx_get_ethtool_stats, + .get_eth_mac_stats = mv88e6xxx_get_eth_mac_stats, .get_sset_count = mv88e6xxx_get_sset_count, .port_max_mtu = mv88e6xxx_get_max_mtu, .port_change_mtu = mv88e6xxx_change_mtu,
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(+)