[RFC net-next 0/2] net: ethtool: add phy(dev) specific stats over netlink

[RFC net-next 0/2] net: ethtool: add phy(dev) specific stats over netlink

[Jakub Kicinski]

These are the half-baked patches I promised in:
https://lore.kernel.org/r/20240828133428.4988b44f@kernel.org/

I'm hoping Oleksij will take these over / forward.

Jakub Kicinski (2):
  net: ethtool: plumb PHY stats to PHY drivers
  net: ethtool: add phy(dev) specific stats over netlink

 Documentation/networking/ethtool-netlink.rst |  1 +
 include/linux/ethtool.h                      | 15 +++++
 include/linux/phy.h                          | 11 ++++
 include/uapi/linux/ethtool.h                 |  2 +
 include/uapi/linux/ethtool_netlink.h         | 15 +++++
 net/ethtool/linkstate.c                      | 25 ++++++++-
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/stats.c                          | 58 ++++++++++++++++++++
 net/ethtool/strset.c                         |  5 ++
 9 files changed, 130 insertions(+), 3 deletions(-)

-- 
2.46.0

[RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Jakub Kicinski]

Feed the existing IEEE PHY counter struct (which currently
only has one entry) and link stats into the PHY driver.
The MAC driver can override the value if it somehow has a better
idea of PHY stats. Since the stats are "undefined" at input
the drivers can't += the values, so we should be safe from
double-counting.

Vladimir, I don't understand MM but doesn't MM share the PHY?
Ocelot seems to aggregate which I did not expect.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: Vladimir Oltean <olteanv@gmail.com>
CC: andrew@lunn.ch
CC: hkallweit1@gmail.com
CC: linux@armlinux.org.uk
CC: woojung.huh@microchip.com
CC: o.rempel@pengutronix.de
---
 include/linux/phy.h     | 10 ++++++++++
 net/ethtool/linkstate.c | 25 ++++++++++++++++++++++---
 net/ethtool/stats.c     | 19 +++++++++++++++++++
 3 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 4a9a11749c55..53942fd7760f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1090,6 +1090,16 @@ struct phy_driver {
 	int (*cable_test_get_status)(struct phy_device *dev, bool *finished);
 
 	/* Get statistics from the PHY using ethtool */
+	/**
+	 * @get_phy_stats: Get well known statistics.
+	 * @get_link_stats: Get well known link statistics.
+	 * The input structure is not zero-initialized and the implementation
+	 * must only set statistics which are actually collected by the device.
+	 */
+	void (*get_phy_stats)(struct phy_device *dev,
+			      struct ethtool_eth_phy_stats *eth_stats);
+	void (*get_link_stats)(struct phy_device *dev,
+			       struct ethtool_link_ext_stats *link_stats);
 	/** @get_sset_count: Number of statistic counters */
 	int (*get_sset_count)(struct phy_device *dev);
 	/** @get_strings: Names of the statistic counters */
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 34d76e87847d..8d3a38cc3d48 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -94,6 +94,27 @@ static int linkstate_get_link_ext_state(struct net_device *dev,
 	return 0;
 }
 
+static void
+ethtool_get_phydev_stats(struct net_device *dev,
+			 struct linkstate_reply_data *data)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (!phydev)
+		return;
+
+	if (dev->phydev)
+		data->link_stats.link_down_events =
+			READ_ONCE(dev->phydev->link_down_events);
+
+	if (!phydev->drv || !phydev->drv->get_link_stats)
+		return;
+
+	mutex_lock(&phydev->lock);
+	phydev->drv->get_link_stats(phydev, &data->link_stats);
+	mutex_unlock(&phydev->lock);
+}
+
 static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 				  struct ethnl_reply_data *reply_base,
 				  const struct genl_info *info)
@@ -127,9 +148,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 			   sizeof(data->link_stats) / 8);
 
 	if (req_base->flags & ETHTOOL_FLAG_STATS) {
-		if (dev->phydev)
-			data->link_stats.link_down_events =
-				READ_ONCE(dev->phydev->link_down_events);
+		ethtool_get_phydev_stats(dev, data);
 
 		if (dev->ethtool_ops->get_link_ext_stats)
 			dev->ethtool_ops->get_link_ext_stats(dev,
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 912f0c4fff2f..cf802b1cda6f 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
+#include <linux/phy.h>
+
 #include "netlink.h"
 #include "common.h"
 #include "bitset.h"
@@ -112,6 +114,19 @@ static int stats_parse_request(struct ethnl_req_info *req_base,
 	return 0;
 }
 
+static void
+ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (!phydev || !phydev->drv || !phydev->drv->get_phy_stats)
+		return;
+
+	mutex_lock(&phydev->lock);
+	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
+	mutex_unlock(&phydev->lock);
+}
+
 static int stats_prepare_data(const struct ethnl_req_info *req_base,
 			      struct ethnl_reply_data *reply_base,
 			      const struct genl_info *info)
@@ -145,6 +160,10 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	data->ctrl_stats.src = src;
 	data->rmon_stats.src = src;
 
+	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
+	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
+		ethtool_get_phydev_stats(dev, data);
+
 	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
 	    dev->ethtool_ops->get_eth_phy_stats)
 		dev->ethtool_ops->get_eth_phy_stats(dev, &data->phy_stats);
