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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] lan9645x_stats_get_stats64()
  (drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c) counts error…

--

net: dsa: lan9645x: add port statistics

This adds port statistics support to the lan9645x DSA driver. The 32 bit
hardware counters are polled into a 64 bit software cache, and the pMAC
counters are summed into the aggregate source. The cache is exposed through
ethtool -S, the standard ethtool groups and ndo_get_stats64.

> 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..a21afb4d98532
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> @@ -0,0 +1,854 @@

[ ... ]

> +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_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] +
> +			    c[SCNT_RX_PMAC_LONG];
> +
> +	stats->multicast = c[SCNT_RX_MC] + c[SCNT_RX_PMAC_MC];
> +
> +	stats->rx_errors = c[SCNT_RX_SHORT] +
> +			   c[SCNT_RX_JABBER] +
> +			   c[SCNT_RX_CRC] +
> +			   c[SCNT_RX_LONG] +
> +			   c[SCNT_RX_PMAC_SHORT] +
> +			   c[SCNT_RX_PMAC_JABBER] +
> +			   c[SCNT_RX_PMAC_CRC] +
> +			   c[SCNT_RX_PMAC_LONG];

[Severity: Medium]
Should short, jabber and oversize frames be counted in rx_packets in
lan9645x_stats_get_stats64()?

rx_packets and rx_errors both add SCNT_RX_SHORT, SCNT_RX_JABBER and
SCNT_RX_LONG, plus their pMAC versions. JABBER and LONG are also added to
rx_length_errors. So every malformed frame received on a user port counts
once as a good packet and once as an error.

The rtnl_link_stats64 kernel-doc in include/uapi/linux/if_link.h says:

  @rx_packets: Number of good packets received by the interface.

The v5 changelog also says CRC and symbol error frames "are already counted
in SZ_* buckets". Since those buckets feed rx_packets, frames with a bad CRC
would be counted as good packets as well.

This also gives a different answer from lan9645x_stats_get_eth_mac_stats(),
which counts good received frames as UC + MC + BC only:

	m->FramesReceivedOK = c[SCNT_RX_UC] +
			      c[SCNT_RX_MC] +
			      c[SCNT_RX_BC] +
			      ...

As a result, rx_packets is larger than FramesReceivedOK by the number of
errored frames. ocelot_port_get_stats64() uses a similar rx_packets formula,
but that does not match the uAPI description either.

Could rx_packets be derived the same way as FramesReceivedOK?

A separate point: c[SCNT_RX_PMAC_FRAG] is added to rx_packets but not to
rx_errors. The comment in lan9645x_stats_get_rmon_stats() says:

	/* ... Only SCNT_RX_PMAC_FRAG counts runts with an invalid
	 * CRC, so fragments is left unset rather than reporting the pMAC alone.
	 */

Going by that comment, a pMAC runt with a bad CRC would be counted as a
good packet and left out of the error total. Is that intended?

v12 removed the eMAC SCNT_RX_FRAG from rx_packets, but the pMAC version is
still there. Since the pMAC gets no traffic without MAC merge, this has
little effect today.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-dsa_lan9645x_switch_driver_base-v15-0-562f316f78b9%40microchip.com