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

[netdev-bot+sashiko@kernel.org]

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