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

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