[PATCH net-next v2] net: phy: acx00: report receive statistics

[PATCH net-next v2] net: phy: acx00: report receive statistics

[James Hilliard]

Expose receive frame and byte totals through the generic PHY statistics
interface as rx_packets and rx_bytes, alongside CRC errors in rx_errors.
Report the hardware totals directly: the packet and byte counters include
errored frames as well as successfully received traffic.

Collect the free-running 32-bit counters through phylib's
update_stats callback and accumulate wrap-safe deltas in 64-bit totals.
Read the high half on either side of the low half and retry until both
high-half reads agree.

Hardware testing confirmed that the receive-byte counter wraps at
4,000,000,000, while the receive-packet and CRC-error counters wrap at
2^32. Use the corresponding modulus for each counter so rollover does
not inflate the software totals.

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 sample only after all counter reads 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. A failed initial snapshot can be
retried by the periodic collector without preventing initialization.

Signed-off-by: James Hilliard <james.hilliard1@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

To: Andrew Lunn <andrew@lunn.ch>
To: Heiner Kallweit <hkallweit1@gmail.com>
To: Russell King <linux@armlinux.org.uk>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/xpowers/acx00.c | 157 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 154 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/xpowers/acx00.c b/drivers/net/phy/xpowers/acx00.c
index d19ba6d8069a..1375784d2837 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,29 @@
 	(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
+
+/*
+ * Hardware testing confirmed that the receive byte counter wraps at
+ * 4,000,000,000 rather than 2^32.
+ */
+#define ACX00_RX_BYTES_WRAP			4000000000ULL
+
+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 +111,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 +139,105 @@ static int acx00_ephy_write_page(struct phy_device *phydev, int page)
 			   FIELD_PREP(ACX00_PAGE_SELECT_MASK, page));
 }
 
+static int acx00_ephy_read_counter(struct phy_device *phydev, u8 reg, u32 *count)
+{
+	int high, low, check;
+
+	/* The free-running counter can carry while its two halves are read. */
+	do {
+		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;
+	} while (high != check);
+
+	*count = (u32)high << 16 | low;
+	return 0;
+}
+
+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)
+		return ret;
+
+	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 = priv->stats[ACX00_STAT_RX_PACKETS];
+	stats->rx_bytes = 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 +250,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 +482,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 +611,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 +697,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 +765,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,
 	},

---
base-commit: 4dd16e31135ea8437a720222df34e8745f385d6d
change-id: 20261024-submit-acx00-phy-stats-v1-d838f77444e8

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

Re: [PATCH net-next v2] net: phy: acx00: report receive statistics

[Andrew Lunn]

On Mon, Nov 02, 2026 at 04:28:28PM -0700, James Hilliard wrote:
> Expose receive frame and byte totals through the generic PHY statistics
> interface as rx_packets and rx_bytes, alongside CRC errors in rx_errors.
> Report the hardware totals directly: the packet and byte counters include
> errored frames as well as successfully received traffic.
> 
> Collect the free-running 32-bit counters through phylib's
> update_stats callback and accumulate wrap-safe deltas in 64-bit totals.
> Read the high half on either side of the low half and retry until both
> high-half reads agree.
> 
> Hardware testing confirmed that the receive-byte counter wraps at
> 4,000,000,000, while the receive-packet and CRC-error counters wrap at
> 2^32. Use the corresponding modulus for each counter so rollover does
> not inflate the software totals.
> 
> 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 sample only after all counter reads 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. A failed initial snapshot can be
> retried by the periodic collector without preventing initialization.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2] net: phy: acx00: report receive statistics

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: acx00: report receive statistics

This patch exposes the ACx00 EPHY receive frame, byte and CRC error
counters through the generic PHY statistics interface. The page 9
hardware counters are sampled from the update_stats callback, and the
deltas are accumulated into 64-bit software totals. Snapshots are taken
before reset and suspend, and a new baseline is set after config_init.

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

[ ... ]

