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

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

On Fri, 2026-10-02 at 21:14 +0000, netdev-bot+sashiko@kernel.org 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 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 3 · Low: 1
> 
> 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_sset_count() returns -EOPNOTSUPP (-95) for
>   every string set other than ETH_SS_STATS.
> - [Medium] lan9645x_stats_get_stats64() counts bad frames in rx_packets.
> - [Medium] The eMAC and pMAC FRAG counters are handled inconsistently.
> - [Low] The commit message says 'The eMAC and pMAC counter sources are
>   decoded, but only the aggregate source is reachable for now'.
> 
> --
> 
> net: dsa: lan9645x: add port statistics
> 
> This adds port counter support to the lan9645x DSA driver. The 32 bit
> hardware counters are copied into 64 bit software counters by a poller
> that runs every three seconds. They are reported through ethtool -S, the
> standard ethtool statistics groups and ndo_get_stats64.
> 
> > The eMAC and pMAC counter sources are decoded, but only the aggregate
> > source is reachable for now, until MAC merge support is added.
> 
> [Severity: Low]
> Is this sentence still accurate? None of these read the ->src field of the
> structure they fill in:
> 
>   lan9645x_stats_get_eth_mac_stats()
>   lan9645x_stats_get_rmon_stats()
>   lan9645x_stats_get_pause_stats()
>   lan9645x_stats_get_eth_phy_stats()
>   lan9645x_stats_get_eth_ctrl_stats()
> 
> All of them always return the eMAC plus pMAC sum.
> 
> The v12 changelog says "Drop the unreachable mac merge related code", so
> the per-source decoding seems to have been removed. This has no runtime
> effect today, because without .get_mm ethtool only allows the aggregate
> source.
> 
> Could the message say instead that the pMAC counters are summed into the
> aggregate, and that per-source reporting is not implemented yet?
> 

Correct, the sentence was left over from before the per-source code was
dropped. v14 says that the pMAC counters are summed into the aggregate
source, and that per-source reporting is not implemented yet.


> > 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..1253f77489931
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> > @@ -0,0 +1,856 @@
> 
> [ ... ]
> 
> > +int lan9645x_stats_get_sset_count(struct lan9645x *lan9645x, int port, int sset)
> > +{
> > +     if (sset != ETH_SS_STATS)
> > +             return -EOPNOTSUPP;
> 
> [Severity: Medium]
> Can this negative return value corrupt the sset counts that the conduit
> netdev reports? DSA replaces the conduit's get_sset_count with
> dsa_conduit_get_sset_count(). For every sset, that function calls:
> 
> net/dsa/conduit.c:dsa_conduit_append_port_sset_count() {
>         if (ds->ops->get_sset_count)
>                 *count += ds->ops->get_sset_count(ds, port, sset);
> }
> 
> Nothing checks for a negative value. For ETH_SS_TEST, ETH_SS_PRIV_FLAGS
> and ETH_SS_PHY_STATS, the conduit therefore reports its own count minus
> 95.
> 
> If the conduit has fewer than 95 entries, the count is negative. Then
> ethtool -t, the netlink priv-flags path and ETHTOOL_GSTRINGS on the
> conduit fail with a bogus errno. The conduit's self-tests and private
> flags also drop out of ethtool -i and the netlink strset reply.
> 
> If the conduit has 95 or more entries, the result is zero or a smaller
> positive value. ethtool_self_test(), ethtool_get_phy_stats_ethtool() and
> strset_prepare_set() size their buffers from that value, but the
> conduit's own callbacks still write their full number of entries. No real
> NIC has that many entries, so this part is theoretical.
> 
> Most other DSA drivers (hellcreek, lan9303, gswip, mt7530, qca8k, a5psw,
> vsc73xx, and others) return 0 for unsupported ssets. Should this return 0
> as well?
> 


Correct, v14 returns 0 for the other string sets, as other DSA drivers
do.

> > +
> > +     return ARRAY_SIZE(lan9645x_port_ethtool_stats);
> > +}
> 
> [ ... ]
> 
> > +     r->undersize_pkts = c[SCNT_RX_SHORT] +
> > +                         c[SCNT_RX_PMAC_SHORT];
> > +     r->oversize_pkts = c[SCNT_RX_LONG] +
> > +                        c[SCNT_RX_PMAC_LONG];
> > +     /* SCNT_RX_FRAG counts frames received after the port is paused, and
> > +      * increments when pause frames arrive from the link partner.
> > +      * It counts neither undersize frames nor errors, so it is left out here
> > +      * and out of rx_packets, unlike lan966x which adds it to rx_errors.
> > +      * SCNT_RX_PMAC_FRAG does count number of runt frames with invalid CRC.
> > +      */
> > +     r->fragments = c[SCNT_RX_PMAC_FRAG];
> 
> [Severity: Medium]
> Can fragments be non-zero without MAC merge support? The pMAC receives no
> traffic until MAC merge is supported, so RMON fragments always reads 0 in
> lan9645x_stats_get_rmon_stats(). That includes the case where runts with
> a bad CRC arrive on the eMAC.
> 
> Documentation/networking/statistics.rst says drivers should leave fields
> they do not report at ETHTOOL_STAT_NOT_SET, not overwrite them with 0. If
> no accurate eMAC fragment count exists, would it be better to leave
> fragments unset?
> 
> The two MACs are also treated differently in lan9645x_stats_get_stats64().
> rx_packets includes c[SCNT_RX_PMAC_FRAG] but not c[SCNT_RX_FRAG]. As a
> result, a bad-CRC runt is counted in rx_packets only when the pMAC
> receives it.
> 
> Does the hardware really use different meanings for the counters at
> offset 0x5 and offset 0x35, which is the same position in the pMAC block?
> If not, could both MACs be handled the same way?
> 