-- 
2.46.0

[RFC net-next 2/2] net: ethtool: add phy(dev) specific stats over netlink

[Jakub Kicinski]

Define a way of getting PHY stats over the netlink API.
According to my limited understanding of PHYs they are more
standard-based than the rest of a network interface, so hopefully
these stats can be extended to cover most cases.

There's a bit of strategic ambiguity between phy and phydev
here, in case some integrated device wants to use these later.
But the new group is separate from eth-phy which is reserved
for IEEE stats.

Oleksij, if this patch works you can add the OA stats into
struct ethtool_phy_stats or struct ethtool_link_ext_stats.
They should show up in ethtool -S eth0 --all-groups
for the former or in ethtool -I eth0 for the latter.
Note link_stats need changes to ethtool CLI, but
they seem better suited for the stats based on their names?

There's a minor TODO for you to define the semantics
of the error counter, based on however the DP83TG720 PHY
behaves.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: corbet@lwn.net
CC: andrew@lunn.ch
CC: hkallweit1@gmail.com
CC: linux@armlinux.org.uk
CC: ecree.xilinx@gmail.com
CC: przemyslaw.kitszel@intel.com
CC: kory.maincent@bootlin.com
CC: maxime.chevallier@bootlin.com
CC: linux-doc@vger.kernel.org
---
 Documentation/networking/ethtool-netlink.rst |  1 +
 include/linux/ethtool.h                      | 15 +++++++
 include/linux/phy.h                          |  3 +-
 include/uapi/linux/ethtool.h                 |  2 +
 include/uapi/linux/ethtool_netlink.h         | 15 +++++++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/stats.c                          | 43 +++++++++++++++++++-
 net/ethtool/strset.c                         |  5 +++
 8 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index ba90457b8b2d..7f6c23644645 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1608,6 +1608,7 @@ Users specify which groups of statistics they are requesting via
  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
  ====================== ======== ===============================================
 
 Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12f6dc567598..0a09ea82e001 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -412,6 +412,21 @@ struct ethtool_eth_phy_stats {
 	);
 };
 
+/* Additional PHY statistics, not defined by IEEE */
+struct ethtool_phy_stats {
+	/* Basic packet / byte counters are meant for PHY drivers */
+	u64 rx_packets;
+	u64 rx_bytes;
+	u64 rx_error; /* TODO: we need to define here whether packet
+		       * counted here is also counted as rx_packets,
+		       * and whether it's passed to the MAC with some
+		       * error indication or MAC never sees it.
+		       */
+	u64 tx_packets;
+	u64 tx_bytes;
+	u64 tx_error; /* TODO: same as for rx */
+};
+
 /* Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*), not otherwise exposed
  * via a more targeted API.
  */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 53942fd7760f..9c3094706c7a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1097,7 +1097,8 @@ struct phy_driver {
 	 * must only set statistics which are actually collected by the device.
 	 */
 	void (*get_phy_stats)(struct phy_device *dev,
-			      struct ethtool_eth_phy_stats *eth_stats);
+			      struct ethtool_eth_phy_stats *eth_stats,
+			      struct ethtool_phy_stats *stats);
 	void (*get_link_stats)(struct phy_device *dev,
 			       struct ethtool_link_ext_stats *link_stats);
 	/** @get_sset_count: Number of statistic counters */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index c405ed63acfa..977c5a8a0d6b 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -681,6 +681,7 @@ enum ethtool_link_ext_substate_module {
  * @ETH_SS_STATS_ETH_MAC: names of IEEE 802.3 MAC statistics
  * @ETH_SS_STATS_ETH_CTRL: names of IEEE 802.3 MAC Control statistics
  * @ETH_SS_STATS_RMON: names of RMON statistics
+ * @ETH_SS_STATS_PHY: names of PHY(dev) statistics
  *
  * @ETH_SS_COUNT: number of defined string sets
  */
@@ -706,6 +707,7 @@ enum ethtool_stringset {
 	ETH_SS_STATS_ETH_MAC,
 	ETH_SS_STATS_ETH_CTRL,
 	ETH_SS_STATS_RMON,
+	ETH_SS_STATS_PHY,
 
 	/* add new constants above here */
 	ETH_SS_COUNT
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 283305f6b063..dc332f8aa3a6 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -820,6 +820,7 @@ enum {
 	ETHTOOL_STATS_ETH_MAC,
 	ETHTOOL_STATS_ETH_CTRL,
 	ETHTOOL_STATS_RMON,
+	ETHTOOL_STATS_PHY,
 
 	/* add new constants above here */
 	__ETHTOOL_STATS_CNT
@@ -935,6 +936,20 @@ enum {
 	ETHTOOL_A_STATS_RMON_MAX = (__ETHTOOL_A_STATS_RMON_CNT - 1)
 };
 
+enum {
+	/* Basic packet counters if PHY has separate counters from the MAC */
+	ETHTOOL_A_STATS_PHY_RX_PKTS,
+	ETHTOOL_A_STATS_PHY_RX_BYTES,
+	ETHTOOL_A_STATS_PHY_RX_ERRORS,
+	ETHTOOL_A_STATS_PHY_TX_PKTS,
+	ETHTOOL_A_STATS_PHY_TX_BYTES,
+	ETHTOOL_A_STATS_PHY_TX_ERRORS,
+
+	/* add new constants above here */
+	__ETHTOOL_A_STATS_PHY_CNT,
+	ETHTOOL_A_STATS_PHY_MAX = (__ETHTOOL_A_STATS_PHY_CNT - 1)
+};
+
 /* MODULE */
 
 enum {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 203b08eb6c6f..3a6ecdcb5b8c 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -505,5 +505,6 @@ extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING
 extern const char stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_ctrl_names[__ETHTOOL_A_STATS_ETH_CTRL_CNT][ETH_GSTRING_LEN];
 extern const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN];
+extern const char stats_phy_names[__ETHTOOL_A_STATS_PHY_CNT][ETH_GSTRING_LEN];
 
 #endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index cf802b1cda6f..8ae3c57cea21 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -22,6 +22,7 @@ struct stats_reply_data {
 		struct ethtool_eth_mac_stats	mac_stats;
 		struct ethtool_eth_ctrl_stats	ctrl_stats;
 		struct ethtool_rmon_stats	rmon_stats;
+		struct ethtool_phy_stats	phydev_stats;
 	);
 	const struct ethtool_rmon_hist_range	*rmon_ranges;
 };
@@ -34,6 +35,7 @@ const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_STATS_ETH_MAC]			= "eth-mac",
 	[ETHTOOL_STATS_ETH_CTRL]		= "eth-ctrl",
 	[ETHTOOL_STATS_RMON]			= "rmon",
