[PATCH net-next v3 0/3] ethtool: expose PHY receive totals

[PATCH net-next v3 0/3] ethtool: expose PHY receive totals

[James Hilliard]

Some PHYs count received frames and bytes including errored traffic. Add
explicit receive-total counters to the generic PHY statistics interface
alongside its counters for successfully received traffic.

Use the new counters to expose ACx00 receive statistics through the phydev
group. Accumulate the hardware counters in 64-bit software totals, retain
them across reset and power transitions, and return cached snapshots to
ethtool.

Convert DP83TG720's existing RX packet statistic to the receive-total
counter, matching its documented inclusion of packets with errors.

Assisted-by: Codex:gpt-6-astra
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes in v3:
- Add generic PHY receive-total packet and byte counters in a preparatory
  patch, retaining the existing successful-traffic counters and their IDs.
- Report ACx00 packet and byte totals through the new fields, leaving
  successful-only counters unset.
- Convert DP83TG720's inclusive RX packet count to the new total field.
- Bound coherent-counter sampling to three attempts and skip unstable
  snapshots without making phylib take the link down.
- Reject out-of-range byte samples before updating the hardware baseline
  or accumulated totals, retaining MDIO and page-restoration errors.
- Rebase onto current net-next.
- Link to v2: https://patch.msgid.link/20261102-submit-acx00-phy-stats-v1-v2-1-3c8a664407c1@gmail.com

Changes in v2:
- Rebase onto current net-next and repost after the merge window.
- No functional changes.
- Link to v1: https://patch.msgid.link/20261025-submit-acx00-phy-stats-v1-v1-1-d3d461e46db0@gmail.com

---
James Hilliard (3):
      ethtool: add PHY receive frame and byte totals
      net: phy: acx00: report receive statistics
      net: phy: dp83tg720: report inclusive RX packet totals

 Documentation/networking/ethtool-netlink.rst |  16 ++-
 drivers/net/phy/dp83tg720.c                  |   2 +-
 drivers/net/phy/xpowers/acx00.c              | 168 ++++++++++++++++++++++++++-
 include/linux/ethtool.h                      |   6 +
 include/uapi/linux/ethtool_netlink.h         |   2 +
 net/ethtool/stats.c                          |   8 +-
 6 files changed, 196 insertions(+), 6 deletions(-)
---
base-commit: e281e85235cfc2acd080093d5deced427c4c9856
change-id: 20261024-submit-acx00-phy-stats-v1-d838f77444e8

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

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

[James Hilliard]

The generic PHY receive packet and byte counters describe successfully
received traffic. Some PHYs instead count all received frames and bytes,
including frames with errors. A frame error count cannot recover the
number of successfully received bytes because damaged frames have
different lengths.

Add optional receive-total counters to ethtool_phy_stats and the phydev
statistics group. Count frame bytes including the Ethernet header, padding
and FCS, and excluding the preamble and start frame delimiter. These
counters describe reception at the PHY, before MAC or host filtering.

Append the new attributes so existing statistic IDs and their meanings
remain unchanged. Publish the names through the existing statistics string
set, and retain the unset-counter convention so drivers can report either
set of counters independently.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 Documentation/networking/ethtool-netlink.rst | 16 +++++++++++++++-
 include/linux/ethtool.h                      |  6 ++++++
 include/uapi/linux/ethtool_netlink.h         |  2 ++
 net/ethtool/stats.c                          |  8 +++++++-
 4 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index eba226c79ec3..5528d0f7afdd 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
  ====================== ======== ===============================================
 
 Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
@@ -1683,6 +1683,20 @@ by ``ETHTOOL_A_STATS_GRP_SS_ID``. Complex statistics (such as RMON histogram
 entries) are also listed inside ``ETHTOOL_A_STATS_GRP`` and do not have
 a string defined in the string set.
 
