[PATCH net-next 0/5] net: dpaa2-mac: export standard statistics

[PATCH net-next 0/5] net: dpaa2-mac: export standard statistics

[Ioana Ciornei]

This patch set adds support for standard ethtool statistics - rmon,
eth-ctrl, eth-mac and pause - to dpaa2-mac and its users dpaa2-eth and
dpaa2-switch.

The first patch extends the firmware APIs related to MAC counters and
adds dpmac_get_statistics() which can be used to retrieve multiple counter
values through a single firmware call.

This new API is put in use in the second patch by gathering all
previously exported ethtool statistics through a single MC firmware
call. In this patch we are also adding the setup and cleanup
infrastructure which will be also used for the standard ethtool
counters.

The third patch adds the actual suppord for rmon, eth-ctrl, eth-mac and
pause statistics in dpaa2-mac and its users.

The last two patches extend the selftest infrastructure with a new
script - ethtool_std_stats.sh - which validates the standard counters
(except rmon which is already covered). This new selftest was run on
LX2160ARDB with both dpaa2-eth and dpaa2-switch interfaces as well as
LS1028ARDB with ENETC and Felix switch interfaces.

Ioana Ciornei (5):
  net: dpaa2-mac: extend APIs related to statistics
  net: dpaa2-mac: retrieve MAC statistics in one firmware command
  net: dpaa2-mac: export standard statistics
  selftests: forwarding: extend ethtool_std_stats_get with pause
    statistics
  selftests: drivers: hw: add tests for the ethtool standard counters

 .../ethernet/freescale/dpaa2/dpaa2-ethtool.c  |  59 ++-
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 381 ++++++++++++++++--
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.h  |  23 +-
 .../freescale/dpaa2/dpaa2-switch-ethtool.c    |  45 ++-
 .../net/ethernet/freescale/dpaa2/dpmac-cmd.h  |  11 +-
 drivers/net/ethernet/freescale/dpaa2/dpmac.c  |  30 +-
 drivers/net/ethernet/freescale/dpaa2/dpmac.h  |  86 +++-
 .../testing/selftests/drivers/net/hw/Makefile |   1 +
 .../drivers/net/hw/ethtool_std_stats.sh       | 192 +++++++++
 tools/testing/selftests/net/forwarding/lib.sh |   8 +-
 10 files changed, 792 insertions(+), 44 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh

-- 
2.25.1

[PATCH net-next 1/5] net: dpaa2-mac: extend APIs related to statistics

[Ioana Ciornei]

Extend the dpmac_counter_id enum with the newly added counters which can
be interrogated through the MC firmware. Also add the
dpmac_get_statistics() API which can be used to retrieve multiple MAC
counters through a single firmware command.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 .../net/ethernet/freescale/dpaa2/dpmac-cmd.h  | 11 ++-
 drivers/net/ethernet/freescale/dpaa2/dpmac.c  | 30 ++++++-
 drivers/net/ethernet/freescale/dpaa2/dpmac.h  | 86 ++++++++++++++++++-
 3 files changed, 123 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h b/drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
index e9ac2ecef3be..a864a99a0f75 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
 /* Copyright 2013-2016 Freescale Semiconductor Inc.
- * Copyright 2019 NXP
+ * Copyright 2019, 2024-2026 NXP
  */
 #ifndef _FSL_DPMAC_CMD_H
 #define _FSL_DPMAC_CMD_H
@@ -28,6 +28,8 @@
 
 #define DPMAC_CMDID_SET_PROTOCOL	DPMAC_CMD(0x0c7)
 