+	[ETHTOOL_STATS_PHY]			= "phydev",
 };
 
 const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
@@ -78,6 +80,15 @@ const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_A_STATS_RMON_JABBER]		= "etherStatsJabbers",
 };
 
+const char stats_phy_names[__ETHTOOL_A_STATS_PHY_CNT][ETH_GSTRING_LEN] = {
+	[ETHTOOL_A_STATS_PHY_RX_PKTS]		= "RxFrames",
+	[ETHTOOL_A_STATS_PHY_RX_BYTES]		= "RxOctets",
+	[ETHTOOL_A_STATS_PHY_RX_ERRORS]		= "RxErrors",
+	[ETHTOOL_A_STATS_PHY_TX_PKTS]		= "TxFrames",
+	[ETHTOOL_A_STATS_PHY_TX_BYTES]		= "TxOctets",
+	[ETHTOOL_A_STATS_PHY_TX_ERRORS]		= "TxErrors",
+};
+
 const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_SRC + 1] = {
 	[ETHTOOL_A_STATS_HEADER]	=
 		NLA_POLICY_NESTED(ethnl_header_policy),
@@ -123,7 +134,8 @@ ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
 		return;
 
 	mutex_lock(&phydev->lock);
-	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
+	phydev->drv->get_phy_stats(phydev, &data->phy_stats,
+				   &data->phydev_stats);
 	mutex_unlock(&phydev->lock);
 }
 
@@ -160,7 +172,8 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	data->ctrl_stats.src = src;
 	data->rmon_stats.src = src;
 
-	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
+	if ((test_bit(ETHTOOL_STATS_PHY, req_info->stat_mask) ||
+	     test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask)) &&
 	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
 		ethtool_get_phydev_stats(dev, data);
 
@@ -213,6 +226,10 @@ static int stats_reply_size(const struct ethnl_req_info *req_base,
 			nla_total_size(4)) *	/* _A_STATS_GRP_HIST_BKT_HI */
 			ETHTOOL_RMON_HIST_MAX * 2;
 	}
+	if (test_bit(ETHTOOL_STATS_PHY, req_info->stat_mask)) {
+		n_stats += sizeof(struct ethtool_phy_stats) / sizeof(u64);
+		n_grps++;
+	}
 
 	len += n_grps * (nla_total_size(0) + /* _A_STATS_GRP */
 			 nla_total_size(4) + /* _A_STATS_GRP_ID */
@@ -266,6 +283,25 @@ static int stats_put_phy_stats(struct sk_buff *skb,
 	return 0;
 }
 
+static int stats_put_phydev_stats(struct sk_buff *skb,
+				  const struct stats_reply_data *data)
+{
+	if (stat_put(skb, ETHTOOL_A_STATS_PHY_RX_PKTS,
+		     data->phydev_stats.rx_packets) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_RX_BYTES,
+		     data->phydev_stats.rx_packets) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_RX_ERRORS,
+		     data->phydev_stats.rx_packets) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_TX_PKTS,
+		     data->phydev_stats.rx_packets) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_TX_BYTES,
+		     data->phydev_stats.rx_packets) ||
+	    stat_put(skb, ETHTOOL_A_STATS_PHY_TX_ERRORS,
+		     data->phydev_stats.rx_packets))
+		return -EMSGSIZE;
+	return 0;
+}
+
 static int stats_put_mac_stats(struct sk_buff *skb,
 			       const struct stats_reply_data *data)
 {
@@ -442,6 +478,9 @@ static int stats_fill_reply(struct sk_buff *skb,
 	if (!ret && test_bit(ETHTOOL_STATS_RMON, req_info->stat_mask))
 		ret = stats_put_stats(skb, data, ETHTOOL_STATS_RMON,
 				      ETH_SS_STATS_RMON, stats_put_rmon_stats);
+	if (!ret && test_bit(ETHTOOL_STATS_PHY, req_info->stat_mask))
+		ret = stats_put_stats(skb, data, ETHTOOL_STATS_PHY,
+				      ETH_SS_STATS_PHY, stats_put_phydev_stats);
 
 	return ret;
 }
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index b3382b3cf325..818cf01f0911 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -105,6 +105,11 @@ static const struct strset_info info_template[] = {
 		.count		= __ETHTOOL_A_STATS_RMON_CNT,
 		.strings	= stats_rmon_names,
 	},
+	[ETH_SS_STATS_PHY] = {
+		.per_dev	= false,
+		.count		= __ETHTOOL_A_STATS_PHY_CNT,
+		.strings	= stats_phy_names,
+	},
 };
 
 struct strset_req_info {
-- 
2.46.0

Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Oleksij Rempel]