+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
+totals describe reception at the PHY, independently of subsequent filtering
+or drops at the MAC or host.
+
+Each counter is optional. Drivers leave unsupported counters at
+``ETHTOOL_STAT_NOT_SET``, and the kernel omits them from the reply. A PHY can
+report total received traffic even when it cannot separate successful traffic.
+
 RMON "histogram" counters count number of packets within given size range.
 Because RFC does not specify the ranges beyond the standard 1518 MTU devices
 differ in definition of buckets. For this reason the definition of packet ranges
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index c3a41fbd5426..7a7b434ab1fa 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -486,6 +486,10 @@ struct ethtool_eth_phy_stats {
  * @tx_packets: Total successfully transmitted frames
  * @tx_bytes: Total successfully transmitted bytes
  * @tx_errors: Total transmitted frames with errors
+ * @rx_packets_total: Total received frames, including frames with errors
+ * @rx_bytes_total: Total received frame bytes, including frames with errors.
+ *	Includes the Ethernet header, padding and FCS, but excludes the preamble
+ *	and start frame delimiter.
  *
  * This structure provides a standardized interface for reporting
  * PHY-level statistics counters. It is designed to expose statistics
@@ -499,6 +503,8 @@ struct ethtool_phy_stats {
 	u64 tx_packets;
 	u64 tx_bytes;
 	u64 tx_errors;
+	u64 rx_packets_total;
+	u64 rx_bytes_total;
 };
 
 /* Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*), not otherwise exposed
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index fa5d645140a4..db26cff28f12 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -202,6 +202,8 @@ enum {
 	ETHTOOL_A_STATS_PHY_TX_PKTS,
 	ETHTOOL_A_STATS_PHY_TX_BYTES,
 	ETHTOOL_A_STATS_PHY_TX_ERRORS,
+	ETHTOOL_A_STATS_PHY_RX_PKTS_TOTAL,
+	ETHTOOL_A_STATS_PHY_RX_BYTES_TOTAL,
 
 	/* add new constants above here */
 	__ETHTOOL_A_STATS_PHY_CNT,
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 9b0d8cb07675..b4c4bdd8fc4b 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -88,6 +88,8 @@ const char stats_phy_names[__ETHTOOL_A_STATS_PHY_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_A_STATS_PHY_TX_PKTS]		= "TxFrames",
 	[ETHTOOL_A_STATS_PHY_TX_BYTES]		= "TxOctets",
 	[ETHTOOL_A_STATS_PHY_TX_ERRORS]		= "TxErrors",
+	[ETHTOOL_A_STATS_PHY_RX_PKTS_TOTAL]	= "RxFramesTotal",
+	[ETHTOOL_A_STATS_PHY_RX_BYTES_TOTAL]	= "RxOctetsTotal",
 };
 
 const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_SRC + 1] = {
@@ -295,7 +297,11 @@ static int stats_put_phydev_stats(struct sk_buff *skb,
 	    stat_put(skb, ETHTOOL_A_STATS_PHY_TX_BYTES,
 		     data->phydev_stats.tx_bytes) ||
 	    stat_put(skb, ETHTOOL_A_STATS_PHY_TX_ERRORS,
-		     data->phydev_stats.tx_errors))
+		     data->phydev_stats.tx_errors) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_RX_PKTS_TOTAL,
+		     data->phydev_stats.rx_packets_total) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_RX_BYTES_TOTAL,
+		     data->phydev_stats.rx_bytes_total))
 		return -EMSGSIZE;
 	return 0;
 }

-- 
2.53.0

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

[Andrew Lunn]

On Mon, Nov 09, 2026 at 07:48:32PM -0700, James Hilliard wrote:
> The generic PHY receive packet and byte counters describe successfully
> received traffic. Some PHYs instead count all received frames and bytes,
> including frames with errors. A frame error count cannot recover the
> number of successfully received bytes because damaged frames have
> different lengths.
> 
> Add optional receive-total counters to ethtool_phy_stats and the phydev
> statistics group. Count frame bytes including the Ethernet header, padding
> and FCS, and excluding the preamble and start frame delimiter. These
> counters describe reception at the PHY, before MAC or host filtering.
> 
> Append the new attributes so existing statistic IDs and their meanings
> remain unchanged. Publish the names through the existing statistics string
> set, and retain the unset-counter convention so drivers can report either
> set of counters independently.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  Documentation/networking/ethtool-netlink.rst | 16 +++++++++++++++-
>  include/linux/ethtool.h                      |  6 ++++++
>  include/uapi/linux/ethtool_netlink.h         |  2 ++
>  net/ethtool/stats.c                          |  8 +++++++-
>  4 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index eba226c79ec3..5528d0f7afdd 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
>   ====================== ======== ===============================================
>  
>  Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
> @@ -1683,6 +1683,20 @@ by ``ETHTOOL_A_STATS_GRP_SS_ID``. Complex statistics (such as RMON histogram
>  entries) are also listed inside ``ETHTOOL_A_STATS_GRP`` and do not have
>  a string defined in the string set.
>  
> +The ``phydev`` group provides separate counters for successfully received
> +traffic and total received traffic.

You need to be careful with naming here. This is a
kAPI. phydev/phylib/phylink are implementation details, a set of
helpers which some MAC drivers use to implement PHYs. But MAC driver
can implement PHYs in different ways, and don't need to use a phydev.

``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.

And what would you call it when the counter does include the preamble
and start frame delimiter?

Take a step back. Just because this PHY has these counters, do we need
to support them? And what happens when the next PHY comes along with
slightly different counters? Do we add them as well?

It is vendor SDK syndrome. The hardware can do this, so lets expose
it.

What is your use case here which justifies these specific counters.

     Andrew

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

[James Hilliard]