+#define DPMAC_CMDID_GET_STATISTICS	DPMAC_CMD(0x0c8)
+
 /* Macros for accessing command fields smaller than 1byte */
 #define DPMAC_MASK(field)        \
 	GENMASK(DPMAC_##field##_SHIFT + DPMAC_##field##_SIZE - 1, \
@@ -82,4 +84,11 @@ struct dpmac_rsp_get_api_version {
 struct dpmac_cmd_set_protocol {
 	u8 eth_if;
 };
+
+struct dpmac_cmd_get_statistics {
+	__le64 iova_cnt;
+	__le64 iova_values;
+	__le32 num_cnt;
+};
+
 #endif /* _FSL_DPMAC_CMD_H */
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpmac.c b/drivers/net/ethernet/freescale/dpaa2/dpmac.c
index f440a4c3b70c..8957665c05fc 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpmac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpmac.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
 /* Copyright 2013-2016 Freescale Semiconductor Inc.
- * Copyright 2019 NXP
+ * Copyright 2019, 2024-2026 NXP
  */
 #include <linux/fsl/mc.h>
 #include "dpmac.h"
@@ -235,3 +235,31 @@ int dpmac_set_protocol(struct fsl_mc_io *mc_io, u32 cmd_flags, u16 token,
 
 	return mc_send_command(mc_io, &cmd);
 }
+
+/**
+ * dpmac_get_statistics() - Get MAC statistics
+ * @mc_io:       Pointer to opaque I/O object
+ * @cmd_flags:   Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:       Token of DPMAC object
+ * @iova_cnt:    IOVA containing the requested MAC counters formatted as an
+ *               array of __le32 representing the dpmac_counter_id.
+ * @iova_values: IOVA containing the values for all the requested counters
+ *               formatted as an array of __le64.
+ * @num_cnt:     Number of counters requested
+ *
+ * Return:       '0' on Success; Error code otherwise.
+ */
+int dpmac_get_statistics(struct fsl_mc_io *mc_io, u32 cmd_flags, u16 token,
+			 u64 iova_cnt, u64 iova_values, u32 num_cnt)
+{
+	struct dpmac_cmd_get_statistics *cmd_params;
+	struct fsl_mc_command cmd = { 0 };
+
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_GET_STATISTICS, cmd_flags, token);
+	cmd_params = (struct dpmac_cmd_get_statistics *)cmd.params;
+	cmd_params->iova_cnt = cpu_to_le64(iova_cnt);
+	cmd_params->iova_values = cpu_to_le64(iova_values);
+	cmd_params->num_cnt = cpu_to_le32(num_cnt);
+
+	return mc_send_command(mc_io, &cmd);
+}
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpmac.h b/drivers/net/ethernet/freescale/dpaa2/dpmac.h
index 17488819ef68..857b40a5b339 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpmac.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpmac.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
 /* Copyright 2013-2016 Freescale Semiconductor Inc.
- * Copyright 2019 NXP
+ * Copyright 2019, 2024-2026 NXP
  */
 #ifndef __FSL_DPMAC_H
 #define __FSL_DPMAC_H
@@ -170,6 +170,50 @@ int dpmac_set_link_state(struct fsl_mc_io *mc_io,
  *			      pause frames.
  * @DPMAC_CNT_EGR_GOOD_FRAME: counts frames transmitted without error, including
  *			      pause frames.
+ * @DPMAC_CNT_EGR_FRAME_64: counts transmitted 64-bytes frames, good or bad.
+ * @DPMAC_CNT_EGR_FRAME_127: counts transmitted 65 to 127-bytes frames, good or bad.
+ * @DPMAC_CNT_EGR_FRAME_255: counts transmitted 128 to 255-bytes frames, good or bad.
+ * @DPMAC_CNT_EGR_FRAME_511: counts transmitted 256 to 511-bytes frames, good or bad.
+ * @DPMAC_CNT_EGR_FRAME_1023: counts transmitted 512 to 1023-bytes frames, good or bad.
+ * @DPMAC_CNT_EGR_FRAME_1518: counts transmitted 1024 to 1518-bytes frames, good or bad.
+ * @DPMAC_CNT_EGR_FRAME_1519_MAX: counts transmitted 1519-bytes frames and
+ * larger (up to max frame length specified), good or bad.
+ * @DPMAC_CNT_ING_ALL_BYTE: counts bytes received in both good and bad packets
+ * @DPMAC_CNT_ING_FCS_ERR: counts frames received with a CRC-32 error but the
+ * frame is otherwise of correct length
+ * @DPMAC_CNT_ING_VLAN_FRAME: counts the received VLAN tagged frames which are valid.
+ * @DPMAC_CNT_ING_UNDERSIZED: counts received frames which were less than 64
+ * bytes long and with a good CRC.
+ * @DPMAC_CNT_ING_CONTROL_FRAME: counts received control frames (type 0x8808)
+ * but not pause frames.
+ * @DPMAC_CNT_ING_FRAME_DISCARD_NOT_TRUNC: counts the fully dropped frames (not
+ * truncated) due to internal errors of the MAC client. Occurs when a received
+ * FIFO overflows.
+ * @DPMAC_CNT_EGR_ALL_BYTE: counts transmitted bytes in both good and bad
+ * packets.
+ * @DPMAC_CNT_EGR_FCS_ERR: counts trasmitted frames with a CRC-32 error except
+ * for underflows.
+ * @DPMAC_CNT_EGR_VLAN_FRAME: counts the transmitted VLAN tagged frames which
+ * are valid.
+ * @DPMAC_CNT_EGR_ALL_FRAME: counts all trasmitted frames, good or bad.
+ * @DPMAC_CNT_EGR_CONTROL_FRAME: counts transmitted control frames (type
+ * 0x8808) but not pause frames.
+ * @DPMAC_CNT_ING_PFC0: number of PFC frames received for class 0.
+ * @DPMAC_CNT_ING_PFC1: number of PFC frames received for class 1.
+ * @DPMAC_CNT_ING_PFC2: number of PFC frames received for class 2.
+ * @DPMAC_CNT_ING_PFC3: number of PFC frames received for class 3.
+ * @DPMAC_CNT_ING_PFC4: number of PFC frames received for class 4.
+ * @DPMAC_CNT_ING_PFC5: number of PFC frames received for class 5.
+ * @DPMAC_CNT_ING_PFC6: number of PFC frames received for class 6.
+ * @DPMAC_CNT_ING_PFC7: number of PFC frames received for class 7.
+ * @DPMAC_CNT_EGR_PFC0: number of PFC frames transmitted for class 0.
+ * @DPMAC_CNT_EGR_PFC1: number of PFC frames transmitted for class 1.
+ * @DPMAC_CNT_EGR_PFC2: number of PFC frames transmitted for class 2.
+ * @DPMAC_CNT_EGR_PFC3: number of PFC frames transmitted for class 3.
+ * @DPMAC_CNT_EGR_PFC4: number of PFC frames transmitted for class 4.
+ * @DPMAC_CNT_EGR_PFC5: number of PFC frames transmitted for class 5.
+ * @DPMAC_CNT_EGR_PFC6: number of PFC frames transmitted for class 6.
+ * @DPMAC_CNT_EGR_PFC7: number of PFC frames transmitted for class 7.
  */
 enum dpmac_counter_id {
 	DPMAC_CNT_ING_FRAME_64,
@@ -199,7 +243,41 @@ enum dpmac_counter_id {
 	DPMAC_CNT_EGR_UCAST_FRAME,
 	DPMAC_CNT_EGR_ERR_FRAME,
 	DPMAC_CNT_ING_GOOD_FRAME,
-	DPMAC_CNT_EGR_GOOD_FRAME
+	DPMAC_CNT_EGR_GOOD_FRAME,
+	DPMAC_CNT_EGR_FRAME_64,
+	DPMAC_CNT_EGR_FRAME_127,
+	DPMAC_CNT_EGR_FRAME_255,
+	DPMAC_CNT_EGR_FRAME_511,
+	DPMAC_CNT_EGR_FRAME_1023,
+	DPMAC_CNT_EGR_FRAME_1518,
+	DPMAC_CNT_EGR_FRAME_1519_MAX,
+	DPMAC_CNT_ING_ALL_BYTE,
+	DPMAC_CNT_ING_FCS_ERR,
+	DPMAC_CNT_ING_VLAN_FRAME,
+	DPMAC_CNT_ING_UNDERSIZED,
+	DPMAC_CNT_ING_CONTROL_FRAME,
+	DPMAC_CNT_ING_FRAME_DISCARD_NOT_TRUNC,
+	DPMAC_CNT_EGR_ALL_BYTE,
+	DPMAC_CNT_EGR_FCS_ERR,
+	DPMAC_CNT_EGR_VLAN_FRAME,
+	DPMAC_CNT_EGR_ALL_FRAME,
+	DPMAC_CNT_EGR_CONTROL_FRAME,
+	DPMAC_CNT_ING_PFC0,
+	DPMAC_CNT_ING_PFC1,
+	DPMAC_CNT_ING_PFC2,
+	DPMAC_CNT_ING_PFC3,
+	DPMAC_CNT_ING_PFC4,
+	DPMAC_CNT_ING_PFC5,
+	DPMAC_CNT_ING_PFC6,
+	DPMAC_CNT_ING_PFC7,
+	DPMAC_CNT_EGR_PFC0,
+	DPMAC_CNT_EGR_PFC1,
+	DPMAC_CNT_EGR_PFC2,
+	DPMAC_CNT_EGR_PFC3,
+	DPMAC_CNT_EGR_PFC4,
+	DPMAC_CNT_EGR_PFC5,
+	DPMAC_CNT_EGR_PFC6,
+	DPMAC_CNT_EGR_PFC7,
 };
 
 int dpmac_get_counter(struct fsl_mc_io *mc_io, u32 cmd_flags, u16 token,
@@ -210,4 +288,8 @@ int dpmac_get_api_version(struct fsl_mc_io *mc_io, u32 cmd_flags,
 
 int dpmac_set_protocol(struct fsl_mc_io *mc_io, u32 cmd_flags, u16 token,
 		       enum dpmac_eth_if protocol);
+
+int dpmac_get_statistics(struct fsl_mc_io *mc_io, u32 cmd_flags, u16 token,
+			 u64 iova_cnt, u64 iova_values, u32 num_cnt);
+
 #endif /* __FSL_DPMAC_H */
-- 
2.25.1

[PATCH net-next 2/5] net: dpaa2-mac: retrieve MAC statistics in one firmware command

[Ioana Ciornei]

The latest MC firmware version added a new command to retrieve all DPMAC
counters in a single firmware call. Use this new command, when possible,
in dpaa2-mac as well.

In order to use the dpmac_get_statistics() API, two DMA memory areas are
used: one to transmit what counters the driver is requesting and one to
receive the values of those counters. These memory areas are allocated
and DMA mapped at probe time so that we don't waste time at runtime.

And since we are planning to add rmon, eth-ctrl and other standard
statistics using the same infrastructure, make the setup and cleanup
processes as generic as possibile through the dpaa2_mac_setup_stats()
and dpaa2_mac_clear_stats() functions.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 195 ++++++++++++++----
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.h  |  10 +-
 2 files changed, 166 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 422ce13a7c94..63dc597dbd7c 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
-/* Copyright 2019 NXP */
+/* Copyright 2019, 2024-2026 NXP */
 
 #include <linux/acpi.h>
 #include <linux/pcs-lynx.h>
@@ -15,7 +15,121 @@
 #define DPMAC_PROTOCOL_CHANGE_VER_MAJOR		4
 #define DPMAC_PROTOCOL_CHANGE_VER_MINOR		8
 
+#define DPMAC_STATS_BUNDLE_VER_MAJOR		4
+#define DPMAC_STATS_BUNDLE_VER_MINOR		10
+
 #define DPAA2_MAC_FEATURE_PROTOCOL_CHANGE	BIT(0)
+#define DPAA2_MAC_FEATURE_STATS_BUNDLE		BIT(1)
+
+struct dpmac_counter {
+	enum dpmac_counter_id id;
+	const char *name;
+};
+
+#define DPMAC_UNSTRUCTURED_COUNTER(counter_id, counter_name)	\
+	{							\
+		.id = counter_id,				\
+		.name = counter_name,				\
+	}
+
+static const struct dpmac_counter dpaa2_mac_ethtool_stats[] = {
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ALL_FRAME,  "[mac] rx all frames"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_GOOD_FRAME,  "[mac] rx frames ok"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ERR_FRAME, "[mac] rx frame errors"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_DISCARD, "[mac] rx frame discards"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_UCAST_FRAME, "[mac] rx u-cast"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_BCAST_FRAME, "[mac] rx b-cast"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_MCAST_FRAME, "[mac] rx m-cast"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_64, "[mac] rx 64 bytes"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_127, "[mac] rx 65-127 bytes"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_255, "[mac] rx 128-255 bytes"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_511, "[mac] rx 256-511 bytes"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1023, "[mac] rx 512-1023 bytes"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1518, "[mac] rx 1024-1518 bytes"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1519_MAX, "[mac] rx 1519-max bytes"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAG, "[mac] rx frags"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_JABBER, "[mac] rx jabber"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ALIGN_ERR, "[mac] rx align errors"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_OVERSIZED, "[mac] rx oversized"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_VALID_PAUSE_FRAME, "[mac] rx pause"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_BYTE, "[mac] rx bytes"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_GOOD_FRAME, "[mac] tx frames ok"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_UCAST_FRAME, "[mac] tx u-cast"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_MCAST_FRAME, "[mac] tx m-cast"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_BCAST_FRAME, "[mac] tx b-cast"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_ERR_FRAME, "[mac] tx frame errors"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_UNDERSIZED, "[mac] tx undersized"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_VALID_PAUSE_FRAME, "[mac] tx b-pause"),
+	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_BYTE, "[mac] tx bytes"),
+};
+
+#define DPAA2_MAC_NUM_ETHTOOL_STATS	ARRAY_SIZE(dpaa2_mac_ethtool_stats)
+
+static void dpaa2_mac_setup_stats(struct dpaa2_mac *mac, struct dpaa2_mac_stats *stats,
+				  size_t num_stats, const struct dpmac_counter *counters)
+{
+	struct device *dev = mac->net_dev->dev.parent;
+	u32 *cnt_idx;
+
+	stats->idx_dma_mem = kcalloc(num_stats, sizeof(u32), GFP_KERNEL);
+	if (!stats->idx_dma_mem)
+		goto out;
+
+	stats->values_dma_mem = kcalloc(num_stats, sizeof(u64), GFP_KERNEL);
+	if (!stats->values_dma_mem)
+		goto err_alloc_values;
+
+	cnt_idx = stats->idx_dma_mem;
+	for (size_t i = 0; i < num_stats; i++)
+		*cnt_idx++ = cpu_to_le32((u32)(counters[i].id));
+
+	stats->idx_iova = dma_map_single(dev, stats->idx_dma_mem,
+					 num_stats * sizeof(u32),
+					 DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, stats->idx_iova))
+		goto err_dma_map_idx;
+
+	stats->values_iova = dma_map_single(dev, stats->values_dma_mem,
+					    num_stats * sizeof(u64),
+					    DMA_FROM_DEVICE);
+	if (dma_mapping_error(dev, stats->values_iova))
+		goto err_dma_map_values;
+
+	return;
+
+err_dma_map_values:
+	dma_unmap_single(dev, stats->idx_iova, num_stats * sizeof(u32),
+			 DMA_TO_DEVICE);
+err_dma_map_idx:
+	kfree(stats->values_dma_mem);
+err_alloc_values:
+	kfree(stats->idx_dma_mem);
+out:
+	stats->idx_dma_mem = NULL;
+	stats->values_dma_mem = NULL;
+}
+
+static void dpaa2_mac_clear_stats(struct dpaa2_mac *mac, struct dpaa2_mac_stats *stats,
+				  size_t num_stats)
+{
+	struct device *dev = mac->net_dev->dev.parent;
+
+	if (stats->idx_dma_mem) {
+		dma_unmap_single(dev, stats->idx_iova,
+				 num_stats * sizeof(u32),
+				 DMA_TO_DEVICE);
+		kfree(stats->idx_dma_mem);
+		stats->idx_dma_mem = NULL;
+	}
+
+	if (stats->values_dma_mem) {
+		dma_unmap_single(dev, stats->values_iova,
+				 num_stats * sizeof(u64),
+				 DMA_FROM_DEVICE);
+		kfree(stats->values_dma_mem);
+		stats->values_dma_mem = NULL;
+	}
+}
 
 static int dpaa2_mac_cmp_ver(struct dpaa2_mac *mac,
 			     u16 ver_major, u16 ver_minor)
@@ -32,6 +146,10 @@ static void dpaa2_mac_detect_features(struct dpaa2_mac *mac)
 	if (dpaa2_mac_cmp_ver(mac, DPMAC_PROTOCOL_CHANGE_VER_MAJOR,
 			      DPMAC_PROTOCOL_CHANGE_VER_MINOR) >= 0)
 		mac->features |= DPAA2_MAC_FEATURE_PROTOCOL_CHANGE;
+
+	if (dpaa2_mac_cmp_ver(mac, DPMAC_STATS_BUNDLE_VER_MAJOR,
+			      DPMAC_STATS_BUNDLE_VER_MINOR) >= 0)
+		mac->features |= DPAA2_MAC_FEATURE_STATS_BUNDLE;
 }
 
 static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
@@ -504,6 +622,10 @@ int dpaa2_mac_open(struct dpaa2_mac *mac)
 	mac->fw_node = fw_node;
 	net_dev->dev.of_node = to_of_node(mac->fw_node);
 
+	if (mac->features & DPAA2_MAC_FEATURE_STATS_BUNDLE)
+		dpaa2_mac_setup_stats(mac, &mac->ethtool_stats,
+				      DPAA2_MAC_NUM_ETHTOOL_STATS, dpaa2_mac_ethtool_stats);
+
 	return 0;
 
 err_close_dpmac:
@@ -515,64 +637,61 @@ void dpaa2_mac_close(struct dpaa2_mac *mac)
 {
 	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
 
+	if (mac->features & DPAA2_MAC_FEATURE_STATS_BUNDLE)
+		dpaa2_mac_clear_stats(mac, &mac->ethtool_stats, DPAA2_MAC_NUM_ETHTOOL_STATS);
+
 	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
 	if (mac->fw_node)
 		fwnode_handle_put(mac->fw_node);
 }
 
-static char dpaa2_mac_ethtool_stats[][ETH_GSTRING_LEN] = {
-	[DPMAC_CNT_ING_ALL_FRAME]		= "[mac] rx all frames",
-	[DPMAC_CNT_ING_GOOD_FRAME]		= "[mac] rx frames ok",
-	[DPMAC_CNT_ING_ERR_FRAME]		= "[mac] rx frame errors",
-	[DPMAC_CNT_ING_FRAME_DISCARD]		= "[mac] rx frame discards",
-	[DPMAC_CNT_ING_UCAST_FRAME]		= "[mac] rx u-cast",
-	[DPMAC_CNT_ING_BCAST_FRAME]		= "[mac] rx b-cast",
-	[DPMAC_CNT_ING_MCAST_FRAME]		= "[mac] rx m-cast",
-	[DPMAC_CNT_ING_FRAME_64]		= "[mac] rx 64 bytes",
-	[DPMAC_CNT_ING_FRAME_127]		= "[mac] rx 65-127 bytes",
-	[DPMAC_CNT_ING_FRAME_255]		= "[mac] rx 128-255 bytes",
-	[DPMAC_CNT_ING_FRAME_511]		= "[mac] rx 256-511 bytes",
-	[DPMAC_CNT_ING_FRAME_1023]		= "[mac] rx 512-1023 bytes",
-	[DPMAC_CNT_ING_FRAME_1518]		= "[mac] rx 1024-1518 bytes",
-	[DPMAC_CNT_ING_FRAME_1519_MAX]		= "[mac] rx 1519-max bytes",
-	[DPMAC_CNT_ING_FRAG]			= "[mac] rx frags",
-	[DPMAC_CNT_ING_JABBER]			= "[mac] rx jabber",
-	[DPMAC_CNT_ING_ALIGN_ERR]		= "[mac] rx align errors",
-	[DPMAC_CNT_ING_OVERSIZED]		= "[mac] rx oversized",
-	[DPMAC_CNT_ING_VALID_PAUSE_FRAME]	= "[mac] rx pause",
-	[DPMAC_CNT_ING_BYTE]			= "[mac] rx bytes",
-	[DPMAC_CNT_EGR_GOOD_FRAME]		= "[mac] tx frames ok",
-	[DPMAC_CNT_EGR_UCAST_FRAME]		= "[mac] tx u-cast",
-	[DPMAC_CNT_EGR_MCAST_FRAME]		= "[mac] tx m-cast",
-	[DPMAC_CNT_EGR_BCAST_FRAME]		= "[mac] tx b-cast",
-	[DPMAC_CNT_EGR_ERR_FRAME]		= "[mac] tx frame errors",
-	[DPMAC_CNT_EGR_UNDERSIZED]		= "[mac] tx undersized",
-	[DPMAC_CNT_EGR_VALID_PAUSE_FRAME]	= "[mac] tx b-pause",
-	[DPMAC_CNT_EGR_BYTE]			= "[mac] tx bytes",
-};
-
-#define DPAA2_MAC_NUM_STATS	ARRAY_SIZE(dpaa2_mac_ethtool_stats)
-
 int dpaa2_mac_get_sset_count(void)
 {
-	return DPAA2_MAC_NUM_STATS;
+	return DPAA2_MAC_NUM_ETHTOOL_STATS;
 }
 
 void dpaa2_mac_get_strings(u8 **data)
 {
 	int i;
 
-	for (i = 0; i < DPAA2_MAC_NUM_STATS; i++)
-		ethtool_puts(data, dpaa2_mac_ethtool_stats[i]);
+	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++)
+		ethtool_puts(data, dpaa2_mac_ethtool_stats[i].name);
 }
 
 void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data)
 {
+	struct device *dev = mac->net_dev->dev.parent;
 	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
+	u64 *cnt_values;
 	int i, err;
 	u64 value;
 
-	for (i = 0; i < DPAA2_MAC_NUM_STATS; i++) {
+	if (!(mac->features & DPAA2_MAC_FEATURE_STATS_BUNDLE))
+		goto fallback;
+
+	if (!mac->ethtool_stats.idx_dma_mem || !mac->ethtool_stats.values_dma_mem)
+		goto fallback;
+
+	err = dpmac_get_statistics(mac->mc_io, 0, dpmac_dev->mc_handle,
+				   mac->ethtool_stats.idx_iova, mac->ethtool_stats.values_iova,
+				   DPAA2_MAC_NUM_ETHTOOL_STATS);
+	if (err)
+		goto fallback;
+
+	dma_sync_single_for_cpu(dev, mac->ethtool_stats.values_iova,
+				DPAA2_MAC_NUM_ETHTOOL_STATS * sizeof(u64),
+				DMA_FROM_DEVICE);
+
+	cnt_values = mac->ethtool_stats.values_dma_mem;
+	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++)
+		*(data + i) = le64_to_cpu(*cnt_values++);
+
+	return;
+
+fallback:
+
+	/* Fallback and retrieve each counter one by one */
+	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++) {
 		err = dpmac_get_counter(mac->mc_io, 0, dpmac_dev->mc_handle,
 					i, &value);
 		if (err) {
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
index 53f8d106d11e..386286209606 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
-/* Copyright 2019 NXP */
+/* Copyright 2019, 2024-2026 NXP */
 #ifndef DPAA2_MAC_H
 #define DPAA2_MAC_H
 
@@ -11,6 +11,12 @@
 #include "dpmac.h"
 #include "dpmac-cmd.h"
 
+struct dpaa2_mac_stats {
+	u32 *idx_dma_mem;
+	u64 *values_dma_mem;
+	dma_addr_t idx_iova, values_iova;
+};
+
 struct dpaa2_mac {
 	struct fsl_mc_device *mc_dev;
 	struct dpmac_link_state state;
@@ -28,6 +34,8 @@ struct dpaa2_mac {
 	struct fwnode_handle *fw_node;
 
 	struct phy *serdes_phy;
+
+	struct dpaa2_mac_stats ethtool_stats;
 };
 
 static inline bool dpaa2_mac_is_type_phy(struct dpaa2_mac *mac)
-- 
2.25.1

Re: [net-next,2/5] net: dpaa2-mac: retrieve MAC statistics in one firmware command

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

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dpaa2-mac: retrieve MAC statistics in one firmware command

The latest MC firmware version added a new command to retrieve all DPMAC
counters in a single firmware call. Use this new command, when possible,
in dpaa2-mac as well. In order to use the dpmac_get_statistics() API,
two DMA memory areas are used: one to transmit what counters the driver
is requesting and one to receive the values of those counters. These
memory areas are allocated and DMA mapped at probe time so that we don't
waste time at runtime.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> index 422ce13a7c94..63dc597dbd7c 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c

[ ... ]

> @@ -15,7 +15,121 @@
>  #define DPMAC_PROTOCOL_CHANGE_VER_MAJOR		4
>  #define DPMAC_PROTOCOL_CHANGE_VER_MINOR		8
>
> +#define DPMAC_STATS_BUNDLE_VER_MAJOR		4
> +#define DPMAC_STATS_BUNDLE_VER_MINOR		10
> +
>  #define DPAA2_MAC_FEATURE_PROTOCOL_CHANGE	BIT(0)
> +#define DPAA2_MAC_FEATURE_STATS_BUNDLE		BIT(1)
> +
> +struct dpmac_counter {
> +	enum dpmac_counter_id id;
> +	const char *name;
> +};
> +
> +#define DPMAC_UNSTRUCTURED_COUNTER(counter_id, counter_name)	\
> +	{							\
> +		.id = counter_id,				\
> +		.name = counter_name,				\
> +	}
> +
> +static const struct dpmac_counter dpaa2_mac_ethtool_stats[] = {
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ALL_FRAME,  "[mac] rx all frames"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_GOOD_FRAME,  "[mac] rx frames ok"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ERR_FRAME, "[mac] rx frame errors"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_DISCARD, "[mac] rx frame discards"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_UCAST_FRAME, "[mac] rx u-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_BCAST_FRAME, "[mac] rx b-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_MCAST_FRAME, "[mac] rx m-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_64, "[mac] rx 64 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_127, "[mac] rx 65-127 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_255, "[mac] rx 128-255 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_511, "[mac] rx 256-511 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1023, "[mac] rx 512-1023 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1518, "[mac] rx 1024-1518 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1519_MAX, "[mac] rx 1519-max bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAG, "[mac] rx frags"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_JABBER, "[mac] rx jabber"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ALIGN_ERR, "[mac] rx align errors"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_OVERSIZED, "[mac] rx oversized"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_VALID_PAUSE_FRAME, "[mac] rx pause"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_BYTE, "[mac] rx bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_GOOD_FRAME, "[mac] tx frames ok"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_UCAST_FRAME, "[mac] tx u-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_MCAST_FRAME, "[mac] tx m-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_BCAST_FRAME, "[mac] tx b-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_ERR_FRAME, "[mac] tx frame errors"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_UNDERSIZED, "[mac] tx undersized"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_VALID_PAUSE_FRAME, "[mac] tx b-pause"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_BYTE, "[mac] tx bytes"),
> +};
> +
> +#define DPAA2_MAC_NUM_ETHTOOL_STATS	ARRAY_SIZE(dpaa2_mac_ethtool_stats)

[ ... ]

>  void dpaa2_mac_close(struct dpaa2_mac *mac)
>  {
>  	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
>
> +	if (mac->features & DPAA2_MAC_FEATURE_STATS_BUNDLE)
> +		dpaa2_mac_clear_stats(mac, &mac->ethtool_stats, DPAA2_MAC_NUM_ETHTOOL_STATS);
> +
>  	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
>  	if (mac->fw_node)
>  		fwnode_handle_put(mac->fw_node);
>  }
>
> -static char dpaa2_mac_ethtool_stats[][ETH_GSTRING_LEN] = {
> -	[DPMAC_CNT_ING_ALL_FRAME]		= "[mac] rx all frames",
> -	[DPMAC_CNT_ING_GOOD_FRAME]		= "[mac] rx frames ok",
> -	[DPMAC_CNT_ING_ERR_FRAME]		= "[mac] rx frame errors",
> -	[DPMAC_CNT_ING_FRAME_DISCARD]		= "[mac] rx frame discards",
> -	[DPMAC_CNT_ING_UCAST_FRAME]		= "[mac] rx u-cast",
> -	[DPMAC_CNT_ING_BCAST_FRAME]		= "[mac] rx b-cast",
> -	[DPMAC_CNT_ING_MCAST_FRAME]		= "[mac] rx m-cast",
> -	[DPMAC_CNT_ING_FRAME_64]		= "[mac] rx 64 bytes",
> -	[DPMAC_CNT_ING_FRAME_127]		= "[mac] rx 65-127 bytes",
> -	[DPMAC_CNT_ING_FRAME_255]		= "[mac] rx 128-255 bytes",
> -	[DPMAC_CNT_ING_FRAME_511]		= "[mac] rx 256-511 bytes",
> -	[DPMAC_CNT_ING_FRAME_1023]		= "[mac] rx 512-1023 bytes",
> -	[DPMAC_CNT_ING_FRAME_1518]		= "[mac] rx 1024-1518 bytes",
> -	[DPMAC_CNT_ING_FRAME_1519_MAX]		= "[mac] rx 1519-max bytes",
> -	[DPMAC_CNT_ING_FRAG]			= "[mac] rx frags",
> -	[DPMAC_CNT_ING_JABBER]			= "[mac] rx jabber",
> -	[DPMAC_CNT_ING_ALIGN_ERR]		= "[mac] rx align errors",
> -	[DPMAC_CNT_ING_OVERSIZED]		= "[mac] rx oversized",
> -	[DPMAC_CNT_ING_VALID_PAUSE_FRAME]	= "[mac] rx pause",
> -	[DPMAC_CNT_ING_BYTE]			= "[mac] rx bytes",
> -	[DPMAC_CNT_EGR_GOOD_FRAME]		= "[mac] tx frames ok",
> -	[DPMAC_CNT_EGR_UCAST_FRAME]		= "[mac] tx u-cast",
> -	[DPMAC_CNT_EGR_MCAST_FRAME]		= "[mac] tx m-cast",
> -	[DPMAC_CNT_EGR_BCAST_FRAME]		= "[mac] tx b-cast",
> -	[DPMAC_CNT_EGR_ERR_FRAME]		= "[mac] tx frame errors",
> -	[DPMAC_CNT_EGR_UNDERSIZED]		= "[mac] tx undersized",
> -	[DPMAC_CNT_EGR_VALID_PAUSE_FRAME]	= "[mac] tx b-pause",
> -	[DPMAC_CNT_EGR_BYTE]			= "[mac] tx bytes",
> -};
> -
> -#define DPAA2_MAC_NUM_STATS	ARRAY_SIZE(dpaa2_mac_ethtool_stats)
> -
>  int dpaa2_mac_get_sset_count(void)
>  {
> -	return DPAA2_MAC_NUM_STATS;
> +	return DPAA2_MAC_NUM_ETHTOOL_STATS;
>  }
>
>  void dpaa2_mac_get_strings(u8 **data)
>  {
>  	int i;
>
> -	for (i = 0; i < DPAA2_MAC_NUM_STATS; i++)
> -		ethtool_puts(data, dpaa2_mac_ethtool_stats[i]);
> +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++)
> +		ethtool_puts(data, dpaa2_mac_ethtool_stats[i].name);
>  }
>
>  void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data)
>  {
> +	struct device *dev = mac->net_dev->dev.parent;
>  	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
> +	u64 *cnt_values;
>  	int i, err;
>  	u64 value;
>
> -	for (i = 0; i < DPAA2_MAC_NUM_STATS; i++) {
> +	if (!(mac->features & DPAA2_MAC_FEATURE_STATS_BUNDLE))
> +		goto fallback;
> +
> +	if (!mac->ethtool_stats.idx_dma_mem || !mac->ethtool_stats.values_dma_mem)
> +		goto fallback;
> +
> +	err = dpmac_get_statistics(mac->mc_io, 0, dpmac_dev->mc_handle,
> +				   mac->ethtool_stats.idx_iova, mac->ethtool_stats.values_iova,
> +				   DPAA2_MAC_NUM_ETHTOOL_STATS);
> +	if (err)
> +		goto fallback;
> +
> +	dma_sync_single_for_cpu(dev, mac->ethtool_stats.values_iova,
> +				DPAA2_MAC_NUM_ETHTOOL_STATS * sizeof(u64),
> +				DMA_FROM_DEVICE);
> +
> +	cnt_values = mac->ethtool_stats.values_dma_mem;
> +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++)
> +		*(data + i) = le64_to_cpu(*cnt_values++);
> +
> +	return;
> +
> +fallback:
> +
> +	/* Fallback and retrieve each counter one by one */
> +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++) {
>  		err = dpmac_get_counter(mac->mc_io, 0, dpmac_dev->mc_handle,
>  					i, &value);
                                        ^

Does the fallback path need to pass dpaa2_mac_ethtool_stats[i].id instead
of i here?

The old code used a sparse array indexed by enum dpmac_counter_id values:

    static char dpaa2_mac_ethtool_stats[][ETH_GSTRING_LEN] = {
        [DPMAC_CNT_ING_ALL_FRAME] = "[mac] rx all frames",
        ...
    };

where the loop index i directly corresponded to the enum value passed to
dpmac_get_counter(). The new code uses a compact array where array index 0
contains DPMAC_CNT_ING_ALL_FRAME (enum value 18), but the fallback code
still passes i (sequential 0, 1, 2...) instead of the stored enum value.

When i=0, the code requests counter 0 (DPMAC_CNT_ING_FRAME_64) but the
name is "[mac] rx all frames" (DPMAC_CNT_ING_ALL_FRAME). This would cause
all statistics to report wrong values for their labels.

>  		if (err) {
>  			netdev_err(mac->net_dev,
>  				   "dpmac_get_counter(%d) failed\n", i);

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

Re: [net-next,2/5] net: dpaa2-mac: retrieve MAC statistics in one firmware command

[Ioana Ciornei]

On Thu, Feb 26, 2026 at 06:26:13PM -0800, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: dpaa2-mac: retrieve MAC statistics in one firmware command
> 
> The latest MC firmware version added a new command to retrieve all DPMAC
> counters in a single firmware call. Use this new command, when possible,
> in dpaa2-mac as well. In order to use the dpmac_get_statistics() API,
> two DMA memory areas are used: one to transmit what counters the driver
> is requesting and one to receive the values of those counters. These
> memory areas are allocated and DMA mapped at probe time so that we don't
> waste time at runtime.
> 
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > index 422ce13a7c94..63dc597dbd7c 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> 

(...)

> > +fallback:
> > +
> > +	/* Fallback and retrieve each counter one by one */
> > +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++) {
> >  		err = dpmac_get_counter(mac->mc_io, 0, dpmac_dev->mc_handle,
> >  					i, &value);
>                                         ^
> 
> Does the fallback path need to pass dpaa2_mac_ethtool_stats[i].id instead
> of i here?
> 
> The old code used a sparse array indexed by enum dpmac_counter_id values:
> 
>     static char dpaa2_mac_ethtool_stats[][ETH_GSTRING_LEN] = {
>         [DPMAC_CNT_ING_ALL_FRAME] = "[mac] rx all frames",
>         ...
>     };
> 
> where the loop index i directly corresponded to the enum value passed to
> dpmac_get_counter(). The new code uses a compact array where array index 0
> contains DPMAC_CNT_ING_ALL_FRAME (enum value 18), but the fallback code
> still passes i (sequential 0, 1, 2...) instead of the stored enum value.
> 
> When i=0, the code requests counter 0 (DPMAC_CNT_ING_FRAME_64) but the
> name is "[mac] rx all frames" (DPMAC_CNT_ING_ALL_FRAME). This would cause
> all statistics to report wrong values for their labels.

Yes, that is indeed true. Thanks for catching it!

Ioana

Re: [PATCH net-next 2/5] net: dpaa2-mac: retrieve MAC statistics in one firmware command

[Simon Horman]

On Wed, Feb 25, 2026 at 05:06:45PM +0200, Ioana Ciornei wrote:

...

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c

...

> +static void dpaa2_mac_setup_stats(struct dpaa2_mac *mac, struct dpaa2_mac_stats *stats,
> +				  size_t num_stats, const struct dpmac_counter *counters)
> +{
> +	struct device *dev = mac->net_dev->dev.parent;
> +	u32 *cnt_idx;

Hi Ioana,

The type of cnt_idx is u32.

> +
> +	stats->idx_dma_mem = kcalloc(num_stats, sizeof(u32), GFP_KERNEL);
> +	if (!stats->idx_dma_mem)
> +		goto out;
> +
> +	stats->values_dma_mem = kcalloc(num_stats, sizeof(u64), GFP_KERNEL);
> +	if (!stats->values_dma_mem)
> +		goto err_alloc_values;
> +
> +	cnt_idx = stats->idx_dma_mem;

As is that of idx_dma_mem. So the types match here.

> +	for (size_t i = 0; i < num_stats; i++)
> +		*cnt_idx++ = cpu_to_le32((u32)(counters[i].id));

But here __le32 values are assigned to elements of cnt_idx.

I think that the type of both cnt_idx and stats->idx_dma_mem
should probably be __le32 * rather than u32 *.

Flagged by Sparse v0.6.5-rc1.


...

>  void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data)
>  {
> +	struct device *dev = mac->net_dev->dev.parent;
>  	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
> +	u64 *cnt_values;
>  	int i, err;
>  	u64 value;

...

> +	cnt_values = mac->ethtool_stats.values_dma_mem;
> +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++)
> +		*(data + i) = le64_to_cpu(*cnt_values++);

Likewise, I think the type of both cnt_values and
mac->ethtool_stats.values_dma_mem should be __le64 8 rather than u64 *.

And there is a similar problem in patch 3/5 centering on the use of
le64_to_cpu() in dpaa2_mac_transfer_stats().

Also flagged by Sparse.

...

Re: [PATCH net-next 2/5] net: dpaa2-mac: retrieve MAC statistics in one firmware command

[Ioana Ciornei]

On Sun, Mar 01, 2026 at 04:09:25PM +0000, Simon Horman wrote:
> On Wed, Feb 25, 2026 at 05:06:45PM +0200, Ioana Ciornei wrote:
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> 
> ...
> 
> > +static void dpaa2_mac_setup_stats(struct dpaa2_mac *mac, struct dpaa2_mac_stats *stats,
> > +				  size_t num_stats, const struct dpmac_counter *counters)
> > +{
> > +	struct device *dev = mac->net_dev->dev.parent;
> > +	u32 *cnt_idx;
> 
> Hi Ioana,
> 
> The type of cnt_idx is u32.
> 
> > +
> > +	stats->idx_dma_mem = kcalloc(num_stats, sizeof(u32), GFP_KERNEL);
> > +	if (!stats->idx_dma_mem)
> > +		goto out;
> > +
> > +	stats->values_dma_mem = kcalloc(num_stats, sizeof(u64), GFP_KERNEL);
> > +	if (!stats->values_dma_mem)
> > +		goto err_alloc_values;
> > +
> > +	cnt_idx = stats->idx_dma_mem;
> 
> As is that of idx_dma_mem. So the types match here.
> 
> > +	for (size_t i = 0; i < num_stats; i++)
> > +		*cnt_idx++ = cpu_to_le32((u32)(counters[i].id));
> 
> But here __le32 values are assigned to elements of cnt_idx.
> 
> I think that the type of both cnt_idx and stats->idx_dma_mem
> should probably be __le32 * rather than u32 *.
> 
> Flagged by Sparse v0.6.5-rc1.

Yes, I missed this. Thanks for catching it!
> 
> 
> ...
> 
> >  void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data)
> >  {
> > +	struct device *dev = mac->net_dev->dev.parent;
> >  	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
> > +	u64 *cnt_values;
> >  	int i, err;
> >  	u64 value;
> 
> ...
> 
> > +	cnt_values = mac->ethtool_stats.values_dma_mem;
> > +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++)
> > +		*(data + i) = le64_to_cpu(*cnt_values++);
> 
> Likewise, I think the type of both cnt_values and
> mac->ethtool_stats.values_dma_mem should be __le64 8 rather than u64 *.
> 
> And there is a similar problem in patch 3/5 centering on the use of
> le64_to_cpu() in dpaa2_mac_transfer_stats().
> 
> Also flagged by Sparse.
> 

Yes, this is valid too. Will fix.

Ioana

[PATCH net-next 3/5] net: dpaa2-mac: export standard statistics

[Ioana Ciornei]

Take advantage of all the newly added MAC counters available through the
MC API and export them through the standard statistics structures -
rmon, eth-ctrl, eth-mac and pause.

A new version based feature is added into dpaa2-mac -
DPAA2_MAC_FEATURE_STANDARD_STATS - and based on the two memory zones
needed for gathering the MAC counters are setup for each statistics
group.

The dpmac_counter structure is extended with a new field - size_t offset
- which is being used to instruct the dpaa2_mac_transfer_stats()
function where exactly to store a counter value inside the standard
statistics structure.

The newly added support is used both in the dpaa2-eth driver as well as
the dpaa2-switch one.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 .../ethernet/freescale/dpaa2/dpaa2-ethtool.c  |  59 +++++-
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 192 ++++++++++++++++++
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.h  |  13 ++
 .../freescale/dpaa2/dpaa2-switch-ethtool.c    |  45 +++-
 4 files changed, 307 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
index baab4f1c908d..6f3b15a404fb 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
 /* Copyright 2014-2016 Freescale Semiconductor Inc.
- * Copyright 2016-2022 NXP
+ * Copyright 2016-2022, 2024-2026 NXP
  */
 
 #include <linux/net_tstamp.h>
@@ -938,6 +938,59 @@ static void dpaa2_eth_get_channels(struct net_device *net_dev,
 				   channels->other_count;
 }
 
+static void dpaa2_eth_get_rmon_stats(struct net_device *net_dev,
+				     struct ethtool_rmon_stats *rmon_stats,
+				     const struct ethtool_rmon_hist_range **ranges)
+{
+	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+
+	mutex_lock(&priv->mac_lock);
+
+	if (dpaa2_eth_has_mac(priv))
+		dpaa2_mac_get_rmon_stats(priv->mac, rmon_stats, ranges);
+
+	mutex_unlock(&priv->mac_lock);
+}
+
+static void dpaa2_eth_get_pause_stats(struct net_device *net_dev,
+				      struct ethtool_pause_stats *pause_stats)
+{
+	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+
+	mutex_lock(&priv->mac_lock);
+
+	if (dpaa2_eth_has_mac(priv))
+		dpaa2_mac_get_pause_stats(priv->mac, pause_stats);
+
+	mutex_unlock(&priv->mac_lock);
+}
+
+static void dpaa2_eth_get_ctrl_stats(struct net_device *net_dev,
+				     struct ethtool_eth_ctrl_stats *ctrl_stats)
+{
+	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+
+	mutex_lock(&priv->mac_lock);
+
+	if (dpaa2_eth_has_mac(priv))
+		dpaa2_mac_get_ctrl_stats(priv->mac, ctrl_stats);
+
+	mutex_unlock(&priv->mac_lock);
+}
+
+static void dpaa2_eth_get_eth_mac_stats(struct net_device *net_dev,
+					struct ethtool_eth_mac_stats *eth_mac_stats)
+{
+	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+
+	mutex_lock(&priv->mac_lock);
+
+	if (dpaa2_eth_has_mac(priv))
+		dpaa2_mac_get_eth_mac_stats(priv->mac, eth_mac_stats);
+
+	mutex_unlock(&priv->mac_lock);
+}
+
 const struct ethtool_ops dpaa2_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
@@ -962,4 +1015,8 @@ const struct ethtool_ops dpaa2_ethtool_ops = {
 	.get_coalesce = dpaa2_eth_get_coalesce,
 	.set_coalesce = dpaa2_eth_set_coalesce,
 	.get_channels = dpaa2_eth_get_channels,
+	.get_rmon_stats = dpaa2_eth_get_rmon_stats,
+	.get_pause_stats = dpaa2_eth_get_pause_stats,
+	.get_eth_ctrl_stats = dpaa2_eth_get_ctrl_stats,
+	.get_eth_mac_stats = dpaa2_eth_get_eth_mac_stats,
 };
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 63dc597dbd7c..abdac6ce7bc4 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -18,20 +18,43 @@
 #define DPMAC_STATS_BUNDLE_VER_MAJOR		4
 #define DPMAC_STATS_BUNDLE_VER_MINOR		10
 
+#define DPMAC_STANDARD_STATS_VER_MAJOR		4
+#define DPMAC_STANDARD_STATS_VER_MINOR		11
+
 #define DPAA2_MAC_FEATURE_PROTOCOL_CHANGE	BIT(0)
 #define DPAA2_MAC_FEATURE_STATS_BUNDLE		BIT(1)
+#define DPAA2_MAC_FEATURE_STANDARD_STATS	BIT(2)
 
 struct dpmac_counter {
 	enum dpmac_counter_id id;
+	size_t offset;
 	const char *name;
 };
 
+#define DPMAC_COUNTER(counter_id, struct_name, struct_offset)	\
+	{							\
+		.id = counter_id,				\
+		.offset = offsetof(struct_name, struct_offset),	\
+	}
+
 #define DPMAC_UNSTRUCTURED_COUNTER(counter_id, counter_name)	\
 	{							\
 		.id = counter_id,				\
 		.name = counter_name,				\
 	}
 
+#define DPMAC_RMON_COUNTER(counter_id, struct_offset)		\
+	DPMAC_COUNTER(counter_id, struct ethtool_rmon_stats, struct_offset)
+
+#define DPMAC_PAUSE_COUNTER(counter_id, struct_offset)		\
+	DPMAC_COUNTER(counter_id, struct ethtool_pause_stats, struct_offset)
+
+#define DPMAC_CTRL_COUNTER(counter_id, struct_offset)		\
+	DPMAC_COUNTER(counter_id, struct ethtool_eth_ctrl_stats, struct_offset)
+
+#define DPMAC_MAC_COUNTER(counter_id, struct_offset)		\
+	DPMAC_COUNTER(counter_id, struct ethtool_eth_mac_stats, struct_offset)
+
 static const struct dpmac_counter dpaa2_mac_ethtool_stats[] = {
 	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ALL_FRAME,  "[mac] rx all frames"),
 	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_GOOD_FRAME,  "[mac] rx frames ok"),
@@ -65,6 +88,60 @@ static const struct dpmac_counter dpaa2_mac_ethtool_stats[] = {
 
 #define DPAA2_MAC_NUM_ETHTOOL_STATS	ARRAY_SIZE(dpaa2_mac_ethtool_stats)
 
+static const struct dpmac_counter dpaa2_mac_rmon_stats[] = {
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_FRAME_64, hist[0]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_FRAME_127, hist[1]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_FRAME_255, hist[2]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_FRAME_511, hist[3]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_FRAME_1023, hist[4]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_FRAME_1518, hist[5]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_FRAME_1519_MAX, hist[6]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_EGR_FRAME_64, hist_tx[0]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_EGR_FRAME_127, hist_tx[1]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_EGR_FRAME_255, hist_tx[2]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_EGR_FRAME_511, hist_tx[3]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_EGR_FRAME_1023, hist_tx[4]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_EGR_FRAME_1518, hist_tx[5]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_EGR_FRAME_1519_MAX, hist_tx[6]),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_EGR_UNDERSIZED, undersize_pkts),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_OVERSIZED, oversize_pkts),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_FRAG, fragments),
+	DPMAC_RMON_COUNTER(DPMAC_CNT_ING_JABBER, jabbers),
+};
+
+#define DPAA2_MAC_NUM_RMON_STATS	ARRAY_SIZE(dpaa2_mac_rmon_stats)
+
+static const struct dpmac_counter dpaa2_mac_pause_stats[] = {
+	DPMAC_PAUSE_COUNTER(DPMAC_CNT_ING_VALID_PAUSE_FRAME, rx_pause_frames),
+	DPMAC_PAUSE_COUNTER(DPMAC_CNT_EGR_VALID_PAUSE_FRAME, tx_pause_frames),
+};
+
+#define DPAA2_MAC_NUM_PAUSE_STATS	ARRAY_SIZE(dpaa2_mac_pause_stats)
+
+static const struct dpmac_counter dpaa2_mac_eth_ctrl_stats[] = {
+	DPMAC_CTRL_COUNTER(DPMAC_CNT_ING_CONTROL_FRAME, MACControlFramesReceived),
+	DPMAC_CTRL_COUNTER(DPMAC_CNT_EGR_CONTROL_FRAME, MACControlFramesTransmitted),
+};
+
+#define DPAA2_MAC_NUM_ETH_CTRL_STATS	ARRAY_SIZE(dpaa2_mac_eth_ctrl_stats)
+
+static const struct dpmac_counter dpaa2_mac_eth_mac_stats[] = {
+	DPMAC_MAC_COUNTER(DPMAC_CNT_EGR_GOOD_FRAME, FramesTransmittedOK),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_ING_GOOD_FRAME, FramesReceivedOK),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_ING_FCS_ERR, FrameCheckSequenceErrors),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_ING_ALIGN_ERR, AlignmentErrors),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_EGR_ALL_BYTE, OctetsTransmittedOK),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_EGR_ERR_FRAME, FramesLostDueToIntMACXmitError),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_ING_ALL_BYTE, OctetsReceivedOK),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_ING_FRAME_DISCARD_NOT_TRUNC, FramesLostDueToIntMACRcvError),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_EGR_MCAST_FRAME, MulticastFramesXmittedOK),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_EGR_BCAST_FRAME, BroadcastFramesXmittedOK),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_ING_MCAST_FRAME, MulticastFramesReceivedOK),
+	DPMAC_MAC_COUNTER(DPMAC_CNT_ING_BCAST_FRAME, BroadcastFramesReceivedOK),
+};
+
+#define DPAA2_MAC_NUM_ETH_MAC_STATS		ARRAY_SIZE(dpaa2_mac_eth_mac_stats)
+
 static void dpaa2_mac_setup_stats(struct dpaa2_mac *mac, struct dpaa2_mac_stats *stats,
 				  size_t num_stats, const struct dpmac_counter *counters)
 {
@@ -150,6 +227,10 @@ static void dpaa2_mac_detect_features(struct dpaa2_mac *mac)
 	if (dpaa2_mac_cmp_ver(mac, DPMAC_STATS_BUNDLE_VER_MAJOR,
 			      DPMAC_STATS_BUNDLE_VER_MINOR) >= 0)
 		mac->features |= DPAA2_MAC_FEATURE_STATS_BUNDLE;
+
+	if (dpaa2_mac_cmp_ver(mac, DPMAC_STANDARD_STATS_VER_MAJOR,
+			      DPMAC_STANDARD_STATS_VER_MINOR) >= 0)
+		mac->features |= DPAA2_MAC_FEATURE_STANDARD_STATS;
 }
 
 static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