On Thu, Aug 29, 2024 at 10:43:41AM -0700, Jakub Kicinski wrote:
> Feed the existing IEEE PHY counter struct (which currently
> only has one entry) and link stats into the PHY driver.
> The MAC driver can override the value if it somehow has a better
> idea of PHY stats. Since the stats are "undefined" at input
> the drivers can't += the values, so we should be safe from
> double-counting.
> 
> Vladimir, I don't understand MM but doesn't MM share the PHY?
> Ocelot seems to aggregate which I did not expect.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Huh.. it is completely different compared to what I was thinking.
If I see it correctly, it will help to replace missing HW stats for some
MACs like ASIX. But it will fail to help diagnose MAC-PHY connections
issues like, wrong RGMII configurations or other kind of damages on the
PCB. Right?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC net-next 2/2] net: ethtool: add phy(dev) specific stats over netlink

[Andrew Lunn]

> +/* Additional PHY statistics, not defined by IEEE */
> +struct ethtool_phy_stats {
> +	/* Basic packet / byte counters are meant for PHY drivers */
> +	u64 rx_packets;
> +	u64 rx_bytes;
> +	u64 rx_error; /* TODO: we need to define here whether packet
> +		       * counted here is also counted as rx_packets,
> +		       * and whether it's passed to the MAC with some
> +		       * error indication or MAC never sees it.
> +		       */
> +	u64 tx_packets;
> +	u64 tx_bytes;
> +	u64 tx_error; /* TODO: same as for rx */
> +};

I'm not sure these are actually useful.

adin.c:
        { "total_frames_checked_count",         0x940A, 0x940B }, /* hi + lo */
        { "length_error_frames_count",          0x940C },
        { "alignment_error_frames_count",       0x940D },
        { "symbol_error_count",                 0x940E },
        { "oversized_frames_count",             0x940F },
        { "undersized_frames_count",            0x9410 },
        { "odd_nibble_frames_count",            0x9411 },
        { "odd_preamble_packet_count",          0x9412 },
        { "dribble_bits_frames_count",          0x9413 },
        { "false_carrier_events_count",         0x9414 },

bcm-phy-lib.c:
        { "phy_receive_errors", -1, MII_BRCM_CORE_BASE12, 0, 16 },
        { "phy_serdes_ber_errors", -1, MII_BRCM_CORE_BASE13, 8, 8 },
        { "phy_false_carrier_sense_errors", -1, MII_BRCM_CORE_BASE13, 0, 8 },
        { "phy_local_rcvr_nok", -1, MII_BRCM_CORE_BASE14, 8, 8 },
        { "phy_remote_rcv_nok", -1, MII_BRCM_CORE_BASE14, 0, 8 },
        { "phy_lpi_count", MDIO_MMD_AN, BRCM_CL45VEN_EEE_LPI_CNT, 0, 16 },

icplus.c:
        { "phy_crc_errors", 1 },
        { "phy_symbol_errors", 11, },

marvell.c:
        { "phy_receive_errors_copper", 0, 21, 16},
        { "phy_idle_errors", 0, 10, 8 },
        { "phy_receive_errors_fiber", 1, 21, 16},

micrel.c:
        { "phy_receive_errors", 21, 16},
        { "phy_idle_errors", 10, 8 },