On Tue, Nov 10, 2026 at 9:42 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Nov 09, 2026 at 07:48:32PM -0700, James Hilliard wrote:
> > The generic PHY receive packet and byte counters describe successfully
> > received traffic. Some PHYs instead count all received frames and bytes,
> > including frames with errors. A frame error count cannot recover the
> > number of successfully received bytes because damaged frames have
> > different lengths.
> >
> > Add optional receive-total counters to ethtool_phy_stats and the phydev
> > statistics group. Count frame bytes including the Ethernet header, padding
> > and FCS, and excluding the preamble and start frame delimiter. These
> > counters describe reception at the PHY, before MAC or host filtering.
> >
> > Append the new attributes so existing statistic IDs and their meanings
> > remain unchanged. Publish the names through the existing statistics string
> > set, and retain the unset-counter convention so drivers can report either
> > set of counters independently.
> >
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> >  Documentation/networking/ethtool-netlink.rst | 16 +++++++++++++++-
> >  include/linux/ethtool.h                      |  6 ++++++
> >  include/uapi/linux/ethtool_netlink.h         |  2 ++
> >  net/ethtool/stats.c                          |  8 +++++++-
> >  4 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> > index eba226c79ec3..5528d0f7afdd 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
> >   ====================== ======== ===============================================
> >
> >  Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
> > @@ -1683,6 +1683,20 @@ by ``ETHTOOL_A_STATS_GRP_SS_ID``. Complex statistics (such as RMON histogram
> >  entries) are also listed inside ``ETHTOOL_A_STATS_GRP`` and do not have
> >  a string defined in the string set.
> >
> > +The ``phydev`` group provides separate counters for successfully received
> > +traffic and total received traffic.
>
> You need to be careful with naming here. This is a
> kAPI. phydev/phylib/phylink are implementation details, a set of
> helpers which some MAC drivers use to implement PHYs. But MAC driver
> can implement PHYs in different ways, and don't need to use a phydev.

"phydev" is already the name exported for ETHTOOL_STATS_PHY by
stats_std_names[] in net/ethtool/stats.c. That documentation change
matches the existing group name; it does not rename the interface.
The proposed counters describe PHY reception independently of how
the driver implements PHY support.

>
> ``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.
>
> And what would you call it when the counter does include the preamble
> and start frame delimiter?
>
> Take a step back. Just because this PHY has these counters, do we need
> to support them? And what happens when the next PHY comes along with
> slightly different counters? Do we add them as well?
>
> It is vendor SDK syndrome. The hardware can do this, so lets expose
> it.
>
> What is your use case here which justifies these specific counters.

The use case is the same traffic monitoring and error diagnosis served
by the existing PHY packet, byte and error counters. The difference is
what the hardware measures: successful reception versus total reception,
including frames with errors.

This appears to be a recurring accounting model across vendors, rather
than an ACx00-specific definition. TI's DP83TG720 provides another PHY
example, which is why this series also updates its existing receive
packet statistic [1].

There are examples elsewhere in Ethernet hardware too. Microchip's
LAN9303 explicitly distinguishes received bytes including bad packets
from received bytes in good packets [2]. Those are MAC counters in a
switch, not PHY counters, but they illustrate the same distinction.
That supports a shared total-versus-successful abstraction, although
each device's exact error and byte-accounting rules still need checking.

The v2 review identified that ACx00's inclusive measurements do not
match the existing successful-only fields [0]. The successful byte
count cannot be derived by subtracting an error count, because damaged
frames have different lengths. The proposed fields preserve that
traffic accounting without changing the existing fields' meaning.

For the preamble/SFD case, a counter including those bytes would not
match the proposed definition unchanged. A driver could use the field
only if it can accurately normalize its measurement to the same
frame-byte definition; otherwise it would leave that field unset.

The intent is to represent a recurring measurement with consistent
semantics, rather than add a generic counter for every variation in
vendor register accounting.

[0] https://lore.kernel.org/r/20261109235715.1618218-1-kuba@kernel.org/
[1] https://www.ti.com/lit/ds/symlink/dp83tg720s-q1.pdf
[2] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/00002308A.pdf

>
>      Andrew

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

[Oleksij Rempel]

On Tue, Nov 10, 2026 at 05:42:33PM +0100, Andrew Lunn wrote:
> On Mon, Nov 09, 2026 at 07:48:32PM -0700, James Hilliard wrote:
> > The generic PHY receive packet and byte counters describe successfully
> > received traffic. Some PHYs instead count all received frames and bytes,
> > including frames with errors. A frame error count cannot recover the
> > number of successfully received bytes because damaged frames have
> > different lengths.
> >
> > Add optional receive-total counters to ethtool_phy_stats and the phydev
> > statistics group. Count frame bytes including the Ethernet header, padding
> > and FCS, and excluding the preamble and start frame delimiter. These
> > counters describe reception at the PHY, before MAC or host filtering.
> >
> > Append the new attributes so existing statistic IDs and their meanings
> > remain unchanged. Publish the names through the existing statistics string
> > set, and retain the unset-counter convention so drivers can report either
> > set of counters independently.
> >
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> >  Documentation/networking/ethtool-netlink.rst | 16 +++++++++++++++-
> >  include/linux/ethtool.h                      |  6 ++++++
> >  include/uapi/linux/ethtool_netlink.h         |  2 ++
> >  net/ethtool/stats.c                          |  8 +++++++-
> >  4 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> > index eba226c79ec3..5528d0f7afdd 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
> >   ====================== ======== ===============================================
> >
> >  Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
> > @@ -1683,6 +1683,20 @@ by ``ETHTOOL_A_STATS_GRP_SS_ID``. Complex statistics (such as RMON histogram
> >  entries) are also listed inside ``ETHTOOL_A_STATS_GRP`` and do not have
> >  a string defined in the string set.
> >
> > +The ``phydev`` group provides separate counters for successfully received
> > +traffic and total received traffic.
>
> You need to be careful with naming here. This is a
> kAPI. phydev/phylib/phylink are implementation details, a set of
> helpers which some MAC drivers use to implement PHYs. But MAC driver
> can implement PHYs in different ways, and don't need to use a phydev.
>
> ``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.
>
> And what would you call it when the counter does include the preamble
> and start frame delimiter?
>
> Take a step back. Just because this PHY has these counters, do we need
> to support them? And what happens when the next PHY comes along with
> slightly different counters? Do we add them as well?
>
> It is vendor SDK syndrome. The hardware can do this, so lets expose
> it.
>
> What is your use case here which justifies these specific counters.

