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

[Jens Emil Schulz Ostergaard]

On Mon, 2026-10-19 at 10:08 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.
> 

Dismissed, intended.

This follows mscc/ocelot. The hardware has no good frames only octet
counter. rx_oct counts good and bad frames, so rx_bytes cannot follow
the uapi description. We make sure rx_packets matches by counting the
same frames as rx_bytes. Bad frames are also reported in rx_errors.

FramesReceivedOK is the 802.3 good frame count and is meant to differ.

SCNT_RX_PMAC_FRAG is the size class below 64 bytes for pMAC runts with
an invalid CRC, so it belongs in rx_packets next to the size buckets.
Its frames are already in rx_errors through SCNT_RX_PMAC_CRC, which
counts CRC errors at any frame size, in the same way a frame of 64
bytes or more with a bad CRC sits in a size bucket and in SCNT_RX_CRC.
Adding it to rx_errors would count those runts twice.

The eMAC SCNT_RX_FRAG is not the same counter. It only counts runts
with an invalid CRC on half-duplex links, and on full-duplex links
counts frames received after the port is paused, which is why it is
not used.


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