Re: [PATCH net-next v11 9/9] net: dsa: lan9645x: add port statistics

This is a reply to:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a%40microchip.com?part=9

b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c

quoted

index fc604edb309b9..6a14ba3661625 100644

--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c

[ ... ]

quoted

@@ -800,6 +866,18 @@ static const struct dsa_switch_ops lan9645x_switch_ops = {
 	/* Multicast database */
 	.port_mdb_add			= lan9645x_mdb_add,
 	.port_mdb_del			= lan9645x_mdb_del,
+
+	/* Port statistics counters. */
+	.get_strings			= lan9645x_get_strings,
+	.get_ethtool_stats		= lan9645x_get_ethtool_stats,
+	.get_sset_count			= lan9645x_get_sset_count,
+	.get_eth_mac_stats		= lan9645x_get_eth_mac_stats,
+	.get_rmon_stats			= lan9645x_get_rmon_stats,
+	.get_stats64			= lan9645x_get_stats64,
+	.get_pause_stats		= lan9645x_get_pause_stats,
+	.get_mm_stats			= lan9645x_get_mm_stats,

Discovered by: opus-5 · Missed by: sashiko-gemini

Is .get_mm_stats reachable without a .get_mm implementation?  A grep over
drivers/net/dsa/microchip/lan9645x/ finds only get_mm_stats, neither
.get_mm nor .set_mm is provided.

dsa_user_get_mm() refuses the request in that case:

net/dsa/user.c:dsa_user_get_mm() {
	if (!ds->ops->get_mm)
		return -EOPNOTSUPP;
	...
}

and mm_prepare_data() leaves before ->get_mm_stats() is invoked:

net/ethtool/mm.c:mm_prepare_data() {
	ret = ops->get_mm(dev, &data->state);
	if (ret)
		goto out_complete;

	if (ops->get_mm_stats && (req_base->flags & ETHTOOL_FLAG_STATS))
		ops->get_mm_stats(dev, &data->stats);
	...
}

So does lan9645x_stats_get_mm_stats() ever run, and does
ethtool --show-mm --include-statistics just return -EOPNOTSUPP?

The same missing .get_mm makes __ethtool_dev_mm_supported() always return
false, and both stats_prepare_data() (net/ethtool/stats.c) and
pause_prepare_data() (net/ethtool/pause.c) reject requests carrying
src == EMAC or src == PMAC on that basis.  Does that leave the
ETHTOOL_MAC_STATS_SRC_EMAC and ETHTOOL_MAC_STATS_SRC_PMAC branches of
lan9645x_stats_get_eth_mac_stats(), lan9645x_stats_get_rmon_stats(),
lan9645x_stats_get_pause_stats(), lan9645x_stats_get_eth_phy_stats() and
lan9645x_stats_get_eth_ctrl_stats() unreachable as well, with only the
default aggregate branch ever executing?

Yes, this is also correct. I will remove the mac merge related code until mac
merge itself lands in the driver.

quoted

+	.get_eth_phy_stats		= lan9645x_get_eth_phy_stats,
+	.get_eth_ctrl_stats		= lan9645x_get_eth_ctrl_stats,
 };

[ ... ]

quoted

diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c

b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c

quoted

new file mode 100644
index 0000000000000..f9c4bcb308991

--- /dev/null
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c

@@ -0,0 +1,935 @@

[ ... ]

quoted