I have the same question. James, your answer mentions traffic monitoring
and error diagnosis, but not a concrete problem on AC200/AC300 that these
totals would help with. My guess is a receive problem that another driver
for this PHY already works around.

Rockchip's vendor driver for the same PHY ID (RK630 and RV1106) reads the
packet and CRC error counters from page 9, registers 0x1b/0x1c and
0x1d/0x1e:

https://github.com/rockchip-linux/kernel/blob/eeb355823a794954737fd75820dfccdab4c57af6/drivers/net/phy/rk630phy.c#L430

The vendor driver does not export them. It polls them to tune the
receiver. If there is a signal but no link, it switches to a second
analog setting and checks again. It switches settings in two more cases:
too few frames arrive, or more than 15% of the frames have a CRC error.
Its comments call this "Long network cable parameters". It also changes
the RX detect threshold depending on the temperature.

Do you see a problem like this on your AC200/AC300 boards? If so, can
you describe it? Then we can decide whether the driver should work
around that problem.

Oleksij

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

[netdev-bot+sashiko]

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

[PATCH net-next v3 2/3] net: phy: acx00: report receive statistics

[James Hilliard]

Expose receive frame and byte totals through the generic PHY statistics
interface as rx_packets_total and rx_bytes_total, alongside CRC errors in
rx_errors. The packet and byte counters include errored frames as well as
successfully received traffic, so leave the successful-only fields unset.

Collect the free-running 32-bit counters through phylib's update_stats
callback and accumulate wrap-safe deltas in 64-bit totals. AC300 hardware
measurements established a receive-byte modulus of 4,000,000,000, while
the packet and CRC-error counters wrap at 2^32. Reject byte samples at or
above the byte modulus before updating either the baseline or the totals.

Read the high half on either side of the low half to detect a carry.
Limit each counter to three attempts so persistent inconsistency cannot
retain the PHY, package-state and MDIO locks indefinitely. Skip an
unstable snapshot and retry on the next poll: reporting a synthetic
error through update_stats would make phylib take the link down.
Keep incomplete samples distinct from MDIO errors so page-restoration
failures still propagate.

Serialize collection and reporting with the package-state mutex so reads
cannot race reset or power-off, and the totals are safe on 32-bit systems.
Commit a snapshot only after all counter reads, validation and page
restoration succeed. The reporting callback returns cached totals without
accessing MDIO.

Take a best-effort final snapshot before soft reset and suspend. Invalidate
the hardware baseline before reset or power-off, including failed power-off
attempts, and establish a fresh baseline after configuration. Preserve the
software totals across these transitions and retain the live baseline when
resume reconfigures an already active PHY. An unusable initial snapshot can
be retried by the periodic collector without preventing initialization.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/phy/xpowers/acx00.c | 168 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 165 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/xpowers/acx00.c b/drivers/net/phy/xpowers/acx00.c
index d19ba6d8069a..9ab98f617444 100644
--- a/drivers/net/phy/xpowers/acx00.c
+++ b/drivers/net/phy/xpowers/acx00.c
@@ -8,6 +8,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/device.h>
+#include <linux/ethtool.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/nvmem-consumer.h>
@@ -31,6 +32,7 @@
 #define ACX00_PAGE_2				2
 #define ACX00_PAGE_6				6
 #define ACX00_PAGE_8				8
+#define ACX00_PAGE_9				9
 
 #define ACX00_PAGE0_GLOBAL_CONFIG_REG		0x13
 #define ACX00_PAGE0_XMII_RX_CLOCK_INVERT	BIT(12)
@@ -76,6 +78,30 @@
 	(ACX00_PAGE8_AUTO_CAL_TX_LEVEL_ADJUST_BYPASS | \
 	 ACX00_PAGE8_AUTO_CAL_LOW_CAL_OPAQUE_BITS)
 
