Re: [PATCH net-next v3 3/3] net: phy: dp83tg720: report inclusive RX packet 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] 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