The two counters do differ in hardware. c_rx_frag (0x05) counts runts
with an invalid CRC only on half-duplex links, on full-duplex links it
counts frames received after the port is paused and increments on
received pause frames. c_rx_pmac_frag (0x35) always counts runts with an
invalid CRC. That is also why only the pMAC counter is part of
rx_packets. I will leave RMON fragments unset, since reporting fragments
from the pMAC alone is misleading


> [ ... ]
> 
> > +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] +
> 
> [Severity: Medium]
> Should rx_packets include SCNT_RX_SHORT, SCNT_RX_JABBER and SCNT_RX_LONG,
> and their pMAC equivalents? The same function counts all of them in
> rx_errors below, and counts JABBER and LONG in rx_length_errors too.
> 
> include/uapi/linux/if_link.h describes rx_packets as "Number of good
> packets received by the interface".
> 
> The SZ_* buckets also include frames with a bad FCS. The v5 changelog
> says: "remove rx_crc, rx_symbol_err from rx_packets, as they are already
> counted in SZ_* buckets".
> 
> So when a link partner sends a runt, jabber, oversized or bad-FCS frame,
> it shows up in both rx_packets and rx_errors. ocelot and lan966x use a
> similar formula.
> 
> Would the RX_UC, RX_MC and RX_BC good frame counters, which already feed
> FramesReceivedOK, be a better source here?

Dismissed, intentional. This follows mscc/ocelot, which computes
rx_packets the same way. The hardware has no good-frames-only octet
counter, c_rx_oct counts octets in good and bad frames, so rx_bytes
cannot follow the uapi description either. rx_packets is therefore made
to count the same frames rx_bytes counts octets for: the size buckets
cover good and bad frames from 64 bytes up to MAXLEN, and short, jabber
and long frames fall outside that range, so every received frame is
counted exactly once. Bad frames are also reported in rx_errors.


> 
> > +                         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];
> > +
> > +     stats->rx_length_errors = c[SCNT_RX_JABBER] +
> > +                               c[SCNT_RX_LONG] +
> > +                               c[SCNT_RX_PMAC_JABBER] +
> > +                               c[SCNT_RX_PMAC_LONG];
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-dsa_lan9645x_switch_driver_base-v13-0-827c2d3617f2%40microchip.com