+#define ACX00_PAGE9_RX_BYTES_HIGH_REG		0x19
+#define ACX00_PAGE9_RX_PACKETS_HIGH_REG		0x1b
+#define ACX00_PAGE9_RX_CRC_ERRORS_HIGH_REG	0x1d
+
+/*
+ * AC300 hardware testing confirmed that the receive byte counter wraps at
+ * 4,000,000,000 rather than 2^32.
+ */
+#define ACX00_RX_BYTES_WRAP			4000000000ULL
+#define ACX00_COUNTER_READ_ATTEMPTS		3
+
+enum acx00_ephy_stat {
+	ACX00_STAT_RX_BYTES,
+	ACX00_STAT_RX_PACKETS,
+	ACX00_STAT_RX_CRC_ERRORS,
+	ACX00_NUM_STATS,
+};
+
+static const u8 acx00_ephy_stat_regs[ACX00_NUM_STATS] = {
+	[ACX00_STAT_RX_BYTES] = ACX00_PAGE9_RX_BYTES_HIGH_REG,
+	[ACX00_STAT_RX_PACKETS] = ACX00_PAGE9_RX_PACKETS_HIGH_REG,
+	[ACX00_STAT_RX_CRC_ERRORS] = ACX00_PAGE9_RX_CRC_ERRORS_HIGH_REG,
+};
+
 /*
  * Another integration of this exact-ID PHY documents its digital vendor
  * register map, which also matches the observed ACx00 reset values. The ACx00
@@ -86,11 +112,15 @@
 struct acx00_ephy_priv {
 	struct phy_device *phydev;
 	struct acx00_ephy_control *control;
-	/* Serializes package state and multi-register power sequences. */
+	/* Serializes package state, power sequences and statistics. */
 	struct mutex state_lock;
 	bool is_ac300;
 	bool use_low_calibration_tuning;
 	bool xmii_rx_clock_inverted;
+	bool stats_active;
+	bool stats_valid;
+	u32 stats_last[ACX00_NUM_STATS];
+	u64 stats[ACX00_NUM_STATS];
 };
 
 static int acx00_ephy_read_page(struct phy_device *phydev)
@@ -110,6 +140,115 @@ static int acx00_ephy_write_page(struct phy_device *phydev, int page)
 			   FIELD_PREP(ACX00_PAGE_SELECT_MASK, page));
 }
 
+/* Return 0 on success, 1 for an unstable sample, or a negative MDIO error. */
+static int acx00_ephy_read_counter(struct phy_device *phydev, u8 reg, u32 *count)
+{
+	int high, low, check;
+	int i;
+
+	/* The free-running counter can carry while its two halves are read. */
+	for (i = 0; i < ACX00_COUNTER_READ_ATTEMPTS; i++) {
+		high = __phy_read(phydev, reg);
+		if (high < 0)
+			return high;
+		low = __phy_read(phydev, reg + 1);
+		if (low < 0)
+			return low;
+		check = __phy_read(phydev, reg);
+		if (check < 0)
+			return check;
+		if (high != check)
+			continue;
+
+		*count = (u32)high << 16 | low;
+		return 0;
+	}
+
+	return 1;
+}
+
+static int __acx00_ephy_update_stats(struct phy_device *phydev)
+{
+	struct acx00_ephy_priv *priv = phydev->priv;
+	u32 counts[ACX00_NUM_STATS];
+	int oldpage;
+	int ret;
+	int i;
+
+	lockdep_assert_held(&priv->state_lock);
+
+	if (!priv->stats_active)
+		return 0;
+
+	oldpage = phy_select_page(phydev, ACX00_PAGE_9);
+	if (oldpage < 0)
+		return phy_restore_page(phydev, oldpage, oldpage);
+
+	for (i = 0; i < ACX00_NUM_STATS; i++) {
+		ret = acx00_ephy_read_counter(phydev, acx00_ephy_stat_regs[i],
+					      &counts[i]);
+		if (ret)
+			break;
+	}
+	ret = phy_restore_page(phydev, oldpage, ret);
+	if (ret < 0)
+		return ret;
+
+	/* Skip unusable samples without turning a statistics miss into a link error. */
+	if (ret || counts[ACX00_STAT_RX_BYTES] >= ACX00_RX_BYTES_WRAP)
+		return 0;
+
+	for (i = 0; i < ACX00_NUM_STATS; i++) {
+		u64 count = counts[i];
+
+		if (priv->stats_valid) {
+			if (count < priv->stats_last[i])
+				count += i == ACX00_STAT_RX_BYTES ?
+					ACX00_RX_BYTES_WRAP : BIT_ULL(32);
+			priv->stats[i] += count - priv->stats_last[i];
+		}
+		priv->stats_last[i] = counts[i];
+	}
+	priv->stats_valid = true;
+
+	return 0;
+}
+
+static int acx00_ephy_update_stats(struct phy_device *phydev)
+{
+	struct acx00_ephy_priv *priv = phydev->priv;
+	int ret;
+
+	mutex_lock(&priv->state_lock);
+	ret = __acx00_ephy_update_stats(phydev);
+	mutex_unlock(&priv->state_lock);
+
+	return ret;
+}
+
+static void acx00_ephy_get_phy_stats(struct phy_device *phydev,
+				     struct ethtool_eth_phy_stats *eth_stats,
+				     struct ethtool_phy_stats *stats)
+{
+	struct acx00_ephy_priv *priv = phydev->priv;
+
+	mutex_lock(&priv->state_lock);
+	stats->rx_packets_total = priv->stats[ACX00_STAT_RX_PACKETS];
+	stats->rx_bytes_total = priv->stats[ACX00_STAT_RX_BYTES];
+	stats->rx_errors = priv->stats[ACX00_STAT_RX_CRC_ERRORS];
+	mutex_unlock(&priv->state_lock);
+}
+
+static void acx00_ephy_snapshot_stats(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Statistics collection must not prevent reset or power management. */
+	ret = __acx00_ephy_update_stats(phydev);
+	if (ret)
+		phydev_dbg(phydev, "failed to snapshot statistics: %pe\n", ERR_PTR(ret));
+}
+
 static int acx00_ephy_control_power_on(struct acx00_ephy_priv *priv)
 {
 	lockdep_assert_held(&priv->state_lock);
@@ -122,6 +261,10 @@ static int acx00_ephy_control_power_off(struct acx00_ephy_priv *priv)
 {
 	lockdep_assert_held(&priv->state_lock);
 
+	/* Even a failed power-off can have reset the hardware counter. */
+	priv->stats_active = false;
+	priv->stats_valid = false;
+
 	return priv->control->power_off(priv->control);
 }
 
@@ -350,6 +493,10 @@ static int acx00_ephy_soft_reset(struct phy_device *phydev)
 
 	mutex_lock(&priv->state_lock);
 
+	acx00_ephy_snapshot_stats(phydev);
+	priv->stats_active = false;
+	priv->stats_valid = false;
+
 	ret = acx00_ephy_set_interface(phydev);
 	if (ret)
 		goto out_power_off;
@@ -475,10 +622,21 @@ static int __acx00_ephy_config_init(struct phy_device *phydev)
 
 	/* Restore the standard EEE policy retained by phylib across resets. */
 	ret = genphy_c45_an_config_eee_aneg(phydev);
-	if (ret <= 0)
+	if (ret < 0)
 		return ret;
+	if (ret > 0) {
+		ret = genphy_restart_aneg(phydev);
+		if (ret)
+			return ret;
+	}
 
-	return genphy_restart_aneg(phydev);
+	if (!priv->stats_active) {
+		/* Establish a new baseline after reset, retaining the software total. */
+		priv->stats_active = true;
+		acx00_ephy_snapshot_stats(phydev);
+	}
+
+	return 0;
 }
 
 static int acx00_ephy_config_init(struct phy_device *phydev)
