[PATCH net-next v8 0/2] Add Frame Preemption MAC Merge support for ICSSG

[PATCH net-next v8 0/2] Add Frame Preemption MAC Merge support for ICSSG

[Meghana Malladi]

This patch series adds QoS support to the ICSSG PRUETH driver.
The first patch implements mqprio qdisc handling and TC offload hooks
so userspace can request TC mappings and queue counts.

It also integrates a driver-side mechanism to program the firmware
with the IET/FPE preemption mask and to kick the firmware verify state
machine when frame preemption is enabled. The second patch adds ethtool
perations for the MAC Merge (Frame Preemption) sublayer, exposing .get_mm,
.set_mm and .get_mm_stats so admins can view and change MAC Merge
parameters and retrieve preemption statistics.

v7: https://lore.kernel.org/all/20260610052511.781752-1-m-malladi@ti.com/

MD Danish Anwar (2):
  net: ti: icssg-prueth: Add Frame Preemption MAC Merge support
  net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

 drivers/net/ethernet/ti/Makefile              |   3 +-
 drivers/net/ethernet/ti/icssg/icssg_common.c  |   1 +
 drivers/net/ethernet/ti/icssg/icssg_config.h  |   9 -
 drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 132 +++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.c  |  10 +-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  21 +-
 drivers/net/ethernet/ti/icssg/icssg_qos.c     | 305 ++++++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_qos.h     |  68 ++++
 drivers/net/ethernet/ti/icssg/icssg_stats.c   |   4 +-
 drivers/net/ethernet/ti/icssg/icssg_stats.h   |  84 ++---
 .../net/ethernet/ti/icssg/icssg_switch_map.h  |   5 +
 11 files changed, 580 insertions(+), 62 deletions(-)
 create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.c
 create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.h


base-commit: 272a65db243bfa34b9277632830e0e06d7e3518e
-- 
2.43.0

[PATCH net-next v8 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[Meghana Malladi]

From: MD Danish Anwar <danishanwar@ti.com>

Introduce QoS infrastructure for Frame Preemption (FPE) support in
the ICSSG Ethernet driver.

prueth_qos_iet tracks FPE enable/active state and verify state
machine status via firmware-reported enum icssg_ietfpe_verify_states.
icssg_config_ietfpe() configures IET FPE in firmware, triggers
verify state machine based on ethtool MAC Merge parameters.
Polls firmware verify status up to 3 times with verify_time_ms
intervals and driver handles timeout by logging error and returning.
In case of any failure during configuration for enable/disable,
IET FPE falls back to disabled state.

For MQPRIO qdisc support all queues are express by default later
gets override by user-provided preemptible_tcs bitmask via tc
qdisc mask. Preempt mask configuration: Maps traffic classes to
queue express/preemptible state and applied only when FPE is
active (Tx enabled).

Verify state machine re-triggers on link up/down events based on
fpe_enabled and fpe_active flags, and for memory protection,
fpe_lock serializes all FPE state mutations, preventing races
between ethtool config, qdisc setup, and link events

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---

v8-v7:
- icssg_iet_set_preempt_mask(): Reset all Q_MAP entries before
  applying new TC mapping when fpe_active is true to clear stale
  preemptible entries from a prior wider TC mapping
- icssg_iet_change_preemptible_tcs(): Skip firmware command when
  FPE is not configured to avoid unnecessary PREMPT_TX_DISABLE
  on every link-up event
- icssg_qos_init(): Remove unnecessary mutex lock/unlock since
  no concurrent accessor exists before register_netdev()
  All the above changes address the sashiko review comments

 drivers/net/ethernet/ti/Makefile             |   3 +-
 drivers/net/ethernet/ti/icssg/icssg_common.c |   1 +
 drivers/net/ethernet/ti/icssg/icssg_config.h |   9 -
 drivers/net/ethernet/ti/icssg/icssg_prueth.c |  10 +-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |   8 +-
 drivers/net/ethernet/ti/icssg/icssg_qos.c    | 305 +++++++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_qos.h    |  68 +++++
 7 files changed, 389 insertions(+), 15 deletions(-)
 create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.c
 create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.h

diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
index f4276c9a77620..d19bcd25c9d07 100644
--- a/drivers/net/ethernet/ti/Makefile
+++ b/drivers/net/ethernet/ti/Makefile
@@ -46,6 +46,7 @@ icssg-y := icssg/icssg_common.o \
 	   icssg/icssg_config.o \
 	   icssg/icssg_mii_cfg.o \
 	   icssg/icssg_stats.o \
-	   icssg/icssg_ethtool.o
+	   icssg/icssg_ethtool.o \
+	   icssg/icssg_qos.o
 
 obj-$(CONFIG_TI_ICSS_IEP) += icssg/icss_iep.o
diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index 4a7d1a6f470b1..d59eef61e7012 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -1754,6 +1754,7 @@ void prueth_netdev_exit(struct prueth *prueth,
 
 	netif_napi_del(&emac->napi_rx);
 
+	mutex_destroy(&emac->qos.iet.fpe_lock);
 	pruss_release_mem_region(prueth->pruss, &emac->dram);
 	free_netdev(emac->ndev);
 	prueth->emac[mac] = NULL;
diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
index 60d69744ffae2..1ac202f855ed4 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
@@ -323,13 +323,4 @@ struct prueth_fdb_slot {
 	u8 fid;
 	u8 fid_c2;
 } __packed;
-
-enum icssg_ietfpe_verify_states {
-	ICSSG_IETFPE_STATE_UNKNOWN = 0,
-	ICSSG_IETFPE_STATE_INITIAL,
-	ICSSG_IETFPE_STATE_VERIFYING,
-	ICSSG_IETFPE_STATE_SUCCEEDED,
-	ICSSG_IETFPE_STATE_FAILED,
-	ICSSG_IETFPE_STATE_DISABLED
-};
 #endif /* __NET_TI_ICSSG_CONFIG_H */
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 591be5c8056b4..283be27a240e9 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -357,11 +357,11 @@ static void emac_adjust_link(struct net_device *ndev)
 		}
 		if (!emac->link) {
 			new_state = true;
-			emac->link = 1;
+			WRITE_ONCE(emac->link, 1);
 		}
 	} else if (emac->link) {
 		new_state = true;
-		emac->link = 0;
+		WRITE_ONCE(emac->link, 0);
 
 		/* f/w should support 100 & 1000 */
 		emac->speed = SPEED_1000;
@@ -392,6 +392,8 @@ static void emac_adjust_link(struct net_device *ndev)
 		} else {
 			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
 		}