@@ -626,6 +707,20 @@ int dpaa2_mac_open(struct dpaa2_mac *mac)
 		dpaa2_mac_setup_stats(mac, &mac->ethtool_stats,
 				      DPAA2_MAC_NUM_ETHTOOL_STATS, dpaa2_mac_ethtool_stats);
 
+	if (mac->features & DPAA2_MAC_FEATURE_STANDARD_STATS) {
+		dpaa2_mac_setup_stats(mac, &mac->rmon_stats,
+				      DPAA2_MAC_NUM_RMON_STATS, dpaa2_mac_rmon_stats);
+
+		dpaa2_mac_setup_stats(mac, &mac->pause_stats,
+				      DPAA2_MAC_NUM_PAUSE_STATS, dpaa2_mac_pause_stats);
+
+		dpaa2_mac_setup_stats(mac, &mac->eth_ctrl_stats,
+				      DPAA2_MAC_NUM_ETH_CTRL_STATS, dpaa2_mac_eth_ctrl_stats);
+
+		dpaa2_mac_setup_stats(mac, &mac->eth_mac_stats,
+				      DPAA2_MAC_NUM_ETH_MAC_STATS, dpaa2_mac_eth_mac_stats);
+	}
+
 	return 0;
 
 err_close_dpmac:
@@ -640,11 +735,108 @@ void dpaa2_mac_close(struct dpaa2_mac *mac)
 	if (mac->features & DPAA2_MAC_FEATURE_STATS_BUNDLE)
 		dpaa2_mac_clear_stats(mac, &mac->ethtool_stats, DPAA2_MAC_NUM_ETHTOOL_STATS);
 
