[PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[Sean Anderson]

Implement the modern interfaces for statistics reporting.


Sean Anderson (2):
  net: cadence: macb: Convert to get_stats64
  net: cadence: macb: Report standard stats

 drivers/net/ethernet/cadence/macb.h      | 130 ++++++++--------
 drivers/net/ethernet/cadence/macb_main.c | 190 ++++++++++++++++++++---
 2 files changed, 235 insertions(+), 85 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next 1/2] net: cadence: macb: Convert to get_stats64

[Sean Anderson]

Convert the existing get_stats implementation to get_stats64. Since we
now report 64-bit values, increase the counters to 64-bits as well.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/cadence/macb.h      | 132 +++++++++++------------
 drivers/net/ethernet/cadence/macb_main.c |  34 +++---
 2 files changed, 81 insertions(+), 85 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 5740c98d8c9f..b4aa2b165bf3 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -951,75 +951,75 @@ struct macb_tx_skb {
  * device stats by a periodic timer.
  */
 struct macb_stats {
-	u32	rx_pause_frames;
-	u32	tx_ok;
-	u32	tx_single_cols;
-	u32	tx_multiple_cols;
-	u32	rx_ok;
-	u32	rx_fcs_errors;
-	u32	rx_align_errors;
-	u32	tx_deferred;
-	u32	tx_late_cols;
-	u32	tx_excessive_cols;
-	u32	tx_underruns;
-	u32	tx_carrier_errors;
-	u32	rx_resource_errors;
-	u32	rx_overruns;
-	u32	rx_symbol_errors;
-	u32	rx_oversize_pkts;
-	u32	rx_jabbers;
-	u32	rx_undersize_pkts;
-	u32	sqe_test_errors;
-	u32	rx_length_mismatch;
-	u32	tx_pause_frames;
+	u64	rx_pause_frames;
+	u64	tx_ok;
+	u64	tx_single_cols;
+	u64	tx_multiple_cols;
+	u64	rx_ok;
+	u64	rx_fcs_errors;
+	u64	rx_align_errors;
+	u64	tx_deferred;
+	u64	tx_late_cols;
+	u64	tx_excessive_cols;
+	u64	tx_underruns;
+	u64	tx_carrier_errors;
+	u64	rx_resource_errors;
+	u64	rx_overruns;
+	u64	rx_symbol_errors;
+	u64	rx_oversize_pkts;
+	u64	rx_jabbers;
+	u64	rx_undersize_pkts;
+	u64	sqe_test_errors;
+	u64	rx_length_mismatch;
+	u64	tx_pause_frames;
 };
 
 struct gem_stats {
-	u32	tx_octets_31_0;
-	u32	tx_octets_47_32;
-	u32	tx_frames;
-	u32	tx_broadcast_frames;
-	u32	tx_multicast_frames;
-	u32	tx_pause_frames;
-	u32	tx_64_byte_frames;
-	u32	tx_65_127_byte_frames;
-	u32	tx_128_255_byte_frames;
-	u32	tx_256_511_byte_frames;
-	u32	tx_512_1023_byte_frames;
-	u32	tx_1024_1518_byte_frames;
-	u32	tx_greater_than_1518_byte_frames;
-	u32	tx_underrun;
-	u32	tx_single_collision_frames;
-	u32	tx_multiple_collision_frames;
-	u32	tx_excessive_collisions;
-	u32	tx_late_collisions;
-	u32	tx_deferred_frames;
-	u32	tx_carrier_sense_errors;
-	u32	rx_octets_31_0;
-	u32	rx_octets_47_32;
-	u32	rx_frames;
-	u32	rx_broadcast_frames;
-	u32	rx_multicast_frames;
-	u32	rx_pause_frames;
-	u32	rx_64_byte_frames;
-	u32	rx_65_127_byte_frames;
-	u32	rx_128_255_byte_frames;
-	u32	rx_256_511_byte_frames;
-	u32	rx_512_1023_byte_frames;
-	u32	rx_1024_1518_byte_frames;
-	u32	rx_greater_than_1518_byte_frames;
-	u32	rx_undersized_frames;
-	u32	rx_oversize_frames;
-	u32	rx_jabbers;
-	u32	rx_frame_check_sequence_errors;
-	u32	rx_length_field_frame_errors;
-	u32	rx_symbol_errors;
-	u32	rx_alignment_errors;
-	u32	rx_resource_errors;
-	u32	rx_overruns;
-	u32	rx_ip_header_checksum_errors;
-	u32	rx_tcp_checksum_errors;
-	u32	rx_udp_checksum_errors;
+	u64	tx_octets_31_0;
+	u64	tx_octets_47_32;
+	u64	tx_frames;
+	u64	tx_broadcast_frames;
+	u64	tx_multicast_frames;
+	u64	tx_pause_frames;
+	u64	tx_64_byte_frames;
+	u64	tx_65_127_byte_frames;
+	u64	tx_128_255_byte_frames;
+	u64	tx_256_511_byte_frames;
+	u64	tx_512_1023_byte_frames;
+	u64	tx_1024_1518_byte_frames;
+	u64	tx_greater_than_1518_byte_frames;
+	u64	tx_underrun;
+	u64	tx_single_collision_frames;
+	u64	tx_multiple_collision_frames;
+	u64	tx_excessive_collisions;
+	u64	tx_late_collisions;
+	u64	tx_deferred_frames;
+	u64	tx_carrier_sense_errors;
+	u64	rx_octets_31_0;
+	u64	rx_octets_47_32;
+	u64	rx_frames;
+	u64	rx_broadcast_frames;
+	u64	rx_multicast_frames;
+	u64	rx_pause_frames;
+	u64	rx_64_byte_frames;
+	u64	rx_65_127_byte_frames;
+	u64	rx_128_255_byte_frames;
+	u64	rx_256_511_byte_frames;
+	u64	rx_512_1023_byte_frames;
+	u64	rx_1024_1518_byte_frames;
+	u64	rx_greater_than_1518_byte_frames;
+	u64	rx_undersized_frames;
+	u64	rx_oversize_frames;
+	u64	rx_jabbers;
+	u64	rx_frame_check_sequence_errors;
+	u64	rx_length_field_frame_errors;
+	u64	rx_symbol_errors;
+	u64	rx_alignment_errors;
+	u64	rx_resource_errors;
+	u64	rx_overruns;
+	u64	rx_ip_header_checksum_errors;
+	u64	rx_tcp_checksum_errors;
+	u64	rx_udp_checksum_errors;
 };
 
 /* Describes the name and offset of an individual statistic register, as
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 48496209fb16..86f0d705e354 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -990,8 +990,8 @@ static int macb_mii_init(struct macb *bp)
 
 static void macb_update_stats(struct macb *bp)
 {
-	u32 *p = &bp->hw_stats.macb.rx_pause_frames;
-	u32 *end = &bp->hw_stats.macb.tx_pause_frames + 1;
+	u64 *p = &bp->hw_stats.macb.rx_pause_frames;
+	u64 *end = &bp->hw_stats.macb.tx_pause_frames + 1;
 	int offset = MACB_PFR;
 
 	WARN_ON((unsigned long)(end - p - 1) != (MACB_TPF - MACB_PFR) / 4);
@@ -3071,7 +3071,7 @@ static void gem_update_stats(struct macb *bp)
 	unsigned int i, q, idx;
 	unsigned long *stat;
 
-	u32 *p = &bp->hw_stats.gem.tx_octets_31_0;
+	u64 *p = &bp->hw_stats.gem.tx_octets_31_0;
 
 	for (i = 0; i < GEM_STATS_LEN; ++i, ++p) {
 		u32 offset = gem_statistics[i].offset;
@@ -3094,15 +3094,12 @@ static void gem_update_stats(struct macb *bp)
 			bp->ethtool_stats[idx++] = *stat;
 }
 
-static struct net_device_stats *gem_get_stats(struct macb *bp)
+static void gem_get_stats(struct macb *bp, struct rtnl_link_stats64 *nstat)
 {
 	struct gem_stats *hwstat = &bp->hw_stats.gem;
-	struct net_device_stats *nstat = &bp->dev->stats;
 
-	if (!netif_running(bp->dev))
-		return nstat;
-
-	gem_update_stats(bp);
+	if (netif_running(bp->dev))
+		gem_update_stats(bp);
 
 	nstat->rx_errors = (hwstat->rx_frame_check_sequence_errors +
 			    hwstat->rx_alignment_errors +
@@ -3131,8 +3128,6 @@ static struct net_device_stats *gem_get_stats(struct macb *bp)
 	nstat->tx_aborted_errors = hwstat->tx_excessive_collisions;
 	nstat->tx_carrier_errors = hwstat->tx_carrier_sense_errors;
 	nstat->tx_fifo_errors = hwstat->tx_underrun;
-
-	return nstat;
 }
 
 static void gem_get_ethtool_stats(struct net_device *dev,
@@ -3183,14 +3178,17 @@ static void gem_get_ethtool_strings(struct net_device *dev, u32 sset, u8 *p)
 	}
 }
 
-static struct net_device_stats *macb_get_stats(struct net_device *dev)
+static void macb_get_stats(struct net_device *dev,
+			   struct rtnl_link_stats64 *nstat)
 {
 	struct macb *bp = netdev_priv(dev);
-	struct net_device_stats *nstat = &bp->dev->stats;
 	struct macb_stats *hwstat = &bp->hw_stats.macb;
 
-	if (macb_is_gem(bp))
-		return gem_get_stats(bp);
+	netdev_stats_to_stats64(nstat, &bp->dev->stats);
+	if (macb_is_gem(bp)) {
+		gem_get_stats(bp, nstat);
+		return;
+	}
 
 	/* read stats from hardware */
 	macb_update_stats(bp);
@@ -3226,8 +3224,6 @@ static struct net_device_stats *macb_get_stats(struct net_device *dev)
 	nstat->tx_carrier_errors = hwstat->tx_carrier_errors;
 	nstat->tx_fifo_errors = hwstat->tx_underruns;
 	/* Don't know about heartbeat or window errors... */
-
-	return nstat;
 }
 
 static int macb_get_regs_len(struct net_device *netdev)