nxp-c45-tja11xx.c:
        { "phy_link_status_drop_cnt",
        { "phy_link_availability_drop_cnt",
        { "phy_link_loss_cnt",
        { "phy_link_failure_cnt",
        { "phy_symbol_error_cnt",
        { "rx_preamble_count",
        { "tx_preamble_count",
        { "rx_ipg_length",
        { "tx_ipg_length",
        { "phy_symbol_error_cnt_ext",
        { "tx_frames_xtd",
        { "tx_frames",
        { "rx_frames_xtd",
        { "rx_frames",
        { "tx_lost_frames_xtd",
        { "tx_lost_frames",
        { "rx_lost_frames_xtd",
        { "rx_lost_frames",

smsc.c:
        { "phy_symbol_errors", 26, 16},

802.3 does not define in PHY statistics, the same as it does not
define any MAC statistics. As a result it is a wild west, vendors
doing whatever they want.

The exception is the Open Alliance, which have defined a number of
different standards defining statistics which Automotive vendors can
optionally follow.

https://opensig.org/automotive-ethernet-specifications/

These could be defined as either one or three groups, with the
expectation more will be added later.

	Andrew

Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Jakub Kicinski]

On Thu, 29 Aug 2024 20:10:00 +0200 Oleksij Rempel wrote:
> On Thu, Aug 29, 2024 at 10:43:41AM -0700, Jakub Kicinski wrote:
> > Feed the existing IEEE PHY counter struct (which currently
> > only has one entry) and link stats into the PHY driver.
> > The MAC driver can override the value if it somehow has a better
> > idea of PHY stats. Since the stats are "undefined" at input
> > the drivers can't += the values, so we should be safe from
> > double-counting.
> > 
> > Vladimir, I don't understand MM but doesn't MM share the PHY?
> > Ocelot seems to aggregate which I did not expect.
> > 
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>  
> 
> Huh.. it is completely different compared to what I was thinking.
> If I see it correctly, it will help to replace missing HW stats for some
> MACs like ASIX. But it will fail to help diagnose MAC-PHY connections
> issues like, wrong RGMII configurations or other kind of damages on the
> PCB. Right?

This is just a pre-req for the next patch, to let phy drivers report
the (very few) stats we have already defined for integrated NIC drivers.
What statistics we choose to add later is up to us, this is just
groundwork.

BTW the series is primarily to allow you to report the packet / error
and OA stats in a structured way, it's not related directly by the
discussion on T1L troubleshooting.

Re: [RFC net-next 2/2] net: ethtool: add phy(dev) specific stats over netlink

[Jakub Kicinski]

On Thu, 29 Aug 2024 20:47:04 +0200 Andrew Lunn wrote:
> > +/* Additional PHY statistics, not defined by IEEE */
> > +struct ethtool_phy_stats {
> > +	/* Basic packet / byte counters are meant for PHY drivers */
> > +	u64 rx_packets;
> > +	u64 rx_bytes;
> > +	u64 rx_error; /* TODO: we need to define here whether packet
> > +		       * counted here is also counted as rx_packets,
> > +		       * and whether it's passed to the MAC with some
> > +		       * error indication or MAC never sees it.
> > +		       */
> > +	u64 tx_packets;
> > +	u64 tx_bytes;
> > +	u64 tx_error; /* TODO: same as for rx */
> > +};  
> 
> I'm not sure these are actually useful.
> 
> adin.c:
>         { "total_frames_checked_count",         0x940A, 0x940B }, /* hi + lo */
>         { "length_error_frames_count",          0x940C },
>         { "alignment_error_frames_count",       0x940D },
>         { "symbol_error_count",                 0x940E },

This one's IEEE, from patch 1.

>         { "oversized_frames_count",             0x940F },
>         { "undersized_frames_count",            0x9410 },

bunch of IEEE stats, but from the MAC space :S

>         { "odd_nibble_frames_count",            0x9411 },
>         { "odd_preamble_packet_count",          0x9412 },
>         { "dribble_bits_frames_count",          0x9413 },
>         { "false_carrier_events_count",         0x9414 },

These may be interesting?

> bcm-phy-lib.c:
>         { "phy_receive_errors", -1, MII_BRCM_CORE_BASE12, 0, 16 },

matching rx errors

>         { "phy_serdes_ber_errors", -1, MII_BRCM_CORE_BASE13, 8, 8 },

Dunno what BER errors is 🤔️

>         { "phy_false_carrier_sense_errors", -1, MII_BRCM_CORE_BASE13, 0, 8 },

false carrier like in adin.c

>         { "phy_local_rcvr_nok", -1, MII_BRCM_CORE_BASE14, 8, 8 },
>         { "phy_remote_rcv_nok", -1, MII_BRCM_CORE_BASE14, 0, 8 },

nok is not okay ? ... 🤷️

>         { "phy_lpi_count", MDIO_MMD_AN, BRCM_CL45VEN_EEE_LPI_CNT, 0, 16 },

Sounds standard :)

> icplus.c:
>         { "phy_crc_errors", 1 },
>         { "phy_symbol_errors", 11, },

Why the PHY wants to check CRC I can only guess, but the other one 
is in patch 1.

... I think going thru them right now is not super useful.

> 802.3 does not define in PHY statistics, the same as it does not
> define any MAC statistics. As a result it is a wild west, vendors
> doing whatever they want.

I think IEEE does define the MIB including some counters. It just does 
a shit job and defines very few.

> The exception is the Open Alliance, which have defined a number of
> different standards defining statistics which Automotive vendors can
> optionally follow.
> 
> https://opensig.org/automotive-ethernet-specifications/
> 
> These could be defined as either one or three groups, with the
> expectation more will be added later.

SG.

To be clear, I'm adding the pkt/error counters because Oleksij was
trying to add defines for these into linux/phy.h

https://lore.kernel.org/all/20240822115939.1387015-3-o.rempel@pengutronix.de/

You acked that which I read as an agreement that there's sufficient
commonality :)

I threw in the byte counters, perhaps unnecessarily. We can drop those
for sure.

Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Maxime Chevallier]

Hello Jakub,

On Thu, 29 Aug 2024 10:43:41 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> Feed the existing IEEE PHY counter struct (which currently
> only has one entry) and link stats into the PHY driver.
> The MAC driver can override the value if it somehow has a better
> idea of PHY stats. Since the stats are "undefined" at input
> the drivers can't += the values, so we should be safe from
> double-counting.
> 
> Vladimir, I don't understand MM but doesn't MM share the PHY?
> Ocelot seems to aggregate which I did not expect.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

[...]

> +static void
> +ethtool_get_phydev_stats(struct net_device *dev,
> +			 struct linkstate_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;

This would be a very nice spot to use the new
ethnl_req_get_phydev(), if there are multiple PHYs on that device.
Being able to access the stats individually can help
troubleshoot HW issues.

[...]

> +static void
> +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;

Here as well, but that's trickier, as the MAC can override the PHY
stats, but it doesn't know which PHY were getting the stats from.

Maybe we could make so that when we pass a phy_index in the netlink
command, we don't allow the mac to override the phy stats ? Or better,
don't allow the mac to override these stats and report the MAC-reported
PHY stats alongside the PHY-reported stats ?

> +
> +	if (!phydev || !phydev->drv || !phydev->drv->get_phy_stats)
> +		return;
> +
> +	mutex_lock(&phydev->lock);
> +	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
> +	mutex_unlock(&phydev->lock);
> +}
> +
>  static int stats_prepare_data(const struct ethnl_req_info *req_base,
>  			      struct ethnl_reply_data *reply_base,
>  			      const struct genl_info *info)
> @@ -145,6 +160,10 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
>  	data->ctrl_stats.src = src;
>  	data->rmon_stats.src = src;
>  
> +	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
> +	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
> +		ethtool_get_phydev_stats(dev, data);

I might be missing something, but I think
ETHTOOL_MAC_STATS_SRC_AGGREGATE is a MM-specific flag and I don't really
see how this applies to getting the PHY stats. I don't know much about
MM though so I could be missing the point.

I'm all in for getting the PHY stats from netlink though :)

Thanks,

Maxime

Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Jakub Kicinski]

On Fri, 30 Aug 2024 10:16:30 +0200 Maxime Chevallier wrote:
> > +static void
> > +ethtool_get_phydev_stats(struct net_device *dev,
> > +			 struct linkstate_reply_data *data)
> > +{
> > +	struct phy_device *phydev = dev->phydev;  
> 
> This would be a very nice spot to use the new
> ethnl_req_get_phydev(), if there are multiple PHYs on that device.
> Being able to access the stats individually can help
> troubleshoot HW issues.
> 
> > +static void
> > +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> > +{
> > +	struct phy_device *phydev = dev->phydev;  
> 
> Here as well, but that's trickier, as the MAC can override the PHY
> stats, but it doesn't know which PHY were getting the stats from.
> 
> Maybe we could make so that when we pass a phy_index in the netlink
> command, we don't allow the mac to override the phy stats ? Or better,
> don't allow the mac to override these stats and report the MAC-reported
> PHY stats alongside the PHY-reported stats ?

Maybe we can flip the order of querying regardless of the PHY that's
targeted? Always query the MAC first and then the PHY, so that the
PHY can override. Presumably the PHY can always provide more detailed
stats than the MAC (IOW if it does provide stats they will be more
accurate).

> > +	if (!phydev || !phydev->drv || !phydev->drv->get_phy_stats)
> > +		return;
> > +
> > +	mutex_lock(&phydev->lock);
> > +	phydev->drv->get_phy_stats(phydev, &data->phy_stats);
> > +	mutex_unlock(&phydev->lock);
> > +}
> > +
> >  static int stats_prepare_data(const struct ethnl_req_info *req_base,
> >  			      struct ethnl_reply_data *reply_base,
> >  			      const struct genl_info *info)
> > @@ -145,6 +160,10 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
> >  	data->ctrl_stats.src = src;
> >  	data->rmon_stats.src = src;
> >  
> > +	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
> > +	    src == ETHTOOL_MAC_STATS_SRC_AGGREGATE)
> > +		ethtool_get_phydev_stats(dev, data);  
> 
> I might be missing something, but I think
> ETHTOOL_MAC_STATS_SRC_AGGREGATE is a MM-specific flag and I don't really
> see how this applies to getting the PHY stats. I don't know much about
> MM though so I could be missing the point.

We need expert insights from Vladimir on that part :)

> I'm all in for getting the PHY stats from netlink though :)

Great! FWIW I'm not sure what the status of these patches is.
I don't know much about PHYs.
I wrote them to help Oleksij out with the "netlink parts".
I'm not sure how much I convinced Andrew about the applicability.
And I don't know if this is enough for Oleksij to take it forward.
So in the unlikely even that you have spare cycles and a PHY you can
test with, do not hesitate to take these, rework, reset the author 
and repost... :)

Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Vladimir Oltean]

On Thu, Aug 29, 2024 at 10:43:41AM -0700, Jakub Kicinski wrote:
> Vladimir, I don't understand MM

MAC Merge / Frame Preemption in a nutshell:

- Frame is express if, after the preamble, it has a "normal" SFD of 0xD5

- Frame is preemptible if, after the preamble, it has an SFD of 0x07,
  0x19, 0xE6, 0x4C, 0x7F, 0xB3, 0x61, 0x52, 0x9E or 0x2A

express MAC handles express frames
preemptible MAC handles preemptible frames

ETHTOOL_MAC_STATS_SRC_EMAC counts express frames
ETHTOOL_MAC_STATS_SRC_PMAC counts preemptible frames
ETHTOOL_MAC_STATS_SRC_AGGREGATE counts both - also works when you don't know

Now you know as much as I do.

> but doesn't MM share the PHY?

It does, yes. There is a single set of MII lines, and distinction
between the express and preemptible MAC is done as described above.

I wouldn't expect the PHY to be aware of MAC Merge / Frame Preemption,
and thus, this component would normally not pay attention to the SFD of
the frames it's counting. The entire feature actually depends on the PHY
being unaware of the SFD, because they don't make PHYs "for" frame preemption.

Although, imaginably, just like we have PHYs which emit PAUSE frames,
and that technically means they have a MAC embedded inside, it would not
be impossible to twist standards such that the PHY handles FPE/MM.
This is only in the realm of theory, AFAIU, and I'm not suggesting we
should model the UAPI based on pure theory.

> Ocelot seems to aggregate which I did not expect.

Ocelot aggregates stats when the request is to aggregate them
(explicit ETHTOOL_MAC_STATS_SRC_AGGREGATE, and also default, for
comparability/ compatibility with unaware drivers). Otherwise it
reports them individually.

Also, the stats it reports into phy_stats->SymbolErrorDuringCarrier are
MAC stats. They count the number of frames received by the MAC with
RX_ER being asserted on the MII interface. So these could be counted by
either the MAC, or the PHY. The MAC is MM-aware, the PHY is probably not.

Though if I follow the thread, I'm not sure if this is exactly useful to
Oleksij, who would like to report an entirely different set of counters.

I never got the impression that the ETHTOOL_STATS_ETH_PHY structured
netlink counters were for NICs with embedded non-phylib PHYs. If they
are - sorry. I thought it was about those MAC counters which are
collected at the interface with the PHY.

Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Maxime Chevallier]

Hi Jakub,

On Fri, 30 Aug 2024 11:30:47 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri, 30 Aug 2024 10:16:30 +0200 Maxime Chevallier wrote:
> > > +static void
> > > +ethtool_get_phydev_stats(struct net_device *dev,
> > > +			 struct linkstate_reply_data *data)
> > > +{
> > > +	struct phy_device *phydev = dev->phydev;    
> > 
> > This would be a very nice spot to use the new
> > ethnl_req_get_phydev(), if there are multiple PHYs on that device.
> > Being able to access the stats individually can help
> > troubleshoot HW issues.
> >   
> > > +static void
> > > +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> > > +{
> > > +	struct phy_device *phydev = dev->phydev;    
> > 
> > Here as well, but that's trickier, as the MAC can override the PHY
> > stats, but it doesn't know which PHY were getting the stats from.
> > 
> > Maybe we could make so that when we pass a phy_index in the netlink
> > command, we don't allow the mac to override the phy stats ? Or better,
> > don't allow the mac to override these stats and report the MAC-reported
> > PHY stats alongside the PHY-reported stats ?  
> 
> Maybe we can flip the order of querying regardless of the PHY that's
> targeted? Always query the MAC first and then the PHY, so that the
> PHY can override. Presumably the PHY can always provide more detailed
> stats than the MAC (IOW if it does provide stats they will be more
> accurate).

I think that could work indeed, good point.

[...]

> > I'm all in for getting the PHY stats from netlink though :)  
> 
> Great! FWIW I'm not sure what the status of these patches is.
> I don't know much about PHYs.
> I wrote them to help Oleksij out with the "netlink parts".
> I'm not sure how much I convinced Andrew about the applicability.
> And I don't know if this is enough for Oleksij to take it forward.
> So in the unlikely even that you have spare cycles and a PHY you can
> test with, do not hesitate to take these, rework, reset the author 
> and repost... :)

I do have some hardware I can test this on, and I'm starting to get
familiar with netlink :) I can give it a try, however I can't guarantee
as of right now that I'll be able to send anything before net-next
closes. I'll ping here if I start moving forward with this :)

Thanks,

Maxime

Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Oleksij Rempel]

Hi all,

On Wed, Sep 04, 2024 at 09:20:24AM +0200, Maxime Chevallier wrote:
> Hi Jakub,
> 
> On Fri, 30 Aug 2024 11:30:47 -0700
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Fri, 30 Aug 2024 10:16:30 +0200 Maxime Chevallier wrote:
> > > > +static void
> > > > +ethtool_get_phydev_stats(struct net_device *dev,
> > > > +			 struct linkstate_reply_data *data)
> > > > +{
> > > > +	struct phy_device *phydev = dev->phydev;    
> > > 
> > > This would be a very nice spot to use the new
> > > ethnl_req_get_phydev(), if there are multiple PHYs on that device.
> > > Being able to access the stats individually can help
> > > troubleshoot HW issues.
> > >   
> > > > +static void
> > > > +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> > > > +{
> > > > +	struct phy_device *phydev = dev->phydev;    
> > > 
> > > Here as well, but that's trickier, as the MAC can override the PHY
> > > stats, but it doesn't know which PHY were getting the stats from.
> > > 
> > > Maybe we could make so that when we pass a phy_index in the netlink
> > > command, we don't allow the mac to override the phy stats ? Or better,
> > > don't allow the mac to override these stats and report the MAC-reported
> > > PHY stats alongside the PHY-reported stats ?  
> > 
> > Maybe we can flip the order of querying regardless of the PHY that's
> > targeted? Always query the MAC first and then the PHY, so that the
> > PHY can override. Presumably the PHY can always provide more detailed
> > stats than the MAC (IOW if it does provide stats they will be more
> > accurate).
> 
> I think that could work indeed, good point.
> 
> [...]
> 
> > > I'm all in for getting the PHY stats from netlink though :)  
> > 
> > Great! FWIW I'm not sure what the status of these patches is.
> > I don't know much about PHYs.
> > I wrote them to help Oleksij out with the "netlink parts".
> > I'm not sure how much I convinced Andrew about the applicability.
> > And I don't know if this is enough for Oleksij to take it forward.
> > So in the unlikely even that you have spare cycles and a PHY you can
> > test with, do not hesitate to take these, rework, reset the author 
> > and repost... :)
> 
> I do have some hardware I can test this on, and I'm starting to get
> familiar with netlink :) I can give it a try, however I can't guarantee
> as of right now that I'll be able to send anything before net-next
> closes. I'll ping here if I start moving forward with this :)

I reserved some time for this work. I hope it will be this year.

In case some one else will start working on it, please let me know :)

Regard,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC net-next 1/2] net: ethtool: plumb PHY stats to PHY drivers

[Maxime Chevallier]

On Wed, 4 Sep 2024 10:05:10 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi all,
> 
> On Wed, Sep 04, 2024 at 09:20:24AM +0200, Maxime Chevallier wrote:
> > Hi Jakub,
> > 
> > On Fri, 30 Aug 2024 11:30:47 -0700
> > Jakub Kicinski <kuba@kernel.org> wrote:
> >   
> > > On Fri, 30 Aug 2024 10:16:30 +0200 Maxime Chevallier wrote:  
> > > > > +static void
> > > > > +ethtool_get_phydev_stats(struct net_device *dev,
> > > > > +			 struct linkstate_reply_data *data)
> > > > > +{
> > > > > +	struct phy_device *phydev = dev->phydev;      
> > > > 
> > > > This would be a very nice spot to use the new
> > > > ethnl_req_get_phydev(), if there are multiple PHYs on that device.
> > > > Being able to access the stats individually can help
> > > > troubleshoot HW issues.
> > > >     
> > > > > +static void
> > > > > +ethtool_get_phydev_stats(struct net_device *dev, struct stats_reply_data *data)
> > > > > +{
> > > > > +	struct phy_device *phydev = dev->phydev;      
> > > > 
> > > > Here as well, but that's trickier, as the MAC can override the PHY
> > > > stats, but it doesn't know which PHY were getting the stats from.
> > > > 
> > > > Maybe we could make so that when we pass a phy_index in the netlink
> > > > command, we don't allow the mac to override the phy stats ? Or better,
> > > > don't allow the mac to override these stats and report the MAC-reported
> > > > PHY stats alongside the PHY-reported stats ?    
> > > 
> > > Maybe we can flip the order of querying regardless of the PHY that's
> > > targeted? Always query the MAC first and then the PHY, so that the
> > > PHY can override. Presumably the PHY can always provide more detailed
> > > stats than the MAC (IOW if it does provide stats they will be more
> > > accurate).  
> > 
> > I think that could work indeed, good point.
> > 
> > [...]
> >   
> > > > I'm all in for getting the PHY stats from netlink though :)    
> > > 
> > > Great! FWIW I'm not sure what the status of these patches is.
> > > I don't know much about PHYs.
> > > I wrote them to help Oleksij out with the "netlink parts".
> > > I'm not sure how much I convinced Andrew about the applicability.
> > > And I don't know if this is enough for Oleksij to take it forward.
> > > So in the unlikely even that you have spare cycles and a PHY you can
> > > test with, do not hesitate to take these, rework, reset the author 
> > > and repost... :)  
> > 
> > I do have some hardware I can test this on, and I'm starting to get
> > familiar with netlink :) I can give it a try, however I can't guarantee
> > as of right now that I'll be able to send anything before net-next
> > closes. I'll ping here if I start moving forward with this :)  
> 
> I reserved some time for this work. I hope it will be this year.
> 
> In case some one else will start working on it, please let me know :)

Ah perfect then :) Let me know if you ever need some extra testing, I
can run this on the hardware I have on hand if that can help.

Best regards,

Maxime