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

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

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

> --- Patch [9]: [PATCH net-next v11 9/9] net: dsa: lan9645x: add port statistics ---
> commit 237edd60065db45a5dc8339c001c3205645d61b0
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
>
> net: dsa: lan9645x: add port statistics
>
> This adds a new lan9645x_stats unit which exposes the per-port hardware
> counters through the DSA ethtool hooks and ndo_get_stats64.  Since the chip
> counters are 32 bit, a 64-bit software shadow is kept and refreshed by a
> delayed work every three seconds.  No Link: tags are present in the commit
> header.
>
> A few questions below.
>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index fc604edb309b9..6a14ba3661625 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
>
> [ ... ]
>
> > @@ -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.

> > +	.get_eth_phy_stats		= lan9645x_get_eth_phy_stats,
> > +	.get_eth_ctrl_stats		= lan9645x_get_eth_ctrl_stats,
> >  };
>
> [ ... ]
>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> > new file mode 100644
> > index 0000000000000..f9c4bcb308991
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> > @@ -0,0 +1,935 @@
>
> [ ... ]
>
> > +void lan9645x_stats_get_stats64(struct lan9645x *lan9645x, int port,
> > +				struct rtnl_link_stats64 *stats)
> > +{
>
> [ ... ]
>
> > +	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?
>
> > +
> > +	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.
>
> > +			    c[SCNT_RX_RED_PRIO_0] +
>
> [ ... ]
>
> > +			    c[SCNT_RX_RED_PRIO_7];
>
> [ ... ]
>
> > +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.

> [ ... ]
>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.h
b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.h
> > new file mode 100644
> > index 0000000000000..cd5db16258a09
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.h
> > @@ -0,0 +1,279 @@
>
> [ ... ]
>
> > +/* Counter indices into stat layout structs */
> > +#define SCNT_FRER_SID_IN_PKT             0
> > +#define SCNT_ISDX_GREEN_OCT              1
>
> [ ... ]
>
> > +#define SCNT_SF_MATCHING_FRAMES_COUNT    0
>
> [ ... ]
>
> > +#define SCNT_ESDX_GREEN_OCT              0
>
> [ ... ]
>
> > +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.

> [ ... ]