@@ -3910,7 +3906,7 @@ static const struct net_device_ops macb_netdev_ops = {
 	.ndo_stop		= macb_close,
 	.ndo_start_xmit		= macb_start_xmit,
 	.ndo_set_rx_mode	= macb_set_rx_mode,
-	.ndo_get_stats		= macb_get_stats,
+	.ndo_get_stats64	= macb_get_stats,
 	.ndo_eth_ioctl		= macb_ioctl,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_change_mtu		= macb_change_mtu,
@@ -4571,7 +4567,7 @@ static const struct net_device_ops at91ether_netdev_ops = {
 	.ndo_open		= at91ether_open,
 	.ndo_stop		= at91ether_close,
 	.ndo_start_xmit		= at91ether_start_xmit,
-	.ndo_get_stats		= macb_get_stats,
+	.ndo_get_stats64	= macb_get_stats,
 	.ndo_set_rx_mode	= macb_set_rx_mode,
 	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_eth_ioctl		= macb_ioctl,
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next 2/2] net: cadence: macb: Report standard stats

[Sean Anderson]

Report standard statistics using the dedicated callbacks instead of
get_ethtool_stats.

OCTTX is split over two registers. Accumulating these registers
separately in gem_stats just means we need to combine them again later.
Instead, combine these stats before saving them, like is done for
ethtool_stats.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/cadence/macb.h      |   6 +-
 drivers/net/ethernet/cadence/macb_main.c | 160 ++++++++++++++++++++++-
 2 files changed, 160 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index b4aa2b165bf3..f69b2b7c8802 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -975,8 +975,7 @@ struct macb_stats {
 };
 
 struct gem_stats {
-	u64	tx_octets_31_0;
-	u64	tx_octets_47_32;
+	u64	tx_octets;
 	u64	tx_frames;
 	u64	tx_broadcast_frames;
 	u64	tx_multicast_frames;
@@ -995,8 +994,7 @@ struct gem_stats {
 	u64	tx_late_collisions;
 	u64	tx_deferred_frames;
 	u64	tx_carrier_sense_errors;
-	u64	rx_octets_31_0;
-	u64	rx_octets_47_32;
+	u64	rx_octets;
 	u64	rx_frames;
 	u64	rx_broadcast_frames;
 	u64	rx_multicast_frames;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 86f0d705e354..4878c14121fb 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3071,7 +3071,7 @@ static void gem_update_stats(struct macb *bp)
 	unsigned int i, q, idx;
 	unsigned long *stat;
 
-	u64 *p = &bp->hw_stats.gem.tx_octets_31_0;
+	u64 *p = &bp->hw_stats.gem.tx_octets;
 
 	for (i = 0; i < GEM_STATS_LEN; ++i, ++p) {
 		u32 offset = gem_statistics[i].offset;
@@ -3084,7 +3084,7 @@ static void gem_update_stats(struct macb *bp)
 			/* Add GEM_OCTTXH, GEM_OCTRXH */
 			val = bp->macb_reg_readl(bp, offset + 4);
 			bp->ethtool_stats[i] += ((u64)val) << 32;
-			*(++p) += val;
+			*(p++) += ((u64)val) << 32;
 		}
 	}
 
@@ -3226,6 +3226,154 @@ static void macb_get_stats(struct net_device *dev,
 	/* Don't know about heartbeat or window errors... */
 }
 
+static void macb_get_pause_stats(struct net_device *dev,
+				 struct ethtool_pause_stats *pause_stats)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct macb_stats *hwstat = &bp->hw_stats.macb;
+
+	macb_update_stats(bp);
+	pause_stats->tx_pause_frames = hwstat->tx_pause_frames;
+	pause_stats->rx_pause_frames = hwstat->rx_pause_frames;
+}
+
+static void gem_get_pause_stats(struct net_device *dev,
+				struct ethtool_pause_stats *pause_stats)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct gem_stats *hwstat = &bp->hw_stats.gem;
+
+	gem_update_stats(bp);
+	pause_stats->tx_pause_frames = hwstat->tx_pause_frames;
+	pause_stats->rx_pause_frames = hwstat->rx_pause_frames;
+}
+
+static void macb_get_eth_mac_stats(struct net_device *dev,
+				   struct ethtool_eth_mac_stats *mac_stats)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct macb_stats *hwstat = &bp->hw_stats.macb;
+
+	macb_update_stats(bp);
+	mac_stats->FramesTransmittedOK = hwstat->tx_ok;
+	mac_stats->SingleCollisionFrames = hwstat->tx_single_cols;
+	mac_stats->MultipleCollisionFrames = hwstat->tx_multiple_cols;
+	mac_stats->FramesReceivedOK = hwstat->rx_ok;
+	mac_stats->FrameCheckSequenceErrors = hwstat->rx_fcs_errors;
+	mac_stats->AlignmentErrors = hwstat->rx_align_errors;
+	mac_stats->FramesWithDeferredXmissions = hwstat->tx_deferred;
+	mac_stats->LateCollisions = hwstat->tx_late_cols;
+	mac_stats->FramesAbortedDueToXSColls = hwstat->tx_excessive_cols;
+	mac_stats->FramesLostDueToIntMACXmitError = hwstat->tx_underruns;
+	mac_stats->CarrierSenseErrors = hwstat->tx_carrier_errors;
+	mac_stats->FramesLostDueToIntMACRcvError = hwstat->rx_overruns;
+	mac_stats->InRangeLengthErrors = hwstat->rx_length_mismatch;
+	mac_stats->FrameTooLongErrors = hwstat->rx_oversize_pkts;
+}
+
+static void gem_get_eth_mac_stats(struct net_device *dev,
+				  struct ethtool_eth_mac_stats *mac_stats)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct gem_stats *hwstat = &bp->hw_stats.gem;
+
+	gem_update_stats(bp);
+	mac_stats->FramesTransmittedOK = hwstat->tx_frames;
+	mac_stats->SingleCollisionFrames = hwstat->tx_single_collision_frames;
+	mac_stats->MultipleCollisionFrames =
+		hwstat->tx_multiple_collision_frames;
+	mac_stats->FramesReceivedOK = hwstat->rx_frames;
+	mac_stats->FrameCheckSequenceErrors =
+		hwstat->rx_frame_check_sequence_errors;
+	mac_stats->AlignmentErrors = hwstat->rx_alignment_errors;
+	mac_stats->OctetsTransmittedOK = hwstat->tx_octets;
+	mac_stats->FramesWithDeferredXmissions = hwstat->tx_deferred_frames;
+	mac_stats->LateCollisions = hwstat->tx_late_collisions;
+	mac_stats->FramesAbortedDueToXSColls = hwstat->tx_excessive_collisions;
+	mac_stats->FramesLostDueToIntMACXmitError = hwstat->tx_underrun;
+	mac_stats->CarrierSenseErrors = hwstat->tx_carrier_sense_errors;
+	mac_stats->OctetsReceivedOK = hwstat->rx_octets;
+	mac_stats->MulticastFramesXmittedOK = hwstat->tx_multicast_frames;
+	mac_stats->BroadcastFramesXmittedOK = hwstat->tx_broadcast_frames;
+	mac_stats->MulticastFramesReceivedOK = hwstat->rx_multicast_frames;
+	mac_stats->BroadcastFramesReceivedOK = hwstat->rx_broadcast_frames;
+	mac_stats->InRangeLengthErrors = hwstat->rx_length_field_frame_errors;
+	mac_stats->FrameTooLongErrors = hwstat->rx_oversize_frames;
+}
+
+/* TODO: Report SQE test errors when added to phy_stats */
+static void macb_get_eth_phy_stats(struct net_device *dev,
+				   struct ethtool_eth_phy_stats *phy_stats)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct macb_stats *hwstat = &bp->hw_stats.macb;
+
+	macb_update_stats(bp);
+	phy_stats->SymbolErrorDuringCarrier = hwstat->rx_symbol_errors;
+}
+
+static void gem_get_eth_phy_stats(struct net_device *dev,
+				  struct ethtool_eth_phy_stats *phy_stats)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct gem_stats *hwstat = &bp->hw_stats.gem;
+
+	gem_update_stats(bp);
+	phy_stats->SymbolErrorDuringCarrier = hwstat->rx_symbol_errors;
+}
+
+static void macb_get_rmon_stats(struct net_device *dev,
+				struct ethtool_rmon_stats *rmon_stats,
+				const struct ethtool_rmon_hist_range **ranges)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct macb_stats *hwstat = &bp->hw_stats.macb;
+
+	macb_update_stats(bp);
+	rmon_stats->undersize_pkts = hwstat->rx_undersize_pkts;
+	rmon_stats->oversize_pkts = hwstat->rx_oversize_pkts;
+	rmon_stats->jabbers = hwstat->rx_jabbers;
+}
+
+static const struct ethtool_rmon_hist_range gem_rmon_ranges[] = {
+	{   64,    64 },
+	{   65,   127 },
+	{  128,   255 },
+	{  256,   511 },
+	{  512,  1023 },
+	{ 1024,  1518 },
+	{ 1519, 16384 },
+	{ },
+};
+
+static void gem_get_rmon_stats(struct net_device *dev,
+			       struct ethtool_rmon_stats *rmon_stats,
+			       const struct ethtool_rmon_hist_range **ranges)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct gem_stats *hwstat = &bp->hw_stats.gem;
+
+	gem_update_stats(bp);
+	rmon_stats->undersize_pkts = hwstat->rx_undersized_frames;
+	rmon_stats->oversize_pkts = hwstat->rx_oversize_frames;
+	rmon_stats->jabbers = hwstat->rx_jabbers;
+	rmon_stats->hist[0] = hwstat->rx_64_byte_frames;
+	rmon_stats->hist[1] = hwstat->rx_65_127_byte_frames;
+	rmon_stats->hist[2] = hwstat->rx_128_255_byte_frames;
+	rmon_stats->hist[3] = hwstat->rx_256_511_byte_frames;
+	rmon_stats->hist[4] = hwstat->rx_512_1023_byte_frames;
+	rmon_stats->hist[5] = hwstat->rx_1024_1518_byte_frames;
+	rmon_stats->hist[6] = hwstat->rx_greater_than_1518_byte_frames;
+	rmon_stats->hist_tx[0] = hwstat->tx_64_byte_frames;
+	rmon_stats->hist_tx[1] = hwstat->tx_65_127_byte_frames;
+	rmon_stats->hist_tx[2] = hwstat->tx_128_255_byte_frames;
+	rmon_stats->hist_tx[3] = hwstat->tx_256_511_byte_frames;
+	rmon_stats->hist_tx[4] = hwstat->tx_512_1023_byte_frames;
+	rmon_stats->hist_tx[5] = hwstat->tx_1024_1518_byte_frames;
+	rmon_stats->hist_tx[6] = hwstat->tx_greater_than_1518_byte_frames;
+	*ranges = gem_rmon_ranges;
+}
+
 static int macb_get_regs_len(struct net_device *netdev)
 {
 	return MACB_GREGS_NBR * sizeof(u32);
@@ -3752,6 +3900,10 @@ static const struct ethtool_ops macb_ethtool_ops = {
 	.get_regs		= macb_get_regs,
 	.get_link		= ethtool_op_get_link,
 	.get_ts_info		= ethtool_op_get_ts_info,
+	.get_pause_stats	= macb_get_pause_stats,
+	.get_eth_mac_stats	= macb_get_eth_mac_stats,
+	.get_eth_phy_stats	= macb_get_eth_phy_stats,
+	.get_rmon_stats		= macb_get_rmon_stats,
 	.get_wol		= macb_get_wol,
 	.set_wol		= macb_set_wol,
 	.get_link_ksettings     = macb_get_link_ksettings,
@@ -3770,6 +3922,10 @@ static const struct ethtool_ops gem_ethtool_ops = {
 	.get_ethtool_stats	= gem_get_ethtool_stats,
 	.get_strings		= gem_get_ethtool_strings,
 	.get_sset_count		= gem_get_sset_count,
+	.get_pause_stats	= gem_get_pause_stats,
+	.get_eth_mac_stats	= gem_get_eth_mac_stats,
+	.get_eth_phy_stats	= gem_get_eth_phy_stats,
+	.get_rmon_stats		= gem_get_rmon_stats,
 	.get_link_ksettings     = macb_get_link_ksettings,
 	.set_link_ksettings     = macb_set_link_ksettings,
 	.get_ringparam		= macb_get_ringparam,
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH net-next 1/2] net: cadence: macb: Convert to get_stats64

[Andrew Lunn]

On Fri, Feb 14, 2025 at 04:27:02PM -0500, Sean Anderson wrote:
> Convert the existing get_stats implementation to get_stats64. Since we
> now report 64-bit values, increase the counters to 64-bits as well.

It would be good to add some comments about why this is safe on 32 bit
systems. Are the needed locks in place that you cannot see a partial
update?

	Andrew

Re: [PATCH net-next 2/2] net: cadence: macb: Report standard stats

[Andrew Lunn]

> +static const struct ethtool_rmon_hist_range gem_rmon_ranges[] = {
> +	{   64,    64 },
> +	{   65,   127 },
> +	{  128,   255 },
> +	{  256,   511 },
> +	{  512,  1023 },
> +	{ 1024,  1518 },
> +	{ 1519, 16384 },
> +	{ },
> +};

static const struct ethtool_rmon_hist_range a5psw_rmon_ranges[] = {
	{ 0, 64 },
	{ 65, 127 },
	{ 128, 255 },
	{ 256, 511 },
	{ 512, 1023 },
	{ 1024, 1518 },
	{ 1519, A5PSW_MAX_MTU },
	{}
};

static const struct ethtool_rmon_hist_range axienet_rmon_ranges[] = {
        {   64,    64 },
        {   65,   127 },
        {  128,   255 },
        {  256,   511 },
        {  512,  1023 },
        { 1024,  1518 },
        { 1519, 16384 },
        { },
};

static const struct ethtool_rmon_hist_range bcmasp_rmon_ranges[] = {
        {    0,   64},
        {   65,  127},
        {  128,  255},
        {  256,  511},
        {  512, 1023},
        { 1024, 1518},
        { 1519, 1522},
        {}
};

static const struct ethtool_rmon_hist_range mlxsw_rmon_ranges[] = {
        {    0,    64 },
        {   65,   127 },
        {  128,   255 },
        {  256,   511 },
        {  512,  1023 },
        { 1024,  1518 },
        { 1519,  2047 },
        { 2048,  4095 },
        { 4096,  8191 },
        { 8192, 10239 },
        {}
};

static const struct ethtool_rmon_hist_range mlx5e_rmon_ranges[] = {
        {    0,    64 },
        {   65,   127 },
        {  128,   255 },
        {  256,   511 },
        {  512,  1023 },
        { 1024,  1518 },
        { 1519,  2047 },
        { 2048,  4095 },
        { 4096,  8191 },
        { 8192, 10239 },
        {}
};

Could we maybe have one central table which drivers share? I assume
IETF defined these bands as part or RMON?

	Andrew

Re: [PATCH net-next 2/2] net: cadence: macb: Report standard stats

[Jakub Kicinski]

On Sat, 15 Feb 2025 00:14:35 +0100 Andrew Lunn wrote:
> Could we maybe have one central table which drivers share? I assume
> IETF defined these bands as part or RMON?

IIRC RMON standardizes these three:

        {    0,    64 },
        {   65,   127 },
        {  128,   255 },
        {  256,   511 },
        {  512,  1023 },
        { 1024,  1518 },

Once we get into jumbo (as you probably noticed) the tables start 
to diverge, mostly because max MTU is different.

On one hand common code is nice, on the other I don't think defining
this table has ever been a source of confusion, and common table won't
buy users anything. But, would be yet another thing we have to check
in review, no?

Re: [PATCH net-next 1/2] net: cadence: macb: Convert to get_stats64

[Sean Anderson]

On 2/14/25 17:59, Andrew Lunn wrote:
> On Fri, Feb 14, 2025 at 04:27:02PM -0500, Sean Anderson wrote:
>> Convert the existing get_stats implementation to get_stats64. Since we
>> now report 64-bit values, increase the counters to 64-bits as well.
> 
> It would be good to add some comments about why this is safe on 32 bit
> systems. Are the needed locks in place that you cannot see a partial
> update?

Well, it's no more unsafe than the current code which does RMW without
synchronization. So I guess I'll send another patch to add some locking.

--Sean

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 14 Feb 2025 16:27:01 -0500 you wrote:
> Implement the modern interfaces for statistics reporting.
> 
> 
> Sean Anderson (2):
>   net: cadence: macb: Convert to get_stats64
>   net: cadence: macb: Report standard stats
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: cadence: macb: Convert to get_stats64
    https://git.kernel.org/netdev/net-next/c/75696dd0fd72
  - [net-next,2/2] net: cadence: macb: Report standard stats
    https://git.kernel.org/netdev/net-next/c/f6af690a295a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[Sean Anderson]

On 2/18/25 21:10, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This series was applied to netdev/net-next.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
> 
> On Fri, 14 Feb 2025 16:27:01 -0500 you wrote:
>> Implement the modern interfaces for statistics reporting.
>> 
>> 
>> Sean Anderson (2):
>>   net: cadence: macb: Convert to get_stats64
>>   net: cadence: macb: Report standard stats
>> 
>> [...]
> 
> Here is the summary with links:
>   - [net-next,1/2] net: cadence: macb: Convert to get_stats64
>     https://git.kernel.org/netdev/net-next/c/75696dd0fd72
>   - [net-next,2/2] net: cadence: macb: Report standard stats
>     https://git.kernel.org/netdev/net-next/c/f6af690a295a
> 
> You are awesome, thank you!

I think this should be deferred until v2 of [1] is applied, to make
backporting the fix easier.

--Sean

[1] lore.kernel.org/netdev/20250218195036.37137-1-sean.anderson@linux.dev

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[Jakub Kicinski]

On Thu, 20 Feb 2025 11:01:10 -0500 Sean Anderson wrote:
> > Here is the summary with links:
> >   - [net-next,1/2] net: cadence: macb: Convert to get_stats64
> >     https://git.kernel.org/netdev/net-next/c/75696dd0fd72
> >   - [net-next,2/2] net: cadence: macb: Report standard stats
> >     https://git.kernel.org/netdev/net-next/c/f6af690a295a
> > 
> > You are awesome, thank you!  
> 
> I think this should be deferred until v2 of [1] is applied, to make
> backporting the fix easier.

ETOOLATE :) networking trees don't rebase

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[Sean Anderson]

On 2/20/25 11:59, Jakub Kicinski wrote:
> On Thu, 20 Feb 2025 11:01:10 -0500 Sean Anderson wrote:
>> > Here is the summary with links:
>> >   - [net-next,1/2] net: cadence: macb: Convert to get_stats64
>> >     https://git.kernel.org/netdev/net-next/c/75696dd0fd72
>> >   - [net-next,2/2] net: cadence: macb: Report standard stats
>> >     https://git.kernel.org/netdev/net-next/c/f6af690a295a
>> > 
>> > You are awesome, thank you!  
>> 
>> I think this should be deferred until v2 of [1] is applied, to make
>> backporting the fix easier.
> 
> ETOOLATE :) networking trees don't rebase