+
+		icssg_qos_link_state_update(ndev);
 	}
 
 	if (emac->link) {
@@ -1652,6 +1654,7 @@ static const struct net_device_ops emac_netdev_ops = {
 	.ndo_hwtstamp_get = icssg_ndo_get_ts_config,
 	.ndo_hwtstamp_set = icssg_ndo_set_ts_config,
 	.ndo_xsk_wakeup = prueth_xsk_wakeup,
+	.ndo_setup_tc = icssg_qos_ndo_setup_tc,
 };
 
 static int prueth_netdev_init(struct prueth *prueth,
@@ -1686,6 +1689,8 @@ static int prueth_netdev_init(struct prueth *prueth,
 
 	INIT_DELAYED_WORK(&emac->stats_work, icssg_stats_work_handler);
 
+	icssg_qos_init(ndev);
+
 	ret = pruss_request_mem_region(prueth->pruss,
 				       port == PRUETH_PORT_MII0 ?
 				       PRUSS_MEM_DRAM0 : PRUSS_MEM_DRAM1,
@@ -1793,6 +1798,7 @@ static int prueth_netdev_init(struct prueth *prueth,
 free:
 	pruss_release_mem_region(prueth->pruss, &emac->dram);
 free_ndev:
+	mutex_destroy(&emac->qos.iet.fpe_lock);
 	emac->ndev = NULL;
 	prueth->emac[mac] = NULL;
 	free_netdev(ndev);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index df93d15c5b786..f73b8f5fca956 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -44,10 +44,11 @@
 #include "icssg_config.h"
 #include "icss_iep.h"
 #include "icssg_switch_map.h"
+#include "icssg_qos.h"
 
-#define PRUETH_MAX_MTU          (2000 - ETH_HLEN - ETH_FCS_LEN)
-#define PRUETH_MIN_PKT_SIZE     (VLAN_ETH_ZLEN)
-#define PRUETH_MAX_PKT_SIZE     (PRUETH_MAX_MTU + ETH_HLEN + ETH_FCS_LEN)
+#define PRUETH_MAX_MTU		(2000 - ETH_HLEN - ETH_FCS_LEN)
+#define PRUETH_MIN_PKT_SIZE	(VLAN_ETH_ZLEN)
+#define PRUETH_MAX_PKT_SIZE	(PRUETH_MAX_MTU + ETH_HLEN + ETH_FCS_LEN)
 
 #define ICSS_SLICE0	0
 #define ICSS_SLICE1	1
@@ -254,6 +255,7 @@ struct prueth_emac {
 	struct bpf_prog *xdp_prog;
 	struct xdp_attachment_info xdpi;
 	int xsk_qid;
+	struct prueth_qos qos;
 };
 
 /* The buf includes headroom compatible with both skb and xdpf */
diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.c b/drivers/net/ethernet/ti/icssg/icssg_qos.c
new file mode 100644
index 0000000000000..27a953d981538
--- /dev/null
+++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Texas Instruments ICSSG PRUETH QoS submodule
+ * Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#include "icssg_prueth.h"
+#include "icssg_switch_map.h"
+
+static void icssg_iet_set_preempt_mask(struct prueth_emac *emac)
+{
+	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
+	struct prueth_qos_mqprio *p_mqprio = &emac->qos.mqprio;
+	struct tc_mqprio_qopt *qopt = &p_mqprio->qopt;
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	int prempt_mask = 0, i;
+	u8 tc, num_tc;
+
+	if (!iet->preemptible_tcs)
+		goto reset_hw;
+
+	if (iet->fpe_active) {
+		/* Reset all Q_MAP entries first to clear any stale preemptible
+		 * entries from a prior wider TC mapping.
+		 */
+		for (i = 0; i < ICSSG_MAX_TC_QUEUES; i++)
+			writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
+
+		/* Configure queues for user requested preemptible tc map */
+		num_tc = p_mqprio->qopt.num_tc;
+		for (tc = 0; tc < num_tc; tc++) {
+			/* check if the tc is preemptive or not */
+			if (iet->preemptible_tcs & BIT(tc)) {
+				/* Set the queues as preemptive queues */
+				for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++) {
+					writeb(BIT(4),
+					       config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
+				}
+			} else {
+				/* Accumulate express queue bits for the mask */
+				for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++)
+					prempt_mask |= BIT(i);
+			}
+		}
+		writeb(prempt_mask, config + EXPRESS_PRE_EMPTIVE_Q_MASK);
+		return;
+	}
+
+reset_hw:
+	/* Reset to default: all queues as express */
+	for (i = 0; i < ICSSG_MAX_TC_QUEUES; i++)
+		writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
+	writeb(ICSSG_EXPRESS_Q_MASK_ALL, config + EXPRESS_PRE_EMPTIVE_Q_MASK);
+}
+
+static int icssg_iet_verify_wait(struct prueth_emac *emac)
+{
+	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	unsigned long delay_us, timeout_us;
+	u32 status;
+	int ret;
+
+	delay_us = iet->verify_time_ms * 1000;
+	timeout_us = delay_us * ICSSG_IET_VERIFY_ATTEMPTS;
+
+	ret = readb_poll_timeout(config + PRE_EMPTION_VERIFY_STATUS,
+				 status,
+				 status == ICSSG_IETFPE_STATE_SUCCEEDED ||
+				 status == ICSSG_IETFPE_STATE_FAILED,
+				 delay_us,
+				 timeout_us);
+
+	iet->verify_status = status;
+	if (!ret && status == ICSSG_IETFPE_STATE_FAILED)
+		return -EIO;
+	return ret;
+}
+
+/* Direct synchronous configuration of IET FPE.
+ * Caller must hold iet->fpe_lock.
+ */
+int icssg_config_ietfpe(struct net_device *ndev, bool enable)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	int ret;
+	u8 val;
+
+	lockdep_assert_held(&iet->fpe_lock);
+
+	if (!READ_ONCE(emac->link)) {
+		netdev_dbg(ndev, "cannot change IET/FPE state when interface is down\n");
+		return 0;
+	}
+
+	/* Update FPE Tx enable bit (PRE_EMPTION_ENABLE_TX) if
+	 * fpe_enabled is set to enable MM in Tx direction
+	 */
+	writeb(enable ? 1 : 0, config + PRE_EMPTION_ENABLE_TX);
+	writew(iet->tx_min_frag_size + ETH_FCS_LEN,
+	       config + PRE_EMPTION_ADD_FRAG_SIZE_LOCAL);
+
+	/* If FPE is to be enabled, first configure MAC Verify state
+	 * machine in firmware as firmware kicks the Verify process
+	 * as soon as ICSSG_EMAC_PORT_PREMPT_TX_ENABLE command is
+	 * received.
+	 */
+	if (enable && iet->mac_verify_configure) {
+		writeb(1, config + PRE_EMPTION_ENABLE_VERIFY);
+		writel(iet->verify_time_ms, config + PRE_EMPTION_VERIFY_TIME);
+	} else {
+		writeb(0, config + PRE_EMPTION_ENABLE_VERIFY);
+		iet->verify_status = ICSSG_IETFPE_STATE_DISABLED;
+	}
+
+	/* Send command to enable FPE Tx side. Rx is always enabled */
+	ret = icssg_set_port_state(emac,
+				   enable ? ICSSG_EMAC_PORT_PREMPT_TX_ENABLE :
+					    ICSSG_EMAC_PORT_PREMPT_TX_DISABLE);
+	if (ret) {
+		netdev_err(ndev, "TX preempt %s command failed\n",
+			   str_enable_disable(enable));
+		goto fallback;
+	}
+
+	if (enable && iet->mac_verify_configure) {
+		ret = icssg_iet_verify_wait(emac);
+		if (ret) {
+			netdev_err(ndev, "MAC Merge verification failed: %s\n",
+				   iet->verify_status == ICSSG_IETFPE_STATE_FAILED ?
+				   "link partner rejected" : "timeout");
+			goto disable_tx;
+		}
+	} else if (enable) {
+		/* Give firmware some time to update
+		 * PRE_EMPTION_ACTIVE_TX state
+		 */
+		usleep_range(100, 200);
+	}
+
+	if (enable) {
+		val = readb(config + PRE_EMPTION_ACTIVE_TX);
+		if (val != 1) {
+			netdev_err(ndev,
+				   "Firmware fails to activate IET/FPE\n");
+			ret = -EIO;
+			goto disable_tx;
+		}
+		iet->fpe_active = true;
+	} else {
+		iet->fpe_active = false;
+	}
+
+	icssg_iet_set_preempt_mask(emac);
+	netdev_dbg(ndev, "IET FPE %s successfully\n",
+		   str_enable_disable(enable));
+	return 0;
+
+disable_tx:
+	icssg_set_port_state(emac, ICSSG_EMAC_PORT_PREMPT_TX_DISABLE);
+fallback:
+	writeb(0, config + PRE_EMPTION_ENABLE_TX);
+	writeb(0, config + PRE_EMPTION_ENABLE_VERIFY);
+	iet->verify_status = ICSSG_IETFPE_STATE_DISABLED;
+	iet->fpe_active = false;
+	icssg_iet_set_preempt_mask(emac);
+	return ret;
+}
+
+void icssg_qos_init(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+
+	mutex_init(&iet->fpe_lock);
+	/* Set default values to prevent garbage values during .get_mm() */
+	iet->verify_time_ms = ICSSG_IET_MAX_VERIFY_TIME;
+	iet->tx_min_frag_size = ETH_ZLEN;
+}
+EXPORT_SYMBOL_GPL(icssg_qos_init);
+
+static int icssg_iet_change_preemptible_tcs(struct prueth_emac *emac)
+{
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	int ret;
+
+	mutex_lock(&iet->fpe_lock);
+	if (!iet->fpe_enabled && !iet->preemptible_tcs) {
+		mutex_unlock(&iet->fpe_lock);
+		return 0;
+	}
+	ret = icssg_config_ietfpe(emac->ndev, iet->fpe_enabled);
+	mutex_unlock(&iet->fpe_lock);
+
+	return ret;
+}
+
+static int emac_tc_query_caps(struct net_device *ndev, void *type_data)
+{
+	struct tc_query_caps_base *base = type_data;
+
+	switch (base->type) {
+	case TC_SETUP_QDISC_MQPRIO: {
+		struct tc_mqprio_caps *caps = base->caps;
+
+		caps->validate_queue_counts = true;
+		return 0;
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int emac_tc_setup_mqprio(struct net_device *ndev, void *type_data)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_mqprio *p_mqprio = &emac->qos.mqprio;
+	struct tc_mqprio_qopt_offload *mqprio = type_data;
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
+	int tc, offset, count;
+
+	/* Validate parameters */
+	if (qopt->num_tc > ICSSG_MAX_TC_QUEUES) {
+		netdev_err(ndev, "Number of traffic classes (%u) exceeds hardware limit\n",
+			   qopt->num_tc);
+		return -EOPNOTSUPP;
+	}
+
+	if (mqprio->flags & TC_MQPRIO_F_SHAPER) {
+		netdev_err(ndev, "traffic shaping is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (mqprio->flags & (TC_MQPRIO_F_MIN_RATE | TC_MQPRIO_F_MAX_RATE)) {
+		netdev_err(ndev, "per-queue rate limiting is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!qopt->num_tc) {
+		netdev_reset_tc(ndev);
+	} else {
+		netdev_set_num_tc(ndev, qopt->num_tc);
+
+		for (tc = 0; tc < qopt->num_tc; tc++) {
+			count = qopt->count[tc];
+			offset = qopt->offset[tc];
+			netdev_set_tc_queue(ndev, tc, count, offset);
+		}
+	}
+
+	mutex_lock(&iet->fpe_lock);
+	if (!qopt->num_tc) {
+		iet->preemptible_tcs = 0;
+	} else {
+		memcpy(&p_mqprio->qopt, qopt, sizeof(*qopt));
+		iet->preemptible_tcs = mqprio->preemptible_tcs;
+	}
+	mutex_unlock(&iet->fpe_lock);
+
+	netdev_dbg(ndev, "dev->num_tc %u dev->real_num_tx_queues %u\n",
+		   ndev->num_tc, ndev->real_num_tx_queues);
+
+	return icssg_iet_change_preemptible_tcs(emac);
+}
+
+int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
+			   void *type_data)
+{
+	switch (type) {
+	case TC_QUERY_CAPS:
+		return emac_tc_query_caps(ndev, type_data);
+	case TC_SETUP_QDISC_MQPRIO:
+		return emac_tc_setup_mqprio(ndev, type_data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+EXPORT_SYMBOL_GPL(icssg_qos_ndo_setup_tc);
+
+void icssg_qos_link_state_update(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	int ret;
+
+	if (!READ_ONCE(emac->link)) {
+		/* Clear FPE active state on link-down so get_mm() reports
+		 * accurate tx_active and verify_status while link is down.
+		 */
+		mutex_lock(&iet->fpe_lock);
+		iet->fpe_active = false;
+		iet->verify_status = ICSSG_IETFPE_STATE_DISABLED;
+		icssg_iet_set_preempt_mask(emac);
+		mutex_unlock(&iet->fpe_lock);
+		return;
+	}
+
+	ret = icssg_iet_change_preemptible_tcs(emac);
+	if (ret)
+		netdev_dbg(ndev, "IET FPE %s failed\n",
+			   str_enable_disable(iet->fpe_enabled));
+}
+EXPORT_SYMBOL_GPL(icssg_qos_link_state_update);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.h b/drivers/net/ethernet/ti/icssg/icssg_qos.h
new file mode 100644
index 0000000000000..e826ce4bcfd96
--- /dev/null
+++ b/drivers/net/ethernet/ti/icssg/icssg_qos.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#ifndef __NET_TI_ICSSG_QOS_H
+#define __NET_TI_ICSSG_QOS_H
+
+#include <linux/atomic.h>
+#include <linux/netdevice.h>
+#include <net/pkt_sched.h>
+
+#define ICSSG_MAX_TC_QUEUES			8
+#define ICSSG_EXPRESS_Q_MASK_ALL		0xFF
+#define ICSSG_IET_MAX_VERIFY_TIME		128
+#define ICSSG_IET_MIN_VERIFY_TIME		1
+#define ICSSG_IET_VERIFY_ATTEMPTS		3
+
+/**
+ * enum icssg_ietfpe_verify_states - status of MM Verify returned by firmware
+ * @ICSSG_IETFPE_STATE_UNKNOWN:
+ *	verification status is unknown
+ * @ICSSG_IETFPE_STATE_INITIAL:
+ *	Firmware returns this if verify state diagram is idle
+ * @ICSSG_IETFPE_STATE_VERIFYING:
+ *	Firmware returns this if verification is ongoing
+ * @ICSSG_IETFPE_STATE_SUCCEEDED:
+ *	Firmware returns this if verify state diagram completes verification
+ * @ICSSG_IETFPE_STATE_FAILED:
+ *	Firmware returns this if verify state diagram fails during verification
+ * @ICSSG_IETFPE_STATE_DISABLED:
+ *	verification is disabled by the driver
+ */
+enum icssg_ietfpe_verify_states {
+	ICSSG_IETFPE_STATE_UNKNOWN = 0,
+	ICSSG_IETFPE_STATE_INITIAL,
+	ICSSG_IETFPE_STATE_VERIFYING,
+	ICSSG_IETFPE_STATE_SUCCEEDED,
+	ICSSG_IETFPE_STATE_FAILED,
+	ICSSG_IETFPE_STATE_DISABLED
+};
+
+struct prueth_qos_mqprio {
+	struct tc_mqprio_qopt qopt;
+};
+
+struct prueth_qos_iet {
+	bool fpe_enabled;
+	bool mac_verify_configure;
+	u32 tx_min_frag_size;
+	u32 verify_time_ms;
+	bool fpe_active;
+	enum icssg_ietfpe_verify_states verify_status;
+	/* fpe mutex protects all FPE operations for synchronization */
+	struct mutex fpe_lock;
+	u8 preemptible_tcs;
+};
+
+struct prueth_qos {
+	struct prueth_qos_iet iet;
+	struct prueth_qos_mqprio mqprio;
+};
+
+void icssg_qos_init(struct net_device *ndev);
+void icssg_qos_link_state_update(struct net_device *ndev);
+int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
+			   void *type_data);
+int icssg_config_ietfpe(struct net_device *ndev, bool enable);
+#endif /* __NET_TI_ICSSG_QOS_H */
-- 
2.43.0

[PATCH net-next v8 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[Meghana Malladi]

From: MD Danish Anwar <danishanwar@ti.com>

Add driver support for viewing and changing the MAC Merge sublayer
parameters via ethtool ops: .set_mm(), .get_mm() and .get_mm_stats().

The minimum size of non-final mPacket fragments supported by the
firmware without leading errors is 64 Bytes (including FCS).
Add pa stats registers to check statistics for preemption,
which can be dumped using ethtool ops.

Fix emac_get_stat_by_name() to return u64 instead of int and return
0 on error instead of -EINVAL. This prevents invalid stat lookups
from corrupting output stats with signed error codes cast to u64.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---

v8-v7:
- Fix the duplicating new entries for emac_get_mm_stats()
  which already have a standard uAPI
- emac_set_mm(): Save and restore all four config fields on
  icssg_config_ietfpe() failure so get_mm() reports the
  configuration actually reflected in hardware
  All the above changes address the sashiko review comments

 drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 132 +++++++++++++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  13 +-
 drivers/net/ethernet/ti/icssg/icssg_stats.c   |   4 +-
 drivers/net/ethernet/ti/icssg/icssg_stats.h   |  84 ++++++-----
 .../net/ethernet/ti/icssg/icssg_switch_map.h  |   5 +
 5 files changed, 191 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
index b715af21d23ac..dc25361afe989 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
@@ -74,7 +74,8 @@ static int emac_get_sset_count(struct net_device *ndev, int stringset)
 		if (emac->prueth->pa_stats)
 			return ICSSG_NUM_ETHTOOL_STATS;
 		else
-			return ICSSG_NUM_ETHTOOL_STATS - ICSSG_NUM_PA_STATS;
+			return ICSSG_NUM_ETHTOOL_STATS -
+			       (ICSSG_NUM_PA_STATS - ICSSG_NUM_PA_STANDARD_STATS);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -93,7 +94,8 @@ static void emac_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
 				ethtool_puts(&p, icssg_all_miig_stats[i].name);
 		if (emac->prueth->pa_stats)
 			for (i = 0; i < ARRAY_SIZE(icssg_all_pa_stats); i++)
-				ethtool_puts(&p, icssg_all_pa_stats[i].name);
+				if (!icssg_all_pa_stats[i].standard_stats)
+					ethtool_puts(&p, icssg_all_pa_stats[i].name);
 		break;
 	default:
 		break;
@@ -114,7 +116,8 @@ static void emac_get_ethtool_stats(struct net_device *ndev,
 
 	if (emac->prueth->pa_stats)
 		for (i = 0; i < ARRAY_SIZE(icssg_all_pa_stats); i++)
-			*(data++) = emac->pa_stats[i];
+			if (!icssg_all_pa_stats[i].standard_stats)
+				*(data++) = emac->pa_stats[i];
 }
 
 static int emac_get_ts_info(struct net_device *ndev,
@@ -294,6 +297,126 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
 	return 0;
 }
 
+static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	enum icssg_ietfpe_verify_states verify_status;
+
+	if (emac->is_sr1)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&iet->fpe_lock);
+	state->tx_enabled = iet->fpe_enabled;
+	state->tx_min_frag_size = iet->tx_min_frag_size;
+	state->verify_enabled = iet->mac_verify_configure;
+	state->verify_time = iet->verify_time_ms;
+	state->tx_active = iet->fpe_active;
+	verify_status = iet->verify_status;
+	mutex_unlock(&iet->fpe_lock);
+
+	state->rx_min_frag_size = ETH_ZLEN;
+	state->pmac_enabled = true;
+
+	switch (verify_status) {
+	case ICSSG_IETFPE_STATE_DISABLED:
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
+		break;
+	case ICSSG_IETFPE_STATE_INITIAL:
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
+		break;
+	case ICSSG_IETFPE_STATE_VERIFYING:
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
+		break;
+	case ICSSG_IETFPE_STATE_SUCCEEDED:
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
+		break;
+	case ICSSG_IETFPE_STATE_FAILED:
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
+		break;
+	default:
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
+		break;
+	}
+
+	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
+	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
+	 */
+	state->max_verify_time = ETHTOOL_MM_MAX_VERIFY_TIME_MS;
+
+	return 0;
+}
+
+static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
+		       struct netlink_ext_ack *extack)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	u32 old_verify_time_ms, old_tx_min_frag_size;
+	bool old_fpe_enabled, old_mac_verify_configure;
+	int err;
+
+	if (emac->is_sr1)
+		return -EOPNOTSUPP;
+
+	if (!cfg->pmac_enabled) {
+		NL_SET_ERR_MSG_MOD(extack, "preemptible MAC is always enabled");
+		return -EOPNOTSUPP;
+	}
+
+	mutex_lock(&iet->fpe_lock);
+	old_verify_time_ms = iet->verify_time_ms;
+	old_tx_min_frag_size = iet->tx_min_frag_size;
+	old_fpe_enabled = iet->fpe_enabled;
+	old_mac_verify_configure = iet->mac_verify_configure;
+
+	iet->verify_time_ms = cfg->verify_time;
+	iet->tx_min_frag_size = cfg->tx_min_frag_size;
+	iet->fpe_enabled = cfg->tx_enabled;
+	iet->mac_verify_configure = cfg->verify_enabled;
+	err = icssg_config_ietfpe(ndev, cfg->tx_enabled);
+	if (err) {
+		/* icssg_config_ietfpe's fallback path already cleared fpe_active.
+		 * Restore all config fields so get_mm() reports the configuration
+		 * that is actually reflected in hardware.
+		 */
+		iet->verify_time_ms = old_verify_time_ms;
+		iet->tx_min_frag_size = old_tx_min_frag_size;
+		iet->fpe_enabled = old_fpe_enabled;
+		iet->mac_verify_configure = old_mac_verify_configure;
+	}
+	mutex_unlock(&iet->fpe_lock);
+
+	return err;
+}
+
+static void emac_get_mm_stats(struct net_device *ndev,
+			      struct ethtool_mm_stats *s)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+
+	if (emac->is_sr1)
+		return;
+
+	if (!emac->prueth->pa_stats)
+		return;
+
+	emac_update_hardware_stats(emac);
+
+	/* Hold stats_lock to prevent icssg_stats_work_handler from updating
+	 * pa_stats[] between reads, which could yield a torn u64 value on
+	 * 32-bit architectures.
+	 */
+	spin_lock(&emac->prueth->stats_lock);
+	/* MACMergeHoldCount stats is not tracked by the firmware */
+	s->MACMergeFrameAssOkCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_OK");
+	s->MACMergeFrameAssErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_ERR");
+	s->MACMergeFragCountRx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_RX");
+	s->MACMergeFragCountTx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_TX");
+	s->MACMergeFrameSmdErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_BAD_FRAG");
+	spin_unlock(&emac->prueth->stats_lock);
+}
+
 const struct ethtool_ops icssg_ethtool_ops = {
 	.get_drvinfo = emac_get_drvinfo,
 	.get_msglevel = emac_get_msglevel,
@@ -317,5 +440,8 @@ const struct ethtool_ops icssg_ethtool_ops = {
 	.set_eee = emac_set_eee,
 	.nway_reset = emac_nway_reset,
 	.get_rmon_stats = emac_get_rmon_stats,
+	.get_mm = emac_get_mm,
+	.set_mm = emac_set_mm,
+	.get_mm_stats = emac_get_mm_stats,
 };
 EXPORT_SYMBOL_GPL(icssg_ethtool_ops);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index f73b8f5fca956..ad9061408050e 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -45,6 +45,7 @@
 #include "icss_iep.h"
 #include "icssg_switch_map.h"
 #include "icssg_qos.h"
+#include "icssg_stats.h"
 
 #define PRUETH_MAX_MTU		(2000 - ETH_HLEN - ETH_FCS_LEN)
 #define PRUETH_MIN_PKT_SIZE	(VLAN_ETH_ZLEN)
@@ -58,12 +59,16 @@
 
 #define ICSSG_MAX_RFLOWS	8	/* per slice */
 
-#define ICSSG_NUM_PA_STATS	32
-#define ICSSG_NUM_MIIG_STATS	60
+#define ICSSG_NUM_PA_STATS	ARRAY_SIZE(icssg_all_pa_stats)
+#define ICSSG_NUM_MIIG_STATS	ARRAY_SIZE(icssg_all_miig_stats)
 /* Number of ICSSG related stats */
 #define ICSSG_NUM_STATS (ICSSG_NUM_MIIG_STATS + ICSSG_NUM_PA_STATS)
+/* MIIG stats with standard uAPI equivalents (excluded from ethtool -S) */
 #define ICSSG_NUM_STANDARD_STATS 31
-#define ICSSG_NUM_ETHTOOL_STATS (ICSSG_NUM_STATS - ICSSG_NUM_STANDARD_STATS)
+/* PA stats with standard uAPI equivalents, exposed via get_mm_stats only */
+#define ICSSG_NUM_PA_STANDARD_STATS 5
+#define ICSSG_NUM_ETHTOOL_STATS (ICSSG_NUM_STATS - ICSSG_NUM_STANDARD_STATS - \
+				 ICSSG_NUM_PA_STANDARD_STATS)
 
 #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
 
@@ -460,7 +465,7 @@ int emac_fdb_flow_id_updated(struct prueth_emac *emac);
 
 void icssg_stats_work_handler(struct work_struct *work);
 void emac_update_hardware_stats(struct prueth_emac *emac);
-int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name);
+u64 emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name);
 
 /* Common functions */
 void prueth_cleanup_rx_chns(struct prueth_emac *emac,
diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
index 7159baa0155cf..cfdb6f5dc5da1 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
@@ -74,7 +74,7 @@ void icssg_stats_work_handler(struct work_struct *work)
 }
 EXPORT_SYMBOL_GPL(icssg_stats_work_handler);
 
-int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
+u64 emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
 {
 	int i;
 
@@ -91,5 +91,5 @@ int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
 	}
 
 	netdev_err(emac->ndev, "Invalid stats %s\n", stat_name);
-	return -EINVAL;
+	return 0;
 }
diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
index 5ec0b38e0c67d..b86da8da23a18 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
@@ -8,8 +8,6 @@
 #ifndef __NET_TI_ICSSG_STATS_H
 #define __NET_TI_ICSSG_STATS_H
 
-#include "icssg_prueth.h"
-
 #define STATS_TIME_LIMIT_1G_MS    25000    /* 25 seconds @ 1G */
 
 struct miig_stats_regs {
@@ -155,50 +153,60 @@ static const struct icssg_miig_stats icssg_all_miig_stats[] = {
 	ICSSG_MIIG_STATS(tx_bytes, true),
 };
 
-#define ICSSG_PA_STATS(field)	\
-{				\
-	#field,			\
-	field,			\
+#define ICSSG_PA_STATS(field, std)	\
+{					\
+	#field,				\
+	field,				\
+	std,				\
 }
 
 struct icssg_pa_stats {
 	char name[ETH_GSTRING_LEN];
 	u32 offset;
+	bool standard_stats;
 };
 
 static const struct icssg_pa_stats icssg_all_pa_stats[] = {
-	ICSSG_PA_STATS(FW_RTU_PKT_DROP),
-	ICSSG_PA_STATS(FW_Q0_OVERFLOW),
-	ICSSG_PA_STATS(FW_Q1_OVERFLOW),
-	ICSSG_PA_STATS(FW_Q2_OVERFLOW),
-	ICSSG_PA_STATS(FW_Q3_OVERFLOW),
-	ICSSG_PA_STATS(FW_Q4_OVERFLOW),
-	ICSSG_PA_STATS(FW_Q5_OVERFLOW),
-	ICSSG_PA_STATS(FW_Q6_OVERFLOW),
-	ICSSG_PA_STATS(FW_Q7_OVERFLOW),
-	ICSSG_PA_STATS(FW_DROPPED_PKT),
-	ICSSG_PA_STATS(FW_RX_ERROR),
-	ICSSG_PA_STATS(FW_RX_DS_INVALID),
-	ICSSG_PA_STATS(FW_TX_DROPPED_PACKET),
-	ICSSG_PA_STATS(FW_TX_TS_DROPPED_PACKET),
-	ICSSG_PA_STATS(FW_INF_PORT_DISABLED),
-	ICSSG_PA_STATS(FW_INF_SAV),
-	ICSSG_PA_STATS(FW_INF_SA_DL),
-	ICSSG_PA_STATS(FW_INF_PORT_BLOCKED),
-	ICSSG_PA_STATS(FW_INF_DROP_TAGGED),
-	ICSSG_PA_STATS(FW_INF_DROP_PRIOTAGGED),
-	ICSSG_PA_STATS(FW_INF_DROP_NOTAG),
-	ICSSG_PA_STATS(FW_INF_DROP_NOTMEMBER),
-	ICSSG_PA_STATS(FW_RX_EOF_SHORT_FRMERR),
-	ICSSG_PA_STATS(FW_RX_B0_DROP_EARLY_EOF),
-	ICSSG_PA_STATS(FW_TX_JUMBO_FRM_CUTOFF),
-	ICSSG_PA_STATS(FW_RX_EXP_FRAG_Q_DROP),
-	ICSSG_PA_STATS(FW_RX_FIFO_OVERRUN),
-	ICSSG_PA_STATS(FW_CUT_THR_PKT),
-	ICSSG_PA_STATS(FW_HOST_RX_PKT_CNT),
-	ICSSG_PA_STATS(FW_HOST_TX_PKT_CNT),
-	ICSSG_PA_STATS(FW_HOST_EGRESS_Q_PRE_OVERFLOW),
-	ICSSG_PA_STATS(FW_HOST_EGRESS_Q_EXP_OVERFLOW),
+	ICSSG_PA_STATS(FW_RTU_PKT_DROP, false),
+	ICSSG_PA_STATS(FW_Q0_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_Q1_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_Q2_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_Q3_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_Q4_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_Q5_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_Q6_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_Q7_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_DROPPED_PKT, false),
+	ICSSG_PA_STATS(FW_RX_ERROR, false),
+	ICSSG_PA_STATS(FW_RX_DS_INVALID, false),
+	ICSSG_PA_STATS(FW_TX_DROPPED_PACKET, false),
+	ICSSG_PA_STATS(FW_TX_TS_DROPPED_PACKET, false),
+	ICSSG_PA_STATS(FW_INF_PORT_DISABLED, false),
+	ICSSG_PA_STATS(FW_INF_SAV, false),
+	ICSSG_PA_STATS(FW_INF_SA_DL, false),
+	ICSSG_PA_STATS(FW_INF_PORT_BLOCKED, false),
+	ICSSG_PA_STATS(FW_INF_DROP_TAGGED, false),
+	ICSSG_PA_STATS(FW_INF_DROP_PRIOTAGGED, false),
+	ICSSG_PA_STATS(FW_INF_DROP_NOTAG, false),
+	ICSSG_PA_STATS(FW_INF_DROP_NOTMEMBER, false),
+	/* MAC Merge counters: exposed via .get_mm_stats standard uAPI,
+	 * not duplicated in the private ethtool -S table.
+	 */
+	ICSSG_PA_STATS(FW_PREEMPT_BAD_FRAG, true),
+	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_ERR, true),
+	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_TX, true),
+	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_OK, true),
+	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_RX, true),
+	ICSSG_PA_STATS(FW_RX_EOF_SHORT_FRMERR, false),
+	ICSSG_PA_STATS(FW_RX_B0_DROP_EARLY_EOF, false),
+	ICSSG_PA_STATS(FW_TX_JUMBO_FRM_CUTOFF, false),
+	ICSSG_PA_STATS(FW_RX_EXP_FRAG_Q_DROP, false),
+	ICSSG_PA_STATS(FW_RX_FIFO_OVERRUN, false),
+	ICSSG_PA_STATS(FW_CUT_THR_PKT, false),
+	ICSSG_PA_STATS(FW_HOST_RX_PKT_CNT, false),
+	ICSSG_PA_STATS(FW_HOST_TX_PKT_CNT, false),
+	ICSSG_PA_STATS(FW_HOST_EGRESS_Q_PRE_OVERFLOW, false),
+	ICSSG_PA_STATS(FW_HOST_EGRESS_Q_EXP_OVERFLOW, false),
 };
 
 #endif /* __NET_TI_ICSSG_STATS_H */