+	if (mac->features & DPAA2_MAC_FEATURE_STANDARD_STATS) {
+		dpaa2_mac_clear_stats(mac, &mac->rmon_stats, DPAA2_MAC_NUM_RMON_STATS);
+		dpaa2_mac_clear_stats(mac, &mac->pause_stats, DPAA2_MAC_NUM_PAUSE_STATS);
+		dpaa2_mac_clear_stats(mac, &mac->eth_ctrl_stats, DPAA2_MAC_NUM_ETH_CTRL_STATS);
+		dpaa2_mac_clear_stats(mac, &mac->eth_mac_stats, DPAA2_MAC_NUM_ETH_MAC_STATS);
+	}
+
 	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
 	if (mac->fw_node)
 		fwnode_handle_put(mac->fw_node);
 }
 
+static void dpaa2_mac_transfer_stats(const struct dpmac_counter *counters,
+				     size_t num_counters, void *s,
+				     u64 *cnt_values)
+{
+	for (size_t i = 0; i < num_counters; i++) {
+		u64 *p = s + counters[i].offset;
+
+		*p = le64_to_cpu(cnt_values[i]);
+	}
+}
+
+static const struct ethtool_rmon_hist_range dpaa2_mac_rmon_ranges[] = {
+	{   64,   64 },
+	{   65,  127 },
+	{  128,  255 },
+	{  256,  511 },
+	{  512, 1023 },
+	{ 1024, 1518 },
+	{ 1519, DPAA2_ETH_MFL },
+	{},
+};
+
+static void dpaa2_mac_get_standard_stats(struct dpaa2_mac *mac, struct dpaa2_mac_stats *stats,
+					 size_t num_cnt, const struct dpmac_counter *counters,
+					 void *s)
+{
+	struct device *dev = mac->net_dev->dev.parent;
+	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
+	int err;
+
+	if (!(mac->features & DPAA2_MAC_FEATURE_STANDARD_STATS))
+		return;
+
+	if (!stats->idx_dma_mem || !stats->values_dma_mem)
+		return;
+
+	err = dpmac_get_statistics(mac->mc_io, 0, dpmac_dev->mc_handle,
+				   stats->idx_iova, stats->values_iova,
+				   num_cnt);
+	if (err) {
+		netdev_err(mac->net_dev, "%s: dpmac_get_statistics() = %d\n",
+			   __func__, err);
+		return;
+	}
+
+	dma_sync_single_for_cpu(dev, stats->values_iova, num_cnt * sizeof(u64),
+				DMA_FROM_DEVICE);
+
+	dpaa2_mac_transfer_stats(counters, num_cnt, s, stats->values_dma_mem);
+}
+
+void dpaa2_mac_get_rmon_stats(struct dpaa2_mac *mac, struct ethtool_rmon_stats *s,
+			      const struct ethtool_rmon_hist_range **ranges)
+{
+	if (s->src != ETHTOOL_MAC_STATS_SRC_AGGREGATE)
+		return;
+
+	dpaa2_mac_get_standard_stats(mac, &mac->rmon_stats, DPAA2_MAC_NUM_RMON_STATS,
+				     dpaa2_mac_rmon_stats, s);
+
+	*ranges = dpaa2_mac_rmon_ranges;
+}
+
+void dpaa2_mac_get_pause_stats(struct dpaa2_mac *mac, struct ethtool_pause_stats *s)
+{
+	if (s->src != ETHTOOL_MAC_STATS_SRC_AGGREGATE)
+		return;
+
+	dpaa2_mac_get_standard_stats(mac, &mac->pause_stats, DPAA2_MAC_NUM_PAUSE_STATS,
+				     dpaa2_mac_pause_stats, s);
+}
+
+void dpaa2_mac_get_ctrl_stats(struct dpaa2_mac *mac, struct ethtool_eth_ctrl_stats *s)
+{
+	if (s->src != ETHTOOL_MAC_STATS_SRC_AGGREGATE)
+		return;
+
+	dpaa2_mac_get_standard_stats(mac, &mac->eth_ctrl_stats, DPAA2_MAC_NUM_ETH_CTRL_STATS,
+				     dpaa2_mac_eth_ctrl_stats, s);
+}
+
+void dpaa2_mac_get_eth_mac_stats(struct dpaa2_mac *mac, struct ethtool_eth_mac_stats *s)
+{
+	if (s->src != ETHTOOL_MAC_STATS_SRC_AGGREGATE)
+		return;
+
+	dpaa2_mac_get_standard_stats(mac, &mac->eth_mac_stats, DPAA2_MAC_NUM_ETH_MAC_STATS,
+				     dpaa2_mac_eth_mac_stats, s);
+}
+
 int dpaa2_mac_get_sset_count(void)
 {
 	return DPAA2_MAC_NUM_ETHTOOL_STATS;
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
index 386286209606..eeb632b9da0a 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
@@ -36,6 +36,10 @@ struct dpaa2_mac {
 	struct phy *serdes_phy;
 
 	struct dpaa2_mac_stats ethtool_stats;
+	struct dpaa2_mac_stats rmon_stats;
+	struct dpaa2_mac_stats pause_stats;
+	struct dpaa2_mac_stats eth_ctrl_stats;
+	struct dpaa2_mac_stats eth_mac_stats;
 };
 
 static inline bool dpaa2_mac_is_type_phy(struct dpaa2_mac *mac)
@@ -61,6 +65,15 @@ void dpaa2_mac_get_strings(u8 **data);
 
 void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data);
 
+void dpaa2_mac_get_rmon_stats(struct dpaa2_mac *mac, struct ethtool_rmon_stats *s,
+			      const struct ethtool_rmon_hist_range **ranges);
+
+void dpaa2_mac_get_pause_stats(struct dpaa2_mac *mac, struct ethtool_pause_stats *s);
+
+void dpaa2_mac_get_ctrl_stats(struct dpaa2_mac *mac, struct ethtool_eth_ctrl_stats *s);
+
+void dpaa2_mac_get_eth_mac_stats(struct dpaa2_mac *mac, struct ethtool_eth_mac_stats *s);
+
 void dpaa2_mac_start(struct dpaa2_mac *mac);
 
 void dpaa2_mac_stop(struct dpaa2_mac *mac);
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c
index a888f6e6e9b0..3203873fae85 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c
@@ -3,7 +3,7 @@
  * DPAA2 Ethernet Switch ethtool support
  *
  * Copyright 2014-2016 Freescale Semiconductor Inc.
- * Copyright 2017-2018 NXP
+ * Copyright 2017-2018, 2024-2026 NXP
  *
  */
 
@@ -210,6 +210,46 @@ static void dpaa2_switch_ethtool_get_stats(struct net_device *netdev,
 	mutex_unlock(&port_priv->mac_lock);
 }
 
+static void dpaa2_switch_get_rmon_stats(struct net_device *netdev,
+					struct ethtool_rmon_stats *rmon_stats,
+					const struct ethtool_rmon_hist_range **ranges)
+{
+	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
+
+	mutex_lock(&port_priv->mac_lock);
+
+	if (dpaa2_switch_port_has_mac(port_priv))
+		dpaa2_mac_get_rmon_stats(port_priv->mac, rmon_stats, ranges);
+
+	mutex_unlock(&port_priv->mac_lock);
+}
+
+static void dpaa2_switch_get_ctrl_stats(struct net_device *net_dev,
+					struct ethtool_eth_ctrl_stats *ctrl_stats)
+{
+	struct ethsw_port_priv *port_priv = netdev_priv(net_dev);
+
+	mutex_lock(&port_priv->mac_lock);
+
+	if (dpaa2_switch_port_has_mac(port_priv))
+		dpaa2_mac_get_ctrl_stats(port_priv->mac, ctrl_stats);
+
+	mutex_unlock(&port_priv->mac_lock);
+}
+
+static void dpaa2_switch_get_eth_mac_stats(struct net_device *net_dev,
+					   struct ethtool_eth_mac_stats *eth_mac_stats)
+{
+	struct ethsw_port_priv *port_priv = netdev_priv(net_dev);
+
+	mutex_lock(&port_priv->mac_lock);
+
+	if (dpaa2_switch_port_has_mac(port_priv))
+		dpaa2_mac_get_eth_mac_stats(port_priv->mac, eth_mac_stats);
+
+	mutex_unlock(&port_priv->mac_lock);
+}
+
 const struct ethtool_ops dpaa2_switch_port_ethtool_ops = {
 	.get_drvinfo		= dpaa2_switch_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
@@ -218,4 +258,7 @@ const struct ethtool_ops dpaa2_switch_port_ethtool_ops = {
 	.get_strings		= dpaa2_switch_ethtool_get_strings,
 	.get_ethtool_stats	= dpaa2_switch_ethtool_get_stats,
 	.get_sset_count		= dpaa2_switch_ethtool_get_sset_count,
+	.get_rmon_stats		= dpaa2_switch_get_rmon_stats,
+	.get_eth_ctrl_stats	= dpaa2_switch_get_ctrl_stats,
+	.get_eth_mac_stats	= dpaa2_switch_get_eth_mac_stats,
 };
-- 
2.25.1

[PATCH net-next 4/5] selftests: forwarding: extend ethtool_std_stats_get with pause statistics

[Ioana Ciornei]

Even though pause frame statistics are not exported through the same
ethtool command, there is no point in adding another helper just for
them. Extent the ethtool_std_stats_get() function so that we are able to
interrogate using the same helper all the standard statistics.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index a9034f0bb58b..efd236ae1c28 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -831,8 +831,12 @@ ethtool_std_stats_get()
 	local name=$1; shift
 	local src=$1; shift
 
-	ethtool --json -S $dev --groups $grp -- --src $src | \
-		jq '.[]."'"$grp"'"."'$name'"'
+	if [[ "$grp" == "pause" ]]; then
+		ethtool -I --json -a $dev | jq '.[].statistics.'$name
+	else
+		ethtool --json -S $dev --groups $grp -- --src $src | \
+			jq '.[]."'"$grp"'"."'$name'"'
+	fi
 }
 
 qdisc_stats_get()
-- 
2.25.1

Re: [PATCH net-next 4/5] selftests: forwarding: extend ethtool_std_stats_get with pause statistics

[Petr Machata]

Ioana Ciornei <ioana.ciornei@nxp.com> writes:

> Even though pause frame statistics are not exported through the same
> ethtool command, there is no point in adding another helper just for
> them. Extent the ethtool_std_stats_get() function so that we are able to
> interrogate using the same helper all the standard statistics.
>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  tools/testing/selftests/net/forwarding/lib.sh | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index a9034f0bb58b..efd236ae1c28 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -831,8 +831,12 @@ ethtool_std_stats_get()
>  	local name=$1; shift
>  	local src=$1; shift
>  
> -	ethtool --json -S $dev --groups $grp -- --src $src | \
> -		jq '.[]."'"$grp"'"."'$name'"'
> +	if [[ "$grp" == "pause" ]]; then
> +		ethtool -I --json -a $dev | jq '.[].statistics.'$name

I think name needs to be quoted here? In fact, unless the pause group is
highly unlikely to ever get a key that contains a dash, it should either
be quoted in the horrible way the else branch does it, or do this much
more readable thing instead:

        jq --arg name "$name" '.[].statistics[$name]'

> +	else
> +		ethtool --json -S $dev --groups $grp -- --src $src | \

Since you are touching this line -- can you fix the missing quoting,
please?

> +			jq '.[]."'"$grp"'"."'$name'"'
> +	fi
>  }
>  
>  qdisc_stats_get()

Re: [PATCH net-next 4/5] selftests: forwarding: extend ethtool_std_stats_get with pause statistics

[Ioana Ciornei]

On Fri, Feb 27, 2026 at 05:38:40PM +0100, Petr Machata wrote:
> 
> Ioana Ciornei <ioana.ciornei@nxp.com> writes:
> 
> > Even though pause frame statistics are not exported through the same
> > ethtool command, there is no point in adding another helper just for
> > them. Extent the ethtool_std_stats_get() function so that we are able to
> > interrogate using the same helper all the standard statistics.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  tools/testing/selftests/net/forwarding/lib.sh | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> > index a9034f0bb58b..efd236ae1c28 100644
> > --- a/tools/testing/selftests/net/forwarding/lib.sh
> > +++ b/tools/testing/selftests/net/forwarding/lib.sh
> > @@ -831,8 +831,12 @@ ethtool_std_stats_get()
> >  	local name=$1; shift
> >  	local src=$1; shift
> >  
> > -	ethtool --json -S $dev --groups $grp -- --src $src | \
> > -		jq '.[]."'"$grp"'"."'$name'"'
> > +	if [[ "$grp" == "pause" ]]; then
> > +		ethtool -I --json -a $dev | jq '.[].statistics.'$name
> 
> I think name needs to be quoted here? In fact, unless the pause group is
> highly unlikely to ever get a key that contains a dash,

I would expect that the pause group is pretty much set and will not get
new counters but, sure, I can add the quotes just to be on the safe
side.

> it should either
> be quoted in the horrible way the else branch does it, or do this much
> more readable thing instead:
> 
>         jq --arg name "$name" '.[].statistics[$name]'
> 

Thanks! Wasn't aware of this type of jq variable usage but indeed it
looks better.

> > +	else
> > +		ethtool --json -S $dev --groups $grp -- --src $src | \
> 
> Since you are touching this line -- can you fix the missing quoting,
> please?

Sure, I will add them.

> 
> > +			jq '.[]."'"$grp"'"."'$name'"'

And I think $name above needs double quoting as well.

Ioana

Re: [PATCH net-next 4/5] selftests: forwarding: extend ethtool_std_stats_get with pause statistics

[Petr Machata]

Ioana Ciornei <ioana.ciornei@nxp.com> writes:

> On Fri, Feb 27, 2026 at 05:38:40PM +0100, Petr Machata wrote:
>> 
>> Ioana Ciornei <ioana.ciornei@nxp.com> writes:
>> 
>> > Even though pause frame statistics are not exported through the same
>> > ethtool command, there is no point in adding another helper just for
>> > them. Extent the ethtool_std_stats_get() function so that we are able to
>> > interrogate using the same helper all the standard statistics.
>> >
>> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>> > ---
>> >  tools/testing/selftests/net/forwarding/lib.sh | 8 ++++++--
>> >  1 file changed, 6 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> > index a9034f0bb58b..efd236ae1c28 100644
>> > --- a/tools/testing/selftests/net/forwarding/lib.sh
>> > +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> > @@ -831,8 +831,12 @@ ethtool_std_stats_get()
>> >  	local name=$1; shift
>> >  	local src=$1; shift
>> >  
>> > -	ethtool --json -S $dev --groups $grp -- --src $src | \
>> > -		jq '.[]."'"$grp"'"."'$name'"'
>> > +	if [[ "$grp" == "pause" ]]; then
>> > +		ethtool -I --json -a $dev | jq '.[].statistics.'$name
>> 
>> I think name needs to be quoted here? In fact, unless the pause group is
>> highly unlikely to ever get a key that contains a dash,
>
> I would expect that the pause group is pretty much set and will not get
> new counters but, sure, I can add the quotes just to be on the safe
> side.

Oh, don't get me wrong, I don't believe $name will contain whitespace.
But I would expect shellcheck to complain about the missing quotes.

>> it should either
>> be quoted in the horrible way the else branch does it, or do this much
>> more readable thing instead:
>> 
>>         jq --arg name "$name" '.[].statistics[$name]'
>> 
>
> Thanks! Wasn't aware of this type of jq variable usage but indeed it
> looks better.
>
>> > +	else
>> > +		ethtool --json -S $dev --groups $grp -- --src $src | \
>> 
>> Since you are touching this line -- can you fix the missing quoting,
>> please?
>
> Sure, I will add them.
>
>> 
>> > +			jq '.[]."'"$grp"'"."'$name'"'
>
> And I think $name above needs double quoting as well.

Oh yeah. That's just going to be an absolute confusion of quotes though.
Can you convert to the --arg form as well? I think it should be:

			jq --arg grp "$grp" --arg name "$name" \
				'.[][$grp][$name]'

[PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Ioana Ciornei]

