[PATCH iwl-next 0/9] igc: Add support for Frame Preemption feature in IGC

[PATCH iwl-next 0/9] igc: Add support for Frame Preemption feature in IGC

[Faizal Rahim]

Introduces support for the FPE feature in the IGC driver.

The patches aligns with the upstream FPE API:
https://patchwork.kernel.org/project/netdevbpf/cover/20230220122343.1156614-1-vladimir.oltean@nxp.com/
https://patchwork.kernel.org/project/netdevbpf/cover/20230119122705.73054-1-vladimir.oltean@nxp.com/

It builds upon earlier work:
https://patchwork.kernel.org/project/netdevbpf/cover/20220520011538.1098888-1-vinicius.gomes@intel.com/

The first four patches in this series are preparation work for the subsequent patches.

The patch series adds the following functionalities to the IGC driver:
a) Configure FPE using `ethtool --set-mm`.
b) Display FPE settings via `ethtool --show-mm`.
c) View FPE statistics using `ethtool --include-statistics --show-mm'.
e) Enable preemptible/express queue with `fp`:
   tc qdisc add ... root taprio \
   fp E E P P

Note:
1. preemption can occur with or without the verification handshake,
   depending on the value of the verify_enabled field, which can be
   configured using ethtool --set-mm.
2. Enabling FPE with mqprio offload is not covered in this series, but
   existing code prevents user from configuring FPE alongside mqprio offload.

Faizal Rahim (6):
  igc: Rename xdp_get_tx_ring() for non-xdp usage
  igc: Add support to set MAC Merge data via ethtool
  igc: Add support for frame preemption verification
  igc: Add support for preemptible traffic class in taprio
  igc: Add support to get MAC Merge data via ethtool
  igc: Add support to get frame preemption statistics via ethtool

Vinicius Costa Gomes (3):
  igc: Optimize the TX packet buffer utilization
  igc: Set the RX packet buffer size for TSN mode
  igc: Add support for receiving frames with all zeroes address

 drivers/net/ethernet/intel/igc/igc.h         |  45 ++-
 drivers/net/ethernet/intel/igc/igc_defines.h |  15 +-
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  96 ++++++
 drivers/net/ethernet/intel/igc/igc_main.c    |  80 ++++-
 drivers/net/ethernet/intel/igc/igc_regs.h    |  19 ++
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 330 ++++++++++++++++++-
 drivers/net/ethernet/intel/igc/igc_tsn.h     |  15 +
 7 files changed, 586 insertions(+), 14 deletions(-)

--
2.25.1

[PATCH iwl-next 1/9] igc: Rename xdp_get_tx_ring() for non-xdp usage

[Faizal Rahim]

Renamed xdp_get_tx_ring() function to a more generic name for use in
upcoming frame preemption patches.

Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      |  2 +-
 drivers/net/ethernet/intel/igc/igc_main.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index eac0f966e0e4..34a6e4d8a652 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -734,7 +734,7 @@ struct igc_nfc_rule *igc_get_nfc_rule(struct igc_adapter *adapter,
 				      u32 location);
 int igc_add_nfc_rule(struct igc_adapter *adapter, struct igc_nfc_rule *rule);
 void igc_del_nfc_rule(struct igc_adapter *adapter, struct igc_nfc_rule *rule);
-
+struct igc_ring *igc_get_tx_ring(struct igc_adapter *adapter, int cpu);
 void igc_ptp_init(struct igc_adapter *adapter);
 void igc_ptp_reset(struct igc_adapter *adapter);
 void igc_ptp_suspend(struct igc_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 27872bdea9bd..05146cc1b92c 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2448,8 +2448,8 @@ static int igc_xdp_init_tx_descriptor(struct igc_ring *ring,
 	return -ENOMEM;
 }
 
-static struct igc_ring *igc_xdp_get_tx_ring(struct igc_adapter *adapter,
-					    int cpu)
+struct igc_ring *igc_get_tx_ring(struct igc_adapter *adapter,
+				 int cpu)
 {
 	int index = cpu;
 
@@ -2473,7 +2473,7 @@ static int igc_xdp_xmit_back(struct igc_adapter *adapter, struct xdp_buff *xdp)
 	if (unlikely(!xdpf))
 		return -EFAULT;
 
-	ring = igc_xdp_get_tx_ring(adapter, cpu);
+	ring = igc_get_tx_ring(adapter, cpu);
 	nq = txring_txq(ring);
 
 	__netif_tx_lock(nq, cpu);
@@ -2551,7 +2551,7 @@ static void igc_finalize_xdp(struct igc_adapter *adapter, int status)
 	struct igc_ring *ring;
 
 	if (status & IGC_XDP_TX) {
-		ring = igc_xdp_get_tx_ring(adapter, cpu);
+		ring = igc_get_tx_ring(adapter, cpu);
 		nq = txring_txq(ring);
 
 		__netif_tx_lock(nq, cpu);
@@ -6677,7 +6677,7 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames,
 	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
 		return -EINVAL;
 
-	ring = igc_xdp_get_tx_ring(adapter, cpu);
+	ring = igc_get_tx_ring(adapter, cpu);
 	nq = txring_txq(ring);
 
 	__netif_tx_lock(nq, cpu);
-- 
2.25.1

[PATCH iwl-next 2/9] igc: Optimize the TX packet buffer utilization

[Faizal Rahim]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>

Packet buffers (RX + TX) total 64KB. Neither RX or TX buffers can be
larger than 34KB. So divide the buffer equally, 32KB for each.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 8e449904aa7d..1f63a523faf2 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -400,7 +400,7 @@
 #define I225_TXPBSIZE_DEFAULT	0x04000014 /* TXPBSIZE default */
 #define IGC_RXPBS_CFG_TS_EN	0x80000000 /* Timestamp in Rx buffer */
 
-#define IGC_TXPBSIZE_TSN	0x04145145 /* 5k bytes buffer for each queue */
+#define IGC_TXPBSIZE_TSN	0x041c71c7 /* 7k bytes buffer for each queue + 4KB for BMC*/
 
 #define IGC_DTXMXPKTSZ_TSN	0x19 /* 1600 bytes of max TX DMA packet size */
 #define IGC_DTXMXPKTSZ_DEFAULT	0x98 /* 9728-byte Jumbo frames */
-- 
2.25.1

[PATCH iwl-next 3/9] igc: Set the RX packet buffer size for TSN mode

[Faizal Rahim]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>

In preparation for supporting frame preemption, when entering TSN mode
set the receive packet buffer to 16KB for the Express MAC, 16KB for
the Preemptible MAC and 2KB for the BMC, according to the datasheet
section 7.1.3.2.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h |  3 +++
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 1f63a523faf2..3a78753ab050 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -400,7 +400,10 @@
 #define I225_TXPBSIZE_DEFAULT	0x04000014 /* TXPBSIZE default */
 #define IGC_RXPBS_CFG_TS_EN	0x80000000 /* Timestamp in Rx buffer */
 
+/* The total (RX + TX) packet buffers must sum to less than 64KB */
 #define IGC_TXPBSIZE_TSN	0x041c71c7 /* 7k bytes buffer for each queue + 4KB for BMC*/
+#define IGC_RXPBSIZE_TSN	0x0000f08f /* 15KB for EXP + 15KB for BE + 2KB for BMC */
+#define IGC_RXPBSIZE_SIZE_MASK	0x0001FFFF
 
 #define IGC_DTXMXPKTSZ_TSN	0x19 /* 1600 bytes of max TX DMA packet size */
 #define IGC_DTXMXPKTSZ_DEFAULT	0x98 /* 9728-byte Jumbo frames */
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 1e44374ca1ff..f0213cfce07d 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -132,13 +132,17 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter)
 {
 	u16 queue_per_tc[4] = { 3, 2, 1, 0 };
 	struct igc_hw *hw = &adapter->hw;
-	u32 tqavctrl;
+	u32 tqavctrl, rxpbs;
 	int i;
 
 	wr32(IGC_GTXOFFSET, 0);
 	wr32(IGC_TXPBS, I225_TXPBSIZE_DEFAULT);
 	wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_DEFAULT);
 
+	rxpbs = rd32(IGC_RXPBS) & ~IGC_RXPBSIZE_SIZE_MASK;
+	rxpbs |= I225_RXPBSIZE_DEFAULT;
+	wr32(IGC_RXPBS, rxpbs);
+
 	if (igc_is_device_id_i226(hw))
 		igc_tsn_restore_retx_default(adapter);
 
@@ -194,7 +198,7 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
 	u32 tqavctrl, baset_l, baset_h;
-	u32 sec, nsec, cycle;
+	u32 sec, nsec, cycle, rxpbs;
 	ktime_t base_time, systim;
 	int i;
 
@@ -202,6 +206,11 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 	wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_TSN);
 	wr32(IGC_TXPBS, IGC_TXPBSIZE_TSN);
 
+	rxpbs = rd32(IGC_RXPBS) & ~IGC_RXPBSIZE_SIZE_MASK;
+	rxpbs |= IGC_RXPBSIZE_TSN;
+
+	wr32(IGC_RXPBS, rxpbs);
+
 	if (igc_is_device_id_i226(hw))
 		igc_tsn_set_retx_qbvfullthreshold(adapter);
 
-- 
2.25.1

[PATCH iwl-next 4/9] igc: Add support for receiving frames with all zeroes address

[Faizal Rahim]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>

The frame preemption verification (as defined by IEEE 802.3-2018
Section 99.4.3) handshake is done by the driver, the default
configuration of the driver is to only receive frames with the driver
address.

So, in preparation for that add a second address to the list of
acceptable addresses.

Because the frame preemption "verify_enable" toggle only affects the
transmission of verification frames, this needs to always be enabled.
As that address is invalid, the impact in practical scenarios should
be minimal. But still a bummer that we have to do this.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      |  1 +
 drivers/net/ethernet/intel/igc/igc_main.c | 17 +++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_tsn.c  |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 34a6e4d8a652..480b54573d60 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -734,6 +734,7 @@ struct igc_nfc_rule *igc_get_nfc_rule(struct igc_adapter *adapter,
 				      u32 location);
 int igc_add_nfc_rule(struct igc_adapter *adapter, struct igc_nfc_rule *rule);
 void igc_del_nfc_rule(struct igc_adapter *adapter, struct igc_nfc_rule *rule);
+int igc_enable_empty_addr_recv(struct igc_adapter *adapter);
 struct igc_ring *igc_get_tx_ring(struct igc_adapter *adapter, int cpu);
 void igc_ptp_init(struct igc_adapter *adapter);
 void igc_ptp_reset(struct igc_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 05146cc1b92c..3f0751a9530c 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -3962,6 +3962,23 @@ static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
 	return 0;
 }
 
+/**
+ * igc_enable_empty_addr_recv - Enable rx of packets with all-zeroes MAC address
+ * @adapter: Pointer to the igc_adapter structure.
+ *
+ * Frame preemption verification requires that packets with the all-zeroes
+ * MAC address are allowed to be received by IGC. This function adds the
+ * all-zeroes destination address to the list of acceptable addresses.
+ *
+ * @return: 0 on success, negative value otherwise.
+ */
+int igc_enable_empty_addr_recv(struct igc_adapter *adapter)
+{
+	u8 empty[ETH_ALEN] = { };
+
+	return igc_add_mac_filter(adapter, IGC_MAC_FILTER_TYPE_DST, empty, -1);
+}
+
 /**
  * igc_set_rx_mode - Secondary Unicast, Multicast and Promiscuous mode set
  * @netdev: network interface device structure
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index f0213cfce07d..5cd54ce435b9 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -434,6 +434,8 @@ int igc_tsn_reset(struct igc_adapter *adapter)
 	unsigned int new_flags;
 	int err = 0;
 
+	igc_enable_empty_addr_recv(adapter);
+
 	new_flags = igc_tsn_new_flags(adapter);
 
 	if (!(new_flags & IGC_FLAG_TSN_ANY_ENABLED))
-- 
2.25.1

Re: [PATCH iwl-next 4/9] igc: Add support for receiving frames with all zeroes address

[Vladimir Oltean]

On Mon, Dec 16, 2024 at 01:47:15AM -0500, Faizal Rahim wrote:
> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> 
> The frame preemption verification (as defined by IEEE 802.3-2018
> Section 99.4.3) handshake is done by the driver, the default
> configuration of the driver is to only receive frames with the driver
> address.
> 
> So, in preparation for that add a second address to the list of
> acceptable addresses.
> 
> Because the frame preemption "verify_enable" toggle only affects the
> transmission of verification frames, this needs to always be enabled.
> As that address is invalid, the impact in practical scenarios should
> be minimal. But still a bummer that we have to do this.

Stuff that happened since this patch was written: "ethtool --set-mm
pmac-enabled on" exists. You don't have to accept verification frames if
the pMAC is disabled. You can enable the reception of 00:00:00:00:00:00
using that, and keep it off by default.

[PATCH iwl-next 5/9] igc: Add support to set MAC Merge data via ethtool

[Faizal Rahim]

Created fpe_t struct to store MAC Merge data and implement the
"ethtool --set-mm" callback. The fpe_t struct will host other frame
preemption related data in future patches.

The following fields are used to set IGC register:
a) pmac_enabled -> TQAVCTRL.PREEMPT_ENA
   This global register sets the preemption scheme, controlling
   preemption capabilities in transmit and receive directions, as well as
   the verification handshake capability.
b) tx_min_frag_size -> TQAVCTRL.MIN_FRAG
   Global register to set minimum fragments.

The fields below don't set any register but will be utilized in the
upcoming patches:
a) verify_time
b) verify_enabled
c) tx_enabled
   Note that IGC doesn't have any register to enforce "tx_enabled"
   (preemption in transmit direction) like some other NIC. This field
   will be used in driver level to control verification procedure and
   managing preemption capability in transmit direction.

At this point, verify response handshake is not enabled yet.

Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         | 24 ++++++++++++-
 drivers/net/ethernet/intel/igc/igc_defines.h |  3 ++
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 30 ++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_main.c    |  2 ++
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 37 ++++++++++++++++++--
 drivers/net/ethernet/intel/igc/igc_tsn.h     |  9 +++++
 6 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 480b54573d60..5a14e9101723 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -40,6 +40,25 @@ void igc_ethtool_set_ops(struct net_device *);
 
 #define IGC_MAX_TX_TSTAMP_REGS		4
 
+/**
+ * @verify_time: see struct ethtool_mm_state
+ * @verify_enabled: see struct ethtool_mm_state
+ * @tx_enabled:
+ * Note that IGC NIC does not have the capability to enable preemption in
+ * "transmit direction". This field is used to manage transmit preemption in
+ * driver level.
+ * @pmac_enabled:
+ * Enable the capability to receive preemptible frames.
+ * @tx_min_frag_size: see struct ethtool_mm_state
+ */
+struct fpe_t {
+	u32 verify_time;
+	bool verify_enabled;
+	bool tx_enabled;
+	bool pmac_enabled;
+	u32 tx_min_frag_size;
+};
+
 enum igc_mac_filter_type {
 	IGC_MAC_FILTER_TYPE_DST = 0,
 	IGC_MAC_FILTER_TYPE_SRC
@@ -332,6 +351,8 @@ struct igc_adapter {
 		struct timespec64 period;
 	} perout[IGC_N_PEROUT];
 
+	struct fpe_t fpe;
+
 	/* LEDs */
 	struct mutex led_mutex;
 	struct igc_led_classdev *leds;
@@ -387,10 +408,11 @@ extern char igc_driver_name[];
 #define IGC_FLAG_TSN_QBV_ENABLED	BIT(17)
 #define IGC_FLAG_TSN_QAV_ENABLED	BIT(18)
 #define IGC_FLAG_TSN_LEGACY_ENABLED	BIT(19)
+#define IGC_FLAG_TSN_PREEMPT_ENABLED	BIT(20)
 
 #define IGC_FLAG_TSN_ANY_ENABLED				\
 	(IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED |	\
-	 IGC_FLAG_TSN_LEGACY_ENABLED)
+	 IGC_FLAG_TSN_LEGACY_ENABLED | IGC_FLAG_TSN_PREEMPT_ENABLED)
 
 #define IGC_FLAG_RSS_FIELD_IPV4_UDP	BIT(6)
 #define IGC_FLAG_RSS_FIELD_IPV6_UDP	BIT(7)
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 3a78753ab050..3088cdd08f35 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -544,6 +544,9 @@
 #define IGC_TQAVCTRL_TRANSMIT_MODE_TSN	0x00000001
 #define IGC_TQAVCTRL_ENHANCED_QAV	0x00000008
 #define IGC_TQAVCTRL_FUTSCDDIS		0x00000080
+#define IGC_TQAVCTRL_PREEMPT_ENA	0x00000002
+#define IGC_TQAVCTRL_MIN_FRAG_MASK	0x0000C000
+#define IGC_TQAVCTRL_MIN_FRAG_SHIFT	14
 
 #define IGC_TXQCTL_QUEUE_MODE_LAUNCHT	0x00000001
 #define IGC_TXQCTL_STRICT_CYCLE		0x00000002
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 817838677817..1954561ec4aa 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -8,6 +8,7 @@
 
 #include "igc.h"
 #include "igc_diag.h"
+#include "igc_tsn.h"
 
 /* forward declaration */
 struct igc_stats {
@@ -1781,6 +1782,34 @@ static int igc_ethtool_set_eee(struct net_device *netdev,
 	return 0;
 }
 
+static int igc_ethtool_set_mm(struct net_device *netdev,
+			      struct ethtool_mm_cfg *cmd,
+			      struct netlink_ext_ack *extack)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	struct fpe_t *fpe = &adapter->fpe;
+
+	if (cmd->tx_min_frag_size < IGC_TX_MIN_FRAG_SIZE ||
+	    cmd->tx_min_frag_size > IGC_TX_MAX_FRAG_SIZE)
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Invalid value for tx-min-frag-size");
+	else
+		fpe->tx_min_frag_size = cmd->tx_min_frag_size;
+
+	if (cmd->verify_time < MIN_VERIFY_TIME ||
+	    cmd->verify_time > MAX_VERIFY_TIME)
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Invalid value for verify-time");
+	else
+		fpe->verify_time = cmd->verify_time;
+
+	fpe->tx_enabled = cmd->tx_enabled;
+	fpe->pmac_enabled = cmd->pmac_enabled;
+	fpe->verify_enabled = cmd->verify_enabled;
+
+	return igc_tsn_offload_apply(adapter);
+}
+
 static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
 					  struct ethtool_link_ksettings *cmd)
 {
@@ -2068,6 +2097,7 @@ static const struct ethtool_ops igc_ethtool_ops = {
 	.set_rxfh		= igc_ethtool_set_rxfh,
 	.get_ts_info		= igc_ethtool_get_ts_info,
 	.get_channels		= igc_ethtool_get_channels,
+	.set_mm			= igc_ethtool_set_mm,
 	.set_channels		= igc_ethtool_set_channels,
 	.get_priv_flags		= igc_ethtool_get_priv_flags,
 	.set_priv_flags		= igc_ethtool_set_priv_flags,
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 3f0751a9530c..b85eaf34d07b 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7144,6 +7144,8 @@ static int igc_probe(struct pci_dev *pdev,
 
 	igc_tsn_clear_schedule(adapter);
 
+	igc_fpe_init(&adapter->fpe);
+
 	/* reset the hardware with the new settings */
 	igc_reset(adapter);
 
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 5cd54ce435b9..b968c02f5fee 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -5,6 +5,18 @@
 #include "igc_hw.h"
 #include "igc_tsn.h"
 
+#define DEFAULT_VERIFY_TIME		10
+#define IGC_MIN_FOR_TX_MIN_FRAG		0
+#define IGC_MAX_FOR_TX_MIN_FRAG		3
+
+void igc_fpe_init(struct fpe_t *fpe)
+{
+	fpe->verify_enabled = false;
+	fpe->verify_time = DEFAULT_VERIFY_TIME;
+	fpe->pmac_enabled = false;
+	fpe->tx_min_frag_size = IGC_TX_MIN_FRAG_SIZE;
+}
+
 static bool is_any_launchtime(struct igc_adapter *adapter)
 {
 	int i;
@@ -49,6 +61,9 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter)
 	if (adapter->strict_priority_enable)
 		new_flags |= IGC_FLAG_TSN_LEGACY_ENABLED;
 
+	if (adapter->fpe.pmac_enabled)
+		new_flags |= IGC_FLAG_TSN_PREEMPT_ENABLED;
+
 	return new_flags;
 }
 
@@ -148,7 +163,8 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter)
 
 	tqavctrl = rd32(IGC_TQAVCTRL);
 	tqavctrl &= ~(IGC_TQAVCTRL_TRANSMIT_MODE_TSN |
-		      IGC_TQAVCTRL_ENHANCED_QAV | IGC_TQAVCTRL_FUTSCDDIS);
+		      IGC_TQAVCTRL_ENHANCED_QAV | IGC_TQAVCTRL_FUTSCDDIS |
+		      IGC_TQAVCTRL_PREEMPT_ENA | IGC_TQAVCTRL_MIN_FRAG_MASK);
 
 	wr32(IGC_TQAVCTRL, tqavctrl);
 
@@ -194,12 +210,22 @@ static void igc_tsn_set_retx_qbvfullthreshold(struct igc_adapter *adapter)
 	wr32(IGC_RETX_CTL, retxctl);
 }
 
+static u8 igc_fpe_get_frag_size_mult(const struct fpe_t *fpe)
+{
+	u32 tx_min_frag_size = fpe->tx_min_frag_size;
+	u8 mult = (tx_min_frag_size / 64) - 1;
+
+	return clamp_t(u8, mult, IGC_MIN_FOR_TX_MIN_FRAG,
+		       IGC_MAX_FOR_TX_MIN_FRAG);
+}
+
 static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
 	u32 tqavctrl, baset_l, baset_h;
 	u32 sec, nsec, cycle, rxpbs;
 	ktime_t base_time, systim;
+	u32 frag_size_mult;
 	int i;
 
 	wr32(IGC_TSAUXC, 0);
@@ -370,10 +396,17 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 		wr32(IGC_TXQCTL(i), txqctl);
 	}
 
-	tqavctrl = rd32(IGC_TQAVCTRL) & ~IGC_TQAVCTRL_FUTSCDDIS;
+	tqavctrl = rd32(IGC_TQAVCTRL) & ~(IGC_TQAVCTRL_FUTSCDDIS |
+		   IGC_TQAVCTRL_MIN_FRAG_MASK | IGC_TQAVCTRL_PREEMPT_ENA);
 
 	tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV;
 
+	if (adapter->fpe.pmac_enabled)
+		tqavctrl |= IGC_TQAVCTRL_PREEMPT_ENA;
+
+	frag_size_mult = igc_fpe_get_frag_size_mult(&adapter->fpe);
+	tqavctrl |= frag_size_mult << IGC_TQAVCTRL_MIN_FRAG_SHIFT;
+
 	adapter->qbv_count++;
 
 	cycle = adapter->cycle_time;
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
index 98ec845a86bf..08e7582f257e 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.h
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
@@ -4,6 +4,15 @@
 #ifndef _IGC_TSN_H_
 #define _IGC_TSN_H_
 
+/* IGC_TX_MIN_FRAG_SIZE is based on the MIN_FRAG field in Section 8.12.2 of the
+ * SW User Manual.
+ */
+#define IGC_TX_MIN_FRAG_SIZE		68
+#define IGC_TX_MAX_FRAG_SIZE		260
+#define MIN_VERIFY_TIME			1
+#define MAX_VERIFY_TIME			128
+
+void igc_fpe_init(struct fpe_t *fpe);
 int igc_tsn_offload_apply(struct igc_adapter *adapter);
 int igc_tsn_reset(struct igc_adapter *adapter);
 void igc_tsn_adjust_txtime_offset(struct igc_adapter *adapter);
-- 
2.25.1

Re: [PATCH iwl-next 5/9] igc: Add support to set MAC Merge data via ethtool

[Vladimir Oltean]

On Mon, Dec 16, 2024 at 01:47:16AM -0500, Faizal Rahim wrote:
> Created fpe_t struct to store MAC Merge data and implement the
> "ethtool --set-mm" callback. The fpe_t struct will host other frame
> preemption related data in future patches.
> 
> The following fields are used to set IGC register:
> a) pmac_enabled -> TQAVCTRL.PREEMPT_ENA
>    This global register sets the preemption scheme, controlling
>    preemption capabilities in transmit and receive directions, as well as
>    the verification handshake capability.

I'm sorry, I'm not able to mentally translate this explanation into
something technical. Which capabilities are we talking about, that this
bit controls? I'm not clear what it does. The kernel-doc description of
pmac_enabled is much more succinct (and at the same time, appears to
contradict this much more elaborate yet unclear description).

> b) tx_min_frag_size -> TQAVCTRL.MIN_FRAG
>    Global register to set minimum fragments.

When you say "global register", you mean global as opposed to what?
Per station interface?

> The fields below don't set any register but will be utilized in the
> upcoming patches:
> a) verify_time
> b) verify_enabled
> c) tx_enabled
>    Note that IGC doesn't have any register to enforce "tx_enabled"
>    (preemption in transmit direction) like some other NIC. This field
>    will be used in driver level to control verification procedure and
>    managing preemption capability in transmit direction.

This is perfectly ok, tx_enabled can be a software setting. At the end
of the day, it's important for
	tx_active == tx_enabled && (!verify_enabled || verify_success)
to dictate the FPE state of the hardware in the TX direction.

> 
> At this point, verify response handshake is not enabled yet.
> 
> Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h         | 24 ++++++++++++-
>  drivers/net/ethernet/intel/igc/igc_defines.h |  3 ++
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 30 ++++++++++++++++
>  drivers/net/ethernet/intel/igc/igc_main.c    |  2 ++
>  drivers/net/ethernet/intel/igc/igc_tsn.c     | 37 ++++++++++++++++++--
>  drivers/net/ethernet/intel/igc/igc_tsn.h     |  9 +++++
>  6 files changed, 102 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 480b54573d60..5a14e9101723 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -40,6 +40,25 @@ void igc_ethtool_set_ops(struct net_device *);
>  
>  #define IGC_MAX_TX_TSTAMP_REGS		4
>  
> +/**
> + * @verify_time: see struct ethtool_mm_state
> + * @verify_enabled: see struct ethtool_mm_state
> + * @tx_enabled:
> + * Note that IGC NIC does not have the capability to enable preemption in
> + * "transmit direction". This field is used to manage transmit preemption in
> + * driver level.
> + * @pmac_enabled:
> + * Enable the capability to receive preemptible frames.
> + * @tx_min_frag_size: see struct ethtool_mm_state
> + */
> +struct fpe_t {
> +	u32 verify_time;
> +	bool verify_enabled;
> +	bool tx_enabled;
> +	bool pmac_enabled;
> +	u32 tx_min_frag_size;
> +};
> +
>  enum igc_mac_filter_type {
>  	IGC_MAC_FILTER_TYPE_DST = 0,
>  	IGC_MAC_FILTER_TYPE_SRC
> @@ -332,6 +351,8 @@ struct igc_adapter {
>  		struct timespec64 period;
>  	} perout[IGC_N_PEROUT];
>  
> +	struct fpe_t fpe;
> +
>  	/* LEDs */
>  	struct mutex led_mutex;
>  	struct igc_led_classdev *leds;
> @@ -387,10 +408,11 @@ extern char igc_driver_name[];
>  #define IGC_FLAG_TSN_QBV_ENABLED	BIT(17)
>  #define IGC_FLAG_TSN_QAV_ENABLED	BIT(18)
>  #define IGC_FLAG_TSN_LEGACY_ENABLED	BIT(19)
> +#define IGC_FLAG_TSN_PREEMPT_ENABLED	BIT(20)
>  
>  #define IGC_FLAG_TSN_ANY_ENABLED				\
>  	(IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED |	\
> -	 IGC_FLAG_TSN_LEGACY_ENABLED)
> +	 IGC_FLAG_TSN_LEGACY_ENABLED | IGC_FLAG_TSN_PREEMPT_ENABLED)
>  
>  #define IGC_FLAG_RSS_FIELD_IPV4_UDP	BIT(6)
>  #define IGC_FLAG_RSS_FIELD_IPV6_UDP	BIT(7)
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 3a78753ab050..3088cdd08f35 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -544,6 +544,9 @@
>  #define IGC_TQAVCTRL_TRANSMIT_MODE_TSN	0x00000001
>  #define IGC_TQAVCTRL_ENHANCED_QAV	0x00000008
>  #define IGC_TQAVCTRL_FUTSCDDIS		0x00000080
> +#define IGC_TQAVCTRL_PREEMPT_ENA	0x00000002
> +#define IGC_TQAVCTRL_MIN_FRAG_MASK	0x0000C000
> +#define IGC_TQAVCTRL_MIN_FRAG_SHIFT	14

Shouldn't these fields be in a particular order? like 0x00000002 should
come after 0x00000001?

>  
>  #define IGC_TXQCTL_QUEUE_MODE_LAUNCHT	0x00000001
>  #define IGC_TXQCTL_STRICT_CYCLE		0x00000002
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 817838677817..1954561ec4aa 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -8,6 +8,7 @@
>  
>  #include "igc.h"
>  #include "igc_diag.h"
> +#include "igc_tsn.h"
>  
>  /* forward declaration */
>  struct igc_stats {
> @@ -1781,6 +1782,34 @@ static int igc_ethtool_set_eee(struct net_device *netdev,
>  	return 0;
>  }
>  
> +static int igc_ethtool_set_mm(struct net_device *netdev,
> +			      struct ethtool_mm_cfg *cmd,
> +			      struct netlink_ext_ack *extack)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	struct fpe_t *fpe = &adapter->fpe;
> +
> +	if (cmd->tx_min_frag_size < IGC_TX_MIN_FRAG_SIZE ||
> +	    cmd->tx_min_frag_size > IGC_TX_MAX_FRAG_SIZE)
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Invalid value for tx-min-frag-size");

Shouldn't the execution actually stop here with an error code?

> +	else
> +		fpe->tx_min_frag_size = cmd->tx_min_frag_size;
> +
> +	if (cmd->verify_time < MIN_VERIFY_TIME ||
> +	    cmd->verify_time > MAX_VERIFY_TIME)
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Invalid value for verify-time");

And here too?

> +	else
> +		fpe->verify_time = cmd->verify_time;
> +
> +	fpe->tx_enabled = cmd->tx_enabled;
> +	fpe->pmac_enabled = cmd->pmac_enabled;
> +	fpe->verify_enabled = cmd->verify_enabled;
> +
> +	return igc_tsn_offload_apply(adapter);

hmm, igc_tsn_offload_apply() is a function which always returns zero.
It seems more natural to make it return void.

> +}
> +
>  static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
>  					  struct ethtool_link_ksettings *cmd)
>  {
> @@ -2068,6 +2097,7 @@ static const struct ethtool_ops igc_ethtool_ops = {
>  	.set_rxfh		= igc_ethtool_set_rxfh,
>  	.get_ts_info		= igc_ethtool_get_ts_info,
>  	.get_channels		= igc_ethtool_get_channels,
> +	.set_mm			= igc_ethtool_set_mm,
>  	.set_channels		= igc_ethtool_set_channels,
>  	.get_priv_flags		= igc_ethtool_get_priv_flags,
>  	.set_priv_flags		= igc_ethtool_set_priv_flags,
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 3f0751a9530c..b85eaf34d07b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -7144,6 +7144,8 @@ static int igc_probe(struct pci_dev *pdev,
>  
>  	igc_tsn_clear_schedule(adapter);
>  
> +	igc_fpe_init(&adapter->fpe);
> +
>  	/* reset the hardware with the new settings */
>  	igc_reset(adapter);
>  
> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
> index 5cd54ce435b9..b968c02f5fee 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
> @@ -5,6 +5,18 @@
>  #include "igc_hw.h"
>  #include "igc_tsn.h"
>  
> +#define DEFAULT_VERIFY_TIME		10
> +#define IGC_MIN_FOR_TX_MIN_FRAG		0
> +#define IGC_MAX_FOR_TX_MIN_FRAG		3
> +
> +void igc_fpe_init(struct fpe_t *fpe)
> +{
> +	fpe->verify_enabled = false;
> +	fpe->verify_time = DEFAULT_VERIFY_TIME;
> +	fpe->pmac_enabled = false;
> +	fpe->tx_min_frag_size = IGC_TX_MIN_FRAG_SIZE;
> +}
> +
>  static bool is_any_launchtime(struct igc_adapter *adapter)
>  {
>  	int i;
> @@ -49,6 +61,9 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter)
>  	if (adapter->strict_priority_enable)
>  		new_flags |= IGC_FLAG_TSN_LEGACY_ENABLED;
>  
> +	if (adapter->fpe.pmac_enabled)
> +		new_flags |= IGC_FLAG_TSN_PREEMPT_ENABLED;
> +
>  	return new_flags;
>  }
>  
> @@ -148,7 +163,8 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter)
>  
>  	tqavctrl = rd32(IGC_TQAVCTRL);
>  	tqavctrl &= ~(IGC_TQAVCTRL_TRANSMIT_MODE_TSN |
> -		      IGC_TQAVCTRL_ENHANCED_QAV | IGC_TQAVCTRL_FUTSCDDIS);
> +		      IGC_TQAVCTRL_ENHANCED_QAV | IGC_TQAVCTRL_FUTSCDDIS |
> +		      IGC_TQAVCTRL_PREEMPT_ENA | IGC_TQAVCTRL_MIN_FRAG_MASK);
>  
>  	wr32(IGC_TQAVCTRL, tqavctrl);
>  
> @@ -194,12 +210,22 @@ static void igc_tsn_set_retx_qbvfullthreshold(struct igc_adapter *adapter)
>  	wr32(IGC_RETX_CTL, retxctl);
>  }
>  
> +static u8 igc_fpe_get_frag_size_mult(const struct fpe_t *fpe)
> +{
> +	u32 tx_min_frag_size = fpe->tx_min_frag_size;
> +	u8 mult = (tx_min_frag_size / 64) - 1;
> +
> +	return clamp_t(u8, mult, IGC_MIN_FOR_TX_MIN_FRAG,
> +		       IGC_MAX_FOR_TX_MIN_FRAG);
> +}

If you translate the continuous range of TX fragment sizes into
discrete multipliers because that's what the hardware works with, why
don't you just reject the non-multiple values using
ethtool_mm_frag_size_min_to_add(), and at the same time use the output
of that function directly to obtain your multiplier? IIUC it gets you
the same result.

> +
>  static int igc_tsn_enable_offload(struct igc_adapter *adapter)
>  {
>  	struct igc_hw *hw = &adapter->hw;
>  	u32 tqavctrl, baset_l, baset_h;
>  	u32 sec, nsec, cycle, rxpbs;
>  	ktime_t base_time, systim;
> +	u32 frag_size_mult;
>  	int i;
>  
>  	wr32(IGC_TSAUXC, 0);
> @@ -370,10 +396,17 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
>  		wr32(IGC_TXQCTL(i), txqctl);
>  	}
>  
> -	tqavctrl = rd32(IGC_TQAVCTRL) & ~IGC_TQAVCTRL_FUTSCDDIS;
> +	tqavctrl = rd32(IGC_TQAVCTRL) & ~(IGC_TQAVCTRL_FUTSCDDIS |
> +		   IGC_TQAVCTRL_MIN_FRAG_MASK | IGC_TQAVCTRL_PREEMPT_ENA);
>  
>  	tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV;
>  
> +	if (adapter->fpe.pmac_enabled)
> +		tqavctrl |= IGC_TQAVCTRL_PREEMPT_ENA;
> +
> +	frag_size_mult = igc_fpe_get_frag_size_mult(&adapter->fpe);
> +	tqavctrl |= frag_size_mult << IGC_TQAVCTRL_MIN_FRAG_SHIFT;
> +
>  	adapter->qbv_count++;
>  
>  	cycle = adapter->cycle_time;
> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
> index 98ec845a86bf..08e7582f257e 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.h
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
> @@ -4,6 +4,15 @@
>  #ifndef _IGC_TSN_H_
>  #define _IGC_TSN_H_
>  
> +/* IGC_TX_MIN_FRAG_SIZE is based on the MIN_FRAG field in Section 8.12.2 of the
> + * SW User Manual.
> + */
> +#define IGC_TX_MIN_FRAG_SIZE		68
> +#define IGC_TX_MAX_FRAG_SIZE		260

Odd. Is there a link to this manual (for I225 I suppose)? Standard values are 60, 124, 188, 252.
Maybe the methodology for calculating these is used here? As things stand,
if the driver reports these values, IIUC, openlldp gets confused and communicates
a LLDP_8023_ADD_ETH_CAPS_ADD_FRAG_SIZE value to the link partner which is higher
than it could have been (68 is rounded up to the next standard TX fragment size,
which is 124). So the link partner will preempt in larger chunks and this will
not reduce latency as much.

If you run tools/testing/selftests/drivers/net/hw/ethtool_mm.sh, that
should also spot some inconsistencies, since some of the tests run openlldp
on both ends.

> +#define MIN_VERIFY_TIME			1
> +#define MAX_VERIFY_TIME			128
> +
> +void igc_fpe_init(struct fpe_t *fpe);
>  int igc_tsn_offload_apply(struct igc_adapter *adapter);
>  int igc_tsn_reset(struct igc_adapter *adapter);
>  void igc_tsn_adjust_txtime_offset(struct igc_adapter *adapter);
> -- 
> 2.25.1
> 
> 

Re: [PATCH iwl-next 5/9] igc: Add support to set MAC Merge data via ethtool

[Vladimir Oltean]

On Mon, Dec 16, 2024 at 08:13:39PM +0200, Vladimir Oltean wrote:
> Maybe the methodology for calculating these is used here?

I wanted to say: "a different methodology is used here?", sorry.

Re: [PATCH iwl-next 5/9] igc: Add support to set MAC Merge data via ethtool

[Abdul Rahim, Faizal]

Hi Vladimir,

On 17/12/2024 2:13 am, Vladimir Oltean wrote:
> On Mon, Dec 16, 2024 at 01:47:16AM -0500, Faizal Rahim wrote:
>> Created fpe_t struct to store MAC Merge data and implement the
>> "ethtool --set-mm" callback. The fpe_t struct will host other frame
>> preemption related data in future patches.
>>
>> The following fields are used to set IGC register:
>> a) pmac_enabled -> TQAVCTRL.PREEMPT_ENA
>>     This global register sets the preemption scheme, controlling
>>     preemption capabilities in transmit and receive directions, as well as
>>     the verification handshake capability.
> 
> I'm sorry, I'm not able to mentally translate this explanation into
> something technical. Which capabilities are we talking about, that this
> bit controls? I'm not clear what it does. The kernel-doc description of
> pmac_enabled is much more succinct (and at the same time, appears to
> contradict this much more elaborate yet unclear description).
> 

Sorry for the unclear explanation, I was having trouble summarizing what it 
does.

Snippets of what TQAVCTRL.PREEMPT_ENA does from i226 documentation:

RX:
When preemption is enabled by the PREEMPT_ENA flag in TQAVCTRL register, 
Express traffic is routed to the high priority packet buffer and Best 
Effort traffic is routed to the low priority packet buffer.				
The receive unit re-assemble the received fragments back to whole packets. 
It classifies the incoming mPackets as whole packets or packets fragments 
that should be re-assembled. Classification is done based on the SMD, 
Fragment Count and the CRC. Foxville categorizes received packets by SMD 
only when preemption is enabled by the TQAVCTRL.PREEMPT_ENA.

TX:
TQAVCTRL.PREEMPT_ENA (Enables the transmit preemption state machine): This 
is a dynamic parameter that can be set while the transmit unit is active. 
Setting takes affect at whole packet boundaries.
Preemptable: Each transmit queue can be set as Preemptive - eTXQ for 
express traffic or Preemptable pTXQ for non-Express traffic. The pTXQs can 
be preempted by The eTXQs when preemption is enabled by the 
TQAVCTRL.PREEMPT_ENA


Based on my testing, this register bit needs to be enabled for the i226 to 
receive verify/response frames, which is why I added the phrase 
"verification handshake capability."

Do you think I should omit the explanation of TQAVCTRL.PREEMPT_ENA ?
Or would it be better for me to attempt to summarize it more clearly in the 
next version? Hmmm.


>> b) tx_min_frag_size -> TQAVCTRL.MIN_FRAG
>>     Global register to set minimum fragments.
> 
> When you say "global register", you mean global as opposed to what?
> Per station interface?

As opposed to something like a "queue context parameter".
In the i226 documentation, the terms "global parameter" and "queue context 
parameter" are commonly used. "Global" refers to NIC-wide settings, whereas 
"queue context" refers to parameters specific to a particular TX queue. I 
thought it would be useful to highlight this distinction for the reader, 
and it aligns with the style of explanation used in the i226 documentation.

Would it be better to remove the word "Global"?

>> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> index 817838677817..1954561ec4aa 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> @@ -8,6 +8,7 @@
>>   
>>   #include "igc.h"
>>   #include "igc_diag.h"
>> +#include "igc_tsn.h"
>>   
>>   /* forward declaration */
>>   struct igc_stats {
>> @@ -1781,6 +1782,34 @@ static int igc_ethtool_set_eee(struct net_device *netdev,
>>   	return 0;
>>   }
>>   
>> +static int igc_ethtool_set_mm(struct net_device *netdev,
>> +			      struct ethtool_mm_cfg *cmd,
>> +			      struct netlink_ext_ack *extack)
>> +{
>> +	struct igc_adapter *adapter = netdev_priv(netdev);
>> +	struct fpe_t *fpe = &adapter->fpe;
>> +
>> +	if (cmd->tx_min_frag_size < IGC_TX_MIN_FRAG_SIZE ||
>> +	    cmd->tx_min_frag_size > IGC_TX_MAX_FRAG_SIZE)
>> +		NL_SET_ERR_MSG_MOD(extack,
>> +				   "Invalid value for tx-min-frag-size");
> 
> Shouldn't the execution actually stop here with an error code?
> 
I initially stop execution, but then I figured if the other parameters are 
set correctly, the feature could still work since this field has a valid 
default value set.

I'll update it to stop execution then.

>> +	else
>> +		fpe->verify_time = cmd->verify_time;
>> +
>> +	fpe->tx_enabled = cmd->tx_enabled;
>> +	fpe->pmac_enabled = cmd->pmac_enabled;
>> +	fpe->verify_enabled = cmd->verify_enabled;
>> +
>> +	return igc_tsn_offload_apply(adapter);
> 
> hmm, igc_tsn_offload_apply() is a function which always returns zero.
> It seems more natural to make it return void.
> 

You mean, to make igc_tsn_offload_apply() return void ?

Currently, igc_tsn_offload_apply() calls igc_tsn_reset(), which can return 
a non-zero value, but igc_tsn_offload_apply() doesn't use that result and 
always returns zero. It seems more logical to modify 
igc_tsn_offload_apply() to handle the return value from igc_tsn_reset().

What do you think ?

But... should this change be in this series though ?


>> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
>> index 5cd54ce435b9..b968c02f5fee 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
>> @@ -194,12 +210,22 @@ static void igc_tsn_set_retx_qbvfullthreshold(struct igc_adapter *adapter)
>>   	wr32(IGC_RETX_CTL, retxctl);
>>   }
>>   
>> +static u8 igc_fpe_get_frag_size_mult(const struct fpe_t *fpe)
>> +{
>> +	u32 tx_min_frag_size = fpe->tx_min_frag_size;
>> +	u8 mult = (tx_min_frag_size / 64) - 1;
>> +
>> +	return clamp_t(u8, mult, IGC_MIN_FOR_TX_MIN_FRAG,
>> +		       IGC_MAX_FOR_TX_MIN_FRAG);
>> +}
> 
> If you translate the continuous range of TX fragment sizes into
> discrete multipliers because that's what the hardware works with, why
> don't you just reject the non-multiple values using
> ethtool_mm_frag_size_min_to_add(), and at the same time use the output
> of that function directly to obtain your multiplier? IIUC it gets you
> the same result.
> 

Yeah, I can reuse ethtool_mm_frag_size_min_to_add(), but it will need some 
modifications. See my reply below

>> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
>> index 98ec845a86bf..08e7582f257e 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_tsn.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
>> @@ -4,6 +4,15 @@
>>   #ifndef _IGC_TSN_H_
>>   #define _IGC_TSN_H_
>>   
>> +/* IGC_TX_MIN_FRAG_SIZE is based on the MIN_FRAG field in Section 8.12.2 of the
>> + * SW User Manual.
>> + */
>> +#define IGC_TX_MIN_FRAG_SIZE		68
>> +#define IGC_TX_MAX_FRAG_SIZE		260
> 
> Odd. Is there a link to this manual (for I225 I suppose)? Standard values are 60, 124, 188, 252.
> Maybe the methodology for calculating these is used here? As things stand,
> if the driver reports these values, IIUC, openlldp gets confused and communicates
> a LLDP_8023_ADD_ETH_CAPS_ADD_FRAG_SIZE value to the link partner which is higher
> than it could have been (68 is rounded up to the next standard TX fragment size,
> which is 124). So the link partner will preempt in larger chunks and this will
> not reduce latency as much.
>> Regarding the TX minimum fragment size, I’m currently looking into it. After
>> speaking with the i226 hardware owners (Shalev Avi), I realized I may have
>> misunderstood the fragment size details. Avi mentioned that the i226
>> supports fragment sizes of 64, 128, 192, and 256 bytes (without mCRC).
>> However, these values are still 4 bytes larger than the standard you
>> mentioned.
> 
> Yes, the standard I mention is 802.1Q section 99.4.4 Transmit processing:
> 
> The earliest starting position of preemption is controlled by the addFragSize
> variable. Preemption does not occur until at least 64 x (1 + addFragSize) – 4
> octets of the preemptable frame have been sent. The addFragSize variable is set
> to the value of the addFragSize field in the received Additional Ethernet
> Capabilities TLV (see 79.3.7).
> 
> The preemptableFragSize state machine variable says that the MAC can
> preempt as soon as enough octets have been put on to wire so as to have
> a fixed minimum length (by default, a valid Ethernet packet). You're
> saying that the i226 MAC performs preemption at least 4 octets later
> than the standard says it could.
> 
>> Avi provided the following explanation for these sizes:
>>
>> "The i226 always performs preemption on 16-byte boundaries. Once we read a
>> 16-byte line from the internal packet buffer memory, we transmit it entirely
>> before deciding on preemption. This avoids keeping partial bytes from the
>> 16-byte line, ensuring that we continue with the preempted frame only after
>> finishing the current 16-byte line."
> 
> User space assumes that, when it gets an addFragSize over LLDP from the
> link partner, it can program the local transmitter to preempt at that
> fragment size, and that the operation will succeed. The formula to
> translate from addFragSize to preemptableFragSize is standard. There is
> no mechanism built into the UAPI for user space to guess, if programming
> a standard value failed, what other custom value could work.
> 
> I guess that leaves 2 options:
> (1) accept and report the standard values, but "secretly" preempt later
>      than expected
> (2) accept any values in the discrete range, and round them up to the
>      first value supported by the NIC, then report that non-standard
>      value. Then keep this as a quirk isolated to the current generation
>      of NICs, and be better with new drivers which accept standard values
>      and don't do any rounding.
> 
> I think I prefer seeing the latter variant. I'm trying to think if this
> is going to cause problems with openlldp or with the lldp_change_add_frag_size()
> selftest, but I think it's generally safe. That test only checks that
> LLDP reacts to the addFragSize of the link partner by programming its
> local addFragSize to the same value. It doesn't check that the
> tx-min-frag-size is exactly equal to the formula derived from that
> addFragSize.

Thanks for the suggestion! I was actually about to suggest option 2 myself, 
but you got there first.

To recap:

Standard range: 60, 124, 188, 252 (without mCRC).
i226 range: 64, 128, 192, 256 (without mCRC).

The current IGC_TX_MIN_FRAG_SIZE is incorrectly set to 68 due to our 
misinterpretation of the i226 documentation:
"The minimum size for non-final preempted fragments is 64 * (1 + MIN_FRAG) 
+ 4 (mCRC)."

The calculation above is for the fragment size on the wire, including mCRC. 
For the TX preemption point and pure fragment size, mCRC should not be 
included, as confirmed by the hardware owner.

On RX, i226 can handle any size, even the standard minimum of 60 octets 
(without mCRC).

What would be ideal for i226:
Min frag user set 60:64 → Multiplier = 0.
Min frag user set 65:128 → Multiplier = 1.
(And so on)

To make this work and reuse the existing code, we’d need to tweak these two 
functions:
ethtool_mm_frag_size_add_to_min(val_min, xxx)
ethtool_mm_frag_size_min_to_add(xx)

With the current code, if I pass 64 octets as val_min to 
ethtool_mm_frag_size_add_to_min(), it returns error.

Proposed modification:
Add a new parameter to ethtool_mm_frag_size_min_to_add() - maybe let's call 
it dev_min_tx_frag_len.

Set dev_min_tx_frag_len = 64 for i226, 60 for other drivers.
This field will be used to:
(1) modify the calculation in ethtool_mm_frag_size_min_to_add()
(2) as a warning prompt to user when the value is not standard, done in 
ethtool_mm_frag_size_add_to_min()

I was thinking (1) would modify the existing:
u32 ethtool_mm_frag_size_add_to_min(u32 val_add)
{
	return (ETH_ZLEN + ETH_FCS_LEN) * (1 + val_add) - ETH_FCS_LEN;
}

To something like:
u32 ethtool_mm_frag_size_add_to_min(u32 val_add, u32 dev_min_tx_frag_len)
{
     return dev_min_tx_frag_len + (val_add * 64);
}

So this will yield:
Standard range (dev_min_tx_frag_len = 60): 60, 124, 188, 252
i226 range (dev_min_tx_frag_len = 64): 64, 128, 192, 256

But what's not so nice is, the rest of other drivers have to set this new 
param when calling ethtool_mm_frag_size_add_to_min().

Is something like this okay ? I'm open to better suggestion.

Re: [PATCH iwl-next 5/9] igc: Add support to set MAC Merge data via ethtool

[Vladimir Oltean]

Hi Faizal,

On Mon, Dec 23, 2024 at 05:23:27PM +0800, Abdul Rahim, Faizal wrote:
> To recap:
> 
> Standard range: 60, 124, 188, 252 (without mCRC).
> i226 range: 64, 128, 192, 256 (without mCRC).
> 
> The current IGC_TX_MIN_FRAG_SIZE is incorrectly set to 68 due to our
> misinterpretation of the i226 documentation:
> "The minimum size for non-final preempted fragments is 64 * (1 + MIN_FRAG) +
> 4 (mCRC)."
> 
> The calculation above is for the fragment size on the wire, including mCRC.
> For the TX preemption point and pure fragment size, mCRC should not be
> included, as confirmed by the hardware owner.
> 
> On RX, i226 can handle any size, even the standard minimum of 60 octets
> (without mCRC).
> 
> What would be ideal for i226:
> Min frag user set 60:64 → Multiplier = 0.
> Min frag user set 65:128 → Multiplier = 1.
> (And so on)
> 
> To make this work and reuse the existing code, we’d need to tweak these two
> functions:
> ethtool_mm_frag_size_add_to_min(val_min, xxx)
> ethtool_mm_frag_size_min_to_add(xx)
> 
> With the current code, if I pass 64 octets as val_min to
> ethtool_mm_frag_size_add_to_min(), it returns error.
> 
> Proposed modification:
> Add a new parameter to ethtool_mm_frag_size_min_to_add() - maybe let's call
> it dev_min_tx_frag_len.
> 
> Set dev_min_tx_frag_len = 64 for i226, 60 for other drivers.
> This field will be used to:
> (1) modify the calculation in ethtool_mm_frag_size_min_to_add()
> (2) as a warning prompt to user when the value is not standard, done in
> ethtool_mm_frag_size_add_to_min()
> 
> I was thinking (1) would modify the existing:
> u32 ethtool_mm_frag_size_add_to_min(u32 val_add)
> {
> 	return (ETH_ZLEN + ETH_FCS_LEN) * (1 + val_add) - ETH_FCS_LEN;
> }
> 
> To something like:
> u32 ethtool_mm_frag_size_add_to_min(u32 val_add, u32 dev_min_tx_frag_len)
> {
>     return dev_min_tx_frag_len + (val_add * 64);
> }
> 
> So this will yield:
> Standard range (dev_min_tx_frag_len = 60): 60, 124, 188, 252
> i226 range (dev_min_tx_frag_len = 64): 64, 128, 192, 256
> 
> But what's not so nice is, the rest of other drivers have to set this new
> param when calling ethtool_mm_frag_size_add_to_min().
> 
> Is something like this okay ? I'm open to better suggestion.

I'm taking a break probably for the rest of the year, and spending time
for the Christmas holidays mostly off lists.

I didn't look through all your replies. Just regarding the one quoted
above: just don't use the ethtool_mm_frag_size_add_to_min() and
ethtool_mm_frag_size_min_to_add() helpers if they aren't useful as-is.
They are designed for a standardized NIC implementation. They are opt-in
from driver code for a reason.

[PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Faizal Rahim]

The i226 hardware doesn't implement the process of verification
internally, this is left to the driver.

Add a simple implementation of the state machine defined in IEEE
802.3-2018, Section 99.4.7. The state machine is started manually by
user after "verify-enabled" command is enabled.

Implementation includes:
1. Send and receive verify frame
2. Verification state handling
3. Send and receive response frame

Tested by triggering verification handshake:
$ sudo ethtool --set-mm enp1s0 pmac-enabled on
$ sudo ethtool --set-mm enp1s0 tx-enabled on
$ sudo ethtool --set-mm enp1s0 verify-enabled on

Note that Ethtool API requires enabling "pmac-enabled on" and
"tx-enabled on" before "verify-enabled on" can be issued.

After the upcoming patch ("igc: Add support to get MAC Merge data via
ethtool") is implemented, verification status can be checked using:
$ ethtool --show-mm enp1s0
  MAC Merge layer state for enp1s0:
  pMAC enabled: on
  TX enabled: on
  TX active: on
  TX minimum fragment size: 252
  RX minimum fragment size: 252
  Verify enabled: on
  Verify time: 128
  Max verify time: 128
  Verification status: SUCCEEDED

Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  16 ++
 drivers/net/ethernet/intel/igc/igc_defines.h |   6 +
 drivers/net/ethernet/intel/igc/igc_ethtool.c |   8 +-
 drivers/net/ethernet/intel/igc/igc_main.c    |  15 +-
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 230 +++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_tsn.h     |   4 +
 6 files changed, 277 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 5a14e9101723..56a426765be7 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -40,6 +40,15 @@ void igc_ethtool_set_ops(struct net_device *);
 
 #define IGC_MAX_TX_TSTAMP_REGS		4
 
+/* Verification state defined as per section 30.14.1.2 in 802.3br spec */
+enum verify_state {
+	VERIFY_FAIL,
+	INIT_VERIFICATION,
+	VERIFIED,
+	SEND_VERIFY,
+	WAIT_FOR_RESPONSE,
+};
+
 /**
  * @verify_time: see struct ethtool_mm_state
  * @verify_enabled: see struct ethtool_mm_state
@@ -52,6 +61,12 @@ void igc_ethtool_set_ops(struct net_device *);
  * @tx_min_frag_size: see struct ethtool_mm_state
  */
 struct fpe_t {
+	struct delayed_work verification_work;
+	unsigned long verify_timeout;
+	bool received_smd_v;
+	bool received_smd_r;
+	unsigned int verify_cnt;
+	enum verify_state verify_state;
 	u32 verify_time;
 	bool verify_enabled;
 	bool tx_enabled;
@@ -758,6 +773,7 @@ int igc_add_nfc_rule(struct igc_adapter *adapter, struct igc_nfc_rule *rule);
 void igc_del_nfc_rule(struct igc_adapter *adapter, struct igc_nfc_rule *rule);
 int igc_enable_empty_addr_recv(struct igc_adapter *adapter);
 struct igc_ring *igc_get_tx_ring(struct igc_adapter *adapter, int cpu);
+void igc_flush_tx_descriptors(struct igc_ring *ring);
 void igc_ptp_init(struct igc_adapter *adapter);
 void igc_ptp_reset(struct igc_adapter *adapter);
 void igc_ptp_suspend(struct igc_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 3088cdd08f35..ba96776d5854 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -308,6 +308,8 @@
 #define IGC_TXD_DTYP_C		0x00000000 /* Context Descriptor */
 #define IGC_TXD_POPTS_IXSM	0x01       /* Insert IP checksum */
 #define IGC_TXD_POPTS_TXSM	0x02       /* Insert TCP/UDP checksum */
+#define IGC_TXD_POPTS_SMD_V	0x10       /* Transmitted packet is a SMD-Verify */
+#define IGC_TXD_POPTS_SMD_R	0x20       /* Transmitted packet is a SMD-Response */
 #define IGC_TXD_CMD_EOP		0x01000000 /* End of Packet */
 #define IGC_TXD_CMD_IC		0x04000000 /* Insert Checksum */
 #define IGC_TXD_CMD_DEXT	0x20000000 /* Desc extension (0 = legacy) */
@@ -370,9 +372,13 @@
 #define IGC_RXD_STAT_VP		0x08	/* IEEE VLAN Packet */
 
 #define IGC_RXDEXT_STATERR_LB	0x00040000
+#define IGC_RXD_STAT_SMD_V	0x2000  /* SMD-Verify packet */
+#define IGC_RXD_STAT_SMD_R	0x4000  /* SMD-Response packet */
 
 /* Advanced Receive Descriptor bit definitions */
 #define IGC_RXDADV_STAT_TSIP	0x08000 /* timestamp in packet */
+#define IGC_RXDADV_STAT_SMD_TYPE_MASK	0x06000
+#define IGC_RXDADV_STAT_SMD_TYPE_SHIFT	13
 
 #define IGC_RXDEXT_STATERR_L4E		0x20000000
 #define IGC_RXDEXT_STATERR_IPE		0x40000000
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 1954561ec4aa..7cde0e5a7320 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1788,6 +1788,7 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
 	struct fpe_t *fpe = &adapter->fpe;
+	bool verify_enabled_changed;
 
 	if (cmd->tx_min_frag_size < IGC_TX_MIN_FRAG_SIZE ||
 	    cmd->tx_min_frag_size > IGC_TX_MAX_FRAG_SIZE)
@@ -1805,7 +1806,12 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
 
 	fpe->tx_enabled = cmd->tx_enabled;
 	fpe->pmac_enabled = cmd->pmac_enabled;
-	fpe->verify_enabled = cmd->verify_enabled;
+	verify_enabled_changed = (cmd->verify_enabled != fpe->verify_enabled);
+
+	if (verify_enabled_changed) {
+		fpe->verify_enabled = cmd->verify_enabled;
+		igc_fpe_verify_enabled_changed(fpe);
+	}
 
 	return igc_tsn_offload_apply(adapter);
 }
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index b85eaf34d07b..e184959ef218 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2534,7 +2534,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter,
 }
 
 /* This function assumes __netif_tx_lock is held by the caller. */
-static void igc_flush_tx_descriptors(struct igc_ring *ring)
+void igc_flush_tx_descriptors(struct igc_ring *ring)
 {
 	/* Once tail pointer is updated, hardware can fetch the descriptors
 	 * any time so we issue a write membar here to ensure all memory
@@ -2585,6 +2585,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 	struct sk_buff *skb = rx_ring->skb;
 	u16 cleaned_count = igc_desc_unused(rx_ring);
 	int xdp_status = 0, rx_buffer_pgcnt;
+	int smd_type;
 
 	while (likely(total_packets < budget)) {
 		struct igc_xdp_buff ctx = { .rx_ts = NULL };
@@ -2622,6 +2623,18 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 			size -= IGC_TS_HDR_LEN;
 		}
 
+		smd_type = igc_fpe_get_smd_type(rx_desc->wb.upper.status_error);
+
+		if (igc_fpe_is_verify_or_response(smd_type, size)) {
+			igc_fpe_preprocess_verify_response(&adapter->fpe,
+							   smd_type);
+
+			/* Advance the ring next-to-clean */
+			igc_is_non_eop(rx_ring, rx_desc);
+			cleaned_count++;
+			continue;
+		}
+
 		if (!skb) {
 			xdp_init_buff(&ctx.xdp, truesize, &rx_ring->xdp_rxq);
 			xdp_prepare_buff(&ctx.xdp, pktbuf - igc_rx_offset(rx_ring),
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index b968c02f5fee..3d39be2219f3 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -1,22 +1,252 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c)  2019 Intel Corporation */
 
+#include <linux/kernel.h>
 #include "igc.h"
+#include "igc_base.h"
 #include "igc_hw.h"
 #include "igc_tsn.h"
 
 #define DEFAULT_VERIFY_TIME		10
+
+#define IGC_SMD_TYPE_SMD_V		0x1
+#define IGC_SMD_TYPE_SMD_R		0x2
 #define IGC_MIN_FOR_TX_MIN_FRAG		0
 #define IGC_MAX_FOR_TX_MIN_FRAG		3
 
+#define MAX_VERIFY_CNT			3
+#define SMD_FRAME_SIZE			60
+#define VERIFY_RESPONSE_DELAY		10
+
+static int igc_fpe_init_smd_frame(struct igc_ring *ring,
+				  struct igc_tx_buffer *buffer,
+				  struct sk_buff *skb)
+{
+	unsigned int size = skb_headlen(skb);
+	dma_addr_t dma;
+
+	dma = dma_map_single(ring->dev, skb->data, size, DMA_TO_DEVICE);
+
+	if (dma_mapping_error(ring->dev, dma)) {
+		netdev_err_once(ring->netdev, "Failed to map DMA for TX\n");
+		return -ENOMEM;
+	}
+
+	buffer->skb = skb;
+	buffer->protocol = 0;
+	buffer->bytecount = skb->len;
+	buffer->gso_segs = 1;
+	buffer->time_stamp = jiffies;
+	dma_unmap_len_set(buffer, len, skb->len);
+	dma_unmap_addr_set(buffer, dma, dma);
+
+	return 0;
+}
+
+static int igc_fpe_init_tx_descriptor(struct igc_ring *ring,
+				      struct sk_buff *skb, int type)
+{
+	struct igc_tx_buffer *buffer;
+	union igc_adv_tx_desc *desc;
+	u32 cmd_type, olinfo_status;
+	int err;
+
+	if (!igc_desc_unused(ring))
+		return -EBUSY;
+
+	if (type != IGC_SMD_TYPE_SMD_V && type != IGC_SMD_TYPE_SMD_R)
+		return -EINVAL;
+
+	buffer = &ring->tx_buffer_info[ring->next_to_use];
+	err = igc_fpe_init_smd_frame(ring, buffer, skb);
+	if (err)
+		return err;
+
+	cmd_type = IGC_ADVTXD_DTYP_DATA | IGC_ADVTXD_DCMD_DEXT |
+		   IGC_ADVTXD_DCMD_IFCS | IGC_TXD_DCMD |
+		   buffer->bytecount;
+	olinfo_status = buffer->bytecount << IGC_ADVTXD_PAYLEN_SHIFT;
+
+	switch (type) {
+	case IGC_SMD_TYPE_SMD_V:
+		olinfo_status |= (IGC_TXD_POPTS_SMD_V << 8);
+		break;
+	case IGC_SMD_TYPE_SMD_R:
+		olinfo_status |= (IGC_TXD_POPTS_SMD_R << 8);
+		break;
+	}
+
+	desc = IGC_TX_DESC(ring, ring->next_to_use);
+	desc->read.cmd_type_len = cpu_to_le32(cmd_type);
+	desc->read.olinfo_status = cpu_to_le32(olinfo_status);
+	desc->read.buffer_addr = cpu_to_le64(dma_unmap_addr(buffer, dma));
+
+	netdev_tx_sent_queue(txring_txq(ring), skb->len);
+
+	buffer->next_to_watch = desc;
+	ring->next_to_use = (ring->next_to_use + 1) % ring->count;
+
+	return 0;
+}
+
+static int igc_fpe_xmit_smd_frame(struct igc_adapter *adapter, int type)
+{
+	int cpu = smp_processor_id();
+	struct netdev_queue *nq;
+	struct igc_ring *ring;
+	struct sk_buff *skb;
+	void *data;
+	int err;
+
+	if (!netif_running(adapter->netdev))
+		return -ENOTCONN;
+
+	ring = igc_get_tx_ring(adapter, cpu);
+	nq = txring_txq(ring);
+
+	skb = alloc_skb(SMD_FRAME_SIZE, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	data = skb_put(skb, SMD_FRAME_SIZE);
+	memset(data, 0, SMD_FRAME_SIZE);
+
+	__netif_tx_lock(nq, cpu);
+
+	err = igc_fpe_init_tx_descriptor(ring, skb, type);
+	igc_flush_tx_descriptors(ring);
+
+	__netif_tx_unlock(nq);
+
+	return err;
+}
+
+static void igc_fpe_send_response(struct igc_adapter *adapter)
+{
+	int err = igc_fpe_xmit_smd_frame(adapter, IGC_SMD_TYPE_SMD_R);
+
+	if (err)
+		netdev_err(adapter->netdev, "Error sending SMD-R frame\n");
+}
+
+static void igc_fpe_handle_verify(struct igc_adapter *adapter)
+{
+	struct fpe_t *fpe = &adapter->fpe;
+	unsigned long verify_time_jiffies;
+	int err;
+
+	switch (fpe->verify_state) {
+	case SEND_VERIFY:
+		fpe->received_smd_r = false;
+		err = igc_fpe_xmit_smd_frame(adapter, IGC_SMD_TYPE_SMD_V);
+
+		if (err)
+			netdev_err(adapter->netdev, "Error sending SMD-V\n");
+
+		fpe->verify_state = WAIT_FOR_RESPONSE;
+		verify_time_jiffies = msecs_to_jiffies(fpe->verify_time);
+		fpe->verify_timeout = jiffies + verify_time_jiffies;
+
+		schedule_delayed_work(&fpe->verification_work,
+				      verify_time_jiffies);
+		break;
+
+	case WAIT_FOR_RESPONSE:
+		if (fpe->received_smd_r) {
+			fpe->verify_state = VERIFIED;
+			fpe->received_smd_r = false;
+		} else if (time_is_before_jiffies(fpe->verify_timeout)) {
+			fpe->verify_cnt++;
+			netdev_warn(adapter->netdev,
+				    "Timeout waiting for SMD-R frame\n");
+
+			if (fpe->verify_cnt > MAX_VERIFY_CNT) {
+				fpe->verify_state = VERIFY_FAIL;
+				netdev_err(adapter->netdev,
+					   "Exceeded attempts sending SMD-V\n");
+			} else {
+				fpe->verify_state = SEND_VERIFY;
+				igc_fpe_handle_verify(adapter);
+			}
+		}
+		break;
+
+	case VERIFY_FAIL:
+	case VERIFIED:
+	case INIT_VERIFICATION:
+		break;
+	}
+}
+
+static void igc_fpe_verification(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct igc_adapter *adapter;
+	struct fpe_t *fpe;
+
+	fpe = container_of(dwork, struct fpe_t, verification_work);
+	adapter = container_of(fpe, struct igc_adapter, fpe);
+
+	if (fpe->received_smd_v) {
+		igc_fpe_send_response(adapter);
+		fpe->received_smd_v = false;
+	}
+
+	if (fpe->verify_enabled)
+		igc_fpe_handle_verify(adapter);
+}
+
 void igc_fpe_init(struct fpe_t *fpe)
 {
+	INIT_DELAYED_WORK(&fpe->verification_work, igc_fpe_verification);
 	fpe->verify_enabled = false;
+	fpe->verify_state = INIT_VERIFICATION;
 	fpe->verify_time = DEFAULT_VERIFY_TIME;
+	fpe->received_smd_v = false;
+	fpe->received_smd_r = false;
+	fpe->verify_cnt = 0;
 	fpe->pmac_enabled = false;
 	fpe->tx_min_frag_size = IGC_TX_MIN_FRAG_SIZE;
 }
 
+void igc_fpe_verify_enabled_changed(struct fpe_t *fpe)
+{
+	if (fpe->verify_enabled && fpe->tx_enabled) {
+		fpe->verify_state = SEND_VERIFY;
+		schedule_delayed_work(&fpe->verification_work,
+				      msecs_to_jiffies(VERIFY_RESPONSE_DELAY));
+	} else {
+		fpe->verify_state = INIT_VERIFICATION;
+		fpe->received_smd_v = false;
+		fpe->received_smd_r = false;
+		fpe->verify_cnt = 0;
+	}
+}
+
+int igc_fpe_get_smd_type(__le32 status_error)
+{
+	u32 status = le32_to_cpu(status_error);
+
+	return (status & IGC_RXDADV_STAT_SMD_TYPE_MASK)
+		>> IGC_RXDADV_STAT_SMD_TYPE_SHIFT;
+}
+
+bool igc_fpe_is_verify_or_response(int smd_type, unsigned int size)
+{
+	return ((smd_type == IGC_SMD_TYPE_SMD_V ||
+		 smd_type == IGC_SMD_TYPE_SMD_R) && size == SMD_FRAME_SIZE);
+}
+
+void igc_fpe_preprocess_verify_response(struct fpe_t *fpe, int smd_type)
+{
+	if (smd_type == IGC_SMD_TYPE_SMD_V)
+		fpe->received_smd_v = true;
+	else if (smd_type == IGC_SMD_TYPE_SMD_R)
+		fpe->received_smd_r = true;
+
+	schedule_delayed_work(&fpe->verification_work, 0);
+}
+
 static bool is_any_launchtime(struct igc_adapter *adapter)
 {
 	int i;
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
index 08e7582f257e..f3d83fbbd1f4 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.h
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
@@ -12,7 +12,11 @@
 #define MIN_VERIFY_TIME			1
 #define MAX_VERIFY_TIME			128
 
+int igc_fpe_get_smd_type(__le32 status_error);
 void igc_fpe_init(struct fpe_t *fpe);
+bool igc_fpe_is_verify_or_response(int smd_type, unsigned int size);
+void igc_fpe_preprocess_verify_response(struct fpe_t *fpe, int smd_type);
+void igc_fpe_verify_enabled_changed(struct fpe_t *fpe);
 int igc_tsn_offload_apply(struct igc_adapter *adapter);
 int igc_tsn_reset(struct igc_adapter *adapter);
 void igc_tsn_adjust_txtime_offset(struct igc_adapter *adapter);
-- 
2.25.1

Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Vladimir Oltean]

[-- Attachment #1: Type: text/plain, Size: 17587 bytes --]

On Mon, Dec 16, 2024 at 01:47:17AM -0500, Faizal Rahim wrote:
> The i226 hardware doesn't implement the process of verification
> internally, this is left to the driver.
> 
> Add a simple implementation of the state machine defined in IEEE
> 802.3-2018, Section 99.4.7. The state machine is started manually by
> user after "verify-enabled" command is enabled.
> 
> Implementation includes:
> 1. Send and receive verify frame
> 2. Verification state handling
> 3. Send and receive response frame
> 
> Tested by triggering verification handshake:
> $ sudo ethtool --set-mm enp1s0 pmac-enabled on
> $ sudo ethtool --set-mm enp1s0 tx-enabled on
> $ sudo ethtool --set-mm enp1s0 verify-enabled on
> 
> Note that Ethtool API requires enabling "pmac-enabled on" and
> "tx-enabled on" before "verify-enabled on" can be issued.
> 
> After the upcoming patch ("igc: Add support to get MAC Merge data via
> ethtool") is implemented, verification status can be checked using:
> $ ethtool --show-mm enp1s0
>   MAC Merge layer state for enp1s0:
>   pMAC enabled: on
>   TX enabled: on
>   TX active: on
>   TX minimum fragment size: 252
>   RX minimum fragment size: 252
>   Verify enabled: on
>   Verify time: 128
>   Max verify time: 128
>   Verification status: SUCCEEDED
> 
> Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
> ---

Am I missing something, or this does not handle link state changes,
where the verification should restart on each link up? (maybe the old
link partner didn't support FPE and the new one does, or vice versa)

Either I don't follow the link between igc_watchdog_task() and any
verification related task, or it doesn't exist.

Anyway, while browsing through this software implementation of a
verification process, I cannot help but think we'd be making a huge
mistake to allow each driver to reimplement it on its own. We just
recently got stmmac to do something fairly clean, with the help and
great perseverence of Furong Xu (now copied).

I spent a bit of time extracting stmmac's core logic and putting it in
ethtool. If Furong had such good will so as to regression-test the
attached patch, do you think you could use this as a starting place
instead, and implement some ops and call some library methods, instead
of writing the entire logic yourself?

>  drivers/net/ethernet/intel/igc/igc.h         |  16 ++
>  drivers/net/ethernet/intel/igc/igc_defines.h |   6 +
>  drivers/net/ethernet/intel/igc/igc_ethtool.c |   8 +-
>  drivers/net/ethernet/intel/igc/igc_main.c    |  15 +-
>  drivers/net/ethernet/intel/igc/igc_tsn.c     | 230 +++++++++++++++++++
>  drivers/net/ethernet/intel/igc/igc_tsn.h     |   4 +
>  6 files changed, 277 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 5a14e9101723..56a426765be7 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -40,6 +40,15 @@ void igc_ethtool_set_ops(struct net_device *);
>  
>  #define IGC_MAX_TX_TSTAMP_REGS		4
>  
> +/* Verification state defined as per section 30.14.1.2 in 802.3br spec */
> +enum verify_state {
> +	VERIFY_FAIL,
> +	INIT_VERIFICATION,
> +	VERIFIED,
> +	SEND_VERIFY,
> +	WAIT_FOR_RESPONSE,
> +};
> +
>  /**
>   * @verify_time: see struct ethtool_mm_state
>   * @verify_enabled: see struct ethtool_mm_state
> @@ -52,6 +61,12 @@ void igc_ethtool_set_ops(struct net_device *);
>   * @tx_min_frag_size: see struct ethtool_mm_state
>   */
>  struct fpe_t {
> +	struct delayed_work verification_work;
> +	unsigned long verify_timeout;
> +	bool received_smd_v;
> +	bool received_smd_r;
> +	unsigned int verify_cnt;
> +	enum verify_state verify_state;

Should have updated the kernel-doc if you started a kernel-doc scheme.

>  	u32 verify_time;
>  	bool verify_enabled;
>  	bool tx_enabled;
> @@ -758,6 +773,7 @@ int igc_add_nfc_rule(struct igc_adapter *adapter, struct igc_nfc_rule *rule);
>  void igc_del_nfc_rule(struct igc_adapter *adapter, struct igc_nfc_rule *rule);
>  int igc_enable_empty_addr_recv(struct igc_adapter *adapter);
>  struct igc_ring *igc_get_tx_ring(struct igc_adapter *adapter, int cpu);
> +void igc_flush_tx_descriptors(struct igc_ring *ring);
>  void igc_ptp_init(struct igc_adapter *adapter);
>  void igc_ptp_reset(struct igc_adapter *adapter);
>  void igc_ptp_suspend(struct igc_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 3088cdd08f35..ba96776d5854 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -308,6 +308,8 @@
>  #define IGC_TXD_DTYP_C		0x00000000 /* Context Descriptor */
>  #define IGC_TXD_POPTS_IXSM	0x01       /* Insert IP checksum */
>  #define IGC_TXD_POPTS_TXSM	0x02       /* Insert TCP/UDP checksum */
> +#define IGC_TXD_POPTS_SMD_V	0x10       /* Transmitted packet is a SMD-Verify */
> +#define IGC_TXD_POPTS_SMD_R	0x20       /* Transmitted packet is a SMD-Response */
>  #define IGC_TXD_CMD_EOP		0x01000000 /* End of Packet */
>  #define IGC_TXD_CMD_IC		0x04000000 /* Insert Checksum */
>  #define IGC_TXD_CMD_DEXT	0x20000000 /* Desc extension (0 = legacy) */
> @@ -370,9 +372,13 @@
>  #define IGC_RXD_STAT_VP		0x08	/* IEEE VLAN Packet */
>  
>  #define IGC_RXDEXT_STATERR_LB	0x00040000
> +#define IGC_RXD_STAT_SMD_V	0x2000  /* SMD-Verify packet */
> +#define IGC_RXD_STAT_SMD_R	0x4000  /* SMD-Response packet */
>  
>  /* Advanced Receive Descriptor bit definitions */
>  #define IGC_RXDADV_STAT_TSIP	0x08000 /* timestamp in packet */
> +#define IGC_RXDADV_STAT_SMD_TYPE_MASK	0x06000
> +#define IGC_RXDADV_STAT_SMD_TYPE_SHIFT	13
>  
>  #define IGC_RXDEXT_STATERR_L4E		0x20000000
>  #define IGC_RXDEXT_STATERR_IPE		0x40000000
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 1954561ec4aa..7cde0e5a7320 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1788,6 +1788,7 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
>  {
>  	struct igc_adapter *adapter = netdev_priv(netdev);
>  	struct fpe_t *fpe = &adapter->fpe;
> +	bool verify_enabled_changed;
>  
>  	if (cmd->tx_min_frag_size < IGC_TX_MIN_FRAG_SIZE ||
>  	    cmd->tx_min_frag_size > IGC_TX_MAX_FRAG_SIZE)
> @@ -1805,7 +1806,12 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
>  
>  	fpe->tx_enabled = cmd->tx_enabled;
>  	fpe->pmac_enabled = cmd->pmac_enabled;
> -	fpe->verify_enabled = cmd->verify_enabled;
> +	verify_enabled_changed = (cmd->verify_enabled != fpe->verify_enabled);

I wonder if it's worth using an intermediary variable when the result is
only evaluated once. The intention is clear enough already, you call a
function named igc_fpe_verify_enabled_changed().

> +
> +	if (verify_enabled_changed) {
> +		fpe->verify_enabled = cmd->verify_enabled;
> +		igc_fpe_verify_enabled_changed(fpe);
> +	}
>  
>  	return igc_tsn_offload_apply(adapter);
>  }
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index b85eaf34d07b..e184959ef218 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -2534,7 +2534,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter,
>  }
>  
>  /* This function assumes __netif_tx_lock is held by the caller. */
> -static void igc_flush_tx_descriptors(struct igc_ring *ring)
> +void igc_flush_tx_descriptors(struct igc_ring *ring)
>  {
>  	/* Once tail pointer is updated, hardware can fetch the descriptors
>  	 * any time so we issue a write membar here to ensure all memory
> @@ -2585,6 +2585,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  	struct sk_buff *skb = rx_ring->skb;
>  	u16 cleaned_count = igc_desc_unused(rx_ring);
>  	int xdp_status = 0, rx_buffer_pgcnt;
> +	int smd_type;
>  
>  	while (likely(total_packets < budget)) {
>  		struct igc_xdp_buff ctx = { .rx_ts = NULL };
> @@ -2622,6 +2623,18 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  			size -= IGC_TS_HDR_LEN;
>  		}
>  
> +		smd_type = igc_fpe_get_smd_type(rx_desc->wb.upper.status_error);
> +
> +		if (igc_fpe_is_verify_or_response(smd_type, size)) {
> +			igc_fpe_preprocess_verify_response(&adapter->fpe,
> +							   smd_type);
> +
> +			/* Advance the ring next-to-clean */
> +			igc_is_non_eop(rx_ring, rx_desc);
> +			cleaned_count++;
> +			continue;
> +		}
> +

Premature optimization is the root of all evil, I know, but in the
future it might be interesting to add a static key here that gets
incremented (enabled) based on pmac_enabled, such that the fast path
does not get to suffer a performance penalty when frame preemption is
supported in the kernel, regardless of whether it is enabled or not.

>  		if (!skb) {
>  			xdp_init_buff(&ctx.xdp, truesize, &rx_ring->xdp_rxq);
>  			xdp_prepare_buff(&ctx.xdp, pktbuf - igc_rx_offset(rx_ring),
> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
> index b968c02f5fee..3d39be2219f3 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
> @@ -1,22 +1,252 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright (c)  2019 Intel Corporation */
>  
> +#include <linux/kernel.h>
>  #include "igc.h"
> +#include "igc_base.h"
>  #include "igc_hw.h"
>  #include "igc_tsn.h"
>  
>  #define DEFAULT_VERIFY_TIME		10
> +
> +#define IGC_SMD_TYPE_SMD_V		0x1
> +#define IGC_SMD_TYPE_SMD_R		0x2
>  #define IGC_MIN_FOR_TX_MIN_FRAG		0
>  #define IGC_MAX_FOR_TX_MIN_FRAG		3
>  
> +#define MAX_VERIFY_CNT			3
> +#define SMD_FRAME_SIZE			60
> +#define VERIFY_RESPONSE_DELAY		10
> +
> +static int igc_fpe_init_smd_frame(struct igc_ring *ring,
> +				  struct igc_tx_buffer *buffer,
> +				  struct sk_buff *skb)
> +{
> +	unsigned int size = skb_headlen(skb);
> +	dma_addr_t dma;
> +
> +	dma = dma_map_single(ring->dev, skb->data, size, DMA_TO_DEVICE);
> +
> +	if (dma_mapping_error(ring->dev, dma)) {
> +		netdev_err_once(ring->netdev, "Failed to map DMA for TX\n");
> +		return -ENOMEM;
> +	}
> +
> +	buffer->skb = skb;
> +	buffer->protocol = 0;
> +	buffer->bytecount = skb->len;
> +	buffer->gso_segs = 1;
> +	buffer->time_stamp = jiffies;
> +	dma_unmap_len_set(buffer, len, skb->len);

Not an expert here, but I believe a common source of DMA API bugs is
using different expressions for the map and unmap length.

Here, IIUC, alloc_skb() always returns a linear packet, so there's no
paged memory and skb->len == skb_headlen(). But it would still look
cleaner to my non-expert eye to use the same expression for both mapping
and unmapping operations.

> +	dma_unmap_addr_set(buffer, dma, dma);
> +
> +	return 0;
> +}
> +
> +static int igc_fpe_init_tx_descriptor(struct igc_ring *ring,
> +				      struct sk_buff *skb, int type)
> +{
> +	struct igc_tx_buffer *buffer;
> +	union igc_adv_tx_desc *desc;
> +	u32 cmd_type, olinfo_status;
> +	int err;
> +
> +	if (!igc_desc_unused(ring))
> +		return -EBUSY;
> +
> +	if (type != IGC_SMD_TYPE_SMD_V && type != IGC_SMD_TYPE_SMD_R)
> +		return -EINVAL;
> +
> +	buffer = &ring->tx_buffer_info[ring->next_to_use];
> +	err = igc_fpe_init_smd_frame(ring, buffer, skb);
> +	if (err)
> +		return err;
> +
> +	cmd_type = IGC_ADVTXD_DTYP_DATA | IGC_ADVTXD_DCMD_DEXT |
> +		   IGC_ADVTXD_DCMD_IFCS | IGC_TXD_DCMD |
> +		   buffer->bytecount;
> +	olinfo_status = buffer->bytecount << IGC_ADVTXD_PAYLEN_SHIFT;
> +
> +	switch (type) {
> +	case IGC_SMD_TYPE_SMD_V:
> +		olinfo_status |= (IGC_TXD_POPTS_SMD_V << 8);
> +		break;
> +	case IGC_SMD_TYPE_SMD_R:
> +		olinfo_status |= (IGC_TXD_POPTS_SMD_R << 8);
> +		break;
> +	}
> +
> +	desc = IGC_TX_DESC(ring, ring->next_to_use);
> +	desc->read.cmd_type_len = cpu_to_le32(cmd_type);
> +	desc->read.olinfo_status = cpu_to_le32(olinfo_status);
> +	desc->read.buffer_addr = cpu_to_le64(dma_unmap_addr(buffer, dma));
> +
> +	netdev_tx_sent_queue(txring_txq(ring), skb->len);
> +
> +	buffer->next_to_watch = desc;
> +	ring->next_to_use = (ring->next_to_use + 1) % ring->count;
> +
> +	return 0;
> +}
> +
> +static int igc_fpe_xmit_smd_frame(struct igc_adapter *adapter, int type)
> +{
> +	int cpu = smp_processor_id();
> +	struct netdev_queue *nq;
> +	struct igc_ring *ring;
> +	struct sk_buff *skb;
> +	void *data;
> +	int err;
> +
> +	if (!netif_running(adapter->netdev))
> +		return -ENOTCONN;
> +
> +	ring = igc_get_tx_ring(adapter, cpu);
> +	nq = txring_txq(ring);
> +
> +	skb = alloc_skb(SMD_FRAME_SIZE, GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	data = skb_put(skb, SMD_FRAME_SIZE);
> +	memset(data, 0, SMD_FRAME_SIZE);
> +
> +	__netif_tx_lock(nq, cpu);
> +
> +	err = igc_fpe_init_tx_descriptor(ring, skb, type);
> +	igc_flush_tx_descriptors(ring);
> +
> +	__netif_tx_unlock(nq);
> +
> +	return err;
> +}
> +
> +static void igc_fpe_send_response(struct igc_adapter *adapter)
> +{
> +	int err = igc_fpe_xmit_smd_frame(adapter, IGC_SMD_TYPE_SMD_R);
> +
> +	if (err)
> +		netdev_err(adapter->netdev, "Error sending SMD-R frame\n");
> +}
> +
> +static void igc_fpe_handle_verify(struct igc_adapter *adapter)
> +{
> +	struct fpe_t *fpe = &adapter->fpe;
> +	unsigned long verify_time_jiffies;
> +	int err;
> +
> +	switch (fpe->verify_state) {
> +	case SEND_VERIFY:
> +		fpe->received_smd_r = false;
> +		err = igc_fpe_xmit_smd_frame(adapter, IGC_SMD_TYPE_SMD_V);
> +
> +		if (err)
> +			netdev_err(adapter->netdev, "Error sending SMD-V\n");
> +
> +		fpe->verify_state = WAIT_FOR_RESPONSE;
> +		verify_time_jiffies = msecs_to_jiffies(fpe->verify_time);
> +		fpe->verify_timeout = jiffies + verify_time_jiffies;
> +
> +		schedule_delayed_work(&fpe->verification_work,
> +				      verify_time_jiffies);
> +		break;
> +
> +	case WAIT_FOR_RESPONSE:
> +		if (fpe->received_smd_r) {
> +			fpe->verify_state = VERIFIED;
> +			fpe->received_smd_r = false;
> +		} else if (time_is_before_jiffies(fpe->verify_timeout)) {
> +			fpe->verify_cnt++;
> +			netdev_warn(adapter->netdev,
> +				    "Timeout waiting for SMD-R frame\n");
> +
> +			if (fpe->verify_cnt > MAX_VERIFY_CNT) {
> +				fpe->verify_state = VERIFY_FAIL;
> +				netdev_err(adapter->netdev,
> +					   "Exceeded attempts sending SMD-V\n");
> +			} else {
> +				fpe->verify_state = SEND_VERIFY;
> +				igc_fpe_handle_verify(adapter);
> +			}
> +		}
> +		break;
> +
> +	case VERIFY_FAIL:
> +	case VERIFIED:
> +	case INIT_VERIFICATION:
> +		break;
> +	}
> +}
> +
> +static void igc_fpe_verification(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct igc_adapter *adapter;
> +	struct fpe_t *fpe;
> +
> +	fpe = container_of(dwork, struct fpe_t, verification_work);
> +	adapter = container_of(fpe, struct igc_adapter, fpe);
> +
> +	if (fpe->received_smd_v) {
> +		igc_fpe_send_response(adapter);
> +		fpe->received_smd_v = false;
> +	}
> +
> +	if (fpe->verify_enabled)
> +		igc_fpe_handle_verify(adapter);
> +}
> +
>  void igc_fpe_init(struct fpe_t *fpe)
>  {
> +	INIT_DELAYED_WORK(&fpe->verification_work, igc_fpe_verification);
>  	fpe->verify_enabled = false;
> +	fpe->verify_state = INIT_VERIFICATION;
>  	fpe->verify_time = DEFAULT_VERIFY_TIME;
> +	fpe->received_smd_v = false;
> +	fpe->received_smd_r = false;
> +	fpe->verify_cnt = 0;
>  	fpe->pmac_enabled = false;
>  	fpe->tx_min_frag_size = IGC_TX_MIN_FRAG_SIZE;
>  }
>  
> +void igc_fpe_verify_enabled_changed(struct fpe_t *fpe)
> +{
> +	if (fpe->verify_enabled && fpe->tx_enabled) {
> +		fpe->verify_state = SEND_VERIFY;
> +		schedule_delayed_work(&fpe->verification_work,
> +				      msecs_to_jiffies(VERIFY_RESPONSE_DELAY));
> +	} else {
> +		fpe->verify_state = INIT_VERIFICATION;
> +		fpe->received_smd_v = false;
> +		fpe->received_smd_r = false;
> +		fpe->verify_cnt = 0;
> +	}
> +}
> +
> +int igc_fpe_get_smd_type(__le32 status_error)
> +{
> +	u32 status = le32_to_cpu(status_error);
> +
> +	return (status & IGC_RXDADV_STAT_SMD_TYPE_MASK)
> +		>> IGC_RXDADV_STAT_SMD_TYPE_SHIFT;
> +}
> +
> +bool igc_fpe_is_verify_or_response(int smd_type, unsigned int size)
> +{
> +	return ((smd_type == IGC_SMD_TYPE_SMD_V ||
> +		 smd_type == IGC_SMD_TYPE_SMD_R) && size == SMD_FRAME_SIZE);
> +}
> +
> +void igc_fpe_preprocess_verify_response(struct fpe_t *fpe, int smd_type)
> +{
> +	if (smd_type == IGC_SMD_TYPE_SMD_V)
> +		fpe->received_smd_v = true;
> +	else if (smd_type == IGC_SMD_TYPE_SMD_R)
> +		fpe->received_smd_r = true;
> +
> +	schedule_delayed_work(&fpe->verification_work, 0);
> +}
> +
>  static bool is_any_launchtime(struct igc_adapter *adapter)
>  {
>  	int i;
> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
> index 08e7582f257e..f3d83fbbd1f4 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.h
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
> @@ -12,7 +12,11 @@
>  #define MIN_VERIFY_TIME			1
>  #define MAX_VERIFY_TIME			128
>  
> +int igc_fpe_get_smd_type(__le32 status_error);
>  void igc_fpe_init(struct fpe_t *fpe);
> +bool igc_fpe_is_verify_or_response(int smd_type, unsigned int size);
> +void igc_fpe_preprocess_verify_response(struct fpe_t *fpe, int smd_type);
> +void igc_fpe_verify_enabled_changed(struct fpe_t *fpe);
>  int igc_tsn_offload_apply(struct igc_adapter *adapter);
>  int igc_tsn_reset(struct igc_adapter *adapter);
>  void igc_tsn_adjust_txtime_offset(struct igc_adapter *adapter);
> -- 
> 2.25.1
> 
> 


[-- Attachment #2: 0001-net-ethtool-mm-extract-stmmac-verification-logic-int.patch --]
[-- Type: text/x-diff, Size: 24752 bytes --]

From d36050935733c1ecd881af5c909a5af71a87207a Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 17 Dec 2024 02:02:53 +0200
Subject: [PATCH] net: ethtool: mm: extract stmmac verification logic into
 common library

It appears that stmmac is not the only hardware which requires a
software-driven verification state machine for the MAC Merge layer.

While on the one hand it's good to encourage hardware implementations,
on the other hand it's quite difficult to tolerate multiple drivers
implementing independently fairly non-trivial logic.

Extract the hardware-independent logic from stmmac into library code and
put it in ethtool. Name the state structure "mmsv" for MAC Merge
Software Verification. Let this expose an operations structure for
executing the hardware stuff: sync hardware with the tx_active boolean
(result of verification process), enable/disable the pMAC, send mPackets,
notify library of external events (reception of mPackets), as well as
link state changes.

Note that it is assumed that the external events are received in hardirq
context. If they are not, it is probably a good idea to disable hardirqs
when calling ethtool_mmsv_event_handle(), because the library does not
do so.

Also, the MM software verification process has no business with the
tx_min_frag_size, that is all the driver's to handle.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  16 +-
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  40 +---
 .../net/ethernet/stmicro/stmmac/stmmac_fpe.c  | 174 +++-----------
 .../net/ethernet/stmicro/stmmac/stmmac_fpe.h  |   5 -
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |   8 +-
 include/linux/ethtool.h                       |  61 +++++
 net/ethtool/mm.c                              | 214 ++++++++++++++++++
 7 files changed, 319 insertions(+), 199 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 1d86439b8a14..7718df98f835 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -147,21 +147,9 @@ struct stmmac_channel {
 };
 
 struct stmmac_fpe_cfg {
-	/* Serialize access to MAC Merge state between ethtool requests
-	 * and link state updates.
-	 */
-	spinlock_t lock;
-
+	struct ethtool_mmsv mmsv;
 	const struct stmmac_fpe_reg *reg;
-	u32 fpe_csr;				/* MAC_FPE_CTRL_STS reg cache */
-
-	enum ethtool_mm_verify_status status;
-	struct timer_list verify_timer;
-	bool verify_enabled;
-	int verify_retries;
-	bool pmac_enabled;
-	u32 verify_time;
-	bool tx_enabled;
+	u32 fpe_csr;	/* MAC_FPE_CTRL_STS reg cache */
 };
 
 struct stmmac_tc_entry {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 1d77389ce953..d755b7fd3056 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -1268,37 +1268,17 @@ static int stmmac_get_mm(struct net_device *ndev,
 			 struct ethtool_mm_state *state)
 {
 	struct stmmac_priv *priv = netdev_priv(ndev);
-	unsigned long flags;
 	u32 frag_size;
 
 	if (!stmmac_fpe_supported(priv))
 		return -EOPNOTSUPP;
 
-	spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
+	ethtool_mmsv_get_mm(&priv->fpe_cfg.mmsv, state);
 
-	state->max_verify_time = STMMAC_FPE_MM_MAX_VERIFY_TIME_MS;
-	state->verify_enabled = priv->fpe_cfg.verify_enabled;
-	state->pmac_enabled = priv->fpe_cfg.pmac_enabled;
-	state->verify_time = priv->fpe_cfg.verify_time;
-	state->tx_enabled = priv->fpe_cfg.tx_enabled;
-	state->verify_status = priv->fpe_cfg.status;
 	state->rx_min_frag_size = ETH_ZLEN;
-
-	/* FPE active if common tx_enabled and
-	 * (verification success or disabled(forced))
-	 */
-	if (state->tx_enabled &&
-	    (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
-	     state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED))
-		state->tx_active = true;
-	else
-		state->tx_active = false;
-
 	frag_size = stmmac_fpe_get_add_frag_size(priv);
 	state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(frag_size);
 
-	spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
-
 	return 0;
 }
 
@@ -1307,7 +1287,6 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
 {
 	struct stmmac_priv *priv = netdev_priv(ndev);
 	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
-	unsigned long flags;
 	u32 frag_size;
 	int err;
 
@@ -1316,23 +1295,8 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
 	if (err)
 		return err;
 
-	/* Wait for the verification that's currently in progress to finish */
-	timer_shutdown_sync(&fpe_cfg->verify_timer);
-
-	spin_lock_irqsave(&fpe_cfg->lock, flags);
-
-	fpe_cfg->verify_enabled = cfg->verify_enabled;
-	fpe_cfg->pmac_enabled = cfg->pmac_enabled;
-	fpe_cfg->verify_time = cfg->verify_time;
-	fpe_cfg->tx_enabled = cfg->tx_enabled;
-
-	if (!cfg->verify_enabled)
-		fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
-
+	ethtool_mmsv_set_mm(&priv->fpe_cfg.mmsv, cfg);
 	stmmac_fpe_set_add_frag_size(priv, frag_size);
-	stmmac_fpe_apply(priv);
-
-	spin_unlock_irqrestore(&fpe_cfg->lock, flags);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
index 3a4bee029c7f..75b470ee621a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
@@ -27,12 +27,6 @@
 #define STMMAC_MAC_FPE_CTRL_STS_SVER	BIT(1)
 #define STMMAC_MAC_FPE_CTRL_STS_EFPE	BIT(0)
 
-/* FPE link-partner hand-shaking mPacket type */
-enum stmmac_mpacket_type {
-	MPACKET_VERIFY = 0,
-	MPACKET_RESPONSE = 1,
-};
-
 struct stmmac_fpe_reg {
 	const u32 mac_fpe_reg;		/* offset of MAC_FPE_CTRL_STS */
 	const u32 mtl_fpe_reg;		/* offset of MTL_FPE_CTRL_STS */
@@ -48,10 +42,10 @@ bool stmmac_fpe_supported(struct stmmac_priv *priv)
 		priv->hw->mac->fpe_map_preemption_class;
 }
 
-static void stmmac_fpe_configure(struct stmmac_priv *priv, bool tx_enable,
-				 bool pmac_enable)
+static void stmmac_fpe_configure_tx(struct ethtool_mmsv *mmsv, bool tx_enable)
 {
-	struct stmmac_fpe_cfg *cfg = &priv->fpe_cfg;
+	struct stmmac_fpe_cfg *cfg = container_of(mmsv, struct stmmac_fpe_cfg, mmsv);
+	struct stmmac_priv *priv = container_of(cfg, struct stmmac_priv, fpe_cfg);
 	const struct stmmac_fpe_reg *reg = cfg->reg;
 	u32 num_rxq = priv->plat->rx_queues_to_use;
 	void __iomem *ioaddr = priv->ioaddr;
@@ -68,6 +62,15 @@ static void stmmac_fpe_configure(struct stmmac_priv *priv, bool tx_enable,
 		cfg->fpe_csr = 0;
 	}
 	writel(cfg->fpe_csr, ioaddr + reg->mac_fpe_reg);
+}
+
+static void stmmac_fpe_configure_pmac(struct ethtool_mmsv *mmsv, bool pmac_enable)
+{
+	struct stmmac_fpe_cfg *cfg = container_of(mmsv, struct stmmac_fpe_cfg, mmsv);
+	struct stmmac_priv *priv = container_of(cfg, struct stmmac_priv, fpe_cfg);
+	const struct stmmac_fpe_reg *reg = cfg->reg;
+	void __iomem *ioaddr = priv->ioaddr;
+	u32 value;
 
 	value = readl(ioaddr + reg->int_en_reg);
 
@@ -85,47 +88,45 @@ static void stmmac_fpe_configure(struct stmmac_priv *priv, bool tx_enable,
 	writel(value, ioaddr + reg->int_en_reg);
 }
 
-static void stmmac_fpe_send_mpacket(struct stmmac_priv *priv,
-				    enum stmmac_mpacket_type type)
+static void stmmac_fpe_send_mpacket(struct ethtool_mmsv *mmsv,
+				    enum ethtool_mpacket type)
 {
-	const struct stmmac_fpe_reg *reg = priv->fpe_cfg.reg;
+	struct stmmac_fpe_cfg *cfg = container_of(mmsv, struct stmmac_fpe_cfg, mmsv);
+	struct stmmac_priv *priv = container_of(cfg, struct stmmac_priv, fpe_cfg);
+	const struct stmmac_fpe_reg *reg = cfg->reg;
 	void __iomem *ioaddr = priv->ioaddr;
-	u32 value = priv->fpe_cfg.fpe_csr;
+	u32 value = cfg->fpe_csr;
 
-	if (type == MPACKET_VERIFY)
+	if (type == ETHTOOL_MPACKET_VERIFY)
 		value |= STMMAC_MAC_FPE_CTRL_STS_SVER;
-	else if (type == MPACKET_RESPONSE)
+	else if (type == ETHTOOL_MPACKET_RESPONSE)
 		value |= STMMAC_MAC_FPE_CTRL_STS_SRSP;
 
 	writel(value, ioaddr + reg->mac_fpe_reg);
 }
 
+static const struct ethtool_mmsv_ops stmmac_mmsv_ops = {
+	.configure_tx = stmmac_fpe_configure_tx,
+	.configure_pmac = stmmac_fpe_configure_pmac,
+	.send_mpacket = stmmac_fpe_send_mpacket,
+};
+
 static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
 {
 	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
+	struct ethtool_mmsv *mmsv = &fpe_cfg->mmsv;
 
-	/* This is interrupt context, just spin_lock() */
-	spin_lock(&fpe_cfg->lock);
-
-	if (!fpe_cfg->pmac_enabled || status == FPE_EVENT_UNKNOWN)
-		goto unlock_out;
+	if (status == FPE_EVENT_UNKNOWN)
+		return;
 
-	/* LP has sent verify mPacket */
 	if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER)
-		stmmac_fpe_send_mpacket(priv, MPACKET_RESPONSE);
+		ethtool_mmsv_event_handle(mmsv, ETHTOOL_MMSV_LP_SENT_VERIFY_MPACKET);
 
-	/* Local has sent verify mPacket */
-	if ((status & FPE_EVENT_TVER) == FPE_EVENT_TVER &&
-	    fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED)
-		fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
+	if ((status & FPE_EVENT_TVER) == FPE_EVENT_TVER)
+		ethtool_mmsv_event_handle(mmsv, ETHTOOL_MMSV_LD_SENT_VERIFY_MPACKET);
 
-	/* LP has sent response mPacket */
-	if ((status & FPE_EVENT_RRSP) == FPE_EVENT_RRSP &&
-	    fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_VERIFYING)
-		fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
-
-unlock_out:
-	spin_unlock(&fpe_cfg->lock);
+	if ((status & FPE_EVENT_RRSP) == FPE_EVENT_RRSP)
+		ethtool_mmsv_event_handle(mmsv, ETHTOOL_MMSV_LP_SENT_RESPONSE_MPACKET);
 }
 
 void stmmac_fpe_irq_status(struct stmmac_priv *priv)
@@ -164,119 +165,16 @@ void stmmac_fpe_irq_status(struct stmmac_priv *priv)
 	stmmac_fpe_event_status(priv, status);
 }
 
-/**
- * stmmac_fpe_verify_timer - Timer for MAC Merge verification
- * @t:  timer_list struct containing private info
- *
- * Verify the MAC Merge capability in the local TX direction, by
- * transmitting Verify mPackets up to 3 times. Wait until link
- * partner responds with a Response mPacket, otherwise fail.
- */
-static void stmmac_fpe_verify_timer(struct timer_list *t)
-{
-	struct stmmac_fpe_cfg *fpe_cfg = from_timer(fpe_cfg, t, verify_timer);
-	struct stmmac_priv *priv = container_of(fpe_cfg, struct stmmac_priv,
-						fpe_cfg);
-	unsigned long flags;
-	bool rearm = false;
-
-	spin_lock_irqsave(&fpe_cfg->lock, flags);
-
-	switch (fpe_cfg->status) {
-	case ETHTOOL_MM_VERIFY_STATUS_INITIAL:
-	case ETHTOOL_MM_VERIFY_STATUS_VERIFYING:
-		if (fpe_cfg->verify_retries != 0) {
-			stmmac_fpe_send_mpacket(priv, MPACKET_VERIFY);
-			rearm = true;
-		} else {
-			fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
-		}
-
-		fpe_cfg->verify_retries--;
-		break;
-
-	case ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED:
-		stmmac_fpe_configure(priv, true, true);
-		break;
-
-	default:
-		break;
-	}
-
-	if (rearm) {
-		mod_timer(&fpe_cfg->verify_timer,
-			  jiffies + msecs_to_jiffies(fpe_cfg->verify_time));
-	}
-
-	spin_unlock_irqrestore(&fpe_cfg->lock, flags);
-}
-
-static void stmmac_fpe_verify_timer_arm(struct stmmac_fpe_cfg *fpe_cfg)
-{
-	if (fpe_cfg->pmac_enabled && fpe_cfg->tx_enabled &&
-	    fpe_cfg->verify_enabled &&
-	    fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_FAILED &&
-	    fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED) {
-		timer_setup(&fpe_cfg->verify_timer, stmmac_fpe_verify_timer, 0);
-		mod_timer(&fpe_cfg->verify_timer, jiffies);
-	}
-}
-
 void stmmac_fpe_init(struct stmmac_priv *priv)
 {
-	priv->fpe_cfg.verify_retries = STMMAC_FPE_MM_MAX_VERIFY_RETRIES;
-	priv->fpe_cfg.verify_time = STMMAC_FPE_MM_MAX_VERIFY_TIME_MS;
-	priv->fpe_cfg.status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
-	timer_setup(&priv->fpe_cfg.verify_timer, stmmac_fpe_verify_timer, 0);
-	spin_lock_init(&priv->fpe_cfg.lock);
+	ethtool_mmsv_init(&priv->fpe_cfg.mmsv, priv->dev,
+			  &stmmac_mmsv_ops);
 
 	if ((!priv->fpe_cfg.reg || !priv->hw->mac->fpe_map_preemption_class) &&
 	    priv->dma_cap.fpesel)
 		dev_info(priv->device, "FPE is not supported by driver.\n");
 }
 
-void stmmac_fpe_apply(struct stmmac_priv *priv)
-{
-	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
-
-	/* If verification is disabled, configure FPE right away.
-	 * Otherwise let the timer code do it.
-	 */
-	if (!fpe_cfg->verify_enabled) {
-		stmmac_fpe_configure(priv, fpe_cfg->tx_enabled,
-				     fpe_cfg->pmac_enabled);
-	} else {
-		fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
-		fpe_cfg->verify_retries = STMMAC_FPE_MM_MAX_VERIFY_RETRIES;
-
-		if (netif_running(priv->dev))
-			stmmac_fpe_verify_timer_arm(fpe_cfg);
-	}
-}
-
-void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
-{
-	struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
-	unsigned long flags;
-
-	timer_shutdown_sync(&fpe_cfg->verify_timer);
-
-	spin_lock_irqsave(&fpe_cfg->lock, flags);
-
-	if (is_up && fpe_cfg->pmac_enabled) {
-		/* VERIFY process requires pmac enabled when NIC comes up */
-		stmmac_fpe_configure(priv, false, true);
-
-		/* New link => maybe new partner => new verification process */
-		stmmac_fpe_apply(priv);
-	} else {
-		/* No link => turn off EFPE */
-		stmmac_fpe_configure(priv, false, false);
-	}
-
-	spin_unlock_irqrestore(&fpe_cfg->lock, flags);
-}
-
 int stmmac_fpe_get_add_frag_size(struct stmmac_priv *priv)
 {
 	const struct stmmac_fpe_reg *reg = priv->fpe_cfg.reg;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
index b884eac7142d..3fc46acf7001 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.h
@@ -9,15 +9,10 @@
 #include <linux/types.h>
 #include <linux/netdevice.h>
 
-#define STMMAC_FPE_MM_MAX_VERIFY_RETRIES	3
-#define STMMAC_FPE_MM_MAX_VERIFY_TIME_MS	128
-
 struct stmmac_priv;
 
-void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up);
 bool stmmac_fpe_supported(struct stmmac_priv *priv);
 void stmmac_fpe_init(struct stmmac_priv *priv);
-void stmmac_fpe_apply(struct stmmac_priv *priv);
 void stmmac_fpe_irq_status(struct stmmac_priv *priv);
 int stmmac_fpe_get_add_frag_size(struct stmmac_priv *priv);
 void stmmac_fpe_set_add_frag_size(struct stmmac_priv *priv, u32 add_frag_size);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 16b8bcfa8b11..eba391568965 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -983,7 +983,7 @@ static void stmmac_mac_link_down(struct phylink_config *config,
 	stmmac_set_eee_pls(priv, priv->hw, false);
 
 	if (stmmac_fpe_supported(priv))
-		stmmac_fpe_link_state_handle(priv, false);
+		ethtool_mmsv_link_state_handle(&priv->fpe_cfg.mmsv, false);
 }
 
 static void stmmac_mac_link_up(struct phylink_config *config,
@@ -1097,7 +1097,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 	}
 
 	if (stmmac_fpe_supported(priv))
-		stmmac_fpe_link_state_handle(priv, true);
+		ethtool_mmsv_link_state_handle(&priv->fpe_cfg.mmsv, true);
 
 	if (priv->plat->flags & STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY)
 		stmmac_hwtstamp_correct_latency(priv, priv);
@@ -4049,7 +4049,7 @@ static int stmmac_release(struct net_device *dev)
 	stmmac_release_ptp(priv);
 
 	if (stmmac_fpe_supported(priv))
-		timer_shutdown_sync(&priv->fpe_cfg.verify_timer);
+		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
 
 	pm_runtime_put(priv->device);
 
@@ -7751,7 +7751,7 @@ int stmmac_suspend(struct device *dev)
 	rtnl_unlock();
 
 	if (stmmac_fpe_supported(priv))
-		timer_shutdown_sync(&priv->fpe_cfg.verify_timer);
+		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
 
 	priv->speed = SPEED_UNKNOWN;
 	return 0;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f711bfd75c4d..276e11d727cc 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -17,9 +17,13 @@
 #include <linux/compat.h>
 #include <linux/if_ether.h>
 #include <linux/netlink.h>
+#include <linux/timer_types.h>
 #include <uapi/linux/ethtool.h>
 #include <uapi/linux/net_tstamp.h>
 
+#define ETHTOOL_MM_MAX_VERIFY_TIME_MS		128
+#define ETHTOOL_MM_MAX_VERIFY_RETRIES		3
+
 struct compat_ethtool_rx_flow_spec {
 	u32		flow_type;
 	union ethtool_flow_union h_u;
@@ -673,6 +677,63 @@ struct ethtool_mm_stats {
 	u64 MACMergeHoldCount;
 };
 
+enum ethtool_mmsv_event {
+	ETHTOOL_MMSV_LP_SENT_VERIFY_MPACKET,
+	ETHTOOL_MMSV_LD_SENT_VERIFY_MPACKET,
+	ETHTOOL_MMSV_LP_SENT_RESPONSE_MPACKET,
+};
+
+/* MAC Merge verification mPacket type */
+enum ethtool_mpacket {
+	ETHTOOL_MPACKET_VERIFY,
+	ETHTOOL_MPACKET_RESPONSE,
+};
+
+struct ethtool_mmsv;
+
+struct ethtool_mmsv_ops {
+	void (*configure_tx)(struct ethtool_mmsv *mmsv, bool tx_active);
+	void (*configure_pmac)(struct ethtool_mmsv *mmsv, bool pmac_enabled);
+	void (*send_mpacket)(struct ethtool_mmsv *mmsv, enum ethtool_mpacket mpacket);
+};
+
+/**
+ * struct ethtool_mmsv - MAC Merge Software Verification
+ * @ops: TODO
+ * @dev: TODO
+ * @lock: serialize access to MAC Merge state between
+ *	  ethtool requests and link state updates.
+ * @status: current verification FSM state
+ * @verify_timer: timer for verification in local TX direction
+ * @verify_enabled: TODO
+ * verify_retries: TODO
+ * pmac_enabled: TODO
+ * verify_time: TODO
+ * tx_enabled: TODO
+ */
+struct ethtool_mmsv {
+	const struct ethtool_mmsv_ops *ops;
+	struct net_device *dev;
+	spinlock_t lock;
+	enum ethtool_mm_verify_status status;
+	struct timer_list verify_timer;
+	bool verify_enabled;
+	int verify_retries;
+	bool pmac_enabled;
+	u32 verify_time;
+	bool tx_enabled;
+};
+
+void ethtool_mmsv_stop(struct ethtool_mmsv *mmsv);
+void ethtool_mmsv_link_state_handle(struct ethtool_mmsv *mmsv, bool up);
+void ethtool_mmsv_event_handle(struct ethtool_mmsv *mmsv,
+			       enum ethtool_mmsv_event event);
+void ethtool_mmsv_get_mm(struct ethtool_mmsv *mmsv,
+			 struct ethtool_mm_state *state);
+void ethtool_mmsv_set_mm(struct ethtool_mmsv *mmsv, struct ethtool_mm_cfg *cfg);
+void ethtool_mmsv_init(struct ethtool_mmsv *mmsv, struct net_device *dev,
+		       const struct ethtool_mmsv_ops *ops);
+
 /**
  * struct ethtool_rxfh_param - RXFH (RSS) parameters
  * @hfunc: Defines the current RSS hash function used by HW (or to be set to).
diff --git a/net/ethtool/mm.c b/net/ethtool/mm.c
index 2816bb23c3ad..3063fe00eef7 100644
--- a/net/ethtool/mm.c
+++ b/net/ethtool/mm.c
@@ -282,3 +282,217 @@ bool ethtool_dev_mm_supported(struct net_device *dev)
 	return supported;
 }
 EXPORT_SYMBOL_GPL(ethtool_dev_mm_supported);
+
+static void ethtool_mmsv_configure_tx(struct ethtool_mmsv *mmsv,
+				      bool tx_active)
+{
+	mmsv->ops->configure_tx(mmsv, tx_active);
+}
+
+static void ethtool_mmsv_configure_pmac(struct ethtool_mmsv *mmsv,
+					bool pmac_enabled)
+{
+	mmsv->ops->configure_tx(mmsv, pmac_enabled);
+}
+
+static void ethtool_mmsv_send_mpacket(struct ethtool_mmsv *mmsv,
+				      enum ethtool_mpacket mpacket)
+{
+	mmsv->ops->send_mpacket(mmsv, mpacket);
+}
+
+/**
+ * ethtool_mmsv_verify_timer - Timer for MAC Merge verification
+ * @t: timer_list struct containing private info
+ *
+ * Verify the MAC Merge capability in the local TX direction, by
+ * transmitting Verify mPackets up to 3 times. Wait until link
+ * partner responds with a Response mPacket, otherwise fail.
+ */
+static void ethtool_mmsv_verify_timer(struct timer_list *t)
+{
+	struct ethtool_mmsv *mmsv = from_timer(mmsv, t, verify_timer);
+	unsigned long flags;
+	bool rearm = false;
+
+	spin_lock_irqsave(&mmsv->lock, flags);
+
+	switch (mmsv->status) {
+	case ETHTOOL_MM_VERIFY_STATUS_INITIAL:
+	case ETHTOOL_MM_VERIFY_STATUS_VERIFYING:
+		if (mmsv->verify_retries != 0) {
+			ethtool_mmsv_send_mpacket(mmsv, ETHTOOL_MPACKET_VERIFY);
+			rearm = true;
+		} else {
+			mmsv->status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
+		}
+
+		mmsv->verify_retries--;
+		break;
+
+	case ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED:
+		ethtool_mmsv_configure_tx(mmsv, true);
+		break;
+
+	default:
+		break;
+	}
+
+	if (rearm) {
+		mod_timer(&mmsv->verify_timer,
+			  jiffies + msecs_to_jiffies(mmsv->verify_time));
+	}
+
+	spin_unlock_irqrestore(&mmsv->lock, flags);
+}
+
+static void ethtool_mmsv_verify_timer_arm(struct ethtool_mmsv *mmsv)
+{
+	if (mmsv->pmac_enabled && mmsv->tx_enabled && mmsv->verify_enabled &&
+	    mmsv->status != ETHTOOL_MM_VERIFY_STATUS_FAILED &&
+	    mmsv->status != ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED) {
+		timer_setup(&mmsv->verify_timer, ethtool_mmsv_verify_timer, 0);
+		mod_timer(&mmsv->verify_timer, jiffies);
+	}
+}
+
+static void ethtool_mmsv_apply(struct ethtool_mmsv *mmsv)
+{
+	/* If verification is disabled, configure FPE right away.
+	 * Otherwise let the timer code do it.
+	 */
+	if (!mmsv->verify_enabled) {
+		ethtool_mmsv_configure_tx(mmsv, mmsv->tx_enabled);
+	} else {
+		mmsv->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
+		mmsv->verify_retries = ETHTOOL_MM_MAX_VERIFY_RETRIES;
+
+		if (netif_running(mmsv->dev))
+			ethtool_mmsv_verify_timer_arm(mmsv);
+	}
+}
+
+void ethtool_mmsv_stop(struct ethtool_mmsv *mmsv)
+{
+	timer_shutdown_sync(&mmsv->verify_timer);
+}
+EXPORT_SYMBOL_GPL(ethtool_mmsv_stop);
+
+void ethtool_mmsv_link_state_handle(struct ethtool_mmsv *mmsv, bool up)
+{
+	unsigned long flags;
+
+	ethtool_mmsv_stop(mmsv);
+
+	spin_lock_irqsave(&mmsv->lock, flags);
+
+	if (up && mmsv->pmac_enabled) {
+		/* VERIFY process requires pMAC enabled when NIC comes up */
+		ethtool_mmsv_configure_pmac(mmsv, true);
+
+		/* New link => maybe new partner => new verification process */
+		ethtool_mmsv_apply(mmsv);
+	} else {
+		/* No link or pMAC not enabled */
+		ethtool_mmsv_configure_pmac(mmsv, false);
+		ethtool_mmsv_configure_tx(mmsv, false);
+	}
+
+	spin_unlock_irqrestore(&mmsv->lock, flags);
+}
+EXPORT_SYMBOL_GPL(ethtool_mmsv_link_state_handle);
+
+void ethtool_mmsv_event_handle(struct ethtool_mmsv *mmsv,
+			       enum ethtool_mmsv_event event)
+{
+	/* This is interrupt context, just spin_lock() */
+	spin_lock(&mmsv->lock);
+
+	if (!mmsv->pmac_enabled)
+		goto unlock;
+
+	switch (event) {
+	case ETHTOOL_MMSV_LP_SENT_VERIFY_MPACKET:
+		/* Link partner has sent verify mPacket */
+		ethtool_mmsv_send_mpacket(mmsv, ETHTOOL_MPACKET_RESPONSE);
+		break;
+	case ETHTOOL_MMSV_LD_SENT_VERIFY_MPACKET:
+		/* Local device has sent verify mPacket */
+		if (mmsv->status != ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED)
+			mmsv->status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
+		break;
+	case ETHTOOL_MMSV_LP_SENT_RESPONSE_MPACKET:
+		/* Link partner has sent response mPacket */
+		if (mmsv->status == ETHTOOL_MM_VERIFY_STATUS_VERIFYING)
+			mmsv->status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
+		break;
+	}
+
+unlock:
+	spin_unlock(&mmsv->lock);
+}
+EXPORT_SYMBOL_GPL(ethtool_mmsv_event_handle);
+
+void ethtool_mmsv_get_mm(struct ethtool_mmsv *mmsv,
+			 struct ethtool_mm_state *state)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&mmsv->lock, flags);
+
+	state->max_verify_time = ETHTOOL_MM_MAX_VERIFY_TIME_MS;
+	state->verify_enabled = mmsv->verify_enabled;
+	state->pmac_enabled = mmsv->pmac_enabled;
+	state->verify_time = mmsv->verify_time;
+	state->tx_enabled = mmsv->tx_enabled;
+	state->verify_status = mmsv->status;
+
+	/* TX is active if administratively enabled, and verification either
+	 * succeeded, or was administratively disabled.
+	 */
+	if (state->tx_enabled &&
+	    (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
+	     state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED))
+		state->tx_active = true;
+	else
+		state->tx_active = false;
+
+	spin_unlock_irqrestore(&mmsv->lock, flags);
+}
+EXPORT_SYMBOL_GPL(ethtool_mmsv_get_mm);
+
+void ethtool_mmsv_set_mm(struct ethtool_mmsv *mmsv, struct ethtool_mm_cfg *cfg)
+{
+	unsigned long flags;
+
+	/* Wait for the verification that's currently in progress to finish */
+	ethtool_mmsv_stop(mmsv);
+
+	spin_lock_irqsave(&mmsv->lock, flags);
+
+	mmsv->verify_enabled = cfg->verify_enabled;
+	mmsv->pmac_enabled = cfg->pmac_enabled;
+	mmsv->verify_time = cfg->verify_time;
+	mmsv->tx_enabled = cfg->tx_enabled;
+
+	if (!cfg->verify_enabled)
+		mmsv->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
+
+	ethtool_mmsv_apply(mmsv);
+
+	spin_unlock_irqrestore(&mmsv->lock, flags);
+}
+EXPORT_SYMBOL_GPL(ethtool_mmsv_set_mm);
+
+void ethtool_mmsv_init(struct ethtool_mmsv *mmsv, struct net_device *dev,
+		       const struct ethtool_mmsv_ops *ops)
+{
+	mmsv->ops = ops;
+	mmsv->dev = dev;
+	mmsv->verify_retries = ETHTOOL_MM_MAX_VERIFY_RETRIES;
+	mmsv->verify_time = ETHTOOL_MM_MAX_VERIFY_TIME_MS;
+	mmsv->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
+	timer_setup(&mmsv->verify_timer, ethtool_mmsv_verify_timer, 0);
+	spin_lock_init(&mmsv->lock);
+}
+EXPORT_SYMBOL_GPL(ethtool_mmsv_init);
-- 
2.43.0

Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Vladimir Oltean]

On Tue, Dec 17, 2024 at 02:22:54AM +0200, Vladimir Oltean wrote:
> I spent a bit of time extracting stmmac's core logic and putting it in
> ethtool.

There's at least one bug in this conversion:

-- >8 --
diff --git a/net/ethtool/mm.c b/net/ethtool/mm.c
index 3063fe00eef7..d305208dd0c8 100644
--- a/net/ethtool/mm.c
+++ b/net/ethtool/mm.c
@@ -292,7 +292,7 @@ static void ethtool_mmsv_configure_tx(struct ethtool_mmsv *mmsv,
 static void ethtool_mmsv_configure_pmac(struct ethtool_mmsv *mmsv,
 					bool pmac_enabled)
 {
-	mmsv->ops->configure_tx(mmsv, pmac_enabled);
+	mmsv->ops->configure_pmac(mmsv, pmac_enabled);
 }
 
 static void ethtool_mmsv_send_mpacket(struct ethtool_mmsv *mmsv,
-- >8 --

Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Abdul Rahim, Faizal]

On 17/12/2024 8:22 am, Vladimir Oltean wrote:
> On Mon, Dec 16, 2024 at 01:47:17AM -0500, Faizal Rahim wrote:
>> The i226 hardware doesn't implement the process of verification
>> internally, this is left to the driver.
>>
>> Add a simple implementation of the state machine defined in IEEE
>> 802.3-2018, Section 99.4.7. The state machine is started manually by
>> user after "verify-enabled" command is enabled.
>>
>> Implementation includes:
>> 1. Send and receive verify frame
>> 2. Verification state handling
>> 3. Send and receive response frame
>>
>> Tested by triggering verification handshake:
>> $ sudo ethtool --set-mm enp1s0 pmac-enabled on
>> $ sudo ethtool --set-mm enp1s0 tx-enabled on
>> $ sudo ethtool --set-mm enp1s0 verify-enabled on
>>
>> Note that Ethtool API requires enabling "pmac-enabled on" and
>> "tx-enabled on" before "verify-enabled on" can be issued.
>>
>> After the upcoming patch ("igc: Add support to get MAC Merge data via
>> ethtool") is implemented, verification status can be checked using:
>> $ ethtool --show-mm enp1s0
>>    MAC Merge layer state for enp1s0:
>>    pMAC enabled: on
>>    TX enabled: on
>>    TX active: on
>>    TX minimum fragment size: 252
>>    RX minimum fragment size: 252
>>    Verify enabled: on
>>    Verify time: 128
>>    Max verify time: 128
>>    Verification status: SUCCEEDED
>>
>> Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
>> ---
> 
> Am I missing something, or this does not handle link state changes,
> where the verification should restart on each link up? (maybe the old
> link partner didn't support FPE and the new one does, or vice versa)
> 
> Either I don't follow the link between igc_watchdog_task() and any
> verification related task, or it doesn't exist.

The latter. I missed this "link state changes" interaction, will rework, 
thanks.

> Anyway, while browsing through this software implementation of a
> verification process, I cannot help but think we'd be making a huge
> mistake to allow each driver to reimplement it on its own. We just
> recently got stmmac to do something fairly clean, with the help and
> great perseverence of Furong Xu (now copied).
> 
> I spent a bit of time extracting stmmac's core logic and putting it in
> ethtool. If Furong had such good will so as to regression-test the
> attached patch, do you think you could use this as a starting place
> instead, and implement some ops and call some library methods, instead
> of writing the entire logic yourself?

Totally agree with moving it to a layer reusable by any driver. Thank you 
so much for the skeleton patch implementing it in ethtool — I’ll expand on 
it from here.

Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Furong Xu]

On Tue, 17 Dec 2024 02:22:54 +0200, Vladimir Oltean <olteanv@gmail.com> wrote:

> Anyway, while browsing through this software implementation of a
> verification process, I cannot help but think we'd be making a huge
> mistake to allow each driver to reimplement it on its own. We just
> recently got stmmac to do something fairly clean, with the help and
> great perseverence of Furong Xu (now copied).
> 
> I spent a bit of time extracting stmmac's core logic and putting it in
> ethtool. If Furong had such good will so as to regression-test the
> attached patch, do you think you could use this as a starting place
> instead, and implement some ops and call some library methods, instead
> of writing the entire logic yourself?
> 

I am quiet busy these days, especially near the end of the year :)

Maybe I can help testing the attached patch when the next time net-next opens.

Thanks.

Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Choong Yong Liang]

On 17/12/2024 8:09 pm, Furong Xu wrote:
> On Tue, 17 Dec 2024 02:22:54 +0200, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
>> Anyway, while browsing through this software implementation of a
>> verification process, I cannot help but think we'd be making a huge
>> mistake to allow each driver to reimplement it on its own. We just
>> recently got stmmac to do something fairly clean, with the help and
>> great perseverence of Furong Xu (now copied).
>>
>> I spent a bit of time extracting stmmac's core logic and putting it in
>> ethtool. If Furong had such good will so as to regression-test the
>> attached patch, do you think you could use this as a starting place
>> instead, and implement some ops and call some library methods, instead
>> of writing the entire logic yourself?
>>
> 
> I am quiet busy these days, especially near the end of the year :)
> 
> Maybe I can help testing the attached patch when the next time net-next opens.
> 
> Thanks.
> 

I'm a colleague of Faizal and I'd be happy to help out with testing the 
patch. I'll take care of testing it on the stmmac side and will sort out 
any issues that come up. If there are any necessary fixes or improvements, 
I'll handle those and provide feedback accordingly.

Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Abdul Rahim, Faizal]

On 17/12/2024 8:22 am, Vladimir Oltean wrote:

>> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
>> index 3088cdd08f35..ba96776d5854 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
>> @@ -308,6 +308,8 @@
>>   #define IGC_TXD_DTYP_C		0x00000000 /* Context Descriptor */
>>   #define IGC_TXD_POPTS_IXSM	0x01       /* Insert IP checksum */
>>   #define IGC_TXD_POPTS_TXSM	0x02       /* Insert TCP/UDP checksum */
>> +#define IGC_TXD_POPTS_SMD_V	0x10       /* Transmitted packet is a SMD-Verify */
>> +#define IGC_TXD_POPTS_SMD_R	0x20       /* Transmitted packet is a SMD-Response */
>>   #define IGC_TXD_CMD_EOP		0x01000000 /* End of Packet */
>>   #define IGC_TXD_CMD_IC		0x04000000 /* Insert Checksum */
>>   #define IGC_TXD_CMD_DEXT	0x20000000 /* Desc extension (0 = legacy) */
>> @@ -370,9 +372,13 @@
>>   #define IGC_RXD_STAT_VP		0x08	/* IEEE VLAN Packet */
>>   
>>   #define IGC_RXDEXT_STATERR_LB	0x00040000
>> +#define IGC_RXD_STAT_SMD_V	0x2000  /* SMD-Verify packet */
>> +#define IGC_RXD_STAT_SMD_R	0x4000  /* SMD-Response packet */
>>   
>>   /* Advanced Receive Descriptor bit definitions */
>>   #define IGC_RXDADV_STAT_TSIP	0x08000 /* timestamp in packet */
>> +#define IGC_RXDADV_STAT_SMD_TYPE_MASK	0x06000
>> +#define IGC_RXDADV_STAT_SMD_TYPE_SHIFT	13
>>   
>>   #define IGC_RXDEXT_STATERR_L4E		0x20000000
>>   #define IGC_RXDEXT_STATERR_IPE		0x40000000
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> index 1954561ec4aa..7cde0e5a7320 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> @@ -1788,6 +1788,7 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
>>   {
>>   	struct igc_adapter *adapter = netdev_priv(netdev);
>>   	struct fpe_t *fpe = &adapter->fpe;
>> +	bool verify_enabled_changed;
>>   
>>   	if (cmd->tx_min_frag_size < IGC_TX_MIN_FRAG_SIZE ||
>>   	    cmd->tx_min_frag_size > IGC_TX_MAX_FRAG_SIZE)
>> @@ -1805,7 +1806,12 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
>>   
>>   	fpe->tx_enabled = cmd->tx_enabled;
>>   	fpe->pmac_enabled = cmd->pmac_enabled;
>> -	fpe->verify_enabled = cmd->verify_enabled;
>> +	verify_enabled_changed = (cmd->verify_enabled != fpe->verify_enabled);
> 
> I wonder if it's worth using an intermediary variable when the result is
> only evaluated once. The intention is clear enough already, you call a
> function named igc_fpe_verify_enabled_changed().
> 
yea when I read this part again, I don't this it's needed.
Will remove the new local var and change the code to:

    	if (cmd->verify_enabled != fpe->verify_enabled) {
                 fpe->verify_enabled = cmd->verify_enabled;
                 igc_fpe_verify_enabled_changed(fpe);

>> +
>> +	if (verify_enabled_changed) {
>> +		fpe->verify_enabled = cmd->verify_enabled;
>> +		igc_fpe_verify_enabled_changed(fpe);
>> +	}
>>   
>>   	return igc_tsn_offload_apply(adapter);
>>   }
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index b85eaf34d07b..e184959ef218 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -2534,7 +2534,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter,
>>   }
>>   
>>   /* This function assumes __netif_tx_lock is held by the caller. */
>> -static void igc_flush_tx_descriptors(struct igc_ring *ring)
>> +void igc_flush_tx_descriptors(struct igc_ring *ring)
>>   {
>>   	/* Once tail pointer is updated, hardware can fetch the descriptors
>>   	 * any time so we issue a write membar here to ensure all memory
>> @@ -2585,6 +2585,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>>   	struct sk_buff *skb = rx_ring->skb;
>>   	u16 cleaned_count = igc_desc_unused(rx_ring);
>>   	int xdp_status = 0, rx_buffer_pgcnt;
>> +	int smd_type;
>>   
>>   	while (likely(total_packets < budget)) {
>>   		struct igc_xdp_buff ctx = { .rx_ts = NULL };
>> @@ -2622,6 +2623,18 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>>   			size -= IGC_TS_HDR_LEN;
>>   		}
>>   
>> +		smd_type = igc_fpe_get_smd_type(rx_desc->wb.upper.status_error);
>> +
>> +		if (igc_fpe_is_verify_or_response(smd_type, size)) {
>> +			igc_fpe_preprocess_verify_response(&adapter->fpe,
>> +							   smd_type);
>> +
>> +			/* Advance the ring next-to-clean */
>> +			igc_is_non_eop(rx_ring, rx_desc);
>> +			cleaned_count++;
>> +			continue;
>> +		}
>> +
> 
> Premature optimization is the root of all evil, I know, but in the
> future it might be interesting to add a static key here that gets
> incremented (enabled) based on pmac_enabled, such that the fast path
> does not get to suffer a performance penalty when frame preemption is
> supported in the kernel, regardless of whether it is enabled or not.
> 

You mean something like this ?

igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
{
	static bool pmac_enabled = adapter->fpe.pmac_enabled;
	..
         ..
	if (pmac_enabled && (igc_fpe_is_verify_or_response(smd_type, size))

[PATCH iwl-next 7/9] igc: Add support for preemptible traffic class in taprio

[Faizal Rahim]

Set queue as preemptible or express for taprio.
This will eventually set queue-specific preemptible field in TXQCTL
register.

A preemptible queue can only be set if it satisfies the conditions in
igc_fpe_is_tx_preempt_allowed(), including the verification handshake
condition. However, the handshake is optional, as users can disable the
"verify_enabled" field, which the function also handles.

Verified that the correct preemptible hardware queue is set using the
following commands:

a) 1:1 TC-to-Queue Mapping
   $ sudo tc qdisc replace dev enp1s0 parent root handle 100 \
     taprio num_tc 4 map 0 1 2 3 0 0 0 0 0 0 0 0 0 0 0 0 \
     queues 1@0 1@1 1@2 1@3 base-time 0 sched-entry S F 100000 \
     fp E E P P

b) Non-1:1 TC-to-Queue Mapping
   $ sudo tc qdisc replace  dev enp1s0 parent root handle 100 \
     taprio num_tc 3 map 0 1 1 1 2 0 0 0 0 0 0 0 0 0 0 0
     queues 2@0 1@2 1@3
     fp E P P

Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  2 +-
 drivers/net/ethernet/intel/igc/igc_defines.h |  1 +
 drivers/net/ethernet/intel/igc/igc_main.c    | 36 ++++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 15 ++++++++
 drivers/net/ethernet/intel/igc/igc_tsn.h     |  1 +
 5 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 56a426765be7..fc1960925e28 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -192,7 +192,7 @@ struct igc_ring {
 	bool launchtime_enable;         /* true if LaunchTime is enabled */
 	ktime_t last_tx_cycle;          /* end of the cycle with a launchtime transmission */
 	ktime_t last_ff_cycle;          /* Last cycle with an active first flag */
-
+	bool preemptible;               /* True if not express */
 	u32 start_time;
 	u32 end_time;
 	u32 max_sdu;
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index ba96776d5854..33c2e4ce7cc8 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -557,6 +557,7 @@
 #define IGC_TXQCTL_QUEUE_MODE_LAUNCHT	0x00000001
 #define IGC_TXQCTL_STRICT_CYCLE		0x00000002
 #define IGC_TXQCTL_STRICT_END		0x00000004
+#define IGC_TXQCTL_PREEMPTIBLE		0x00000008
 #define IGC_TXQCTL_QAV_SEL_MASK		0x000000C0
 #define IGC_TXQCTL_QAV_SEL_CBS0		0x00000080
 #define IGC_TXQCTL_QAV_SEL_CBS1		0x000000C0
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index e184959ef218..2787a91965d1 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6216,6 +6216,39 @@ static bool is_base_time_past(ktime_t base_time, const struct timespec64 *now)
 	return timespec64_compare(now, &b) > 0;
 }
 
+static u32 igc_map_tc_to_queue(const struct igc_adapter *adapter,
+			       unsigned long preemptible_tcs)
+{
+	struct net_device *dev = adapter->netdev;
+	u32 i, queue = 0;
+
+	for (i = 0; i < dev->num_tc; i++) {
+		u32 offset, count;
+
+		if (!(preemptible_tcs & BIT(i)))
+			continue;
+
+		offset = dev->tc_to_txq[i].offset;
+		count = dev->tc_to_txq[i].count;
+		queue |= GENMASK(offset + count - 1, offset);
+	}
+
+	return queue;
+}
+
+static void igc_save_preempt_queue(struct igc_adapter *adapter,
+				   const struct tc_mqprio_qopt_offload *mqprio)
+{
+	u32 preemptible_queue = igc_map_tc_to_queue(adapter,
+						    mqprio->preemptible_tcs);
+
+	for (int i = 0; i < adapter->num_tx_queues; i++) {
+		struct igc_ring *tx_ring = adapter->tx_ring[i];
+
+		tx_ring->preemptible = preemptible_queue & BIT(i);
+	}
+}
+
 static bool validate_schedule(struct igc_adapter *adapter,
 			      const struct tc_taprio_qopt_offload *qopt)
 {
@@ -6302,6 +6335,7 @@ static int igc_qbv_clear_schedule(struct igc_adapter *adapter)
 		ring->start_time = 0;
 		ring->end_time = NSEC_PER_SEC;
 		ring->max_sdu = 0;
+		ring->preemptible = false;
 	}
 
 	spin_lock_irqsave(&adapter->qbv_tx_lock, flags);
@@ -6458,6 +6492,8 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 			ring->max_sdu = 0;
 	}
 
+	igc_save_preempt_queue(adapter, &qopt->mqprio);
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 3d39be2219f3..efd2a9f676d8 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -209,6 +209,17 @@ void igc_fpe_init(struct fpe_t *fpe)
 	fpe->tx_min_frag_size = IGC_TX_MIN_FRAG_SIZE;
 }
 
+static bool igc_fpe_is_verifed(const struct fpe_t *fpe)
+{
+	return (fpe->verify_enabled && fpe->verify_state == VERIFIED);
+}
+
+bool igc_fpe_is_tx_preempt_allowed(const struct fpe_t *fpe)
+{
+	return (fpe->pmac_enabled && fpe->tx_enabled &&
+		(igc_fpe_is_verifed(fpe) || !fpe->verify_enabled));
+}
+
 void igc_fpe_verify_enabled_changed(struct fpe_t *fpe)
 {
 	if (fpe->verify_enabled && fpe->tx_enabled) {
@@ -539,6 +550,10 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 		if (ring->launchtime_enable)
 			txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
 
+		if (igc_fpe_is_tx_preempt_allowed(&adapter->fpe) &&
+		    ring->preemptible)
+			txqctl |= IGC_TXQCTL_PREEMPTIBLE;
+
 		/* Skip configuring CBS for Q2 and Q3 */
 		if (i > 1)
 			goto skip_cbs;
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
index f3d83fbbd1f4..2b67ecae99c9 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.h
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
@@ -14,6 +14,7 @@
 
 int igc_fpe_get_smd_type(__le32 status_error);
 void igc_fpe_init(struct fpe_t *fpe);
+bool igc_fpe_is_tx_preempt_allowed(const struct fpe_t *fpe);
 bool igc_fpe_is_verify_or_response(int smd_type, unsigned int size);
 void igc_fpe_preprocess_verify_response(struct fpe_t *fpe, int smd_type);
 void igc_fpe_verify_enabled_changed(struct fpe_t *fpe);
-- 
2.25.1

[PATCH iwl-next 8/9] igc: Add support to get MAC Merge data via ethtool

[Faizal Rahim]

Implement "ethtool --show-mm" callback for IGC.

Tested with command:
$ ethtool --show-mm enp1s0.
  MAC Merge layer state for enp1s0:
  pMAC enabled: on
  TX enabled: on
  TX active: on
  TX minimum fragment size: 252
  RX minimum fragment size: 252
  Verify enabled: on
  Verify time: 128
  Max verify time: 128
  Verification status: SUCCEEDED

Verified that the fields value are retrieved correctly.

Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  2 +-
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 20 ++++++++++++
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 33 ++++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_tsn.h     |  1 +
 4 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index fc1960925e28..3199da9b87ba 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -40,7 +40,7 @@ void igc_ethtool_set_ops(struct net_device *);
 
 #define IGC_MAX_TX_TSTAMP_REGS		4
 
-/* Verification state defined as per section 30.14.1.2 in 802.3br spec */
+/* Verify state defined as per section 99.4.8, Figure 99-8 in 802.3br spec */
 enum verify_state {
 	VERIFY_FAIL,
 	INIT_VERIFICATION,
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 7cde0e5a7320..16aa6e4e1727 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1782,6 +1782,25 @@ static int igc_ethtool_set_eee(struct net_device *netdev,
 	return 0;
 }
 
+static int igc_ethtool_get_mm(struct net_device *netdev,
+			      struct ethtool_mm_state *cmd)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	struct fpe_t *fpe = &adapter->fpe;
+
+	cmd->tx_min_frag_size = fpe->tx_min_frag_size;
+	cmd->rx_min_frag_size = fpe->tx_min_frag_size;
+	cmd->pmac_enabled = fpe->pmac_enabled;
+	cmd->verify_enabled = fpe->verify_enabled;
+	cmd->verify_time = fpe->verify_time;
+	cmd->tx_active = igc_fpe_is_tx_preempt_allowed(&adapter->fpe);
+	cmd->tx_enabled = fpe->tx_enabled;
+	cmd->verify_status = igc_fpe_get_verify_status(&adapter->fpe);
+	cmd->max_verify_time = MAX_VERIFY_TIME;
+
+	return 0;
+}
+
 static int igc_ethtool_set_mm(struct net_device *netdev,
 			      struct ethtool_mm_cfg *cmd,
 			      struct netlink_ext_ack *extack)
@@ -2103,6 +2122,7 @@ static const struct ethtool_ops igc_ethtool_ops = {
 	.set_rxfh		= igc_ethtool_set_rxfh,
 	.get_ts_info		= igc_ethtool_get_ts_info,
 	.get_channels		= igc_ethtool_get_channels,
+	.get_mm			= igc_ethtool_get_mm,
 	.set_mm			= igc_ethtool_set_mm,
 	.set_channels		= igc_ethtool_set_channels,
 	.get_priv_flags		= igc_ethtool_get_priv_flags,
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index efd2a9f676d8..919a7f088a72 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -258,6 +258,39 @@ void igc_fpe_preprocess_verify_response(struct fpe_t *fpe, int smd_type)
 	schedule_delayed_work(&fpe->verification_work, 0);
 }
 
+enum ethtool_mm_verify_status igc_fpe_get_verify_status(const struct fpe_t *fpe)
+{
+	enum ethtool_mm_verify_status verify_status;
+
+	switch (fpe->verify_state) {
+	case VERIFY_FAIL:
+		verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
+		break;
+
+	case INIT_VERIFICATION:
+		if (fpe->verify_enabled)
+			verify_status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
+		else
+			verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
+		break;
+
+	case VERIFIED:
+		verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
+		break;
+
+	case SEND_VERIFY:
+	case WAIT_FOR_RESPONSE:
+		verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
+		break;
+
+	default:
+		verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
+		break;
+	}
+
+	return verify_status;
+}
+
 static bool is_any_launchtime(struct igc_adapter *adapter)
 {
 	int i;
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
index 2b67ecae99c9..913f983652e4 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.h
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
@@ -13,6 +13,7 @@
 #define MAX_VERIFY_TIME			128
 
 int igc_fpe_get_smd_type(__le32 status_error);
+enum ethtool_mm_verify_status igc_fpe_get_verify_status(const struct fpe_t *fpe);
 void igc_fpe_init(struct fpe_t *fpe);
 bool igc_fpe_is_tx_preempt_allowed(const struct fpe_t *fpe);
 bool igc_fpe_is_verify_or_response(int smd_type, unsigned int size);
-- 
2.25.1

Re: [PATCH iwl-next 8/9] igc: Add support to get MAC Merge data via ethtool

[Vladimir Oltean]

On Mon, Dec 16, 2024 at 01:47:19AM -0500, Faizal Rahim wrote:
> Implement "ethtool --show-mm" callback for IGC.
> 
> Tested with command:
> $ ethtool --show-mm enp1s0.
>   MAC Merge layer state for enp1s0:
>   pMAC enabled: on
>   TX enabled: on
>   TX active: on
>   TX minimum fragment size: 252
>   RX minimum fragment size: 252

I'm going to ask "why so high?" and then I'm going to answer that I
suspect this is a positive feedback loop created by openlldp, because of
the driver incorrectly reporting:

- 60 as 68, ..., 252 as 260, and openlldp always (correctly) rounding up
  these non-standard values to the closest upper multiple of an
  addFragSize, which is all that can be advertised over LLDP
- on RX what was configured on TX (see below), which in turn makes the
  link partner again want to readjust (increase) its TX, to satisfy the
  new RX requirement

But I'm open to hearing the correct answer, coming from you :)

>   Verify enabled: on
>   Verify time: 128
>   Max verify time: 128
>   Verification status: SUCCEEDED
> 
> Verified that the fields value are retrieved correctly.
> 
> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
> ---
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 7cde0e5a7320..16aa6e4e1727 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1782,6 +1782,25 @@ static int igc_ethtool_set_eee(struct net_device *netdev,
>  	return 0;
>  }
>  
> +static int igc_ethtool_get_mm(struct net_device *netdev,
> +			      struct ethtool_mm_state *cmd)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	struct fpe_t *fpe = &adapter->fpe;
> +
> +	cmd->tx_min_frag_size = fpe->tx_min_frag_size;
> +	cmd->rx_min_frag_size = fpe->tx_min_frag_size;

This is most likely a mistake. rx_min_frag_size means what is the
smallest fragment size that the i225 can receive. Whereas tx_min_frag_size
means what minimum fragment size it is configured to transmit (based,
among others, on the link partner's minimum RX requirements).
To say that the i225's minimum RX fragment size depends on how small it
was configured to transmit seems wrong. I would expect a constant, or if
this is correct, an explanation. TI treats rx_min_frag_size != ETH_ZLEN
as errata.

> +	cmd->pmac_enabled = fpe->pmac_enabled;
> +	cmd->verify_enabled = fpe->verify_enabled;
> +	cmd->verify_time = fpe->verify_time;
> +	cmd->tx_active = igc_fpe_is_tx_preempt_allowed(&adapter->fpe);
> +	cmd->tx_enabled = fpe->tx_enabled;
> +	cmd->verify_status = igc_fpe_get_verify_status(&adapter->fpe);
> +	cmd->max_verify_time = MAX_VERIFY_TIME;
> +
> +	return 0;
> +}

Re: [PATCH iwl-next 8/9] igc: Add support to get MAC Merge data via ethtool

[Abdul Rahim, Faizal]

On 17/12/2024 8:35 am, Vladimir Oltean wrote:
> On Mon, Dec 16, 2024 at 01:47:19AM -0500, Faizal Rahim wrote:
>> Implement "ethtool --show-mm" callback for IGC.
>>
>> Tested with command:
>> $ ethtool --show-mm enp1s0.
>>    MAC Merge layer state for enp1s0:
>>    pMAC enabled: on
>>    TX enabled: on
>>    TX active: on
>>    TX minimum fragment size: 252
>>    RX minimum fragment size: 252
> 
> I'm going to ask "why so high?" and then I'm going to answer that I
> suspect this is a positive feedback loop created by openlldp, because of
> the driver incorrectly reporting:
> 
> - 60 as 68, ..., 252 as 260, and openlldp always (correctly) rounding up
>    these non-standard values to the closest upper multiple of an
>    addFragSize, which is all that can be advertised over LLDP
> - on RX what was configured on TX (see below), which in turn makes the
>    link partner again want to readjust (increase) its TX, to satisfy the
>    new RX requirement
> 
> But I'm open to hearing the correct answer, coming from you :)
> 

Actually ... it was so high 252 ... because I mistakenly copied the result 
from my past openlldp test that did:			
sudo lldptool -T -i enp1s0 -V addEthCaps addFragSize=3
Which sets is to 252 ..sorry causing confusion

Without OpenLLDP, with just ethtool and with default tx min frag size, it 
will look like:			
user@localhost:~$ sudo ethtool --show-mm enp1s0
MAC Merge layer state for enp1s0:
pMAC enabled: off
TX enabled: off
TX active: off
TX minimum fragment size: 68
RX minimum fragment size: 68
Verify enabled: off
Verify time: 10
Max verify time: 128
Verification status: DISABLED

When verify handshake is done with OpenLLDP, it will look like:
user@localhost:~$ sudo lldptool -t -i enp1s0 -V addEthCaps
Additional Ethernet Capabilities TLV
         Preemption capability supported
         Preemption capability enabled
         Preemption capability active
         Additional fragment size: 1 (124 octets)

user@localhost:~$ sudo ethtool --show-mm enp1s0
MAC Merge layer state for enp1s0:
pMAC enabled: on
TX enabled: on
TX active: on
TX minimum fragment size: 124
RX minimum fragment size: 124
Verify enabled: on
Verify time: 128
Max verify time: 128
Verification status: SUCCEEDED

Which makes sense, due to the rounding up 68 to the closest upper multiple 
of addFragSize which is 124 octet in OpenLLDP, as what you mentioned.


>> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> index 7cde0e5a7320..16aa6e4e1727 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> @@ -1782,6 +1782,25 @@ static int igc_ethtool_set_eee(struct net_device *netdev,
>>   	return 0;
>>   }
>>   
>> +static int igc_ethtool_get_mm(struct net_device *netdev,
>> +			      struct ethtool_mm_state *cmd)
>> +{
>> +	struct igc_adapter *adapter = netdev_priv(netdev);
>> +	struct fpe_t *fpe = &adapter->fpe;
>> +
>> +	cmd->tx_min_frag_size = fpe->tx_min_frag_size;
>> +	cmd->rx_min_frag_size = fpe->tx_min_frag_size;
> 
> This is most likely a mistake. rx_min_frag_size means what is the
> smallest fragment size that the i225 can receive. Whereas tx_min_frag_size
> means what minimum fragment size it is configured to transmit (based,
> among others, on the link partner's minimum RX requirements).
> To say that the i225's minimum RX fragment size depends on how small it
> was configured to transmit seems wrong. I would expect a constant, or if
> this is correct, an explanation. TI treats rx_min_frag_size != ETH_ZLEN
> as errata.
> 

My bad.
I got your point, it's clearly explained, thanks :).
Just got to know i226 is able to handle any frag size for RX.
Since standard for min TX is 60, I'll use 60 then.

[PATCH iwl-next 9/9] igc: Add support to get frame preemption statistics via ethtool

[Faizal Rahim]

Implemented "ethtool --include-statistics --show-mm" callback for IGC.

Tested preemption scenario to check preemption statistics:
1) Trigger verification handshake on both boards:
    $ sudo ethtool --set-mm enp1s0 pmac-enabled on
    $ sudo ethtool --set-mm enp1s0 tx-enabled on
    $ sudo ethtool --set-mm enp1s0 verify-enabled on
2) Set preemptible or express queue in taprio for tx board:
    $ sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \
      num_tc 4 map 0 1 2 3 0 0 0 0 0 0 0 0 0 0 0 0 \
      queues 1@0 1@1 1@2 1@3 base-time 0 sched-entry S F 100000 \
      fp E E P P
3) Send large size packets on preemptible queue
4) Send small size packets on express queue to preempt packets in
   preemptible queue
5) Show preemption statistics on the receiving board:
   $ ethtool --include-statistics --show-mm enp1s0
     MAC Merge layer state for enp1s0:
     pMAC enabled: on
     TX enabled: on
     TX active: on
     TX minimum fragment size: 252
     RX minimum fragment size: 252
     Verify enabled: on
     Verify time: 128
     Max verify time: 128
     Verification status: SUCCEEDED
     Statistics:
     	MACMergeFrameAssErrorCount: 0
	MACMergeFrameSmdErrorCount: 0
	MACMergeFrameAssOkCount: 511
	MACMergeFragCountRx: 764
	MACMergeFragCountTx: 0
	MACMergeHoldCount: 0

Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 40 ++++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    | 19 ++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 16aa6e4e1727..90a9dbb0d901 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1835,6 +1835,45 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
 	return igc_tsn_offload_apply(adapter);
 }
 
+/**
+ * igc_ethtool_get_frame_ass_error - Get the frame assembly error count.
+ * @dev: Pointer to the net_device structure.
+ * @return: The count of frame assembly errors.
+ */
+static u64 igc_ethtool_get_frame_ass_error(struct net_device *dev)
+{
+	struct igc_adapter *adapter = netdev_priv(dev);
+	u32 ooo_smdc, ooo_frame_cnt, ooo_frag_cnt; /* Out of order statistics */
+	struct igc_hw *hw = &adapter->hw;
+	u32 miss_frame_frag_cnt;
+	u32 reg_value;
+
+	reg_value = rd32(IGC_PRMEXPRCNT);
+	ooo_smdc = reg_value & IGC_PRMEXPRCNT_OOO_SMDC;
+	ooo_frame_cnt = (reg_value & IGC_PRMEXPRCNT_OOO_FRAME_CNT)
+			 >> IGC_PRMEXPRCNT_OOO_FRAME_CNT_SHIFT;
+	ooo_frag_cnt = (reg_value & IGC_PRMEXPRCNT_OOO_FRAG_CNT)
+			>> IGC_PRMEXPRCNT_OOO_FRAG_CNT_SHIFT;
+	miss_frame_frag_cnt = (reg_value & IGC_PRMEXPRCNT_MISS_FRAME_FRAG_CNT)
+			      >> IGC_PRMEXPRCNT_MISS_FRAME_FRAG_CNT_SHIFT;
+
+	return ooo_smdc + ooo_frame_cnt + ooo_frag_cnt + miss_frame_frag_cnt;
+}
+
+static void igc_ethtool_get_mm_stats(struct net_device *dev,
+				     struct ethtool_mm_stats *stats)
+{
+	struct igc_adapter *adapter = netdev_priv(dev);
+	struct igc_hw *hw = &adapter->hw;
+
+	stats->MACMergeFrameAssErrorCount = igc_ethtool_get_frame_ass_error(dev);
+	stats->MACMergeFrameSmdErrorCount = 0; /* Not available in IGC */
+	stats->MACMergeFrameAssOkCount = rd32(IGC_PRMPTDRCNT);
+	stats->MACMergeFragCountRx =  rd32(IGC_PRMEVNTRCNT);
+	stats->MACMergeFragCountTx = rd32(IGC_PRMEVNTTCNT);
+	stats->MACMergeHoldCount = 0; /* Not available in IGC */
+}
+
 static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
 					  struct ethtool_link_ksettings *cmd)
 {
@@ -2124,6 +2163,7 @@ static const struct ethtool_ops igc_ethtool_ops = {
 	.get_channels		= igc_ethtool_get_channels,
 	.get_mm			= igc_ethtool_get_mm,
 	.set_mm			= igc_ethtool_set_mm,
+	.get_mm_stats		= igc_ethtool_get_mm_stats,
 	.set_channels		= igc_ethtool_set_channels,
 	.get_priv_flags		= igc_ethtool_get_priv_flags,
 	.set_priv_flags		= igc_ethtool_set_priv_flags,
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index 12ddc5793651..f40946cce35a 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -222,6 +222,25 @@
 
 #define IGC_FTQF(_n)	(0x059E0 + (4 * (_n)))  /* 5-tuple Queue Fltr */
 
+/* Time sync registers - preemption statistics */
+#define IGC_PRMEVNTTCNT		0x04298	/* TX Preemption event counter */
+#define IGC_PRMEVNTRCNT		0x0429C	/* RX Preemption event counter */
+#define IGC_PRMPTDRCNT		0x04284	/* Good RX Preempted Packets */
+
+ /* Preemption Exception Counter */
+#define IGC_PRMEXPRCNT					0x042A0
+/* Received out of order packets with SMD-C and NOT ReumeRx */
+#define IGC_PRMEXPRCNT_OOO_SMDC 0x000000FF
+/* Received out of order packets with SMD-C and wrong Frame CNT */
+#define IGC_PRMEXPRCNT_OOO_FRAME_CNT			0x0000FF00
+#define IGC_PRMEXPRCNT_OOO_FRAME_CNT_SHIFT		8
+/* Received out of order packets with SMD-C and wrong Frag CNT */
+#define IGC_PRMEXPRCNT_OOO_FRAG_CNT			0x00FF0000
+#define IGC_PRMEXPRCNT_OOO_FRAG_CNT_SHIFT		16
+/* Received packets with SMD-S and ReumeRx */
+#define IGC_PRMEXPRCNT_MISS_FRAME_FRAG_CNT		0xFF000000
+#define IGC_PRMEXPRCNT_MISS_FRAME_FRAG_CNT_SHIFT	24
+
 /* Transmit Scheduling Registers */
 #define IGC_TQAVCTRL		0x3570
 #define IGC_TXQCTL(_n)		(0x3344 + 0x4 * (_n))
-- 
2.25.1

Re: [PATCH iwl-next 9/9] igc: Add support to get frame preemption statistics via ethtool

[Vladimir Oltean]

On Mon, Dec 16, 2024 at 01:47:20AM -0500, Faizal Rahim wrote:
> Implemented "ethtool --include-statistics --show-mm" callback for IGC.
> 
> Tested preemption scenario to check preemption statistics:
> 1) Trigger verification handshake on both boards:
>     $ sudo ethtool --set-mm enp1s0 pmac-enabled on
>     $ sudo ethtool --set-mm enp1s0 tx-enabled on
>     $ sudo ethtool --set-mm enp1s0 verify-enabled on
> 2) Set preemptible or express queue in taprio for tx board:
>     $ sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \
>       num_tc 4 map 0 1 2 3 0 0 0 0 0 0 0 0 0 0 0 0 \
>       queues 1@0 1@1 1@2 1@3 base-time 0 sched-entry S F 100000 \
>       fp E E P P

Hmm, the prio_tc_map pattern changed since the last time I looked at igc
examples? It was in decreasing order before? How do you handle backwards
compatibility with the Tx ring strict priority default configuration?
I haven't downloaded the entire set locally, will do so later.

> 3) Send large size packets on preemptible queue
> 4) Send small size packets on express queue to preempt packets in
>    preemptible queue
> 5) Show preemption statistics on the receiving board:
>    $ ethtool --include-statistics --show-mm enp1s0
>      MAC Merge layer state for enp1s0:
>      pMAC enabled: on
>      TX enabled: on
>      TX active: on
>      TX minimum fragment size: 252
>      RX minimum fragment size: 252
>      Verify enabled: on
>      Verify time: 128
>      Max verify time: 128
>      Verification status: SUCCEEDED
>      Statistics:
>      	MACMergeFrameAssErrorCount: 0
> 	MACMergeFrameSmdErrorCount: 0
> 	MACMergeFrameAssOkCount: 511
> 	MACMergeFragCountRx: 764
> 	MACMergeFragCountTx: 0
> 	MACMergeHoldCount: 0
> 
> Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 40 ++++++++++++++++++++
>  drivers/net/ethernet/intel/igc/igc_regs.h    | 19 ++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 16aa6e4e1727..90a9dbb0d901 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1835,6 +1835,45 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
>  	return igc_tsn_offload_apply(adapter);
>  }
>  
> +/**
> + * igc_ethtool_get_frame_ass_error - Get the frame assembly error count.
> + * @dev: Pointer to the net_device structure.
> + * @return: The count of frame assembly errors.

I may be wrong, but I think the syntax for kernel-doc is "Returns: "

> + */
> +static u64 igc_ethtool_get_frame_ass_error(struct net_device *dev)
> +{
> +	struct igc_adapter *adapter = netdev_priv(dev);
> +	u32 ooo_smdc, ooo_frame_cnt, ooo_frag_cnt; /* Out of order statistics */
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 miss_frame_frag_cnt;
> +	u32 reg_value;
> +
> +	reg_value = rd32(IGC_PRMEXPRCNT);
> +	ooo_smdc = reg_value & IGC_PRMEXPRCNT_OOO_SMDC;
> +	ooo_frame_cnt = (reg_value & IGC_PRMEXPRCNT_OOO_FRAME_CNT)
> +			 >> IGC_PRMEXPRCNT_OOO_FRAME_CNT_SHIFT;
> +	ooo_frag_cnt = (reg_value & IGC_PRMEXPRCNT_OOO_FRAG_CNT)
> +			>> IGC_PRMEXPRCNT_OOO_FRAG_CNT_SHIFT;
> +	miss_frame_frag_cnt = (reg_value & IGC_PRMEXPRCNT_MISS_FRAME_FRAG_CNT)
> +			      >> IGC_PRMEXPRCNT_MISS_FRAME_FRAG_CNT_SHIFT;

Candidates for FIELD_GET()?

> +
> +	return ooo_smdc + ooo_frame_cnt + ooo_frag_cnt + miss_frame_frag_cnt;
> +}
> +
> +static void igc_ethtool_get_mm_stats(struct net_device *dev,
> +				     struct ethtool_mm_stats *stats)
> +{
> +	struct igc_adapter *adapter = netdev_priv(dev);
> +	struct igc_hw *hw = &adapter->hw;
> +
> +	stats->MACMergeFrameAssErrorCount = igc_ethtool_get_frame_ass_error(dev);
> +	stats->MACMergeFrameSmdErrorCount = 0; /* Not available in IGC */
> +	stats->MACMergeFrameAssOkCount = rd32(IGC_PRMPTDRCNT);
> +	stats->MACMergeFragCountRx =  rd32(IGC_PRMEVNTRCNT);
> +	stats->MACMergeFragCountTx = rd32(IGC_PRMEVNTTCNT);
> +	stats->MACMergeHoldCount = 0; /* Not available in IGC */

Don't report counters as zero when in reality you don't know.

Just don't assign values to these. mm_prepare_data() -> ethtool_stats_init()
presets them to 0xffffffffffffffff (ETHTOOL_STAT_NOT_SET), and
mm_put_stats() -> mm_put_stat() detects whether they are still equal to
this value, and if they are, does not report netlink attributes for them.

> +}
> +
>  static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
>  					  struct ethtool_link_ksettings *cmd)
>  {
> @@ -2124,6 +2163,7 @@ static const struct ethtool_ops igc_ethtool_ops = {
>  	.get_channels		= igc_ethtool_get_channels,
>  	.get_mm			= igc_ethtool_get_mm,
>  	.set_mm			= igc_ethtool_set_mm,
> +	.get_mm_stats		= igc_ethtool_get_mm_stats,
>  	.set_channels		= igc_ethtool_set_channels,
>  	.get_priv_flags		= igc_ethtool_get_priv_flags,
>  	.set_priv_flags		= igc_ethtool_set_priv_flags,
> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
> index 12ddc5793651..f40946cce35a 100644
> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
> @@ -222,6 +222,25 @@
>  
>  #define IGC_FTQF(_n)	(0x059E0 + (4 * (_n)))  /* 5-tuple Queue Fltr */
>  
> +/* Time sync registers - preemption statistics */
> +#define IGC_PRMEVNTTCNT		0x04298	/* TX Preemption event counter */
> +#define IGC_PRMEVNTRCNT		0x0429C	/* RX Preemption event counter */
> +#define IGC_PRMPTDRCNT		0x04284	/* Good RX Preempted Packets */
> +
> + /* Preemption Exception Counter */
> +#define IGC_PRMEXPRCNT					0x042A0
> +/* Received out of order packets with SMD-C and NOT ReumeRx */
> +#define IGC_PRMEXPRCNT_OOO_SMDC 0x000000FF
> +/* Received out of order packets with SMD-C and wrong Frame CNT */
> +#define IGC_PRMEXPRCNT_OOO_FRAME_CNT			0x0000FF00
> +#define IGC_PRMEXPRCNT_OOO_FRAME_CNT_SHIFT		8
> +/* Received out of order packets with SMD-C and wrong Frag CNT */
> +#define IGC_PRMEXPRCNT_OOO_FRAG_CNT			0x00FF0000
> +#define IGC_PRMEXPRCNT_OOO_FRAG_CNT_SHIFT		16
> +/* Received packets with SMD-S and ReumeRx */

What is ReumeRx?

> +#define IGC_PRMEXPRCNT_MISS_FRAME_FRAG_CNT		0xFF000000
> +#define IGC_PRMEXPRCNT_MISS_FRAME_FRAG_CNT_SHIFT	24
> +
>  /* Transmit Scheduling Registers */
>  #define IGC_TQAVCTRL		0x3570
>  #define IGC_TXQCTL(_n)		(0x3344 + 0x4 * (_n))
> -- 
> 2.25.1
> 
> 

Re: [PATCH iwl-next 9/9] igc: Add support to get frame preemption statistics via ethtool

[Abdul Rahim, Faizal]

On 17/12/2024 12:05 am, Vladimir Oltean wrote:
> On Mon, Dec 16, 2024 at 01:47:20AM -0500, Faizal Rahim wrote:
>> Implemented "ethtool --include-statistics --show-mm" callback for IGC.
>>
>> Tested preemption scenario to check preemption statistics:
>> 1) Trigger verification handshake on both boards:
>>      $ sudo ethtool --set-mm enp1s0 pmac-enabled on
>>      $ sudo ethtool --set-mm enp1s0 tx-enabled on
>>      $ sudo ethtool --set-mm enp1s0 verify-enabled on
>> 2) Set preemptible or express queue in taprio for tx board:
>>      $ sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \
>>        num_tc 4 map 0 1 2 3 0 0 0 0 0 0 0 0 0 0 0 0 \
>>        queues 1@0 1@1 1@2 1@3 base-time 0 sched-entry S F 100000 \
>>        fp E E P P
> 
> Hmm, the prio_tc_map pattern changed since the last time I looked at igc
> examples? It was in decreasing order before? How do you handle backwards
> compatibility with the Tx ring strict priority default configuration?
> I haven't downloaded the entire set locally, will do so later.
> 

I tested like this for i226:
     CMD=(
         "tc qdisc replace dev $IFACE parent root handle 100 taprio "
         "num_tc 4 "
         "map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 "
         "queues 1@0 1@1 1@2 1@3 "
         "base-time $BASE "
         "${SCHED_ENTRY[*]} "
         "flags 0x1 "
         "txtime-delay $TXTIME_DELAY "
         "clockid CLOCK_TAI "

But I mistakenly copied the mapping from a different scenario where socket 
priority -> tc -> hw_queue mapping is not important to my test objective in 
that scenario.

I'll update the description to use decreasing order then.

>> +
>> +	return ooo_smdc + ooo_frame_cnt + ooo_frag_cnt + miss_frame_frag_cnt;
>> +}
>> +
>> +static void igc_ethtool_get_mm_stats(struct net_device *dev,
>> +				     struct ethtool_mm_stats *stats)
>> +{
>> +	struct igc_adapter *adapter = netdev_priv(dev);
>> +	struct igc_hw *hw = &adapter->hw;
>> +
>> +	stats->MACMergeFrameAssErrorCount = igc_ethtool_get_frame_ass_error(dev);
>> +	stats->MACMergeFrameSmdErrorCount = 0; /* Not available in IGC */
>> +	stats->MACMergeFrameAssOkCount = rd32(IGC_PRMPTDRCNT);
>> +	stats->MACMergeFragCountRx =  rd32(IGC_PRMEVNTRCNT);
>> +	stats->MACMergeFragCountTx = rd32(IGC_PRMEVNTTCNT);
>> +	stats->MACMergeHoldCount = 0; /* Not available in IGC */
> 
> Don't report counters as zero when in reality you don't know.
> 
> Just don't assign values to these. mm_prepare_data() -> ethtool_stats_init()
> presets them to 0xffffffffffffffff (ETHTOOL_STAT_NOT_SET), and
> mm_put_stats() -> mm_put_stat() detects whether they are still equal to
> this value, and if they are, does not report netlink attributes for them.
> 

Got it.


>> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
>> index 12ddc5793651..f40946cce35a 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
>> @@ -222,6 +222,25 @@
>>   
>>   #define IGC_FTQF(_n)	(0x059E0 + (4 * (_n)))  /* 5-tuple Queue Fltr */
>>   
>> +/* Time sync registers - preemption statistics */
>> +#define IGC_PRMEVNTTCNT		0x04298	/* TX Preemption event counter */
>> +#define IGC_PRMEVNTRCNT		0x0429C	/* RX Preemption event counter */
>> +#define IGC_PRMPTDRCNT		0x04284	/* Good RX Preempted Packets */
>> +
>> + /* Preemption Exception Counter */
>> +#define IGC_PRMEXPRCNT					0x042A0
>> +/* Received out of order packets with SMD-C and NOT ReumeRx */
>> +#define IGC_PRMEXPRCNT_OOO_SMDC 0x000000FF
>> +/* Received out of order packets with SMD-C and wrong Frame CNT */
>> +#define IGC_PRMEXPRCNT_OOO_FRAME_CNT			0x0000FF00
>> +#define IGC_PRMEXPRCNT_OOO_FRAME_CNT_SHIFT		8
>> +/* Received out of order packets with SMD-C and wrong Frag CNT */
>> +#define IGC_PRMEXPRCNT_OOO_FRAG_CNT			0x00FF0000
>> +#define IGC_PRMEXPRCNT_OOO_FRAG_CNT_SHIFT		16
>> +/* Received packets with SMD-S and ReumeRx */
> 
> What is ReumeRx?

Resume receive. It’s a typo in the i226 documentation that I (shamelessly) 
copied into the code without checking properly. It's meant to indicate that 
an RX flag in the i226 firmware is set. I’ll remove the 'ResumeRX' part 
since it adds confusion.