Re: [PATCH net-next v3 1/3] ethtool: add PHY receive frame and byte totals

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In Documentation/networking/ethtool-netlink.rst, the stats-group
  table row for ETHTOOL_STATS_PHY (line 1668) changes the documented…
- [Low] The patch defines exactly which bytes rx_bytes_total /
  RxOctetsTotal counts: Ethernet header, padding and FCS, but not…

--

ethtool: add PHY receive frame and byte totals

This patch adds optional rx_packets_total and rx_bytes_total counters to
struct ethtool_phy_stats. They are exposed in the phydev statistics group
as ETHTOOL_A_STATS_PHY_RX_PKTS_TOTAL and ETHTOOL_A_STATS_PHY_RX_BYTES_TOTAL.
Both totals include frames with errors, the byte total is defined to
include the Ethernet header, padding and FCS, and the netlink documentation
is updated to match.

> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index eba226c79ec36..5528d0f7afdd9 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -1665,7 +1665,7 @@ the ``ETHTOOL_A_STATS_GROUPS`` bitset. Currently defined values are:
>   ETHTOOL_STATS_ETH_PHY  eth-phy  Basic IEEE 802.3 PHY statistics (30.3.2.1.*)
>   ETHTOOL_STATS_ETH_CTRL eth-ctrl Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*)
>   ETHTOOL_STATS_RMON     rmon     RMON (RFC 2819) statistics
> - ETHTOOL_STATS_PHY      phy      Additional PHY statistics, not defined by IEEE
> + ETHTOOL_STATS_PHY      phydev   Additional PHY statistics, not defined by IEEE

[Severity: Low]
This isn't a bug, but this hunk changes the documented group name for
ETHTOOL_STATS_PHY from "phy" to "phydev", and the commit message does not
mention it.

The new name is correct and matches the kernel:

net/ethtool/stats.c:stats_std_names[] {
    ...
	[ETHTOOL_STATS_PHY]			= "phydev",
    ...
}

The new paragraph below refers to the phydev group, so keeping the fix in
this patch seems reasonable. Could the commit message mention the
correction, or could it go in a separate patch?

[ ... ]

> +The ``phydev`` group provides separate counters for successfully received
> +traffic and total received traffic. ``ETHTOOL_A_STATS_PHY_RX_PKTS``
> +(``RxFrames``) and ``ETHTOOL_A_STATS_PHY_RX_BYTES`` (``RxOctets``) count
> +successfully received frames and bytes. ``ETHTOOL_A_STATS_PHY_RX_PKTS_TOTAL``
> +(``RxFramesTotal``) and ``ETHTOOL_A_STATS_PHY_RX_BYTES_TOTAL`` (``RxOctetsTotal``)
> +include frames with errors. The total byte count includes the Ethernet header,
> +padding and FCS, but excludes the preamble and start frame delimiter. These

[Severity: Low]
Should these byte boundaries also apply to RxOctets?

This paragraph says which bytes RxOctetsTotal counts, but it gives no
such rule for RxOctets. The kernel-doc for rx_bytes in struct
ethtool_phy_stats in include/linux/ethtool.h still only says:

 * @rx_bytes: Total successfully received bytes

A driver could report rx_bytes without FCS and rx_bytes_total with FCS,
and both would match the documentation. In that case, RxOctetsTotal minus
RxOctets would not be the number of bytes in errored frames.

No PHY driver in drivers/net/phy sets ethtool_phy_stats.rx_bytes yet. Can
the definition be made the same for both counters now, before a driver
depends on it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109-submit-acx00-phy-stats-v1-v3-0-fbc6327c5be4%40gmail.com