Add a new selftest - ethtool_std_stats.sh - which validates the
eth-ctrl, eth-mac and pause standard statistics exported by an
interface. Not all counters can be validated in software, especially
those that are keeping track of errors. Counters such as
SingleCollisionFrames, FrameCheckSequenceErrors etc are not tested nor
included in this new selftest.

The central part of this patch is the traffic_test() function which
gathers the 'before' counter values, sends a batch of traffic and then
interrogates again the same counters in order to determine if the delta
is on target. The function receives an array through which the caller
can request what counters to be interrogated and, for each of them, what
is their target delta value.

The output from this selftest looks as follows on a LX2160ARDB board:

$ ./ethtool_std_stats.sh eth0 eth1
TEST: eth-ctrl tx on eth0 (MACControlFramesTransmitted on eth0)     [ OK ]
TEST: eth-ctrl tx on eth0 (MACControlFramesReceived on eth1)        [ OK ]
TEST: eth-ctrl tx on eth1 (MACControlFramesTransmitted on eth1)     [ OK ]
TEST: eth-ctrl tx on eth1 (MACControlFramesReceived on eth0)        [ OK ]
TEST: eth-mac bcast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
TEST: eth-mac bcast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
TEST: eth-mac bcast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
TEST: eth-mac bcast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
TEST: eth-mac bcast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
TEST: eth-mac bcast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
TEST: eth-mac bcast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
TEST: eth-mac bcast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
TEST: eth-mac mcast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
TEST: eth-mac mcast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
TEST: eth-mac mcast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
TEST: eth-mac mcast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
TEST: eth-mac mcast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
TEST: eth-mac mcast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
TEST: eth-mac mcast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
TEST: eth-mac mcast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
TEST: eth-mac ucast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
TEST: eth-mac ucast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
TEST: eth-mac ucast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
TEST: eth-mac ucast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
TEST: eth-mac ucast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
TEST: eth-mac ucast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
TEST: eth-mac ucast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
TEST: eth-mac ucast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
TEST: pause tx on eth0 (tx_pause_frames on eth0)                    [ OK ]
TEST: pause tx on eth0 (rx_pause_frames on eth1)                    [ OK ]
TEST: pause tx on eth1 (tx_pause_frames on eth1)                    [ OK ]
TEST: pause tx on eth1 (rx_pause_frames on eth0)                    [ OK ]

Please note that not all MACs are counting the software injected pause
frames as real Tx pause. For example, on a LS1028ARDB the selftest
output will reflect the fact that neither the ENETC MAC, nor the Felix
switch MAC are able to detect Tx pause frames injected by software.

$ ./ethtool_std_stats.sh eno0 swp0
(...)
TEST: Not all MACs detect injected pause frames                     [XFAIL]
TEST: pause tx on eno0 (rx_pause_frames on swp0)                    [ OK ]
TEST: Not all MACs detect injected pause frames                     [XFAIL]
TEST: pause tx on swp0 (rx_pause_frames on eno0)                    [ OK ]

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 .../testing/selftests/drivers/net/hw/Makefile |   1 +
 .../drivers/net/hw/ethtool_std_stats.sh       | 192 ++++++++++++++++++
 2 files changed, 193 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh

diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
index a64140333a46..d447813a14b2 100644
--- a/tools/testing/selftests/drivers/net/hw/Makefile
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -26,6 +26,7 @@ TEST_PROGS = \
 	ethtool_extended_state.sh \
 	ethtool_mm.sh \
 	ethtool_rmon.sh \
+	ethtool_std_stats.sh \
 	hw_stats_l3.sh \
 	hw_stats_l3_gre.sh \
 	iou-zcrx.py \
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh b/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
new file mode 100755
index 000000000000..4ce8f140a18c
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
@@ -0,0 +1,192 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#shellcheck disable=SC2034 # SC does not see the global variables
+
+ALL_TESTS="
+	test_eth_ctrl_stats
+	test_eth_mac_stats
+	test_pause_stats
+"
+STABLE_MAC_ADDRS=yes
+NUM_NETIFS=2
+lib_dir=$(dirname "$0")
+# shellcheck source=./../../../net/forwarding/lib.sh
+source "$lib_dir"/../../../net/forwarding/lib.sh
+
+traffic_test()
+{
+	local iface=$1; shift
+	local neigh=$1; shift
+	local num_tx=$1; shift
+	local pkt_format="$1"; shift
+	local title="$1"; shift
+	declare -a counters=( "${@:2:$1}" ); shift "$(( $1 + 1 ))"
+	local before after delta target_high extra
+	local int grp counter target unit
+	local num_rx=$((num_tx * 2))
+	local xfail_message
+	local src="aggregate"
+
+	for i in "${!counters[@]}"; do
+		read -r int grp counter target unit xfail_message <<< "${counters[$i]}"
+		before[i]=$(ethtool_std_stats_get "$int" "$grp" "$counter" "$src")
+	done
+
+	# shellcheck disable=SC2086 # needs split options
+	$MZ "$iface" -q -c "$num_tx" $pkt_format
+
+	# shellcheck disable=SC2086 # needs split options
+	$MZ "$neigh" -q -c "$num_rx" $pkt_format
+
+	for i in "${!counters[@]}"; do
+		read -r int grp counter target unit xfail_message<<< "${counters[$i]}"
+
+		after[i]=$(ethtool_std_stats_get "$int" "$grp" "$counter" "$src")
+		if [[ "${after[$i]}" == "null" ]]; then
+			log_test_skip "$int does not support $grp-$counter"
+			continue;
+		fi
+
+		delta=$((after[i] - before[i]))
+
+		# Allow an extra 1% tolerance for random packets sent by the stack
+		extra=$((num_pkts * unit / 100))
+		target_high=$((target + extra))
+
+		RET=0
+		[ "$delta" -ge "$target" ] && [ "$delta" -le "$target_high" ]
+		err="$?"
+		if [[ $err != 0  ]] && [[ -n $xfail_message ]]; then
+			log_test_xfail "$xfail_message"
+			continue;
+		fi
+		check_err "$err" "$grp-$counter is not valid on $int (expected $target, got $delta)"
+		log_test "$title" "$counter on $int"
+	done
+}
+
+test_eth_ctrl_stats()
+{
+	local pkt_format="-a own -b bcast 88:08 -p 64"
+	local num_pkts=1000
+	local counters
+
+	counters=("$h1 eth-ctrl MACControlFramesTransmitted $num_pkts 1"
+		  "$h2 eth-ctrl MACControlFramesReceived $num_pkts 1")
+	traffic_test "$h1" "$h2" "$num_pkts" "$pkt_format" \
+		"eth-ctrl tx on $h1" \
+		"${#counters[@]}" "${counters[@]}"
+
+	counters=("$h2 eth-ctrl MACControlFramesTransmitted $num_pkts 1"
+		  "$h1 eth-ctrl MACControlFramesReceived $num_pkts 1")
+	traffic_test "$h2" "$h1" "$num_pkts" "$pkt_format" \
+		"eth-ctrl tx on $h2" \
+		"${#counters[@]}" "${counters[@]}"
+}
+
+test_eth_mac_stats()
+{
+	local pkt_size=100
+	local pkt_size_fcs=$((pkt_size + 4))
+	local bcast_pkt_format="-a own -b bcast -p $pkt_size"
+	local mcast_pkt_format="-a own -b -b 01:00:5E:00:00:01 -p $pkt_size"
+	local ucast_pkt_format="-a own -b $h2_mac -p $pkt_size"
+	local num_pkts=2000
+	local octets=$((pkt_size_fcs * num_pkts))
+	local counters
+
+	counters=("$h1 eth-mac BroadcastFramesXmittedOK $num_pkts 1"
+		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
+		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
+		  "$h1 eth-mac MulticastFramesXmittedOK 0 1"
+		  "$h2 eth-mac BroadcastFramesReceivedOK $num_pkts 1"
+		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
+		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
+		  "$h2 eth-mac MulticastFramesReceivedOK 0 1")
+	traffic_test "$h1" "$h2" "$num_pkts" "$bcast_pkt_format" \
+		"eth-mac bcast tx on $h1" \
+		"${#counters[@]}" "${counters[@]}"
+
+	counters=("$h1 eth-mac BroadcastFramesXmittedOK 0 1"
+		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
+		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
+		  "$h1 eth-mac MulticastFramesXmittedOK $num_pkts 1"
+		  "$h2 eth-mac BroadcastFramesReceivedOK 0 1"
+		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
+		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
+		  "$h2 eth-mac MulticastFramesReceivedOK $num_pkts 1")
+	traffic_test "$h1" "$h2" "$num_pkts" "$mcast_pkt_format" \
+		"eth-mac mcast tx on $h1" \
+		"${#counters[@]}" "${counters[@]}"
+
+	counters=("$h1 eth-mac BroadcastFramesXmittedOK 0 1"
+		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
+		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
+		  "$h1 eth-mac MulticastFramesXmittedOK 0 1"
+		  "$h2 eth-mac BroadcastFramesReceivedOK 0 1"
+		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
+		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
+		  "$h2 eth-mac MulticastFramesReceivedOK 0 1")
+	traffic_test "$h1" "$h2" "$num_pkts" "$ucast_pkt_format" \
+		"eth-mac ucast tx on $h1" \
+		"${#counters[@]}" "${counters[@]}"
+}
+
+test_pause_stats()
+{
+	local pkt_format="-a own -b 01:80:c2:00:00:01 88:08:00:01:00:01"
+	local xfail_message="Not all MACs detect injected pause frames"
+	local num_pkts=2000
+	local counters i
+
+	# Check that there is pause frame support
+	for ((i = 1; i <= NUM_NETIFS; ++i)); do
+		if ! ethtool -I --json -a "${NETIFS[p$i]}" > /dev/null 2>&1; then
+			log_test_skip "No support for pause frames, skip tests"
+			exit
+		fi
+	done
+
+	counters=("$h1 pause tx_pause_frames $num_pkts 1 $xfail_message"
+		  "$h2 pause rx_pause_frames $num_pkts 1")
+	traffic_test "$h1" "$h2" "$num_pkts" "$pkt_format" \
+		"pause tx on $h1" \
+		"${#counters[@]}" "${counters[@]}"
+
+	counters=("$h2 pause tx_pause_frames $num_pkts 1 $xfail_message"
+		  "$h1 pause rx_pause_frames $num_pkts 1")
+	traffic_test "$h2" "$h1" "$num_pkts" "$pkt_format" \
+		"pause tx on $h2" \
+		"${#counters[@]}" "${counters[@]}"
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	h2=${NETIFS[p2]}
+
+	h2_mac=$(mac_get "$h2")
+
+	for iface in $h1 $h2; do
+		ip link set dev "$iface" up
+	done
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	for iface in $h2 $h1; do
+		ip link set dev "$iface" down
+	done
+}
+
+check_ethtool_counter_group_support
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit "$EXIT_STATUS"
-- 
2.25.1

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Andrew Lunn]

On Wed, Feb 25, 2026 at 05:06:48PM +0200, Ioana Ciornei wrote:
> Add a new selftest - ethtool_std_stats.sh - which validates the
> eth-ctrl, eth-mac and pause standard statistics exported by an
> interface. Not all counters can be validated in software, especially
> those that are keeping track of errors. Counters such as
> SingleCollisionFrames, FrameCheckSequenceErrors etc are not tested nor
> included in this new selftest.

Hi Ioana

Thanks for the test!

Do we actually expect errors when running such a test? How many times
have you run this test and seen any of the error counters be anything
other than 0?

Which do you think is more likely:

1) A real error happens

2) Bug in the driver so that it reports a value in the wrong place?

Maybe we should check the error counters are zero? 

      Andrew

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Ioana Ciornei]

On Thu, Feb 26, 2026 at 12:38:58AM +0100, Andrew Lunn wrote:
> On Wed, Feb 25, 2026 at 05:06:48PM +0200, Ioana Ciornei wrote:
> > Add a new selftest - ethtool_std_stats.sh - which validates the
> > eth-ctrl, eth-mac and pause standard statistics exported by an
> > interface. Not all counters can be validated in software, especially
> > those that are keeping track of errors. Counters such as
> > SingleCollisionFrames, FrameCheckSequenceErrors etc are not tested nor
> > included in this new selftest.
> 
> Hi Ioana
> 
> Thanks for the test!
> 
> Do we actually expect errors when running such a test? How many times
> have you run this test and seen any of the error counters be anything
> other than 0?

No, we don't expect any errors with this test and I didn't see any error
counters incremenent in the tens of times that I ran the selftest. But,
to be fair, I was not looking for them thoroughly through testing.

> 
> Which do you think is more likely:
> 
> 1) A real error happens
> 
> 2) Bug in the driver so that it reports a value in the wrong place?
> 

I would say that having a driver bug is much likely than, for example,
having an FCS error.

> Maybe we should check the error counters are zero? 

Ok, I will extend the test to check the errors against zero and see how it behaves.

Thanks!

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Ioana Ciornei]

On Thu, Feb 26, 2026 at 09:03:32AM +0200, Ioana Ciornei wrote:
> On Thu, Feb 26, 2026 at 12:38:58AM +0100, Andrew Lunn wrote:
> > On Wed, Feb 25, 2026 at 05:06:48PM +0200, Ioana Ciornei wrote:
> > > Add a new selftest - ethtool_std_stats.sh - which validates the
> > > eth-ctrl, eth-mac and pause standard statistics exported by an
> > > interface. Not all counters can be validated in software, especially
> > > those that are keeping track of errors. Counters such as
> > > SingleCollisionFrames, FrameCheckSequenceErrors etc are not tested nor
> > > included in this new selftest.
> > 
> > Hi Ioana
> > 
> > Thanks for the test!
> > 
> > Do we actually expect errors when running such a test? How many times
> > have you run this test and seen any of the error counters be anything
> > other than 0?
> 
> No, we don't expect any errors with this test and I didn't see any error
> counters incremenent in the tens of times that I ran the selftest. But,
> to be fair, I was not looking for them thoroughly through testing.
> 
> > 
> > Which do you think is more likely:
> > 
> > 1) A real error happens
> > 
> > 2) Bug in the driver so that it reports a value in the wrong place?
> > 
> 
> I would say that having a driver bug is much likely than, for example,
> having an FCS error.
> 
> > Maybe we should check the error counters are zero? 
> 
> Ok, I will extend the test to check the errors against zero and see how it behaves.
> 

I am back with a bit more information. The counters which were not
checked in this version can be grouped in two categories:

- Error counters such as:

	u64 FrameCheckSequenceErrors;
	u64 AlignmentErrors;
	u64 FramesLostDueToIntMACXmitError;
	u64 CarrierSenseErrors;
	u64 FramesLostDueToIntMACRcvError;
	u64 InRangeLengthErrors;
	u64 OutOfRangeLengthField;
	u64 FrameTooLongErrors;
	u64 FramesAbortedDueToXSColls;

  I did extend the selftest with these ones so that we check them
  against zero. I ran the test hundreds of times and I did not see any
  problems.

- Collision related counters (not really errors):

	u64 SingleCollisionFrames;
	u64 MultipleCollisionFrames;
	u64 FramesWithDeferredXmissions;
	u64 LateCollisions;
	u64 FramesWithExcessiveDeferral;

  With these I don't know what to do. Theoretically, they could be
  non-zero in half-duplex circumstances which means that checking for
  zero would not be entirely accurate.


 Ioana

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Andrew Lunn]

> I am back with a bit more information. The counters which were not
> checked in this version can be grouped in two categories:
> 
> - Error counters such as:
> 
> 	u64 FrameCheckSequenceErrors;
> 	u64 AlignmentErrors;
> 	u64 FramesLostDueToIntMACXmitError;
> 	u64 CarrierSenseErrors;
> 	u64 FramesLostDueToIntMACRcvError;
> 	u64 InRangeLengthErrors;
> 	u64 OutOfRangeLengthField;
> 	u64 FrameTooLongErrors;
> 	u64 FramesAbortedDueToXSColls;
> 
>   I did extend the selftest with these ones so that we check them
>   against zero. I ran the test hundreds of times and I did not see any
>   problems.

Great.

> 
> - Collision related counters (not really errors):
> 
> 	u64 SingleCollisionFrames;
> 	u64 MultipleCollisionFrames;
> 	u64 FramesWithDeferredXmissions;
> 	u64 LateCollisions;
> 	u64 FramesWithExcessiveDeferral;
> 
>   With these I don't know what to do. Theoretically, they could be
>   non-zero in half-duplex circumstances which means that checking for
>   zero would not be entirely accurate.

How about, fail the test if any are greater than 1% of the number of
packets transmitted/received? My _guess_ is, if you have 1% packet
loss, networking is not going to be happy anyway. It probably means
you have one end doing Half duplex and the other Full. That is a
typical configuration error you see causing collisions. Not that i've
actually seen this for maybe a decade!

Failing the test, with a comment about checking duplex configuration,
seems sensible.

     Andrew

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Ioana Ciornei]

On Thu, Feb 26, 2026 at 02:31:26PM +0100, Andrew Lunn wrote:
> > I am back with a bit more information. The counters which were not
> > checked in this version can be grouped in two categories:
> > 
> > - Error counters such as:
> > 
> > 	u64 FrameCheckSequenceErrors;
> > 	u64 AlignmentErrors;
> > 	u64 FramesLostDueToIntMACXmitError;
> > 	u64 CarrierSenseErrors;
> > 	u64 FramesLostDueToIntMACRcvError;
> > 	u64 InRangeLengthErrors;
> > 	u64 OutOfRangeLengthField;
> > 	u64 FrameTooLongErrors;
> > 	u64 FramesAbortedDueToXSColls;
> > 
> >   I did extend the selftest with these ones so that we check them
> >   against zero. I ran the test hundreds of times and I did not see any
> >   problems.
> 
> Great.
> 
> > 
> > - Collision related counters (not really errors):
> > 
> > 	u64 SingleCollisionFrames;
> > 	u64 MultipleCollisionFrames;
> > 	u64 FramesWithDeferredXmissions;
> > 	u64 LateCollisions;
> > 	u64 FramesWithExcessiveDeferral;
> > 
> >   With these I don't know what to do. Theoretically, they could be
> >   non-zero in half-duplex circumstances which means that checking for
> >   zero would not be entirely accurate.
> 
> How about, fail the test if any are greater than 1% of the number of
> packets transmitted/received? My _guess_ is, if you have 1% packet
> loss, networking is not going to be happy anyway. It probably means
> you have one end doing Half duplex and the other Full. That is a
> typical configuration error you see causing collisions. Not that i've
> actually seen this for maybe a decade!
> 
> Failing the test, with a comment about checking duplex configuration,
> seems sensible.