diff --git a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
index 7e053b8af3ece..855fd4ed0b3f6 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
@@ -256,6 +256,11 @@
 #define FW_INF_DROP_PRIOTAGGED		0x0148
 #define FW_INF_DROP_NOTAG		0x0150
 #define FW_INF_DROP_NOTMEMBER		0x0158
+#define FW_PREEMPT_BAD_FRAG		0x0160
+#define FW_PREEMPT_ASSEMBLY_ERR		0x0168
+#define FW_PREEMPT_FRAG_CNT_TX		0x0170
+#define FW_PREEMPT_ASSEMBLY_OK		0x0178
+#define FW_PREEMPT_FRAG_CNT_RX		0x0180
 #define FW_RX_EOF_SHORT_FRMERR		0x0188
 #define FW_RX_B0_DROP_EARLY_EOF		0x0190
 #define FW_TX_JUMBO_FRM_CUTOFF		0x0198
-- 
2.43.0

Re: [PATCH net-next v8 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 2

- [Medium] emac_tc_setup_mqprio() commits all software state (netdev TC
  mapping via netdev_set_num_tc()/netdev_set_tc_queue(),…
- [Medium] icssg_iet_set_preempt_mask() builds EXPRESS_PRE_EMPTIVE_Q_MASK
  (0x0029) from scratch (`int prempt_mask = 0`) and only ORs in BIT(i)…
- [Medium] icssg_config_ietfpe() and icssg_qos_link_state_update() gate
  all FPE work on a single lockless READ_ONCE(emac->link) check, but…
- [Low] Concern (validated as a FALSE POSITIVE for the series): the FPE
  enable path added in icssg_qos.c (icssg_config_ietfpe() enable branch,…
- [Low] Unbalanced mutex lifecycle across the two ICSSG drivers:
  mutex_init(&emac->qos.iet.fpe_lock) is performed only by…

--

net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

This adds a QoS submodule to the ICSSG driver that programs IET Frame
Preemption in firmware, tracks the MAC Merge verify state machine, and
maps mqprio traffic classes to express/preemptible queues.  FPE state is
re-driven on link up/down and serialized with a new per-port fpe_lock.

A few questions about the new code below.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 4a7d1a6f470b1..d59eef61e7012 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -1754,6 +1754,7 @@ void prueth_netdev_exit(struct prueth *prueth,
>  
>  	netif_napi_del(&emac->napi_rx);
>  
> +	mutex_destroy(&emac->qos.iet.fpe_lock);

[Severity: Low]
prueth_netdev_exit() is shared by both ICSSG drivers, but the matching
mutex_init() only happens in icssg_qos_init(), which is called from
prueth_netdev_init() in icssg_prueth.c.

icssg_prueth_sr1.c has its own static prueth_netdev_init() that never
calls icssg_qos_init(), yet it reaches this teardown from both its probe
unwind path and prueth_remove().  Does that mean every SR1 emac has its
fpe_lock destroyed without ever being initialized?

Since alloc_etherdev_mq() zeroes the private area, mutex_destroy() only
sees owner == 0 today (and it is an empty inline without
CONFIG_DEBUG_MUTEXES), so nothing warns.  Would it be cleaner to
initialize the lock in the same layer that destroys it, so that any
future shared code in icssg_common.c taking fpe_lock does not end up
locking an uninitialized mutex on SR1?

>  	pruss_release_mem_region(prueth->pruss, &emac->dram);
>  	free_netdev(emac->ndev);
>  	prueth->emac[mac] = NULL;

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c8056b4..283be27a240e9 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -357,11 +357,11 @@ static void emac_adjust_link(struct net_device *ndev)
>  		}
>  		if (!emac->link) {
>  			new_state = true;
> -			emac->link = 1;
> +			WRITE_ONCE(emac->link, 1);
>  		}
>  	} else if (emac->link) {
>  		new_state = true;
> -		emac->link = 0;
> +		WRITE_ONCE(emac->link, 0);
>  
>  		/* f/w should support 100 & 1000 */
>  		emac->speed = SPEED_1000;
> @@ -392,6 +392,8 @@ static void emac_adjust_link(struct net_device *ndev)
>  		} else {
>  			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
>  		}
> +
> +		icssg_qos_link_state_update(ndev);
>  	}

[Severity: Medium]
emac->link is written here from the phylib state machine under
phydev->lock only, while the FPE path gates on it with a single lockless
read in icssg_config_ietfpe():

	if (!READ_ONCE(emac->link)) {
		netdev_dbg(ndev, "cannot change IET/FPE state when interface is down\n");
		return 0;
	}

Can the link drop while icssg_config_ietfpe() is mid-transaction?

CPU0 (tc mqprio or ethtool) holds fpe_lock, observes link == 1, writes
PRE_EMPTION_ENABLE_TX = 1 and PRE_EMPTION_ENABLE_VERIFY = 1, sends
ICSSG_EMAC_PORT_PREMPT_TX_ENABLE and then sleeps in
icssg_iet_verify_wait() for up to ICSSG_IET_VERIFY_ATTEMPTS *
verify_time_ms (384 ms at the default of 128).

CPU1 (phylib work) runs emac_adjust_link():

	WRITE_ONCE(emac->link, 0);
	...
	icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
	icssg_qos_link_state_update(ndev);	/* blocks on fpe_lock */

so CPU0 keeps enabling FPE on a port that firmware has already disabled,
and if PRE_EMPTION_ACTIVE_TX still reads 1 it sets fpe_active = true.

When CPU1 finally takes fpe_lock, icssg_qos_link_state_update() only
clears fpe_active and verify_status and resets the queue maps.  Should it
also clear PRE_EMPTION_ENABLE_TX and PRE_EMPTION_ENABLE_VERIFY, so the
firmware configuration and the driver's view do not diverge?

Also, prueth_emac_common_start() still contains a plain

	emac->link = 0;

store outside both phydev->lock and fpe_lock, so the annotations added
here do not cover every writer.  Was that store meant to be converted
too?

>  
>  	if (emac->link) {

[ ... ]

> @@ -1686,6 +1689,8 @@ static int prueth_netdev_init(struct prueth *prueth,
>  
>  	INIT_DELAYED_WORK(&emac->stats_work, icssg_stats_work_handler);
>  
> +	icssg_qos_init(ndev);
> +
>  	ret = pruss_request_mem_region(prueth->pruss,
>  				       port == PRUETH_PORT_MII0 ?
>  				       PRUSS_MEM_DRAM0 : PRUSS_MEM_DRAM1,

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.c b/drivers/net/ethernet/ti/icssg/icssg_qos.c
> new file mode 100644
> index 0000000000000..27a953d981538
> --- /dev/null
> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c
> @@ -0,0 +1,305 @@

[ ... ]

> +static void icssg_iet_set_preempt_mask(struct prueth_emac *emac)
> +{
> +	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
> +	struct prueth_qos_mqprio *p_mqprio = &emac->qos.mqprio;
> +	struct tc_mqprio_qopt *qopt = &p_mqprio->qopt;
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	int prempt_mask = 0, i;
> +	u8 tc, num_tc;
> +
> +	if (!iet->preemptible_tcs)
> +		goto reset_hw;
> +
> +	if (iet->fpe_active) {
> +		/* Reset all Q_MAP entries first to clear any stale preemptible
> +		 * entries from a prior wider TC mapping.
> +		 */
> +		for (i = 0; i < ICSSG_MAX_TC_QUEUES; i++)
> +			writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
> +
> +		/* Configure queues for user requested preemptible tc map */
> +		num_tc = p_mqprio->qopt.num_tc;
> +		for (tc = 0; tc < num_tc; tc++) {
> +			/* check if the tc is preemptive or not */
> +			if (iet->preemptible_tcs & BIT(tc)) {
> +				/* Set the queues as preemptive queues */
> +				for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++) {
> +					writeb(BIT(4),
> +					       config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
> +				}
> +			} else {
> +				/* Accumulate express queue bits for the mask */
> +				for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++)
> +					prempt_mask |= BIT(i);
> +			}
> +		}
> +		writeb(prempt_mask, config + EXPRESS_PRE_EMPTIVE_Q_MASK);

[Severity: Medium]
prempt_mask starts at zero and only accumulates queues belonging to
non-preemptible traffic classes, but this writeb() replaces the whole
eight-bit queue mask.  Are the queues that no traffic class maps to
handled correctly here?

With the default emac->tx_ch_num == 1 (real_num_tx_queues == 1) and
preemptible_tcs == BIT(0), the sequence programmed is:

	Q_MAP[0]  = BIT(4)	/* preemptible */
	Q_MAP[1..7] = 0		/* express */
	Q_MASK    = 0x00	/* no queue is express */

The reset path a few lines below defines the opposite encoding for the
same registers:

	/* Reset to default: all queues as express */
	for (i = 0; i < ICSSG_MAX_TC_QUEUES; i++)
		writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
	writeb(ICSSG_EXPRESS_Q_MASK_ALL, config + EXPRESS_PRE_EMPTIVE_Q_MASK);

so Q_MAP == 0 together with Q_MASK == 0xFF means express, while the
active path leaves those same entries at Q_MAP == 0 with their Q_MASK bit
cleared.  Should the bits for queues outside the mqprio map stay set?

Gaps below real_num_tx_queues are reachable as well.
mqprio_validate_queue_counts() in net/sched/sch_mqprio_lib.c only
requires counts to be non-zero, within real_num_tx_queues and
non-overlapping:

	if (qopt->offset[i] >= dev->real_num_tx_queues ||
	    last > dev->real_num_tx_queues) {

It does not require the map to start at queue 0 or to be contiguous.
Queues used outside the TC map (QUEUE_NUM_UNTAGGED,
SPL_PKT_DEFAULT_PRIORITY, and the XDP/XSK path which picks
cpu % emac->tx_ch_num) are the ones that end up with a Q_MAP and Q_MASK
that disagree.

> +		return;
> +	}
> +
> +reset_hw:
> +	/* Reset to default: all queues as express */
> +	for (i = 0; i < ICSSG_MAX_TC_QUEUES; i++)
> +		writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
> +	writeb(ICSSG_EXPRESS_Q_MASK_ALL, config + EXPRESS_PRE_EMPTIVE_Q_MASK);
> +}

[ ... ]

> +void icssg_qos_init(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +
> +	mutex_init(&iet->fpe_lock);
> +	/* Set default values to prevent garbage values during .get_mm() */

[Severity: Low]
This isn't a bug, but this comment (and the similar one in
icssg_qos_link_state_update()) refers to .get_mm(), which is only added
to icssg_ethtool_ops by the following patch in the series.

Within this patch alone nothing writes iet->fpe_enabled or
iet->mac_verify_configure, and mqprio_parse_tc_entries() rejects
TC_FP_PREEMPTIBLE while ethtool_dev_mm_supported() is false, so the
enable side of icssg_config_ietfpe() is not yet reachable.

Could the changelog mention that the MAC Merge uAPI arrives in the next
patch, so the forward reference in these comments is not confusing on its
own?

> +	iet->verify_time_ms = ICSSG_IET_MAX_VERIFY_TIME;
> +	iet->tx_min_frag_size = ETH_ZLEN;
> +}
> +EXPORT_SYMBOL_GPL(icssg_qos_init);
> +
> +static int icssg_iet_change_preemptible_tcs(struct prueth_emac *emac)
> +{
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	int ret;
> +
> +	mutex_lock(&iet->fpe_lock);
> +	if (!iet->fpe_enabled && !iet->preemptible_tcs) {
> +		mutex_unlock(&iet->fpe_lock);
> +		return 0;
> +	}
> +	ret = icssg_config_ietfpe(emac->ndev, iet->fpe_enabled);
> +	mutex_unlock(&iet->fpe_lock);
> +
> +	return ret;
> +}

[ ... ]

> +static int emac_tc_setup_mqprio(struct net_device *ndev, void *type_data)
> +{

[ ... ]

> +	if (!qopt->num_tc) {
> +		netdev_reset_tc(ndev);
> +	} else {
> +		netdev_set_num_tc(ndev, qopt->num_tc);
> +
> +		for (tc = 0; tc < qopt->num_tc; tc++) {
> +			count = qopt->count[tc];
> +			offset = qopt->offset[tc];
> +			netdev_set_tc_queue(ndev, tc, count, offset);
> +		}
> +	}
> +
> +	mutex_lock(&iet->fpe_lock);
> +	if (!qopt->num_tc) {
> +		iet->preemptible_tcs = 0;
> +	} else {
> +		memcpy(&p_mqprio->qopt, qopt, sizeof(*qopt));
> +		iet->preemptible_tcs = mqprio->preemptible_tcs;
> +	}
> +	mutex_unlock(&iet->fpe_lock);
> +
> +	netdev_dbg(ndev, "dev->num_tc %u dev->real_num_tx_queues %u\n",
> +		   ndev->num_tc, ndev->real_num_tx_queues);
> +
> +	return icssg_iet_change_preemptible_tcs(emac);

[Severity: Medium]
By the time this runs, the netdev TC mapping, p_mqprio->qopt and
iet->preemptible_tcs have all been committed, and a failure is returned
straight to mqprio_init() with no rollback.

icssg_config_ietfpe() can genuinely fail:

  - icssg_set_port_state() returns -ETIMEDOUT when the firmware R30
    command is not acknowledged within the read_poll_timeout() window
  - the function returns -EIO when PRE_EMPTION_ACTIVE_TX != 1, or when
    icssg_iet_verify_wait() reports ICSSG_IETFPE_STATE_FAILED or times
    out

On that failure path mqprio_init() aborts, and since priv->hw_offload was
never set, mqprio_destroy() takes the non-offload branch:

net/sched/sch_mqprio.c:mqprio_destroy() {
	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
		mqprio_disable_offload(sch);
	else
		netdev_set_num_tc(dev, 0);
}

so the driver is never called back.  Does emac->qos.iet.preemptible_tcs
and emac->qos.mqprio.qopt then describe a qdisc that does not exist?

If so, icssg_qos_link_state_update() -> icssg_iet_change_preemptible_tcs()
will re-apply that state on every subsequent link-up, using offsets and
counts that were validated against a real_num_tx_queues value which may
have changed since (ethtool -L).  Should the software state be rolled
back when the firmware configuration fails?

> +}

[ ... ]

> +void icssg_qos_link_state_update(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	int ret;
> +
> +	if (!READ_ONCE(emac->link)) {
> +		/* Clear FPE active state on link-down so get_mm() reports
> +		 * accurate tx_active and verify_status while link is down.
> +		 */
> +		mutex_lock(&iet->fpe_lock);
> +		iet->fpe_active = false;
> +		iet->verify_status = ICSSG_IETFPE_STATE_DISABLED;
> +		icssg_iet_set_preempt_mask(emac);
> +		mutex_unlock(&iet->fpe_lock);
> +		return;
> +	}
> +
> +	ret = icssg_iet_change_preemptible_tcs(emac);
> +	if (ret)
> +		netdev_dbg(ndev, "IET FPE %s failed\n",
> +			   str_enable_disable(iet->fpe_enabled));
> +}
> +EXPORT_SYMBOL_GPL(icssg_qos_link_state_update);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915063052.2274986-1-m-malladi%40ti.com

Re: [PATCH net-next v8 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 3 (1) · Low: 2

- [High] emac_set_mm()
  (drivers/net/ethernet/ti/icssg/icssg_ethtool.c:378-386) restores all…
- [Medium] emac_set_mm() stores cfg->tx_min_frag_size verbatim
  (icssg_ethtool.c:374) and icssg_config_ietfpe() programs…
- [Medium] The commit message justifies the emac_get_stat_by_name()
  signature change as preventing "invalid stat lookups from corrupting…
- [Medium] iet->verify_status is never initialized to
  ICSSG_IETFPE_STATE_DISABLED.
- [Low] The patch inverts the header dependency to make the stat counts
  ARRAY_SIZE()-derived: icssg_stats.h drops its `#include…
- [Low] The shared teardown helper prueth_netdev_exit()
  (icssg_common.c:1757) unconditionally calls…

Pre-existing issues:
- [Medium] emac->stats[]/emac->pa_stats[] are 64-bit accumulators written
  by icssg_stats_work_handler() -> emac_update_hardware_stats() under…

--

net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

Adds .get_mm(), .set_mm() and .get_mm_stats() to the icssg ethtool ops,
along with new PA stats registers for the preemption counters. Also
changes emac_get_stat_by_name() to return u64 and 0 instead of -EINVAL,
and moves the stat table sizes to ARRAY_SIZE().

No Link: tags present in the commit header.

A few questions on the new code below.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> index b715af21d23ac..dc25361afe989 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c

[ ... ]

> @@ -114,7 +116,8 @@ static void emac_get_ethtool_stats(struct net_device *ndev,
>  
>  	if (emac->prueth->pa_stats)
>  		for (i = 0; i < ARRAY_SIZE(icssg_all_pa_stats); i++)
> -			*(data++) = emac->pa_stats[i];
> +			if (!icssg_all_pa_stats[i].standard_stats)
> +				*(data++) = emac->pa_stats[i];
>  }

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but the
new emac_get_mm_stats() added further down takes prueth->stats_lock for
the pa_stats[] reads and documents why, while emac_get_ethtool_stats()
here, icssg_ndo_get_stats64() and emac_get_rmon_stats() read the same
accumulators with no lock held (emac_update_hardware_stats() has already
dropped it by then).

The writer accumulates under the lock:

drivers/net/ethernet/ti/icssg/icssg_stats.c:emac_update_hardware_stats() {
	spin_lock(&prueth->stats_lock);
	...
		emac->stats[i] += val;
	...
			emac->pa_stats[i] += val;
	...
	spin_unlock(&prueth->stats_lock);
}

and icssg_stats_work_handler() runs that from system_long_wq on another
CPU. Can ethtool -S or ip -s link observe a torn u64 on 32-bit ARM, or a
multi-term sum such as the rx_dropped accumulation in
icssg_ndo_get_stats64() built from two different update generations?

Would it make sense to give these readers the same one-line treatment as
emac_get_mm_stats(), or to convert the accumulators to u64_stats_t?

> @@ -294,6 +297,126 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
>  	return 0;
>  }
>  
> +static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	enum icssg_ietfpe_verify_states verify_status;
> +
> +	if (emac->is_sr1)
> +		return -EOPNOTSUPP;

[Severity: Low]
This asymmetry comes from the earlier patch in the series, "net: ti:
icssg-prueth: Add Frame Preemption MAC Merge support", rather than from
this one, but fpe_lock is initialized only by icssg_qos_init(), whose
sole caller is the SR2 prueth_netdev_init(). The SR1 prueth_netdev_init()
in icssg_prueth_sr1.c never calls it, yet both teardown paths land in the
shared helper:

drivers/net/ethernet/ti/icssg/icssg_common.c:prueth_netdev_exit() {
	mutex_destroy(&emac->qos.iet.fpe_lock);
	pruss_release_mem_region(prueth->pruss, &emac->dram);
	free_netdev(emac->ndev);
}

Nothing observable happens, since mutex_destroy() on a zeroed, never
locked mutex only does DEBUG_LOCKS_WARN_ON(mutex_is_locked(lock)) and
clears magic, and SR1 never takes fpe_lock because the three new ops all
return early on emac->is_sr1.

Would it be cleaner to pair the two, either by calling icssg_qos_init()
from the SR1 netdev init or by skipping the destroy when emac->is_sr1?

> +
> +	mutex_lock(&iet->fpe_lock);
> +	state->tx_enabled = iet->fpe_enabled;
> +	state->tx_min_frag_size = iet->tx_min_frag_size;
> +	state->verify_enabled = iet->mac_verify_configure;
> +	state->verify_time = iet->verify_time_ms;
> +	state->tx_active = iet->fpe_active;
> +	verify_status = iet->verify_status;
> +	mutex_unlock(&iet->fpe_lock);
> +
> +	state->rx_min_frag_size = ETH_ZLEN;
> +	state->pmac_enabled = true;
> +
> +	switch (verify_status) {

[ ... ]

> +	default:
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
> +		break;
> +	}

[Severity: Medium]
Is iet->verify_status ever given an initial value? icssg_qos_init() seeds
the other fields this function reads, but not this one:

drivers/net/ethernet/ti/icssg/icssg_qos.c:icssg_qos_init() {
	mutex_init(&iet->fpe_lock);
	/* Set default values to prevent garbage values during .get_mm() */
	iet->verify_time_ms = ICSSG_IET_MAX_VERIFY_TIME;
	iet->tx_min_frag_size = ETH_ZLEN;
}

so it stays at the zero value ICSSG_IETFPE_STATE_UNKNOWN from the
zero-filled netdev private area. It is only assigned in
icssg_config_ietfpe()/icssg_iet_verify_wait() and in the link-down branch
of icssg_qos_link_state_update(), and simply bringing a fresh interface up
returns before any of them:

icssg_iet_change_preemptible_tcs() {
	if (!iet->fpe_enabled && !iet->preemptible_tcs) {
		mutex_unlock(&iet->fpe_lock);
		return 0;
	}
}

Does ethtool --show-mm on a freshly probed interface then report
verify-status UNKNOWN together with verify-enabled off, where other MM
implementations report DISABLED?

> +
> +	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
> +	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
> +	 */
> +	state->max_verify_time = ETHTOOL_MM_MAX_VERIFY_TIME_MS;
> +
> +	return 0;
> +}
> +
> +static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
> +		       struct netlink_ext_ack *extack)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	u32 old_verify_time_ms, old_tx_min_frag_size;
> +	bool old_fpe_enabled, old_mac_verify_configure;
> +	int err;
> +
> +	if (emac->is_sr1)
> +		return -EOPNOTSUPP;
> +
> +	if (!cfg->pmac_enabled) {
> +		NL_SET_ERR_MSG_MOD(extack, "preemptible MAC is always enabled");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	mutex_lock(&iet->fpe_lock);
> +	old_verify_time_ms = iet->verify_time_ms;
> +	old_tx_min_frag_size = iet->tx_min_frag_size;
> +	old_fpe_enabled = iet->fpe_enabled;
> +	old_mac_verify_configure = iet->mac_verify_configure;
> +
> +	iet->verify_time_ms = cfg->verify_time;
> +	iet->tx_min_frag_size = cfg->tx_min_frag_size;

[Severity: Medium]
Should cfg->tx_min_frag_size be run through
ethtool_mm_frag_size_min_to_add() before being stored? The netlink policy
only range checks it:

net/ethtool/mm.c:
	[ETHTOOL_A_MM_TX_MIN_FRAG_SIZE]	= NLA_POLICY_RANGE(NLA_U32, 60, 252),

so a value like 61 reaches the driver, icssg_config_ietfpe() writes
tx_min_frag_size + ETH_FCS_LEN, i.e. 65 octets, into
PRE_EMPTION_ADD_FRAG_SIZE_LOCAL, and get_mm() echoes 61 back as the
effective setting with no extack error.

The sibling TI driver validates the same field:

drivers/net/ethernet/ti/am65-cpsw-ethtool.c:am65_cpsw_set_mm() {
	err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size,
					      &add_frag_size, extack);
	if (err)
		return err;
}

whose kernel-doc states only the addFragSize-representable values
60/124/188/252 are accepted.

The commit message also says:

    The minimum size of non-final mPacket fragments supported by the
    firmware without leading errors is 64 Bytes (including FCS).

Should sizes the firmware cannot honour be rejected with an extack
message rather than silently programmed?

> +	iet->fpe_enabled = cfg->tx_enabled;
> +	iet->mac_verify_configure = cfg->verify_enabled;
> +	err = icssg_config_ietfpe(ndev, cfg->tx_enabled);
> +	if (err) {
> +		/* icssg_config_ietfpe's fallback path already cleared fpe_active.
> +		 * Restore all config fields so get_mm() reports the configuration
> +		 * that is actually reflected in hardware.
> +		 */
> +		iet->verify_time_ms = old_verify_time_ms;
> +		iet->tx_min_frag_size = old_tx_min_frag_size;
> +		iet->fpe_enabled = old_fpe_enabled;
> +		iet->mac_verify_configure = old_mac_verify_configure;
> +	}

[Severity: High]
Does this restore really leave get_mm() reporting the configuration that
is in hardware?

icssg_config_ietfpe() programs the new fragment size before any failure
point:

drivers/net/ethernet/ti/icssg/icssg_qos.c:icssg_config_ietfpe() {
	writeb(enable ? 1 : 0, config + PRE_EMPTION_ENABLE_TX);
	writew(iet->tx_min_frag_size + ETH_FCS_LEN,
	       config + PRE_EMPTION_ADD_FRAG_SIZE_LOCAL);
	...
}

and neither error exit puts that register back:

disable_tx:
	icssg_set_port_state(emac, ICSSG_EMAC_PORT_PREMPT_TX_DISABLE);
fallback:
	writeb(0, config + PRE_EMPTION_ENABLE_TX);
	writeb(0, config + PRE_EMPTION_ENABLE_VERIFY);
	iet->verify_status = ICSSG_IETFPE_STATE_DISABLED;
	iet->fpe_active = false;
	icssg_iet_set_preempt_mask(emac);
	return ret;

So after a failed set_mm, PRE_EMPTION_ADD_FRAG_SIZE_LOCAL keeps the
rejected size while get_mm() reports old_tx_min_frag_size, and TX
preemption has been switched off in hardware while iet->fpe_enabled is
republished as true whenever FPE was previously running.

Can that also turn an identical retry into a silent no-op? ethnl_set_mm()
seeds cfg from get_mm() and skips the driver when nothing changed:

net/ethtool/mm.c:ethnl_set_mm() {
	ret = dev->ethtool_ops->get_mm(dev, &state);
	...
	mm_state_to_cfg(&state, &cfg);
	...
	if (!mod)
		return 0;
}

so re-running ethtool --set-mm <dev> tx-enabled on returns success with
PRE_EMPTION_ENABLE_TX still 0.

And can the restored fpe_enabled == true be consumed later without any
user request, on the next link event?

drivers/net/ethernet/ti/icssg/icssg_qos.c:icssg_iet_change_preemptible_tcs() {
	mutex_lock(&iet->fpe_lock);
	if (!iet->fpe_enabled && !iet->preemptible_tcs) {
		mutex_unlock(&iet->fpe_lock);
		return 0;
	}
	ret = icssg_config_ietfpe(emac->ndev, iet->fpe_enabled);
}

The failure paths are ordinary runtime conditions: verification rejected
by the link partner or timing out, firmware not setting
PRE_EMPTION_ACTIVE_TX, or the port state command failing.

> +	mutex_unlock(&iet->fpe_lock);
> +
> +	return err;
> +}
> +
> +static void emac_get_mm_stats(struct net_device *ndev,
> +			      struct ethtool_mm_stats *s)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +
> +	if (emac->is_sr1)
> +		return;
> +
> +	if (!emac->prueth->pa_stats)
> +		return;
> +
> +	emac_update_hardware_stats(emac);
> +
> +	/* Hold stats_lock to prevent icssg_stats_work_handler from updating
> +	 * pa_stats[] between reads, which could yield a torn u64 value on
> +	 * 32-bit architectures.
> +	 */
> +	spin_lock(&emac->prueth->stats_lock);
> +	/* MACMergeHoldCount stats is not tracked by the firmware */
> +	s->MACMergeFrameAssOkCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_OK");
> +	s->MACMergeFrameAssErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_ERR");
> +	s->MACMergeFragCountRx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_RX");
> +	s->MACMergeFragCountTx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_TX");
> +	s->MACMergeFrameSmdErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_BAD_FRAG");
> +	spin_unlock(&emac->prueth->stats_lock);
> +}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index f73b8f5fca956..ad9061408050e 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -45,6 +45,7 @@
>  #include "icss_iep.h"
>  #include "icssg_switch_map.h"
>  #include "icssg_qos.h"
> +#include "icssg_stats.h"
>  
>  #define PRUETH_MAX_MTU		(2000 - ETH_HLEN - ETH_FCS_LEN)
>  #define PRUETH_MIN_PKT_SIZE	(VLAN_ETH_ZLEN)
> @@ -58,12 +59,16 @@
>  
>  #define ICSSG_MAX_RFLOWS	8	/* per slice */
>  
> -#define ICSSG_NUM_PA_STATS	32
> -#define ICSSG_NUM_MIIG_STATS	60
> +#define ICSSG_NUM_PA_STATS	ARRAY_SIZE(icssg_all_pa_stats)
> +#define ICSSG_NUM_MIIG_STATS	ARRAY_SIZE(icssg_all_miig_stats)

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> index 7159baa0155cf..cfdb6f5dc5da1 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> @@ -74,7 +74,7 @@ void icssg_stats_work_handler(struct work_struct *work)
>  }
>  EXPORT_SYMBOL_GPL(icssg_stats_work_handler);
>  
> -int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
> +u64 emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
>  {
>  	int i;
>  
> @@ -91,5 +91,5 @@ int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
>  	}
>  
>  	netdev_err(emac->ndev, "Invalid stats %s\n", stat_name);
> -	return -EINVAL;
> +	return 0;
>  }

[Severity: Medium]
The commit message describes this hunk as:

    Fix emac_get_stat_by_name() to return u64 instead of int and return
    0 on error instead of -EINVAL. This prevents invalid stat lookups
    from corrupting output stats with signed error codes cast to u64.

Is that path reachable? Every caller passes a literal name that exists in
icssg_all_miig_stats[] or icssg_all_pa_stats[], and each caller that uses
PA names is guarded, e.g. in emac_get_mm_stats():

	if (!emac->prueth->pa_stats)
		return;

so the netdev_err()/-EINVAL branch never runs.

The change does look like it fixes something else that the message does
not mention: the old int return truncated the u64 accumulators built by
emac_update_hardware_stats() and sign-extended them into the u64 fields
of rtnl_link_stats64 and ethtool_rmon_stats:

drivers/net/ethernet/ti/icssg/icssg_common.c:icssg_ndo_get_stats64() {
	stats->rx_bytes       = emac_get_stat_by_name(emac, "rx_bytes");
	...
}

Once rx_bytes or tx_bytes crosses 2^31, would ip -s link have reported
roughly 1.8e19 before this patch?

Should this be split out as its own patch with a Fixes: tag and a
description of the truncation, so that it can be picked up for stable,
rather than being bundled into the MAC Merge feature patch?

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> index 5ec0b38e0c67d..b86da8da23a18 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> @@ -8,8 +8,6 @@
>  #ifndef __NET_TI_ICSSG_STATS_H
>  #define __NET_TI_ICSSG_STATS_H
>  
> -#include "icssg_prueth.h"
> -
>  #define STATS_TIME_LIMIT_1G_MS    25000    /* 25 seconds @ 1G */

[Severity: Low]
With this include gone, icssg_stats.h has no includes of its own, yet it
still uses ETH_GSTRING_LEN, offsetof() and the FW_* offsets from
icssg_switch_map.h. It compiles today only because icssg_prueth.h
includes linux/ethtool.h and icssg_switch_map.h ahead of the new
"#include icssg_stats.h".

Would it be worth making icssg_stats.h self-contained by including
linux/ethtool.h and icssg_switch_map.h directly, so a future .c file that
includes it first still builds?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915063052.2274986-1-m-malladi%40ti.com