@@ -550,6 +708,8 @@ static int acx00_ephy_suspend(struct phy_device *phydev)
 	mutex_lock(&phydev->lock);
 	mutex_lock(&priv->state_lock);
 
+	acx00_ephy_snapshot_stats(phydev);
+
 	ret = genphy_suspend(phydev);
 	if (ret)
 		goto out_unlock;
@@ -616,6 +776,8 @@ static struct phy_driver acx00_ephy_driver[] = {
 		.soft_reset = acx00_ephy_soft_reset,
 		.config_init = acx00_ephy_config_init,
 		.disable_autonomous_eee = acx00_ephy_disable_autonomous_eee,
+		.get_phy_stats = acx00_ephy_get_phy_stats,
+		.update_stats = acx00_ephy_update_stats,
 		.suspend = acx00_ephy_suspend,
 		.resume = acx00_ephy_resume,
 	},

-- 
2.53.0

Re: [PATCH net-next v3 2/3] net: phy: acx00: report receive statistics

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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] The receive-byte wrap modulus was measured on AC300 only, but
  the driver applies it to AC200 as well.
- [Medium] acx00_ephy_soft_reset() takes its 'final' statistics snapshot
  on the assumption that the EPHY has not been reset yet, but phylib can…
- [Medium] __acx00_ephy_update_stats() can only account for one hardware
  rollover between two samples (lines 204-208).

--

net: phy: acx00: report receive statistics

This patch reports the ACx00 EPHY receive frame, byte and CRC error
counters through the generic PHY statistics interface. The 32-bit
hardware counters are sampled from phylib's update_stats callback and
added up as wrap-adjusted deltas in 64-bit software totals. The hardware
baseline is invalidated around soft reset and power-off.

> diff --git a/drivers/net/phy/xpowers/acx00.c b/drivers/net/phy/xpowers/acx00.c
> index d19ba6d8069a4..9ab98f6174442 100644
> --- a/drivers/net/phy/xpowers/acx00.c
> +++ b/drivers/net/phy/xpowers/acx00.c

[ ... ]

> @@ -76,6 +78,30 @@
>  	(ACX00_PAGE8_AUTO_CAL_TX_LEVEL_ADJUST_BYPASS | \
>  	 ACX00_PAGE8_AUTO_CAL_LOW_CAL_OPAQUE_BITS)
>  
> +#define ACX00_PAGE9_RX_BYTES_HIGH_REG		0x19
> +#define ACX00_PAGE9_RX_PACKETS_HIGH_REG		0x1b
> +#define ACX00_PAGE9_RX_CRC_ERRORS_HIGH_REG	0x1d
> +
> +/*
> + * AC300 hardware testing confirmed that the receive byte counter wraps at
> + * 4,000,000,000 rather than 2^32.
> + */
> +#define ACX00_RX_BYTES_WRAP			4000000000ULL

[Severity: Medium]
This modulus was only measured on AC300. Does it also hold for AC200?

The single "X-Powers AC200/AC300 EPHY" phy_driver entry installs
acx00_ephy_update_stats() and acx00_ephy_get_phy_stats() for both
packages. __acx00_ephy_update_stats() never checks priv->is_ac300.