+void lan9645x_stats_get_stats64(struct lan9645x *lan9645x, int port,
+				struct rtnl_link_stats64 *stats)
+{

[ ... ]

quoted

+	stats->rx_packets = c[SCNT_RX_SHORT] +
+			    c[SCNT_RX_FRAG] +
+			    c[SCNT_RX_JABBER] +
+			    c[SCNT_RX_SZ_64] +
+			    c[SCNT_RX_SZ_65_127] +
+			    c[SCNT_RX_SZ_128_255] +
+			    c[SCNT_RX_SZ_256_511] +
+			    c[SCNT_RX_SZ_512_1023] +
+			    c[SCNT_RX_SZ_1024_1526] +
+			    c[SCNT_RX_SZ_JUMBO] +
+			    c[SCNT_RX_LONG] +
+			    c[SCNT_RX_PMAC_SHORT] +
+			    c[SCNT_RX_PMAC_FRAG] +
+			    c[SCNT_RX_PMAC_JABBER] +
+			    c[SCNT_RX_PMAC_SZ_64] +
+			    c[SCNT_RX_PMAC_SZ_65_127] +
+			    c[SCNT_RX_PMAC_SZ_128_255] +
+			    c[SCNT_RX_PMAC_SZ_256_511] +
+			    c[SCNT_RX_PMAC_SZ_512_1023] +
+			    c[SCNT_RX_PMAC_SZ_1024_1526] +
+			    c[SCNT_RX_PMAC_SZ_JUMBO];

Discovered by: opus-5 · Missed by: sashiko-gemini

Is the missing c[SCNT_RX_PMAC_LONG] here intentional?  The eMAC oversize
counter c[SCNT_RX_LONG] is summed into rx_packets, but the pMAC term list
stops at SCNT_RX_PMAC_SZ_JUMBO.  The same asymmetry appears in
stats->rx_dropped below, which also starts from c[SCNT_RX_LONG] alone.

Yes, this is a mistake, I will add it.

Every other term in this function is paired, and rx_errors does include
the pMAC counterpart:

	stats->rx_errors = c[SCNT_RX_SHORT] +
			   ...
			   c[SCNT_RX_PMAC_LONG];

SCNT_RX_PMAC_LONG (offset 0x42) is present in
lan9645x_port_stats_layout[] and is accumulated by
__lan9645x_stats_view_idx_transfer(), so on a port running frame
preemption, would oversize frames on the preemptible MAC show up in
rx_errors while being absent from rx_packets and rx_dropped?

quoted

+
+	stats->multicast = c[SCNT_RX_MC] + c[SCNT_RX_PMAC_MC];
+
+	stats->rx_errors = c[SCNT_RX_SHORT] +
+			   c[SCNT_RX_FRAG] +
+			   c[SCNT_RX_JABBER] +
+			   c[SCNT_RX_CRC] +
+			   c[SCNT_RX_SYMBOL_ERR] +
+			   c[SCNT_RX_LONG] +
+			   c[SCNT_RX_PMAC_SHORT] +
+			   c[SCNT_RX_PMAC_FRAG] +
+			   c[SCNT_RX_PMAC_JABBER] +
+			   c[SCNT_RX_PMAC_CRC] +
+			   c[SCNT_RX_PMAC_SYMBOL_ERR] +
+			   c[SCNT_RX_PMAC_LONG];
+
+	stats->rx_dropped = c[SCNT_RX_LONG] +
+			    c[SCNT_DR_LOCAL] +
+			    c[SCNT_DR_TAIL] +
+			    c[SCNT_RX_CAT_DROP] +

Discovered by: opus-5 · Missed by: sashiko-gemini

Should one oversize frame be reported three times?  c[SCNT_RX_LONG] is
added to rx_packets, to rx_errors and to rx_dropped, so a single frame
appears in all three fields of ip -s link and /proc/net/dev.  The same
applies, twice over, to c[SCNT_RX_SHORT], c[SCNT_RX_FRAG],
c[SCNT_RX_JABBER] and their pMAC twins, which land in both rx_packets and
rx_errors.

The uAPI kernel-doc in include/uapi/linux/if_link.h describes these as
distinct categories:

 * @rx_packets: Number of good packets received by the interface.
 * @rx_errors: Total number of bad packets received on this network device.
 * @rx_dropped: Number of packets received but not processed, e.g. due to
 *	lack of resources or unsupported protocol.

and points length violations at @rx_length_errors, which this driver
leaves at zero.

For comparison, ocelot_port_get_stats64()
(drivers/net/ethernet/mscc/ocelot_stats.c) does fold SHORTS, FRAGMENTS,
JABBERS and LONGS into rx_packets, but it does not populate rx_errors at
all and does not add RX_LONGS to rx_dropped.  Was RX_LONG in rx_dropped
meant to be rx_length_errors instead?

I will add rx_length_errors, and move RX_LONG from rx_dropped to there.

quoted

+			    c[SCNT_RX_RED_PRIO_0] +

[ ... ]

quoted

+			    c[SCNT_RX_RED_PRIO_7];

[ ... ]

quoted

+void lan9645x_stats_init(struct lan9645x *lan9645x)
+{
+	struct lan9645x_stats *stats = lan9645x->stats;
+
+	queue_delayed_work(stats->queue, &stats->work,
+			   LAN9645X_STATS_CHECK_DELAY);
+}

Discovered by: opus-5 · Missed by: sashiko-gemini

Where does the software shadow and the hardware counter get a common
baseline?

lan9645x_stats_add_cnt() keeps the previous hardware value in the low half
of the shadow and infers a wrap from it:

	if (val < (*cnt & U32_MAX))
		*cnt += (u64)1 << 32; /* value has wrapped */

	*cnt = (*cnt & ~(u64)U32_MAX) + val;

vstat->cnts is devm_kcalloc()'d once in lan9645x_stats_alloc() during
probe and never re-zeroed, and SYS_STAT_CFG.STAT_CLEAR_SHOT is defined in
lan9645x_regs.h but never written.  lan9645x_stats_init() runs from
lan9645x_setup() after lan9645x_reset_switch() has driven
SYS_RESET_CFG.CORE_ENA=0 and run SYS_RAM_INIT, and it only re-arms the
poll work.

If the core reset does clear the counter RAM, then on a second .setup()
of an already-probed instance (DSA tree teardown and re-setup, which the
.setup/.teardown stats hooks appear written for) hardware restarts at 0
while the shadow still holds the old values, so the first poll sees
val = 0 < (*cnt & U32_MAX) for every non-zero counter and adds
4294967296 to each, permanently.

If the reset does not clear them, an unbind/rebind starts with a zeroed
shadow against non-zero hardware, so the fresh interface reports traffic
it never saw.

ocelot_init() clears the counters explicitly for the same block:

drivers/net/ethernet/mscc/ocelot.c:ocelot_init() {
		/* Clear all counters (5 groups) */
		ocelot_write(ocelot, SYS_STAT_CFG_STAT_VIEW(port) |
				     SYS_STAT_CFG_STAT_CLEAR_SHOT(0x7f),
			     SYS_STAT_CFG);
}

Would it make sense to clear the hardware counters and/or memset the
shadow in lan9645x_stats_init() so the two always start together?

Yes I think this makes sense, I will add this.

[ ... ]

quoted

diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.h

b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.h

quoted

new file mode 100644
index 0000000000000..cd5db16258a09

--- /dev/null
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.h

@@ -0,0 +1,279 @@

[ ... ]

quoted

+/* Counter indices into stat layout structs */
+#define SCNT_FRER_SID_IN_PKT             0
+#define SCNT_ISDX_GREEN_OCT              1

[ ... ]

quoted

+#define SCNT_SF_MATCHING_FRAMES_COUNT    0

[ ... ]

quoted

+#define SCNT_ESDX_GREEN_OCT              0

[ ... ]

quoted

+enum lan9645x_view_stat_type {
+	LAN9645X_STAT_PORTS = 0,
+	LAN9645X_STAT_ISDX,
+	LAN9645X_STAT_ESDX,
+	LAN9645X_STAT_SFID,
+
+	LAN9645X_STAT_NUM,
+};

Discovered by: opus-5 · Missed by: sashiko-gemini

This isn't a bug, but the ISDX, ESDX, SFID and FRER parts of this
framework have no user in the patch.  A grep over
drivers/net/dsa/microchip/lan9645x/ shows SCNT_ISDX_*, SCNT_SF_*,
SCNT_ESDX_* and SCNT_FRER_SID_IN_PKT matching only in this header, and
lan9645x_view_stat_cfgs[] populates just [LAN9645X_STAT_PORTS].

lan9645x_stats_view_update() has PORTS as its only non-default case, so
the loop in lan9645x_stats_update():

	for (int vtype = 0; vtype < LAN9645X_STAT_NUM; vtype++)
		lan9645x_stats_view_update(lan9645x, vtype);

does three no-op iterations over zeroed view[] entries on every poll.

Also, struct lan9645x_view_stats.name is only ever written
(.name = "ports") and never read, and the back-pointer assignment
vstat->stats = lan9645x->stats in lan9645x_view_stat_init() has no
reader.  Since the commit message only describes adding "statistics
support for the port counters", could the unused infrastructure be
dropped until the view that needs it lands?

Yes I will remove the unused views and fields.

[ ... ]