Well, I was expecting a revert and then I would resend this series once
the linked patch was applied.

I guess I can send another patch adding locking?

--Sean

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[Jakub Kicinski]

On Thu, 20 Feb 2025 12:25:09 -0500 Sean Anderson wrote:
> >> I think this should be deferred until v2 of [1] is applied, to make
> >> backporting the fix easier.  
> > 
> > ETOOLATE :) networking trees don't rebase  
> 
> Well, I was expecting a revert and then I would resend this series once
> the linked patch was applied.
> 
> I guess I can send another patch adding locking?

Assuming the fix is applied to net:
Will the trees conflict?
If no - will the code in net still work (just lacking lock protection)?

If there is a conflict you can share a resolution with me and I'll slap
it on as part of the merge.

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[Sean Anderson]

On 2/20/25 13:18, Jakub Kicinski wrote:
> On Thu, 20 Feb 2025 12:25:09 -0500 Sean Anderson wrote:
>> >> I think this should be deferred until v2 of [1] is applied, to make
>> >> backporting the fix easier.  
>> > 
>> > ETOOLATE :) networking trees don't rebase  
>> 
>> Well, I was expecting a revert and then I would resend this series once
>> the linked patch was applied.
>> 
>> I guess I can send another patch adding locking?
> 
> Assuming the fix is applied to net:
> Will the trees conflict?