On AC200, every sample with a byte count at or above 4e9 is thrown
away. The packet and CRC-error totals also do not advance for that poll:

	if (ret || counts[ACX00_STAT_RX_BYTES] >= ACX00_RX_BYTES_WRAP)
		return 0;

Byte wraps are also folded modulo 4e9:

		count += i == ACX00_STAT_RX_BYTES ?
			ACX00_RX_BYTES_WRAP : BIT_ULL(32);

Suppose the AC200 byte counter really wraps at 2^32. Wouldn't
rx_bytes_total then lose 294,967,296 bytes per wrap? At 100 Mbit/s that
is about every 5 to 6 minutes.

Could the commit message and comment say whether AC200 was measured?
Alternatively, could the byte statistic be limited to AC300?

A related point: the commit message does not confirm that the hardware
byte counter matches the rx_bytes_total definition from the preceding
"ethtool: add PHY receive frame and byte totals" patch. That definition
counts the Ethernet header, padding and FCS, and excludes the preamble
and SFD. Has this been checked on hardware?

[ ... ]

> @@ -350,6 +493,10 @@ static int acx00_ephy_soft_reset(struct phy_device *phydev)
>  
>  	mutex_lock(&priv->state_lock);
>  
> +	acx00_ephy_snapshot_stats(phydev);
> +	priv->stats_active = false;
> +	priv->stats_valid = false;
> +

[Severity: Medium]
Can this snapshot run after the hardware has already been reset?

For SIOCSMIIREG on MII_BMCR, phy_mii_ioctl() writes the value to the PHY
first and only calls phy_init_hw() afterwards:

drivers/net/phy/phy.c:phy_mii_ioctl() {
    ...
		else
			mdiobus_write(phydev->mdio.bus, prtad, devad, val);

		if (prtad == phydev->mdio.addr &&
		    devad == MII_BMCR &&
		    val & BMCR_RESET)
			return phy_init_hw(phydev);
    ...
}

phy_init_hw() then calls acx00_ephy_soft_reset(), which takes the
snapshot while stats_valid is still true. If the reset cleared the page 9
counters, they now read below stats_last[].
__acx00_ephy_update_stats() counts that as one full wrap:

			if (count < priv->stats_last[i])
				count += i == ACX00_STAT_RX_BYTES ?
					ACX00_RX_BYTES_WRAP : BIT_ULL(32);

For example, CRC errors dropping from 5 to 0 would add 4,294,967,291 to
rx_errors. Bytes dropping from 1,000,000 to 0 would add 3,999,000,000 to
rx_bytes_total. These are cumulative software totals, so the error would
never go away.

phy_detach_internal() seems to have a similar path. With WoL enabled,
phy_suspend() returns -EBUSY without calling acx00_ephy_suspend(), so
stats_active and stats_valid stay true. phy_detach_internal() then
asserts the reset GPIO with phy_device_reset(phydev, 1). On re-attach,
phy_init_hw() deasserts it, and soft_reset then takes its snapshot
against the old baseline.

The commit message says "Invalidate the hardware baseline before reset".
For resets that the phylib core applies before soft_reset runs, isn't
the old baseline still used after the reset?

[ ... ]

> @@ -475,10 +622,21 @@ static int __acx00_ephy_config_init(struct phy_device *phydev)
>  
>  	/* Restore the standard EEE policy retained by phylib across resets. */
>  	ret = genphy_c45_an_config_eee_aneg(phydev);
> -	if (ret <= 0)
> +	if (ret < 0)
>  		return ret;
> +	if (ret > 0) {
> +		ret = genphy_restart_aneg(phydev);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	return genphy_restart_aneg(phydev);
> +	if (!priv->stats_active) {
> +		/* Establish a new baseline after reset, retaining the software total. */
> +		priv->stats_active = true;
> +		acx00_ephy_snapshot_stats(phydev);
> +	}
> +
> +	return 0;
>  }

[Severity: Medium]
Could keeping the live baseline here lose full byte-counter wraps?

__acx00_ephy_update_stats() can account for at most one rollover between
two samples. phylib only calls update_stats from the PHY_NOLINK and
PHY_RUNNING states in _phy_state_machine(). After phy_stop(), nothing is
sampled while the PHY is in PHY_HALTED.

With WoL enabled, phy_suspend() skips drv->suspend:

	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
		return -EBUSY;

So acx00_ephy_suspend() never takes a snapshot or clears stats_active.
The PHY stays powered and keeps counting received traffic.

On phy_start(), the resume path is:

__phy_resume()
  acx00_ephy_resume()
    __acx00_ephy_power_on_and_resume()
      __acx00_ephy_config_init()

__acx00_ephy_config_init() sees stats_active == true and keeps the old
baseline. The next poll then computes one delta covering the whole
halted interval.

At 100 Mbit/s the byte counter wraps about every 320 seconds. Wouldn't
each extra full wrap during the unpolled interval be silently dropped
from rx_bytes_total? That would leave the traffic received while halted
counted modulo 4e9.

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

[PATCH net-next v3 3/3] net: phy: dp83tg720: report inclusive RX packet totals

[James Hilliard]

