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

[netdev-bot+sashiko@kernel.org]

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

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

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() counts bad frames in
  rtnl_link_stats64.rx_packets.
- [Low] In lan9645x_stats_get_stats64(), rx_errors is the sum
  c[SCNT_RX_SHORT] + c[SCNT_RX_JABBER] + c[SCNT_RX_CRC] +…
- [Low] __lan9645x_stats_view_idx_hw_read() selects the port with
  lan_wr(SYS_STAT_CFG_STAT_VIEW_SET(idx), lan9645x, SYS_STAT_CFG).

--

net: dsa: lan9645x: add port statistics

This commit adds port statistics support to the lan9645x DSA driver. A
delayed work polls the 32-bit hardware counters every 3 seconds and
accumulates them into 64-bit software counters. The counters are exposed
through ethtool -S, the standard ethtool stats groups and get_stats64, with
the pMAC counters summed into the aggregate source.

> 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 @@

[ ... ]

> +static void lan9645x_stats_update_counter(u64 *sw, u32 hw)
> +{
> +	if (hw < (*sw & U32_MAX))
> +		*sw += (u64)1 << 32; /* value has wrapped */
> +
> +	*sw = (*sw & ~(u64)U32_MAX) + hw;
> +}
> +
> +static int __lan9645x_stats_view_idx_hw_read(struct lan9645x *lan9645x,
> +					     enum lan9645x_view_stat_type vtype,
> +					     int idx)
> +{

[ ... ]

> +	lan_wr(SYS_STAT_CFG_STAT_VIEW_SET(idx), lan9645x, SYS_STAT_CFG);

[Severity: Low]
What happens to the counters if this lan_wr() fails?

lan_wr() returns void and only does WARN_ON_ONCE(regmap_write(...)), so
this function never sees a failed STAT_VIEW select. The SYS_CNT window
read by the lan_bulk_rd() calls below is the same for every port. If the
reads still succeed, buf holds the counters of the previously selected
port.

__lan9645x_stats_view_idx_transfer() then passes those values through
lan9645x_stats_update_counter() into the requested port's shadow. If the
wrong value is larger, the shadow takes it. On the next correct poll,
this check:

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

adds 2^32. That counter then stays about 4G too high until the next
setup.

lan9645x_stats_clear_view() has the same unchecked write:

	lan_wr(SYS_STAT_CFG_STAT_VIEW_SET(idx) |
	       SYS_STAT_CFG_STAT_CLEAR_SHOT_SET(groups),
	       lan9645x, SYS_STAT_CFG);

If that write is dropped, the poll for !CLEAR_SHOT passes at once and a
clear that never happened is reported as success. That case looks
harmless, since the shadow later takes the uncleared hardware value.

ocelot_port_update_stats() follows the same pattern, and every other
lan_wr() in this driver also ignores write errors. Fixing this would
probably mean having the helper return the regmap_write() result.

> +
> +	/* Each region for this index contains counters which are at sequential
> +	 * addresses, so we can use bulk reads to ease lock pressure a bit.
> +	 */
> +	for (int r = 0; r < vstats->num_regions; r++) {
> +		region = vstats->regions[r];
> +		err = lan_bulk_rd(&vstats->buf[region.cnts_base_idx],
> +				  region.cnt, lan9645x,
> +				  SYS_CNT(region.base_offset));

[ ... ]

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

[ ... ]

> +	stats->rx_bytes = c[SCNT_RX_OCT] + c[SCNT_RX_PMAC_OCT];
> +
> +	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];

[Severity: Medium]
Should rx_packets include the error counters?

The rtnl_link_stats64 documentation in include/uapi/linux/if_link.h
describes rx_packets as "Number of good packets received by the
interface". It describes rx_errors as "Total number of bad packets
received on this network device".

SCNT_RX_SHORT, SCNT_RX_JABBER and SCNT_RX_LONG, plus their pMAC
versions, are added to both rx_packets and rx_errors. Each runt, jabber
and oversize frame is therefore counted as good and as bad.

The SZ_* buckets also seem to include CRC-errored frames. The v5
changelog removed rx_crc from rx_packets "as they are already counted in
SZ_* buckets". So rx_packets ends up counting every frame received.

This also does not match FramesReceivedOK in
lan9645x_stats_get_eth_mac_stats(), which is UC + MC + BC. The two
standard interfaces would report different good RX counts for the same
port.

ocelot_port_get_stats64() uses a similar formula. Is that the intended
meaning of rx_packets for this new driver?

Separately, c[SCNT_RX_PMAC_FRAG] is added to rx_packets but not to
rx_errors, while c[SCNT_RX_PMAC_SHORT] is added to both. The comment in
lan9645x_stats_get_rmon_stats() says SCNT_RX_PMAC_FRAG counts runts with
an invalid CRC.

Should SCNT_RX_PMAC_FRAG go into rx_errors too, or be removed from
rx_packets? This has no effect until MAC merge sends traffic to the pMAC.

> +
> +	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];
> +
> +	stats->rx_length_errors = c[SCNT_RX_JABBER] +
> +				  c[SCNT_RX_LONG] +
> +				  c[SCNT_RX_PMAC_JABBER] +
> +				  c[SCNT_RX_PMAC_LONG];
> +
> +	/* SCNT_RX_CRC counts CRC errors, alignment errors and RX_ER events at
> +	 * any frame size, so a short frame with a bad CRC is counted here.
> +	 */
> +	stats->rx_crc_errors = c[SCNT_RX_CRC] + c[SCNT_RX_PMAC_CRC];

[Severity: Low]
Is a jabber frame counted twice in rx_errors?

This comment says SCNT_RX_CRC counts CRC errors "at any frame size". The
v12 changelog describes SCNT_RX_JABBER as "the too-long-with-bad-CRC half
of the event".

If both are true, an oversize frame with a bad FCS increments both
SCNT_RX_CRC and SCNT_RX_JABBER. rx_errors above adds the two together.

If the hardware keeps these two counters disjoint, is the "any frame
size" wording in this comment accurate?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-dsa_lan9645x_switch_driver_base-v14-0-3d2a72661213%40microchip.com