Yes.

> If no - will the code in net still work (just lacking lock protection)?
> 
> If there is a conflict you can share a resolution with me and I'll slap
> it on as part of the merge.

OK, what's the best way to create that? git rerere?

--Sean

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[Jakub Kicinski]

On Thu, 20 Feb 2025 13:22:29 -0500 Sean Anderson wrote:
> > If no - will the code in net still work (just lacking lock protection)?
> > 
> > If there is a conflict you can share a resolution with me and I'll slap
> > it on as part of the merge.  
> 
> OK, what's the best way to create that? git rerere?

rerere image or a three way diff will work

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting

[Sean Anderson]

On 2/20/25 13:32, Jakub Kicinski wrote:
> On Thu, 20 Feb 2025 13:22:29 -0500 Sean Anderson wrote:
>> > If no - will the code in net still work (just lacking lock protection)?
>> > 
>> > If there is a conflict you can share a resolution with me and I'll slap
>> > it on as part of the merge.  
>> 
>> OK, what's the best way to create that? git rerere?
> 
> rerere image or a three way diff will work

--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3098,16 +3098,9 @@
 {
 	struct gem_stats *hwstat = &bp->hw_stats.gem;
 
-<<<<<<<
-	if (!netif_running(bp->dev))
-		return nstat;
-
 	spin_lock_irq(&bp->stats_lock);
-	gem_update_stats(bp);
-=======
 	if (netif_running(bp->dev))
 		gem_update_stats(bp);
->>>>>>>
 
 	nstat->rx_errors = (hwstat->rx_frame_check_sequence_errors +
 			    hwstat->rx_alignment_errors +
@@ -3136,12 +3129,7 @@
 	nstat->tx_aborted_errors = hwstat->tx_excessive_collisions;
 	nstat->tx_carrier_errors = hwstat->tx_carrier_sense_errors;
 	nstat->tx_fifo_errors = hwstat->tx_underrun;
-<<<<<<<
-=======
 	spin_unlock_irq(&bp->stats_lock);
-
-	return nstat;
->>>>>>>
 }
 
 static void gem_get_ethtool_stats(struct net_device *dev,
@@ -3240,11 +3228,8 @@
 	nstat->tx_carrier_errors = hwstat->tx_carrier_errors;
 	nstat->tx_fifo_errors = hwstat->tx_underruns;
 	/* Don't know about heartbeat or window errors... */
-<<<<<<<
 	spin_unlock_irq(&bp->stats_lock);
-=======
 }
->>>>>>>
 
 static void macb_get_pause_stats(struct net_device *dev,
 				 struct ethtool_pause_stats *pause_stats)

Re: [PATCH net-next 0/2] net: cadence: macb: Modernize statistics reporting (manual merge)

[Jakub Kicinski]

On Thu, 20 Feb 2025 13:42:25 -0500 Sean Anderson wrote:
> On 2/20/25 13:32, Jakub Kicinski wrote:
> > On Thu, 20 Feb 2025 13:22:29 -0500 Sean Anderson wrote:  
> >> OK, what's the best way to create that? git rerere?  
> > 
> > rerere image or a three way diff will work  
> 
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

That should work, thanks!