Seems reasonable. Thanks for the help!

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Jakub Kicinski]

On Thu, 26 Feb 2026 16:18:21 +0200 Ioana Ciornei wrote:
> > How about, fail the test if any are greater than 1% of the number of
> > packets transmitted/received? My _guess_ is, if you have 1% packet
> > loss, networking is not going to be happy anyway. It probably means
> > you have one end doing Half duplex and the other Full. That is a
> > typical configuration error you see causing collisions. Not that i've
> > actually seen this for maybe a decade!
> > 
> > Failing the test, with a comment about checking duplex configuration,
> > seems sensible.  
> 
> Seems reasonable. Thanks for the help!

FWIW the expectation is that the test should be able to run even on
systems / boards with a single interface. So the control traffic
(communicating with the traffic generator) will run over the same
interface as the test. 1% error is unachievable. I'd only check the
lower bound, and use some sanity value for the upper bound (2^30 ?)
if at all

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Ioana Ciornei]

On Thu, Feb 26, 2026 at 06:25:11PM -0800, Jakub Kicinski wrote:
> On Thu, 26 Feb 2026 16:18:21 +0200 Ioana Ciornei wrote:
> > > How about, fail the test if any are greater than 1% of the number of
> > > packets transmitted/received? My _guess_ is, if you have 1% packet
> > > loss, networking is not going to be happy anyway. It probably means
> > > you have one end doing Half duplex and the other Full. That is a
> > > typical configuration error you see causing collisions. Not that i've
> > > actually seen this for maybe a decade!
> > > 
> > > Failing the test, with a comment about checking duplex configuration,
> > > seems sensible.  
> > 
> > Seems reasonable. Thanks for the help!
> 
> FWIW the expectation is that the test should be able to run even on
> systems / boards with a single interface. So the control traffic
> (communicating with the traffic generator) will run over the same
> interface as the test. 1% error is unachievable. I'd only check the
> lower bound, and use some sanity value for the upper bound (2^30 ?)
> if at all

Really? I didn't know of that expectation at all.

I did take ethtool_rmon.sh as an example and that selftest as well
takes NUM_NETIFS=2 and does check for both a lower bound and upper bound
that takes into account a 1% deviance from the target.

How would the test even work with only a single interface?

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Andrew Lunn]

On Fri, Feb 27, 2026 at 09:34:28AM +0200, Ioana Ciornei wrote:
> On Thu, Feb 26, 2026 at 06:25:11PM -0800, Jakub Kicinski wrote:
> > On Thu, 26 Feb 2026 16:18:21 +0200 Ioana Ciornei wrote:
> > > > How about, fail the test if any are greater than 1% of the number of
> > > > packets transmitted/received? My _guess_ is, if you have 1% packet
> > > > loss, networking is not going to be happy anyway. It probably means
> > > > you have one end doing Half duplex and the other Full. That is a
> > > > typical configuration error you see causing collisions. Not that i've
> > > > actually seen this for maybe a decade!
> > > > 
> > > > Failing the test, with a comment about checking duplex configuration,
> > > > seems sensible.  
> > > 
> > > Seems reasonable. Thanks for the help!
> > 
> > FWIW the expectation is that the test should be able to run even on
> > systems / boards with a single interface. So the control traffic
> > (communicating with the traffic generator) will run over the same
> > interface as the test. 1% error is unachievable. I'd only check the
> > lower bound, and use some sanity value for the upper bound (2^30 ?)
> > if at all
> 
> Really? I didn't know of that expectation at all.
> 
> I did take ethtool_rmon.sh as an example and that selftest as well
> takes NUM_NETIFS=2 and does check for both a lower bound and upper bound
> that takes into account a 1% deviance from the target.
> 
> How would the test even work with only a single interface?

Just to add to this, for the 1% i was referring to counters for
collisions. If the control traffic is causing collisions the system it
just as wrongly configured as generated traffic causing collisions.

For 'everyday' systems, i doubt Half Duplex is ever used, but
automotive with a T1 PHY might. So we might need to review this 1%
once somebody runs this test on such a system.

     Andrew

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Jakub Kicinski]

On Fri, 27 Feb 2026 15:17:02 +0100 Andrew Lunn wrote:
> On Fri, Feb 27, 2026 at 09:34:28AM +0200, Ioana Ciornei wrote:
> > On Thu, Feb 26, 2026 at 06:25:11PM -0800, Jakub Kicinski wrote:  
> > > FWIW the expectation is that the test should be able to run even on
> > > systems / boards with a single interface. So the control traffic
> > > (communicating with the traffic generator) will run over the same
> > > interface as the test. 1% error is unachievable. I'd only check the
> > > lower bound, and use some sanity value for the upper bound (2^30 ?)
> > > if at all  
> > 
> > Really? I didn't know of that expectation at all.
> > 
> > I did take ethtool_rmon.sh as an example and that selftest as well
> > takes NUM_NETIFS=2 and does check for both a lower bound and upper bound
> > that takes into account a 1% deviance from the target.
> > 
> > How would the test even work with only a single interface?  
> 
> Just to add to this, for the 1% i was referring to counters for
> collisions. If the control traffic is causing collisions the system it
> just as wrongly configured as generated traffic causing collisions.
> 
> For 'everyday' systems, i doubt Half Duplex is ever used, but
> automotive with a T1 PHY might. So we might need to review this 1%
> once somebody runs this test on such a system.

Right, right, errors and exceptions are probably fine.
I was referring to checking overall byte / packet counters with 1%
tolerance. Sorry if I misread the discussion or the patch.

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Jakub Kicinski]

On Fri, 27 Feb 2026 09:34:28 +0200 Ioana Ciornei wrote:
> On Thu, Feb 26, 2026 at 06:25:11PM -0800, Jakub Kicinski wrote:
> > On Thu, 26 Feb 2026 16:18:21 +0200 Ioana Ciornei wrote:  
> > > > How about, fail the test if any are greater than 1% of the number of
> > > > packets transmitted/received? My _guess_ is, if you have 1% packet
> > > > loss, networking is not going to be happy anyway. It probably means
> > > > you have one end doing Half duplex and the other Full. That is a
> > > > typical configuration error you see causing collisions. Not that i've
> > > > actually seen this for maybe a decade!
> > > > 
> > > > Failing the test, with a comment about checking duplex configuration,
> > > > seems sensible.    
> > > 
> > > Seems reasonable. Thanks for the help!  
> > 
> > FWIW the expectation is that the test should be able to run even on
> > systems / boards with a single interface. So the control traffic
> > (communicating with the traffic generator) will run over the same
> > interface as the test. 1% error is unachievable. I'd only check the
> > lower bound, and use some sanity value for the upper bound (2^30 ?)
> > if at all  
> 
> Really? I didn't know of that expectation at all.
> 
> I did take ethtool_rmon.sh as an example and that selftest as well
> takes NUM_NETIFS=2 and does check for both a lower bound and upper bound
> that takes into account a 1% deviance from the target.

I called out in the other thread that the bash scripts in this dir
pre-date any serious CI use. They are only there to get them out of 
the way of SW-only CI testing.

> How would the test even work with only a single interface?

Hopefully the readme mentioned in my other reply clarifies.

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Jakub Kicinski]

On Wed, 25 Feb 2026 17:06:48 +0200 Ioana Ciornei wrote:
> +ALL_TESTS="
> +	test_eth_ctrl_stats
> +	test_eth_mac_stats
> +	test_pause_stats
> +"
> +STABLE_MAC_ADDRS=yes
> +NUM_NETIFS=2
> +lib_dir=$(dirname "$0")
> +# shellcheck source=./../../../net/forwarding/lib.sh
> +source "$lib_dir"/../../../net/forwarding/lib.sh

Argh, at some point we should probably decide whether we have 
a preference on which "library" / set of env vars we use under 
drivers/net/hw. Adding Petr to CC.

The existing tests under drivers/net/hw which source forwarding/lib.sh
pre-date the "NIC setup" described in 
tools/testing/selftests/drivers/net/README.rst

Should we ask "NIC setup" to be used for all tests which only need
NUM_NETIFS=2 ? These are basically simple tests where one netif is 
a traffic generator and the other is the DUT. And IIUC the "forwarding
setup" can't be used when the traffic generator is controlled over SSH?

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Wed, 25 Feb 2026 17:06:48 +0200 Ioana Ciornei wrote:
>> +ALL_TESTS="
>> +	test_eth_ctrl_stats
>> +	test_eth_mac_stats
>> +	test_pause_stats
>> +"
>> +STABLE_MAC_ADDRS=yes
>> +NUM_NETIFS=2
>> +lib_dir=$(dirname "$0")
>> +# shellcheck source=./../../../net/forwarding/lib.sh
>> +source "$lib_dir"/../../../net/forwarding/lib.sh
>
> Argh, at some point we should probably decide whether we have 
> a preference on which "library" / set of env vars we use under 
> drivers/net/hw. Adding Petr to CC.

I think the expectation is that by default, tests written in Bash are
run on one machine without remotes.

I think this fundamentally stems from the fact that running processes in
Python is a bit unwieldy, so it makes sense to have helpers, so
everybody uses them, so you can have helpers grow brains to do things
like over-the-ssh configuration. In Bash, running a traffic generator is
easier than working with arrays. So the helpers tend not to be as useful
and we don't generally have them. At least not in any consistent way.

Eyeing the file, the requirement for the remote interface to be up and
configured with an IP address is a bit surprising to me. I would think a
down state is the most natural, and have the test bring it up and
configure it in a way that it needs. I'm thinking maybe this is to allow
testing a sole interface on like an embedded device?

Anyway, that's a fairly strong differency between how Bash tests are
typically written and the NIC setup. I think basically all existing
tests assume the devices are theirs to tamper with.

> The existing tests under drivers/net/hw which source forwarding/lib.sh
> pre-date the "NIC setup" described in 
> tools/testing/selftests/drivers/net/README.rst
>
> Should we ask "NIC setup" to be used for all tests which only need
> NUM_NETIFS=2 ? These are basically simple tests where one netif is 
> a traffic generator and the other is the DUT. And IIUC the "forwarding
> setup" can't be used when the traffic generator is controlled over SSH?

In principle nothing prevents lib.sh from growing brains to support
these remote shenanigans. I think it's just that so far nobody cared
enough to actually do it.

I think that a helper that in effect does "run this on a machine where
$swp1 is" is mostly what is needed. That and "make sure $swp1 and $swp2
are on the same machine". It's going to be annoying to work with though,
because you need to annotate every single command. I bet there's a nice
syntax to make it not activelly annoying.

If we have this, it might make sense to require tests to make use of it.
(With an explicit opt-out for special cases.) But I do not want every
test to have to reinvent this wheel and cargo-cult snippets from other
tests.

BTW, my guess is that even many multi-port tests that I wrote boil down
to just a bunch of fairly independent loopbacks whose far ends could be
on remote machines. It's not a priori nonsense to me that one would run
a test like this, or whatever magic we'd use:

    ./test.sh ssh://petr@10.1.2.3:eth1 swp1 veth1 ns://foo:veth2

And it just works, because only swp1 and swp2 need to be bridged, the
rest can be remote, and the traffic generation helper knows that to pump
traffic to ssh://10.1.2.3:eth1, obviously you need to ssh there first.
But the library would need to have helpers for this, and the tests would
need to use them.

At least ethtool counters would cause problems obviously.

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Jakub Kicinski]

On Fri, 27 Feb 2026 14:53:06 +0100 Petr Machata wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> > On Wed, 25 Feb 2026 17:06:48 +0200 Ioana Ciornei wrote:  
> >> +ALL_TESTS="
> >> +	test_eth_ctrl_stats
> >> +	test_eth_mac_stats
> >> +	test_pause_stats
> >> +"
> >> +STABLE_MAC_ADDRS=yes
> >> +NUM_NETIFS=2
> >> +lib_dir=$(dirname "$0")
> >> +# shellcheck source=./../../../net/forwarding/lib.sh
> >> +source "$lib_dir"/../../../net/forwarding/lib.sh  
> >
> > Argh, at some point we should probably decide whether we have 
> > a preference on which "library" / set of env vars we use under 
> > drivers/net/hw. Adding Petr to CC.  
> 
> I think the expectation is that by default, tests written in Bash are
> run on one machine without remotes.

I think we need to define the guidance by properties of the test, not
the machine? Of course _tests_ which don't need a traffic source can
simply consume the NETIF evn variable, so its trivial to write them in
any language without much library support. But for tests which need
traffic input we need a different distinction than "does the author
care about single-interface machines", so to speak?

> I think this fundamentally stems from the fact that running processes in
> Python is a bit unwieldy, so it makes sense to have helpers, so
> everybody uses them, so you can have helpers grow brains to do things
> like over-the-ssh configuration. In Bash, running a traffic generator is
> easier than working with arrays. So the helpers tend not to be as useful
> and we don't generally have them. At least not in any consistent way.
> 
> Eyeing the file, the requirement for the remote interface to be up and
> configured with an IP address is a bit surprising to me. I would think a
> down state is the most natural, and have the test bring it up and
> configure it in a way that it needs. I'm thinking maybe this is to allow
> testing a sole interface on like an embedded device?
> 
> Anyway, that's a fairly strong differency between how Bash tests are
> typically written and the NIC setup. I think basically all existing
> tests assume the devices are theirs to tamper with.
> 
> > The existing tests under drivers/net/hw which source forwarding/lib.sh
> > pre-date the "NIC setup" described in 
> > tools/testing/selftests/drivers/net/README.rst
> >
> > Should we ask "NIC setup" to be used for all tests which only need
> > NUM_NETIFS=2 ? These are basically simple tests where one netif is 
> > a traffic generator and the other is the DUT. And IIUC the "forwarding
> > setup" can't be used when the traffic generator is controlled over SSH?  
> 
> In principle nothing prevents lib.sh from growing brains to support
> these remote shenanigans. I think it's just that so far nobody cared
> enough to actually do it.
> 
> I think that a helper that in effect does "run this on a machine where
> $swp1 is" is mostly what is needed. That and "make sure $swp1 and $swp2
> are on the same machine". It's going to be annoying to work with though,
> because you need to annotate every single command. I bet there's a nice
> syntax to make it not activelly annoying.
> 
> If we have this, it might make sense to require tests to make use of it.
> (With an explicit opt-out for special cases.) But I do not want every
> test to have to reinvent this wheel and cargo-cult snippets from other
> tests.
> 
> BTW, my guess is that even many multi-port tests that I wrote boil down
> to just a bunch of fairly independent loopbacks whose far ends could be
> on remote machines. It's not a priori nonsense to me that one would run
> a test like this, or whatever magic we'd use:
> 
>     ./test.sh ssh://petr@10.1.2.3:eth1 swp1 veth1 ns://foo:veth2
> 
> And it just works, because only swp1 and swp2 need to be bridged, the
> rest can be remote, and the traffic generation helper knows that to pump
> traffic to ssh://10.1.2.3:eth1, obviously you need to ssh there first.
> But the library would need to have helpers for this, and the tests would
> need to use them.

Right, to be clear I primarily care about being able to run these tests
with traffic generator on a remote system. Otherwise someone who cares
about single-port systems will have to add a separate test I guess?
And we will have to maintain two tests? :S

A nice to have is for all drivers/net/hw tests to use the "NIC" env
variables (move tests which really only make sense on multi-port
devices to drivers/net/forwarding?) But I think "translating"
forwarding variables to NIC if NUMIF=2 could easily be done
automatically by the test / lib so that's lower priority.

> At least ethtool counters would cause problems obviously.

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 27 Feb 2026 14:53:06 +0100 Petr Machata wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> > On Wed, 25 Feb 2026 17:06:48 +0200 Ioana Ciornei wrote:  
>> >> +ALL_TESTS="
>> >> +	test_eth_ctrl_stats
>> >> +	test_eth_mac_stats
>> >> +	test_pause_stats
>> >> +"
>> >> +STABLE_MAC_ADDRS=yes
>> >> +NUM_NETIFS=2
>> >> +lib_dir=$(dirname "$0")
>> >> +# shellcheck source=./../../../net/forwarding/lib.sh
>> >> +source "$lib_dir"/../../../net/forwarding/lib.sh  
>> >
>> > Argh, at some point we should probably decide whether we have 
>> > a preference on which "library" / set of env vars we use under 
>> > drivers/net/hw. Adding Petr to CC.  
>> 
>> I think the expectation is that by default, tests written in Bash are
>> run on one machine without remotes.
>
> I think we need to define the guidance by properties of the test, not
> the machine? Of course _tests_ which don't need a traffic source can

Oh yeah, I'm just stating that's how it currently is and how we got here.

> simply consume the NETIF evn variable, so its trivial to write them in
> any language without much library support. But for tests which need
> traffic input we need a different distinction than "does the author
> care about single-interface machines", so to speak?

I agree that adherence to the drivers/net/README protocol is valuable to
some users and would be good to uphold if reasonable in a given tests.
If that's what you have in mind.

There are going to be tests where it's not a great fit, but I think that
of those NUM_NETIFS=2 tests that we currently have, maybe
ethtool_extended_state has a good reason to be obstinate, because it
sets up negotiations and needs an extra unplugged netdevice.