The DP83TG720S-Q1 datasheet describes PKT_STAT_4 and PKT_STAT_5
(0x063c and 0x063d) as the total received packet count, including packets
with errors. The driver currently reports this count through rx_packets,
which is defined as successfully received packets.

Report the accumulated count through rx_packets_total instead. This
exposes it as RxFramesTotal rather than RxFrames in the ethtool phydev
statistics group, matching the hardware counter's semantics.

Fixes: 677d895af1cc ("net: phy: dp83tg720: add statistics support")
Link: https://www.ti.com/lit/ds/symlink/dp83tg720s-q1.pdf
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/phy/dp83tg720.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83tg720.c b/drivers/net/phy/dp83tg720.c
index 391c1d868808..5824eff92937 100644
--- a/drivers/net/phy/dp83tg720.c
+++ b/drivers/net/phy/dp83tg720.c
@@ -309,7 +309,7 @@ static void dp83tg720_get_phy_stats(struct phy_device *phydev,
 
 	stats->tx_packets = priv->stats.tx_pkt_cnt;
 	stats->tx_errors = priv->stats.tx_err_pkt_cnt;
-	stats->rx_packets = priv->stats.rx_pkt_cnt;
+	stats->rx_packets_total = priv->stats.rx_pkt_cnt;
 	stats->rx_errors = priv->stats.rx_err_pkt_cnt;
 }
 

-- 
2.53.0

Re: [PATCH net-next v3 3/3] net: phy: dp83tg720: report inclusive RX packet totals

[netdev-bot+sashiko]

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] The patch carries `Fixes: 677d895af1cc ("net: phy: dp83tg720: add
  statistics support")`, which tells AUTOSEL/stable to backport it to…

Pre-existing issues:
- [Medium] After this patch, two TI PHY drivers with the same counter
  layout map it to different uAPI attributes.

--

net: phy: dp83tg720: report inclusive RX packet totals

This commit moves the accumulated PKT_STAT_4/PKT_STAT_5 RX packet count in
dp83tg720_get_phy_stats() from rx_packets to rx_packets_total. The datasheet
says the hardware counter includes errored packets. Userspace now sees it as
RxFramesTotal instead of RxFrames in the ethtool phydev statistics group.

Link: https://www.ti.com/lit/ds/symlink/dp83tg720s-q1.pdf

> diff --git a/drivers/net/phy/dp83tg720.c b/drivers/net/phy/dp83tg720.c
> index 391c1d8688083..5824eff92937c 100644
> --- a/drivers/net/phy/dp83tg720.c
> +++ b/drivers/net/phy/dp83tg720.c
> @@ -309,7 +309,7 @@ static void dp83tg720_get_phy_stats(struct phy_device *phydev,
>  
>  	stats->tx_packets = priv->stats.tx_pkt_cnt;
>  	stats->tx_errors = priv->stats.tx_err_pkt_cnt;
> -	stats->rx_packets = priv->stats.rx_pkt_cnt;
> +	stats->rx_packets_total = priv->stats.rx_pkt_cnt;

[Severity: Low]
Can this be backported on its own, given the Fixes: tag?

    Fixes: 677d895af1cc ("net: phy: dp83tg720: add statistics support")

The rx_packets_total member of struct ethtool_phy_stats doesn't exist at
the baseline. It comes from the earlier patch in this series, "ethtool: add
PHY receive frame and byte totals". That patch also adds the kerneldoc, the
ETHTOOL_A_STATS_PHY_RX_PKTS_TOTAL uAPI attribute and the "RxFramesTotal"
string.

Stable tooling will pick this up for kernels that contain 677d895af1cc.
There it would either fail to build in dp83tg720_get_phy_stats() or bring
a new uAPI attribute into stable. It would also change which phydev stats
userspace sees for this PHY.

Should the Fixes: tag be dropped? Alternatively, could the commit message
state the dependency on the ethtool patch and say it isn't meant for
stable alone?

[Severity: Medium]
This isn't a bug introduced by this patch, but it creates a difference
between this driver and dp83td510.c, which still does this in
dp83td510_get_phy_stats():

drivers/net/phy/dp83td510.c:dp83td510_get_phy_stats() {
    ...
	stats->rx_packets = priv->stats.rx_pkt_cnt;
	stats->rx_errors = priv->stats.rx_err_pkt_cnt;
    ...
}

Both drivers describe the registers the same way:

 * - DP83TD510E_PKT_STAT_4: Contains RX packet count bits [15:0].
 * - DP83TD510E_PKT_STAT_5: Contains RX packet count bits [31:16].
 * - DP83TD510E_PKT_STAT_6: Contains RX error packet count.

The update_stats code and the rx_pkt_cnt/rx_err_pkt_cnt fields are also
the same. include/linux/ethtool.h documents rx_packets as "Total
successfully received frames".

Does the DP83TD510E datasheet also define PKT_STAT_4/5 as a total that
includes errored packets? If so, dp83td510 would report RxFrames with the
same overcount this patch fixes for dp83tg720. Userspace would then see
different meanings for RxFrames depending on which TI PHY is attached.

Should dp83td510_get_phy_stats() get the same change?

>  	stats->rx_errors = priv->stats.rx_err_pkt_cnt;
>  }
>

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