> @@ -110,6 +139,105 @@ static int acx00_ephy_write_page(struct phy_device *phydev, int page)
>  			   FIELD_PREP(ACX00_PAGE_SELECT_MASK, page));
>  }
>  
> +static int acx00_ephy_read_counter(struct phy_device *phydev, u8 reg, u32 *count)
> +{
> +	int high, low, check;
> +
> +	/* The free-running counter can carry while its two halves are read. */
> +	do {
> +		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;
> +	} while (high != check);

[Severity: Medium]
Is there anything that bounds this loop? It only exits on an MDIO error
or when high == check. There is no limit on the number of passes.

When this runs, __acx00_ephy_update_stats() has already taken the bus
mdio_lock through phy_select_page(), and priv->state_lock is held. On the
periodic path phydev->lock is held as well:

_phy_state_machine()
  phy_update_stats()
    acx00_ephy_update_stats()
      __acx00_ephy_update_stats()
        phy_select_page()        /* takes mdio_lock */
        acx00_ephy_read_counter()

Suppose the high half reads differently on every pass. That could come
from noisy or faulty register reads. It could also come from a pass
delayed by about 5 ms while the RX byte high half is advancing at line
rate.

Would the state machine worker and every other MDIO user on the bus then
block, including the AC300 control path? Would suspend, resume,
soft_reset, config_init and get_phy_stats block too, since they need
state_lock or phydev->lock?

Would a small retry limit that returns -EIO or -EAGAIN be preferable?

> +
> +	*count = (u32)high << 16 | low;
> +	return 0;
> +}
> +
> +static int __acx00_ephy_update_stats(struct phy_device *phydev)
> +{

[ ... ]

> +	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];

[Severity: Medium]
Can this subtraction underflow for the RX byte counter?

ACX00_RX_BYTES_WRAP is 4,000,000,000, but acx00_ephy_read_counter() can
return any value up to 0xFFFFFFFF. Nothing checks that a sample is below
the wrap value before it is stored in stats_last[].

If stats_last[] is at least 4e9 and a later sample satisfies
count + 4e9 < stats_last[], the u64 subtraction wraps. For example, with
stats_last = 0xFFFFFFFF and count = 1000:

  4,000,001,000 - 4,294,967,295  ->  about 1.8e19

That result is then added to priv->stats[ACX00_STAT_RX_BYTES].

One way to get such a sample is a PHY that stops driving MDIO while
stats_active is true. Consistent 0xFFFF reads pass the high == check
test and produce 0xFFFFFFFF. Another is a variant whose byte counter
does not wrap at exactly 4e9, since that value comes from testing on
one setup.

priv->stats[] is deliberately kept across reset, power-off and suspend.
Would one bad sample leave rx_bytes wrong for the life of the device?

Would a modular delta such as (count + WRAP - last) % WRAP, or a range
check on the sample, avoid this?

> +		}
> +		priv->stats_last[i] = counts[i];
> +	}
> +	priv->stats_valid = true;
> +
> +	return 0;
> +}

[ ... ]

> +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 = priv->stats[ACX00_STAT_RX_PACKETS];
> +	stats->rx_bytes = priv->stats[ACX00_STAT_RX_BYTES];
> +	stats->rx_errors = priv->stats[ACX00_STAT_RX_CRC_ERRORS];

[Severity: Medium]
Do these values match the documented meaning of the fields? The kernel-doc
for struct ethtool_phy_stats in include/linux/ethtool.h says:

 * @rx_packets: Total successfully received frames
 * @rx_bytes: Total successfully received bytes
 * @rx_errors: Total received frames with errors (e.g., CRC errors)

The commit message says "the packet and byte counters include errored
frames as well as successfully received traffic", and
__acx00_ephy_update_stats() accumulates them unchanged. That means a CRC
errored frame is counted in both rx_errors and rx_packets, and its octets
are counted in rx_bytes.

stats_put_phydev_stats() in net/ethtool/stats.c exports these values as
ETHTOOL_A_STATS_PHY_RX_PKTS and ETHTOOL_A_STATS_PHY_RX_BYTES. For this PHY
those attributes would therefore mean something different from the
documented contract.

rx_packets could be corrected by subtracting the CRC error count.
rx_bytes cannot be corrected without a per-frame byte count. Should
rx_bytes be left unreported, or should the shared documentation be
changed instead?

> +	mutex_unlock(&priv->state_lock);
> +}

[ ... ]
-- 
pw-bot: cr