>> 
>> > The existing tests under drivers/net/hw which source forwarding/lib.sh
>> > pre-date the "NIC setup" described in 
>> > tools/testing/selftests/drivers/net/README.rst
>> >
>> > Should we ask "NIC setup" to be used for all tests which only need
>> > NUM_NETIFS=2 ? These are basically simple tests where one netif is 
>> > a traffic generator and the other is the DUT. And IIUC the "forwarding
>> > setup" can't be used when the traffic generator is controlled over SSH?  
>> 
>> In principle nothing prevents lib.sh from growing brains to support
>> these remote shenanigans. I think it's just that so far nobody cared
>> enough to actually do it.
>> 
>> I think that a helper that in effect does "run this on a machine where
>> $swp1 is" is mostly what is needed. That and "make sure $swp1 and $swp2
>> are on the same machine". It's going to be annoying to work with though,
>> because you need to annotate every single command. I bet there's a nice
>> syntax to make it not activelly annoying.
>> 
>> If we have this, it might make sense to require tests to make use of it.
>> (With an explicit opt-out for special cases.) But I do not want every
>> test to have to reinvent this wheel and cargo-cult snippets from other
>> tests.
>> 
>> BTW, my guess is that even many multi-port tests that I wrote boil down
>> to just a bunch of fairly independent loopbacks whose far ends could be
>> on remote machines. It's not a priori nonsense to me that one would run
>> a test like this, or whatever magic we'd use:
>> 
>>     ./test.sh ssh://petr@10.1.2.3:eth1 swp1 veth1 ns://foo:veth2
>> 
>> And it just works, because only swp1 and swp2 need to be bridged, the
>> rest can be remote, and the traffic generation helper knows that to pump
>> traffic to ssh://10.1.2.3:eth1, obviously you need to ssh there first.
>> But the library would need to have helpers for this, and the tests would
>> need to use them.
>
> Right, to be clear I primarily care about being able to run these tests
> with traffic generator on a remote system. Otherwise someone who cares
> about single-port systems will have to add a separate test I guess?
> And we will have to maintain two tests? :S
>
> A nice to have is for all drivers/net/hw tests to use the "NIC" env
> variables (move tests which really only make sense on multi-port
> devices to drivers/net/forwarding?) But I think "translating"
> forwarding variables to NIC if NUMIF=2 could easily be done
> automatically by the test / lib so that's lower priority.

The reason for my diatribe above was like, if forwarding/lib.sh is
clever enough to handle these things transparently, then the NIC case
might be supported through a wrapper that just does the right thing, and
the test can be overtly just a run of the mill forwarding test.

The requirement for the remote netdevice to be up and preconfigured
throws a wrench in this idea though, so dunno.

One way or another, _some_ brains need to reside somewhere in a library.
I'm not sure forwarding.sh is the place. But I don't want to end up in
the same situation as like lib/half_the_tests.sh that reinvent logging
from first principles again and again.

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Ioana Ciornei]

On Sat, Feb 28, 2026 at 10:11:26AM +0100, Petr Machata wrote:
> 
> Jakub Kicinski <kuba@kernel.org> writes:
> 
> > On Fri, 27 Feb 2026 14:53:06 +0100 Petr Machata wrote:
> >> Jakub Kicinski <kuba@kernel.org> writes:
> >> > On Wed, 25 Feb 2026 17:06:48 +0200 Ioana Ciornei wrote:  
> >> >> +ALL_TESTS="
> >> >> +	test_eth_ctrl_stats
> >> >> +	test_eth_mac_stats
> >> >> +	test_pause_stats
> >> >> +"
> >> >> +STABLE_MAC_ADDRS=yes
> >> >> +NUM_NETIFS=2
> >> >> +lib_dir=$(dirname "$0")
> >> >> +# shellcheck source=./../../../net/forwarding/lib.sh
> >> >> +source "$lib_dir"/../../../net/forwarding/lib.sh  
> >> >
> >> > Argh, at some point we should probably decide whether we have 
> >> > a preference on which "library" / set of env vars we use under 
> >> > drivers/net/hw. Adding Petr to CC.  
> >> 
> >> I think the expectation is that by default, tests written in Bash are
> >> run on one machine without remotes.
> >
> > I think we need to define the guidance by properties of the test, not
> > the machine? Of course _tests_ which don't need a traffic source can
> 
> Oh yeah, I'm just stating that's how it currently is and how we got here.
> 
> > simply consume the NETIF evn variable, so its trivial to write them in
> > any language without much library support. But for tests which need
> > traffic input we need a different distinction than "does the author
> > care about single-interface machines", so to speak?
> 
> I agree that adherence to the drivers/net/README protocol is valuable to
> some users and would be good to uphold if reasonable in a given tests.
> If that's what you have in mind.
> 
> There are going to be tests where it's not a great fit, but I think that
> of those NUM_NETIFS=2 tests that we currently have, maybe
> ethtool_extended_state has a good reason to be obstinate, because it
> sets up negotiations and needs an extra unplugged netdevice.

I would add here even ethtool_rmon.sh and this new test that I
submitted. If you are running with a traffic generator on another board
then you can no longer check that the counter's value is as expected
(with a 1% tolerance), you can only check the lower bound.

Additionally, if you are using the same single port also for control
traffic towards the remote traffic generator, then you surely cannot
reliably check that counters that should not be incremented are indeed
not incremented.

This means that some tests will not benefit at all from working with a
remote link partner, it will make them weaker.

Ioana

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Jakub Kicinski]

On Mon, 2 Mar 2026 14:11:21 +0200 Ioana Ciornei wrote:
> > I agree that adherence to the drivers/net/README protocol is valuable to
> > some users and would be good to uphold if reasonable in a given tests.
> > If that's what you have in mind.
> > 
> > There are going to be tests where it's not a great fit, but I think that
> > of those NUM_NETIFS=2 tests that we currently have, maybe
> > ethtool_extended_state has a good reason to be obstinate, because it
> > sets up negotiations and needs an extra unplugged netdevice.  
> 
> I would add here even ethtool_rmon.sh and this new test that I

I think I already told you that ethool_rmon predates the NIC tests
and bringing it up in this discussion is irrelevant.

> submitted. If you are running with a traffic generator on another board
> then you can no longer check that the counter's value is as expected
> (with a 1% tolerance), you can only check the lower bound.

1% tolerance is impractical for any CI with high test count.
The test will be flaky. And I really doubt that the 1% tolerance
is really necessary to catch most bugs. We're not trying to validate
silicon here.

> Additionally, if you are using the same single port also for control
> traffic towards the remote traffic generator, then you surely cannot
> reliably check that counters that should not be incremented are indeed
> not incremented.

I both told you in this conversation how to check the counters,
and written some existing tests for counters.

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Ioana Ciornei]

On Mon, Mar 02, 2026 at 04:07:43PM -0800, Jakub Kicinski wrote:
> On Mon, 2 Mar 2026 14:11:21 +0200 Ioana Ciornei wrote:
> > > I agree that adherence to the drivers/net/README protocol is valuable to
> > > some users and would be good to uphold if reasonable in a given tests.
> > > If that's what you have in mind.
> > > 
> > > There are going to be tests where it's not a great fit, but I think that
> > > of those NUM_NETIFS=2 tests that we currently have, maybe
> > > ethtool_extended_state has a good reason to be obstinate, because it
> > > sets up negotiations and needs an extra unplugged netdevice.  
> > 
> > I would add here even ethtool_rmon.sh and this new test that I
> 
> I think I already told you that ethool_rmon predates the NIC tests
> and bringing it up in this discussion is irrelevant.
> 
> > submitted. If you are running with a traffic generator on another board
> > then you can no longer check that the counter's value is as expected
> > (with a 1% tolerance), you can only check the lower bound.
> 
> 1% tolerance is impractical for any CI with high test count.
> The test will be flaky. And I really doubt that the 1% tolerance
> is really necessary to catch most bugs. We're not trying to validate
> silicon here.
> 
> > Additionally, if you are using the same single port also for control
> > traffic towards the remote traffic generator, then you surely cannot
> > reliably check that counters that should not be incremented are indeed
> > not incremented.
> 
> I both told you in this conversation how to check the counters,
> and written some existing tests for counters.

Judging by your response it's clear to me that you wanted to transmit
something that didn't actually get to me. I am afraid that it's not
clear to me what exactly is your feedback and what do you expect as a
next step.

What I did get:
- The new test should work with a single netdevice (and a remote
  endpoint for traffic generation).
- The test should not check for any upper bound for the ethtool counter
  value.
- The test is expected to follow drivers/net/README.rst.

Does this mean that your feedback is to convert the bash variant that I
submitted into a python one which uses lib.py?

If this is your intention, what is the plan for the rmon statistics (or
any drivers/net/hw/ bash tests that pre-date the README)? Do you see
those eventually getting converted to lib.py? I am merely asking so that
I know if I should convert them or they are to be left as is.

If I got this all wrong, I'm sorry.

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Jakub Kicinski]

On Tue, 3 Mar 2026 15:53:41 +0200 Ioana Ciornei wrote:
> > > I would add here even ethtool_rmon.sh and this new test that I  
> > 
> > I think I already told you that ethool_rmon predates the NIC tests
> > and bringing it up in this discussion is irrelevant.
> >   
> > > submitted. If you are running with a traffic generator on another board
> > > then you can no longer check that the counter's value is as expected
> > > (with a 1% tolerance), you can only check the lower bound.  
> > 
> > 1% tolerance is impractical for any CI with high test count.
> > The test will be flaky. And I really doubt that the 1% tolerance
> > is really necessary to catch most bugs. We're not trying to validate
> > silicon here.
> >   
> > > Additionally, if you are using the same single port also for control
> > > traffic towards the remote traffic generator, then you surely cannot
> > > reliably check that counters that should not be incremented are indeed
> > > not incremented.  
> > 
> > I both told you in this conversation how to check the counters,
> > and written some existing tests for counters.  
> 
> Judging by your response it's clear to me that you wanted to transmit
> something that didn't actually get to me. I am afraid that it's not
> clear to me what exactly is your feedback and what do you expect as a
> next step.
> 
> What I did get:
> - The new test should work with a single netdevice (and a remote
>   endpoint for traffic generation).

yes

> - The test should not check for any upper bound for the ethtool counter
>   value.

more or less.. you can check for crazy values (bitflips etc).
Either pick a value too high to be reasonable (100Gbps * time)
or hardcode some high threshold (2^31?)

> - The test is expected to follow drivers/net/README.rst.

yes

> Does this mean that your feedback is to convert the bash variant that I
> submitted into a python one which uses lib.py?

It'd certainly be easier for you, but I'm not against building out 
the necessary support to run traffic from remote in bash.

> If this is your intention, what is the plan for the rmon statistics (or
> any drivers/net/hw/ bash tests that pre-date the README)? Do you see
> those eventually getting converted to lib.py? I am merely asking so that
> I know if I should convert them or they are to be left as is.

Yes, I was planning on doing the conversion once we have some real HW
testing going in NIPA. If you're willing to convert them - that'd be
great!

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Petr Machata]

Ioana Ciornei <ioana.ciornei@nxp.com> writes:

> Add a new selftest - ethtool_std_stats.sh - which validates the
> eth-ctrl, eth-mac and pause standard statistics exported by an
> interface. Not all counters can be validated in software, especially
> those that are keeping track of errors. Counters such as
> SingleCollisionFrames, FrameCheckSequenceErrors etc are not tested nor
> included in this new selftest.
>
> The central part of this patch is the traffic_test() function which
> gathers the 'before' counter values, sends a batch of traffic and then
> interrogates again the same counters in order to determine if the delta
> is on target. The function receives an array through which the caller
> can request what counters to be interrogated and, for each of them, what
> is their target delta value.
>
> The output from this selftest looks as follows on a LX2160ARDB board:
>
> $ ./ethtool_std_stats.sh eth0 eth1
> TEST: eth-ctrl tx on eth0 (MACControlFramesTransmitted on eth0)     [ OK ]
> TEST: eth-ctrl tx on eth0 (MACControlFramesReceived on eth1)        [ OK ]
> TEST: eth-ctrl tx on eth1 (MACControlFramesTransmitted on eth1)     [ OK ]
> TEST: eth-ctrl tx on eth1 (MACControlFramesReceived on eth0)        [ OK ]
> TEST: eth-mac bcast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac bcast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac bcast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac bcast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac bcast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac bcast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> TEST: eth-mac bcast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> TEST: eth-mac bcast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac mcast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac mcast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac mcast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac mcast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac mcast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac mcast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> TEST: eth-mac mcast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> TEST: eth-mac mcast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac ucast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac ucast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac ucast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac ucast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac ucast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac ucast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> TEST: eth-mac ucast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> TEST: eth-mac ucast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> TEST: pause tx on eth0 (tx_pause_frames on eth0)                    [ OK ]
> TEST: pause tx on eth0 (rx_pause_frames on eth1)                    [ OK ]
> TEST: pause tx on eth1 (tx_pause_frames on eth1)                    [ OK ]
> TEST: pause tx on eth1 (rx_pause_frames on eth0)                    [ OK ]
>
> Please note that not all MACs are counting the software injected pause
> frames as real Tx pause. For example, on a LS1028ARDB the selftest
> output will reflect the fact that neither the ENETC MAC, nor the Felix
> switch MAC are able to detect Tx pause frames injected by software.
>
> $ ./ethtool_std_stats.sh eno0 swp0
> (...)
> TEST: Not all MACs detect injected pause frames                     [XFAIL]
> TEST: pause tx on eno0 (rx_pause_frames on swp0)                    [ OK ]
> TEST: Not all MACs detect injected pause frames                     [XFAIL]
> TEST: pause tx on swp0 (rx_pause_frames on eno0)                    [ OK ]
>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  .../testing/selftests/drivers/net/hw/Makefile |   1 +
>  .../drivers/net/hw/ethtool_std_stats.sh       | 192 ++++++++++++++++++
>  2 files changed, 193 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
>
> diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> index a64140333a46..d447813a14b2 100644
> --- a/tools/testing/selftests/drivers/net/hw/Makefile
> +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> @@ -26,6 +26,7 @@ TEST_PROGS = \
>  	ethtool_extended_state.sh \
>  	ethtool_mm.sh \
>  	ethtool_rmon.sh \
> +	ethtool_std_stats.sh \
>  	hw_stats_l3.sh \
>  	hw_stats_l3_gre.sh \
>  	iou-zcrx.py \
> diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh b/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
> new file mode 100755
> index 000000000000..4ce8f140a18c
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
> @@ -0,0 +1,192 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#shellcheck disable=SC2034 # SC does not see the global variables
> +
> +ALL_TESTS="
> +	test_eth_ctrl_stats
> +	test_eth_mac_stats
> +	test_pause_stats
> +"
> +STABLE_MAC_ADDRS=yes
> +NUM_NETIFS=2
> +lib_dir=$(dirname "$0")
> +# shellcheck source=./../../../net/forwarding/lib.sh
> +source "$lib_dir"/../../../net/forwarding/lib.sh
> +
> +traffic_test()
> +{
> +	local iface=$1; shift
> +	local neigh=$1; shift
> +	local num_tx=$1; shift
> +	local pkt_format="$1"; shift
> +	local title="$1"; shift
> +	declare -a counters=( "${@:2:$1}" ); shift "$(( $1 + 1 ))"

Please make this local -a. declare inside a function also introduces a
local variable, so the effect is the same, but using local is more
obvious.

BTW, just a suggestion, personally I'd just make the counters a pure
"rest" argument:

	local -a counters=("$@")

simply because functions usually don't need more than one, and it's
easier to use on call site, and easier to understand what's going on.

> +	local before after delta target_high extra
> +	local int grp counter target unit
> +	local num_rx=$((num_tx * 2))
> +	local xfail_message
> +	local src="aggregate"

And local i.

> +
> +	for i in "${!counters[@]}"; do
> +		read -r int grp counter target unit xfail_message <<< "${counters[$i]}"
> +		before[i]=$(ethtool_std_stats_get "$int" "$grp" "$counter" "$src")
> +	done
> +
> +	# shellcheck disable=SC2086 # needs split options
> +	$MZ "$iface" -q -c "$num_tx" $pkt_format
> +
> +	# shellcheck disable=SC2086 # needs split options
> +	$MZ "$neigh" -q -c "$num_rx" $pkt_format
> +
> +	for i in "${!counters[@]}"; do

There should be a RET=0 here, so that the check_err below gets a clean slate.

> +		read -r int grp counter target unit xfail_message<<< "${counters[$i]}"
> +
> +		after[i]=$(ethtool_std_stats_get "$int" "$grp" "$counter" "$src")
> +		if [[ "${after[$i]}" == "null" ]]; then
> +			log_test_skip "$int does not support $grp-$counter"
> +			continue;
> +		fi
> +
> +		delta=$((after[i] - before[i]))
> +
> +		# Allow an extra 1% tolerance for random packets sent by the stack
> +		extra=$((num_pkts * unit / 100))
> +		target_high=$((target + extra))
> +
> +		RET=0
> +		[ "$delta" -ge "$target" ] && [ "$delta" -le "$target_high" ]
> +		err="$?"
> +		if [[ $err != 0  ]] && [[ -n $xfail_message ]]; then
> +			log_test_xfail "$xfail_message"

I think this should mention the $title as well. I think that the
xfail_message could be the log_test's opt_str, so:

                        log_test_xfail "$title" "$xfail_message"

A bit annoying that log_test_xfail clears the retmsg. It does so so that
messages from previous runs do not show up, which is desirable in
general, but here it would be handy.

> +			continue;
> +		fi
> +		check_err "$err" "$grp-$counter is not valid on $int (expected $target, got $delta)"
> +		log_test "$title" "$counter on $int"
> +	done
> +}
> +
> +test_eth_ctrl_stats()
> +{
> +	local pkt_format="-a own -b bcast 88:08 -p 64"
> +	local num_pkts=1000
> +	local counters

local -a

(Though yeah, bash doesn't care.)

> +
> +	counters=("$h1 eth-ctrl MACControlFramesTransmitted $num_pkts 1"
> +		  "$h2 eth-ctrl MACControlFramesReceived $num_pkts 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$pkt_format" \
> +		"eth-ctrl tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +
> +	counters=("$h2 eth-ctrl MACControlFramesTransmitted $num_pkts 1"
> +		  "$h1 eth-ctrl MACControlFramesReceived $num_pkts 1")
> +	traffic_test "$h2" "$h1" "$num_pkts" "$pkt_format" \
> +		"eth-ctrl tx on $h2" \
> +		"${#counters[@]}" "${counters[@]}"
> +}
> +
> +test_eth_mac_stats()
> +{
> +	local pkt_size=100
> +	local pkt_size_fcs=$((pkt_size + 4))
> +	local bcast_pkt_format="-a own -b bcast -p $pkt_size"
> +	local mcast_pkt_format="-a own -b -b 01:00:5E:00:00:01 -p $pkt_size"
> +	local ucast_pkt_format="-a own -b $h2_mac -p $pkt_size"
> +	local num_pkts=2000
> +	local octets=$((pkt_size_fcs * num_pkts))
> +	local counters

local -a

> +
> +	counters=("$h1 eth-mac BroadcastFramesXmittedOK $num_pkts 1"
> +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> +		  "$h1 eth-mac MulticastFramesXmittedOK 0 1"
> +		  "$h2 eth-mac BroadcastFramesReceivedOK $num_pkts 1"
> +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> +		  "$h2 eth-mac MulticastFramesReceivedOK 0 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$bcast_pkt_format" \
> +		"eth-mac bcast tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +
> +	counters=("$h1 eth-mac BroadcastFramesXmittedOK 0 1"
> +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> +		  "$h1 eth-mac MulticastFramesXmittedOK $num_pkts 1"
> +		  "$h2 eth-mac BroadcastFramesReceivedOK 0 1"
> +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> +		  "$h2 eth-mac MulticastFramesReceivedOK $num_pkts 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$mcast_pkt_format" \
> +		"eth-mac mcast tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +
> +	counters=("$h1 eth-mac BroadcastFramesXmittedOK 0 1"
> +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> +		  "$h1 eth-mac MulticastFramesXmittedOK 0 1"
> +		  "$h2 eth-mac BroadcastFramesReceivedOK 0 1"
> +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> +		  "$h2 eth-mac MulticastFramesReceivedOK 0 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$ucast_pkt_format" \
> +		"eth-mac ucast tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +}
> +
> +test_pause_stats()
> +{
> +	local pkt_format="-a own -b 01:80:c2:00:00:01 88:08:00:01:00:01"
> +	local xfail_message="Not all MACs detect injected pause frames"
> +	local num_pkts=2000
> +	local counters i

counters should be declared local -a

> +
> +	# Check that there is pause frame support
> +	for ((i = 1; i <= NUM_NETIFS; ++i)); do
> +		if ! ethtool -I --json -a "${NETIFS[p$i]}" > /dev/null 2>&1; then
> +			log_test_skip "No support for pause frames, skip tests"
> +			exit
> +		fi
> +	done
> +
> +	counters=("$h1 pause tx_pause_frames $num_pkts 1 $xfail_message"
> +		  "$h2 pause rx_pause_frames $num_pkts 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$pkt_format" \
> +		"pause tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +
> +	counters=("$h2 pause tx_pause_frames $num_pkts 1 $xfail_message"
> +		  "$h1 pause rx_pause_frames $num_pkts 1")
> +	traffic_test "$h2" "$h1" "$num_pkts" "$pkt_format" \
> +		"pause tx on $h2" \
> +		"${#counters[@]}" "${counters[@]}"
> +}
> +
> +setup_prepare()
> +{
> +	h1=${NETIFS[p1]}
> +	h2=${NETIFS[p2]}
> +
> +	h2_mac=$(mac_get "$h2")
> +
> +	for iface in $h1 $h2; do

These should be quoted.
iface should be local.

> +		ip link set dev "$iface" up

Plesae use defer:

		ip link set dev "$iface" up
		defer ip link set dev "$iface" down

Then drop the hand-rolled cleanup() altogether. Retain the "trap cleanup
EXIT" -- it will pick up forwarding/lib.sh's cleanup(), which calls
pre_cleanup and defer_scopes_cleanup().

> +	done
> +}
> +
> +cleanup()
> +{
> +	pre_cleanup
> +
> +	for iface in $h2 $h1; do
> +		ip link set dev "$iface" down
> +	done
> +}
> +
> +check_ethtool_counter_group_support
> +trap cleanup EXIT
> +
> +setup_prepare
> +setup_wait
> +
> +tests_run
> +
> +exit "$EXIT_STATUS"

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Ioana Ciornei]

On Fri, Feb 27, 2026 at 04:45:47PM +0100, Petr Machata wrote:
> 
> Ioana Ciornei <ioana.ciornei@nxp.com> writes:
> 
> > Add a new selftest - ethtool_std_stats.sh - which validates the
> > eth-ctrl, eth-mac and pause standard statistics exported by an
> > interface. Not all counters can be validated in software, especially
> > those that are keeping track of errors. Counters such as
> > SingleCollisionFrames, FrameCheckSequenceErrors etc are not tested nor
> > included in this new selftest.
> >
> > The central part of this patch is the traffic_test() function which
> > gathers the 'before' counter values, sends a batch of traffic and then
> > interrogates again the same counters in order to determine if the delta
> > is on target. The function receives an array through which the caller
> > can request what counters to be interrogated and, for each of them, what
> > is their target delta value.
> >
> > The output from this selftest looks as follows on a LX2160ARDB board:
> >
> > $ ./ethtool_std_stats.sh eth0 eth1
> > TEST: eth-ctrl tx on eth0 (MACControlFramesTransmitted on eth0)     [ OK ]
> > TEST: eth-ctrl tx on eth0 (MACControlFramesReceived on eth1)        [ OK ]
> > TEST: eth-ctrl tx on eth1 (MACControlFramesTransmitted on eth1)     [ OK ]
> > TEST: eth-ctrl tx on eth1 (MACControlFramesReceived on eth0)        [ OK ]
> > TEST: eth-mac bcast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> > TEST: eth-mac bcast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> > TEST: eth-mac bcast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> > TEST: eth-mac bcast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> > TEST: eth-mac bcast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> > TEST: eth-mac bcast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> > TEST: eth-mac bcast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> > TEST: eth-mac bcast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> > TEST: eth-mac mcast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> > TEST: eth-mac mcast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> > TEST: eth-mac mcast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> > TEST: eth-mac mcast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> > TEST: eth-mac mcast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> > TEST: eth-mac mcast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> > TEST: eth-mac mcast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> > TEST: eth-mac mcast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> > TEST: eth-mac ucast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> > TEST: eth-mac ucast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> > TEST: eth-mac ucast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> > TEST: eth-mac ucast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> > TEST: eth-mac ucast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> > TEST: eth-mac ucast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> > TEST: eth-mac ucast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> > TEST: eth-mac ucast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> > TEST: pause tx on eth0 (tx_pause_frames on eth0)                    [ OK ]
> > TEST: pause tx on eth0 (rx_pause_frames on eth1)                    [ OK ]
> > TEST: pause tx on eth1 (tx_pause_frames on eth1)                    [ OK ]
> > TEST: pause tx on eth1 (rx_pause_frames on eth0)                    [ OK ]
> >
> > Please note that not all MACs are counting the software injected pause
> > frames as real Tx pause. For example, on a LS1028ARDB the selftest
> > output will reflect the fact that neither the ENETC MAC, nor the Felix
> > switch MAC are able to detect Tx pause frames injected by software.
> >
> > $ ./ethtool_std_stats.sh eno0 swp0
> > (...)
> > TEST: Not all MACs detect injected pause frames                     [XFAIL]
> > TEST: pause tx on eno0 (rx_pause_frames on swp0)                    [ OK ]
> > TEST: Not all MACs detect injected pause frames                     [XFAIL]
> > TEST: pause tx on swp0 (rx_pause_frames on eno0)                    [ OK ]
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  .../testing/selftests/drivers/net/hw/Makefile |   1 +
> >  .../drivers/net/hw/ethtool_std_stats.sh       | 192 ++++++++++++++++++
> >  2 files changed, 193 insertions(+)
> >  create mode 100755 tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
> >
> > diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> > index a64140333a46..d447813a14b2 100644
> > --- a/tools/testing/selftests/drivers/net/hw/Makefile
> > +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> > @@ -26,6 +26,7 @@ TEST_PROGS = \
> >  	ethtool_extended_state.sh \
> >  	ethtool_mm.sh \
> >  	ethtool_rmon.sh \
> > +	ethtool_std_stats.sh \
> >  	hw_stats_l3.sh \
> >  	hw_stats_l3_gre.sh \
> >  	iou-zcrx.py \
> > diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh b/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
> > new file mode 100755
> > index 000000000000..4ce8f140a18c
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
> > @@ -0,0 +1,192 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +#shellcheck disable=SC2034 # SC does not see the global variables
> > +
> > +ALL_TESTS="
> > +	test_eth_ctrl_stats
> > +	test_eth_mac_stats
> > +	test_pause_stats
> > +"
> > +STABLE_MAC_ADDRS=yes
> > +NUM_NETIFS=2
> > +lib_dir=$(dirname "$0")
> > +# shellcheck source=./../../../net/forwarding/lib.sh
> > +source "$lib_dir"/../../../net/forwarding/lib.sh
> > +
> > +traffic_test()
> > +{
> > +	local iface=$1; shift
> > +	local neigh=$1; shift
> > +	local num_tx=$1; shift
> > +	local pkt_format="$1"; shift
> > +	local title="$1"; shift
> > +	declare -a counters=( "${@:2:$1}" ); shift "$(( $1 + 1 ))"
> 
> Please make this local -a. declare inside a function also introduces a
> local variable, so the effect is the same, but using local is more
> obvious.

Ok.

> 
> BTW, just a suggestion, personally I'd just make the counters a pure
> "rest" argument:
> 
> 	local -a counters=("$@")
> 
> simply because functions usually don't need more than one, and it's
> easier to use on call site, and easier to understand what's going on.

Why I had this overly complicated retrieval of the array is because I
started out with multiple arrays and then restructured but forgot about
this. Will change to just retrieve the "rest".

> > +	local before after delta target_high extra
> > +	local int grp counter target unit
> > +	local num_rx=$((num_tx * 2))
> > +	local xfail_message
> > +	local src="aggregate"
> 
> And local i.

Sure.

> 
> > +
> > +	for i in "${!counters[@]}"; do
> > +		read -r int grp counter target unit xfail_message <<< "${counters[$i]}"
> > +		before[i]=$(ethtool_std_stats_get "$int" "$grp" "$counter" "$src")
> > +	done
> > +
> > +	# shellcheck disable=SC2086 # needs split options
> > +	$MZ "$iface" -q -c "$num_tx" $pkt_format
> > +
> > +	# shellcheck disable=SC2086 # needs split options
> > +	$MZ "$neigh" -q -c "$num_rx" $pkt_format
> > +
> > +	for i in "${!counters[@]}"; do
> 
> There should be a RET=0 here, so that the check_err below gets a clean slate.

You want me to move the RET=0 from some lines below to here, right?

> 
> > +		read -r int grp counter target unit xfail_message<<< "${counters[$i]}"
> > +
> > +		after[i]=$(ethtool_std_stats_get "$int" "$grp" "$counter" "$src")
> > +		if [[ "${after[$i]}" == "null" ]]; then
> > +			log_test_skip "$int does not support $grp-$counter"
> > +			continue;
> > +		fi
> > +
> > +		delta=$((after[i] - before[i]))
> > +
> > +		# Allow an extra 1% tolerance for random packets sent by the stack
> > +		extra=$((num_pkts * unit / 100))
> > +		target_high=$((target + extra))
> > +
> > +		RET=0
> > +		[ "$delta" -ge "$target" ] && [ "$delta" -le "$target_high" ]
> > +		err="$?"
> > +		if [[ $err != 0  ]] && [[ -n $xfail_message ]]; then
> > +			log_test_xfail "$xfail_message"
> 
> I think this should mention the $title as well. I think that the
> xfail_message could be the log_test's opt_str, so:
> 
>                         log_test_xfail "$title" "$xfail_message"

Good point.

> 
> A bit annoying that log_test_xfail clears the retmsg. It does so so that
> messages from previous runs do not show up, which is desirable in
> general, but here it would be handy.
> 
> > +			continue;
> > +		fi
> > +		check_err "$err" "$grp-$counter is not valid on $int (expected $target, got $delta)"
> > +		log_test "$title" "$counter on $int"
> > +	done
> > +}
> > +
> > +test_eth_ctrl_stats()
> > +{
> > +	local pkt_format="-a own -b bcast 88:08 -p 64"
> > +	local num_pkts=1000
> > +	local counters
> 
> local -a
> 
> (Though yeah, bash doesn't care.)

Ok.

> 
> > +
> > +	counters=("$h1 eth-ctrl MACControlFramesTransmitted $num_pkts 1"
> > +		  "$h2 eth-ctrl MACControlFramesReceived $num_pkts 1")
> > +	traffic_test "$h1" "$h2" "$num_pkts" "$pkt_format" \
> > +		"eth-ctrl tx on $h1" \
> > +		"${#counters[@]}" "${counters[@]}"
> > +
> > +	counters=("$h2 eth-ctrl MACControlFramesTransmitted $num_pkts 1"
> > +		  "$h1 eth-ctrl MACControlFramesReceived $num_pkts 1")
> > +	traffic_test "$h2" "$h1" "$num_pkts" "$pkt_format" \
> > +		"eth-ctrl tx on $h2" \
> > +		"${#counters[@]}" "${counters[@]}"
> > +}
> > +
> > +test_eth_mac_stats()
> > +{
> > +	local pkt_size=100
> > +	local pkt_size_fcs=$((pkt_size + 4))
> > +	local bcast_pkt_format="-a own -b bcast -p $pkt_size"
> > +	local mcast_pkt_format="-a own -b -b 01:00:5E:00:00:01 -p $pkt_size"
> > +	local ucast_pkt_format="-a own -b $h2_mac -p $pkt_size"
> > +	local num_pkts=2000
> > +	local octets=$((pkt_size_fcs * num_pkts))
> > +	local counters
> 
> local -a

Ok.

> 
> > +
> > +	counters=("$h1 eth-mac BroadcastFramesXmittedOK $num_pkts 1"
> > +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> > +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> > +		  "$h1 eth-mac MulticastFramesXmittedOK 0 1"
> > +		  "$h2 eth-mac BroadcastFramesReceivedOK $num_pkts 1"
> > +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> > +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> > +		  "$h2 eth-mac MulticastFramesReceivedOK 0 1")
> > +	traffic_test "$h1" "$h2" "$num_pkts" "$bcast_pkt_format" \
> > +		"eth-mac bcast tx on $h1" \
> > +		"${#counters[@]}" "${counters[@]}"
> > +
> > +	counters=("$h1 eth-mac BroadcastFramesXmittedOK 0 1"
> > +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> > +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> > +		  "$h1 eth-mac MulticastFramesXmittedOK $num_pkts 1"
> > +		  "$h2 eth-mac BroadcastFramesReceivedOK 0 1"
> > +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> > +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> > +		  "$h2 eth-mac MulticastFramesReceivedOK $num_pkts 1")
> > +	traffic_test "$h1" "$h2" "$num_pkts" "$mcast_pkt_format" \
> > +		"eth-mac mcast tx on $h1" \
> > +		"${#counters[@]}" "${counters[@]}"
> > +
> > +	counters=("$h1 eth-mac BroadcastFramesXmittedOK 0 1"
> > +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> > +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> > +		  "$h1 eth-mac MulticastFramesXmittedOK 0 1"
> > +		  "$h2 eth-mac BroadcastFramesReceivedOK 0 1"
> > +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> > +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> > +		  "$h2 eth-mac MulticastFramesReceivedOK 0 1")
> > +	traffic_test "$h1" "$h2" "$num_pkts" "$ucast_pkt_format" \
> > +		"eth-mac ucast tx on $h1" \
> > +		"${#counters[@]}" "${counters[@]}"
> > +}
> > +
> > +test_pause_stats()
> > +{
> > +	local pkt_format="-a own -b 01:80:c2:00:00:01 88:08:00:01:00:01"
> > +	local xfail_message="Not all MACs detect injected pause frames"
> > +	local num_pkts=2000
> > +	local counters i
> 
> counters should be declared local -a

Ok.

> 
> > +
> > +	# Check that there is pause frame support
> > +	for ((i = 1; i <= NUM_NETIFS; ++i)); do
> > +		if ! ethtool -I --json -a "${NETIFS[p$i]}" > /dev/null 2>&1; then
> > +			log_test_skip "No support for pause frames, skip tests"
> > +			exit
> > +		fi
> > +	done
> > +
> > +	counters=("$h1 pause tx_pause_frames $num_pkts 1 $xfail_message"
> > +		  "$h2 pause rx_pause_frames $num_pkts 1")
> > +	traffic_test "$h1" "$h2" "$num_pkts" "$pkt_format" \
> > +		"pause tx on $h1" \
> > +		"${#counters[@]}" "${counters[@]}"
> > +
> > +	counters=("$h2 pause tx_pause_frames $num_pkts 1 $xfail_message"
> > +		  "$h1 pause rx_pause_frames $num_pkts 1")
> > +	traffic_test "$h2" "$h1" "$num_pkts" "$pkt_format" \
> > +		"pause tx on $h2" \
> > +		"${#counters[@]}" "${counters[@]}"
> > +}
> > +
> > +setup_prepare()
> > +{
> > +	h1=${NETIFS[p1]}
> > +	h2=${NETIFS[p2]}
> > +
> > +	h2_mac=$(mac_get "$h2")
> > +
> > +	for iface in $h1 $h2; do
> 
> These should be quoted.
> iface should be local.

Ok.

> > +		ip link set dev "$iface" up
> 
> Plesae use defer:
> 
> 		ip link set dev "$iface" up
> 		defer ip link set dev "$iface" down
> 
> Then drop the hand-rolled cleanup() altogether. Retain the "trap cleanup
> EXIT" -- it will pick up forwarding/lib.sh's cleanup(), which calls
> pre_cleanup and defer_scopes_cleanup().

Sure! Thanks.

Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Petr Machata]

Ioana Ciornei <ioana.ciornei@nxp.com> writes:

> On Fri, Feb 27, 2026 at 04:45:47PM +0100, Petr Machata wrote:
>> 
>> Ioana Ciornei <ioana.ciornei@nxp.com> writes:
>> 
>> > +	for i in "${!counters[@]}"; do
>> 
>> There should be a RET=0 here, so that the check_err below gets a clean slate.
>
> You want me to move the RET=0 from some lines below to here, right?

My bad, I missed that there is a RET=0 below. Keep it as it is.