net: stmmac: FPE via ethtool + tc

PATCH Superseded Furong Xu 3 participants 15 messages Jul 31, 2024 – Aug 6, 2024

Subsystems (4)

Networking Network drivers Arm STM32 stmmac Ethernet driver

lore

Share & tools

lore

Furong Xu

author

2024-07-31 10:43 UTC #0 lore raw

cover net: stmmac: FPE via ethtool + tc

Move the Frame Preemption(FPE) over to the new standard API which uses ethtool-mm/tc-mqprio/tc-taprio.

Furong Xu (5):
net: stmmac: configure FPE via ethtool-mm
net: stmmac: support fp parameter of tc-mqprio
net: stmmac: support fp parameter of tc-taprio
net: stmmac: drop unneeded FPE handshake code
net: stmmac: silence FPE kernel logs

 .../net/ethernet/stmicro/stmmac/dwmac4_core.c |   6 +
 drivers/net/ethernet/stmicro/stmmac/dwmac5.c  |  37 +++++-
 drivers/net/ethernet/stmicro/stmmac/dwmac5.h  |   7 ++
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  14 +++
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |   3 +
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 111 ++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  25 ++--
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   |  95 ++++++++++-----
 include/linux/stmmac.h                        |   2 +-
 9 files changed, 248 insertions(+), 52 deletions(-)

Signature · 1 line

2.34.1

Furong Xu

author

2024-07-31 10:44 UTC #1 lore raw

PATCH 1/5 net: stmmac: configure FPE via ethtool-mm

Implement ethtool --show-mm and --set-mm callbacks.

0) Setup two DWMAC CORE 5.10a devices, connect in a back-to-back manner.

1) After kernel booted, get initial status:
ethtool --include-statistics --json --show-mm eth1

[ {
        "ifname": "eth1",
        "pmac-enabled": true,
        "tx-enabled": false,
        "tx-active": false,
        "tx-min-frag-size": 60,
        "rx-min-frag-size": 60,
        "verify-enabled": false,
        "verify-time": 0,
        "max-verify-time": 128,
        "verify-status": "DISABLED",
        "statistics": {
            "MACMergeFrameAssErrorCount": 0,
            "MACMergeFrameSmdErrorCount": 0,
            "MACMergeFrameAssOkCount": 0,
            "MACMergeFragCountRx": 0,
            "MACMergeFragCountTx": 0,
            "MACMergeHoldCount": 0
        }
    } ]

3) Enable FPE on local device, keep FPE disabled on remote device:
ethtool --set-mm eth1 verify-enabled on verify-time 100 tx-enabled on

4) Get status on local device:
ethtool --include-statistics --json --show-mm eth1

[ {
        "ifname": "eth1",
        "pmac-enabled": true,
        "tx-enabled": true,
        "tx-active": false,
        "tx-min-frag-size": 60,
        "rx-min-frag-size": 60,
        "verify-enabled": true,
        "verify-time": 100,
        "max-verify-time": 128,
        "verify-status": "VERIFYING",
        "statistics": {
            "MACMergeFrameAssErrorCount": 0,
            "MACMergeFrameSmdErrorCount": 0,
            "MACMergeFrameAssOkCount": 0,
            "MACMergeFragCountRx": 0,
            "MACMergeFragCountTx": 0,
            "MACMergeHoldCount": 0
        }
    } ]

5) Enable FPE on remote device, handshake can be finished now:
ethtool --set-mm eth1 verify-enabled on verify-time 100 tx-enabled on

6) Get status on local and remote device, both report SUCCEEDED:
ethtool --include-statistics --json --show-mm eth1

[ {
        "ifname": "eth1",
        "pmac-enabled": true,
        "tx-enabled": true,
        "tx-active": true,
        "tx-min-frag-size": 60,
        "rx-min-frag-size": 60,
        "verify-enabled": true,
        "verify-time": 100,
        "max-verify-time": 128,
        "verify-status": "SUCCEEDED",
        "statistics": {
            "MACMergeFrameAssErrorCount": 0,
            "MACMergeFrameSmdErrorCount": 0,
            "MACMergeFrameAssOkCount": 0,
            "MACMergeFragCountRx": 0,
            "MACMergeFragCountTx": 0,
            "MACMergeHoldCount": 0
        }
    } ]

7) tc-mqprio or tc-taprio can be used to select whether traffic classes are express or preemptible.

Signed-off-by Furong Xu <[email protected]>

---
 .../net/ethernet/stmicro/stmmac/dwmac4_core.c |   4 +
 drivers/net/ethernet/stmicro/stmmac/dwmac5.c  |  17 +++
 drivers/net/ethernet/stmicro/stmmac/dwmac5.h  |   5 +
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |   6 +
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |   3 +
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 111 ++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |   9 +-
 include/linux/stmmac.h                        |   1 +
 8 files changed, 149 insertions(+), 7 deletions(-)

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c +4 -0

index f98741d2607e..af871cce767e 100644
@@ -1268,6 +1268,8 @@ const struct stmmac_ops dwmac410_ops = {
 	.fpe_configure = dwmac5_fpe_configure,
 	.fpe_send_mpacket = dwmac5_fpe_send_mpacket,
 	.fpe_irq_status = dwmac5_fpe_irq_status,
+	.fpe_get_add_frag_size = dwmac5_fpe_get_add_frag_size,
+	.fpe_set_add_frag_size = dwmac5_fpe_set_add_frag_size,
 	.add_hw_vlan_rx_fltr = dwmac4_add_hw_vlan_rx_fltr,
 	.del_hw_vlan_rx_fltr = dwmac4_del_hw_vlan_rx_fltr,
 	.restore_hw_vlan_rx_fltr = dwmac4_restore_hw_vlan_rx_fltr,

@@ -1320,6 +1322,8 @@ const struct stmmac_ops dwmac510_ops = {
 	.fpe_configure = dwmac5_fpe_configure,
 	.fpe_send_mpacket = dwmac5_fpe_send_mpacket,
 	.fpe_irq_status = dwmac5_fpe_irq_status,
+	.fpe_get_add_frag_size = dwmac5_fpe_get_add_frag_size,
+	.fpe_set_add_frag_size = dwmac5_fpe_set_add_frag_size,
 	.add_hw_vlan_rx_fltr = dwmac4_add_hw_vlan_rx_fltr,
 	.del_hw_vlan_rx_fltr = dwmac4_del_hw_vlan_rx_fltr,
 	.restore_hw_vlan_rx_fltr = dwmac4_restore_hw_vlan_rx_fltr,

drivers/net/ethernet/stmicro/stmmac/dwmac5.c +17 -0

index e02cebc3f1b7..5d132bada3fe 100644
@@ -638,3 +638,20 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
 
 	writel(value, ioaddr + MAC_FPE_CTRL_STS);
 }
+
+int dwmac5_fpe_get_add_frag_size(void __iomem *ioaddr)
+{
+	return FIELD_GET(AFSZ, readl(ioaddr + MTL_FPE_CTRL_STS));
+}
+
+void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size)
+{
+	u32 value;
+
+	value = readl(ioaddr + MTL_FPE_CTRL_STS);
+
+	value &= ~AFSZ;
+	value |= FIELD_PREP(AFSZ, add_frag_size);
+
+	writel(value, ioaddr + MTL_FPE_CTRL_STS);
+}

drivers/net/ethernet/stmicro/stmmac/dwmac5.h +5 -0

index bf33a51d229e..6b1c9d2da308 100644
@@ -39,6 +39,9 @@
 #define MAC_PPSx_INTERVAL(x)		(0x00000b88 + ((x) * 0x10))
 #define MAC_PPSx_WIDTH(x)		(0x00000b8c + ((x) * 0x10))
 
+#define MTL_FPE_CTRL_STS		0x00000c90
+#define AFSZ				GENMASK(1, 0)
+
 #define MTL_RXP_CONTROL_STATUS		0x00000ca0
 #define RXPI				BIT(31)
 #define NPE				GENMASK(23, 16)

@@ -109,5 +112,7 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
 			     struct stmmac_fpe_cfg *cfg,
 			     enum stmmac_mpacket_type type);
 int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev);
+int dwmac5_fpe_get_add_frag_size(void __iomem *ioaddr);
+void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size);
 
 #endif /* __DWMAC5_H__ */

drivers/net/ethernet/stmicro/stmmac/hwif.h +6 -0

index e53c32362774..f868ffc3e64f 100644
@@ -425,6 +425,8 @@ struct stmmac_ops {
 				 struct stmmac_fpe_cfg *cfg,
 				 enum stmmac_mpacket_type type);
 	int (*fpe_irq_status)(void __iomem *ioaddr, struct net_device *dev);
+	int (*fpe_get_add_frag_size)(void __iomem *ioaddr);
+	void (*fpe_set_add_frag_size)(void __iomem *ioaddr, u32 add_frag_size);
 };
 
 #define stmmac_core_init(__priv, __args...) \

@@ -529,6 +531,10 @@ struct stmmac_ops {
 	stmmac_do_void_callback(__priv, mac, fpe_send_mpacket, __args)
 #define stmmac_fpe_irq_status(__priv, __args...) \
 	stmmac_do_callback(__priv, mac, fpe_irq_status, __args)
+#define stmmac_fpe_get_add_frag_size(__priv, __args...) \
+	stmmac_do_callback(__priv, mac, fpe_get_add_frag_size, __args)
+#define stmmac_fpe_set_add_frag_size(__priv, __args...) \
+	stmmac_do_void_callback(__priv, mac, fpe_set_add_frag_size, __args)
 
 /* PTP and HW Timer helpers */
 struct stmmac_hwtimestamp {

drivers/net/ethernet/stmicro/stmmac/stmmac.h +3 -0

index b23b920eedb1..5228493bc68c 100644
@@ -345,6 +345,9 @@ struct stmmac_priv {
 	struct work_struct fpe_task;
 	char wq_name[IFNAMSIZ + 4];
 
+	/* Serialize access to MAC Merge state between ethtool requests */
+	struct mutex mm_lock;
+
 	/* TC Handling */
 	unsigned int tc_entries_max;
 	unsigned int tc_off_max;

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +111 -0

129 lines not on this page · raw message

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +2 -7

index 12689774d755..9b1cf81c50ea 100644
@@ -7384,7 +7384,6 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
 	enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state;
 	enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state;
 	bool *hs_enable = &fpe_cfg->hs_enable;
-	bool *enable = &fpe_cfg->enable;
 	int retries = 20;
 
 	while (retries-- > 0) {

@@ -7394,11 +7393,6 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
 
 		if (*lo_state == FPE_STATE_ENTERING_ON &&
 		    *lp_state == FPE_STATE_ENTERING_ON) {
-			stmmac_fpe_configure(priv, priv->ioaddr,
-					     fpe_cfg,
-					     priv->plat->tx_queues_to_use,
-					     priv->plat->rx_queues_to_use,
-					     *enable);
 
 			netdev_info(priv->dev, "configured FPE\n");

@@ -7418,7 +7412,7 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
 						MPACKET_VERIFY);
 		}
 		/* Sleep then retry */
-		msleep(500);
+		msleep(fpe_cfg->verify_time);
 	}
 
 	clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state);

@@ -7720,6 +7714,7 @@ int stmmac_dvr_probe(struct device *device,
 	stmmac_napi_add(ndev);
 
 	mutex_init(&priv->lock);
+	mutex_init(&priv->mm_lock);
 
 	/* If a specific clk_csr value is passed from the platform
 	 * this means that the CSR Clock Range selection cannot be

include/linux/stmmac.h +1 -0

index 84e13bd5df28..707a6916e51a 100644
@@ -162,6 +162,7 @@ struct stmmac_fpe_cfg {
 	enum stmmac_fpe_state lp_fpe_state;	/* Link Partner FPE state */
 	enum stmmac_fpe_state lo_fpe_state;	/* Local station FPE state */
 	u32 fpe_csr;				/* MAC_FPE_CTRL_STS reg cache */
+	u32 verify_time;			/* see struct ethtool_mm_state */
 };
 
 struct stmmac_safety_feature_cfg {

Signature · 1 line

2.34.1

Vladimir Oltean

2024-08-01 20:04 UTC #2 lore raw

Re: [PATCH net-next v1 1/5] net: stmmac: configure FPE via ethtool-mm

drivers/net/ethernet/stmicro/stmmac/stmmac.h struct stmmac_priv { · line 349

Quoted 10 lines · Furong Xu

drivers/net/ethernet/stmicro/stmmac/stmmac.h +2 -0

index b23b920eedb1..5228493bc68c 100644
@@ -345,6 +345,9 @@ struct stmmac_priv {
 	struct work_struct fpe_task;
 	char wq_name[IFNAMSIZ + 4];
 
+	/* Serialize access to MAC Merge state between ethtool requests */
+	struct mutex mm_lock;

The thing is, ethtool requests are already serialized by the rtnl_mutex. The purpose of a driver-level locking scheme is to serialize the ethtool requests with things like interrupts, work queues, etc, to avoid corrupting driver state.

Surprisingly, even if the code runs indeed very much unlocked, I don't see too many races with severe consequences. There are some exceptions though. Like for example, stmmac_fpe_handshake(enable=false) races on priv->plat->fpe_cfg->lo_fpe_state and priv->plat->fpe_cfg->lp_fpe_state with stmmac_fpe_lp_task().

static void stmmac_fpe_lp_task(struct work_struct *work)

{
	enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state;
	enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state;

	/* Bail out immediately if FPE handshake is OFF */
	if (*lo_state == FPE_STATE_OFF || !*hs_enable)
		break;

	if (*lo_state == FPE_STATE_ENTERING_ON &&
	    *lp_state == FPE_STATE_ENTERING_ON) {

		netdev_dbg(priv->dev, "configured FPE\n");

									Another CPU runs here:
									stmmac_set_mm()
									-> stmmac_fpe_handshake(enable=false)
									   -> priv->plat->fpe_cfg->lo_fpe_state = FPE_STATE_OFF;
									   -> priv->plat->fpe_cfg->lp_fpe_state = FPE_STATE_OFF;

		*lo_state = FPE_STATE_ON;
		*lp_state = FPE_STATE_ON;
		netdev_dbg(priv->dev, "!!! BOTH FPE stations ON\n");
		break;
	}
	...
}

Simply put, due to lack of locking, stmmac_set_mm() can try to stop the FPE verification task but fail, since it can lose the race.

I would expect a way for stmmac_fpe_handshake() to be able to stop further FPE interrupts from taking place (or at least from being processed), and then flush_workqueue(&priv->fpe_task) to make sure that all pending stmmac_fpe_lp_task()s have finished.

Quoted 17 lines

drivers/net/ethernet/stmicro/stmmac/stmmac.h +1 -0

+
 	/* TC Handling */
 	unsigned int tc_entries_max;
 	unsigned int tc_off_max;

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +1 -0

index 7008219fd88d..ca85e8694285 100644
@@ -19,6 +19,7 @@
 #include "stmmac.h"
 #include "dwmac_dma.h"
 #include "dwxgmac2.h"
+#include "dwmac5.h"
 
 #define REG_SPACE_SIZE	0x1060
 #define GMAC4_REG_SPACE_SIZE	0x116C

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c static int stmmac_set_tunable(struct net_device *dev, · line 1278

Quoted 8 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +5 -0

@@ -1270,6 +1271,113 @@ static int stmmac_set_tunable(struct net_device *dev,
 	return ret;
 }
 
+static int stmmac_get_mm(struct net_device *ndev,
+			 struct ethtool_mm_state *state)
+{
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	enum stmmac_fpe_state lo_state = priv->plat->fpe_cfg->lo_fpe_state;

I expect this to have to serialize with the writers of lo_fpe_state on some sort of lock. May have to be a spinlock (making the priv->mm_lock mutex inadequate in its current form), since stmmac_interrupt() is not threaded and thus runs in atomic context.

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c static int stmmac_set_tunable(struct net_device *dev, · line 1287

Quoted 9 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +9 -0

+	u32 add_frag_size;
+
+	if (!priv->dma_cap.fpesel)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&priv->mm_lock);
+
+	/* If FPE is supported by hardware, preemptible MAC is always enabled */
+	state->pmac_enabled = true;

Documentation/networking/ethtool-netlink.rst: "set if RX of preemptible and SMD-V frames is enabled". You can use the pmac_enabled knob as a hook to call stmmac_fpe_start_wq() and stmmac_fpe_stop_wq(), as well as whether to process FPE interrupt status bits.

The idea being that as long as the pMAC is enabled, you are obliged to respond to mPackets from the link partner (to allow his side* of the verification process to run). Otherwise you are not - you should behave like a FPE-incapable NIC. In fact, this is what the manual_failed_verification() portion of the kselftest attempts to test.

*This is actually a misconception of the current driver implementation. The two verification processes are completely independent, since each refers to its TX side only. One device can have ethtool --set-mm verify-enabled on, and the other off. There is no reason at all to keep a local device state + a link partner state, and to make any decision in the driver based on the LP state. Like here:

stmmac_fpe_lp_task():

	if (*lo_state == FPE_STATE_ENTERING_ON &&
	    *lp_state == FPE_STATE_ENTERING_ON) { // lp_state does not matter

		netdev_dbg(priv->dev, "configured FPE\n");

		*lo_state = FPE_STATE_ON;
		*lp_state = FPE_STATE_ON;
		netdev_dbg(priv->dev, "!!! BOTH FPE stations ON\n");
		break;
	}

	if ((*lo_state == FPE_STATE_CAPABLE ||
	     *lo_state == FPE_STATE_ENTERING_ON) &&
	     *lp_state != FPE_STATE_ON) { // lp_state does not matter
		netdev_dbg(priv->dev, SEND_VERIFY_MPAKCET_FMT,
			   *lo_state, *lp_state);
		stmmac_fpe_send_mpacket(priv, priv->ioaddr,
					fpe_cfg,
					MPACKET_VERIFY);
	}

To my superficial reading of the driver, the distinction between FPE_STATE_ENTERING_ON and FPE_STATE_ON exists solely to wait for the LP to finish its verification as well.

I think you noticed that is a non-goal as well, because your stmmac_get_mm() implementation reports ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED when we are in either of the 2 states: FPE_STATE_ENTERING_ON or FPE_STATE_ON.

I hope you don't mind me if I say that since waiting both sides to finish verification is a non-goal, I disagree with your choice of papering over the existence of the 2 FPE driver states. I would recommend consolidating them into a single one.

Another mistake relating to this (TX verification processes are not necessarily symmetric) is:

stmmac_set_mm()
-> stmmac_fpe_handshake(priv, cfg->verify_enabled)
-> priv->plat->fpe_cfg->hs_enable = enable

stmmac_fpe_event_status():

	if (status == FPE_EVENT_UNKNOWN || !*hs_enable)
		return;

	if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER) {
		/* If LP has sent Verify mPacket, send back a Responds mPacket */
		if (*hs_enable)
			stmmac_fpe_send_mpacket(priv, priv->ioaddr,
						fpe_cfg,
						MPACKET_RESPONSE);
	}

We should always respond to the link partner's Verify mPacket as long as pmac_enabled=true, and not just if hs_enable=true.

Quoted 9 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +9 -0

+
+	state->verify_time = priv->plat->fpe_cfg->verify_time;
+
+	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
+	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
+	 */
+	state->max_verify_time = 128;
+
+	if (lo_state == FPE_STATE_CAPABLE)

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c static int stmmac_set_tunable(struct net_device *dev, · line 1306

Quoted 10 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +10 -0

+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
+	else if (lo_state == FPE_STATE_ENTERING_ON || lo_state == FPE_STATE_ON)
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
+	else if (lo_state == FPE_STATE_OFF)
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
+	else
+		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
+
+	/* Cannot read MAC_FPE_CTRL_STS register here, or FPE interrupt events
+	 * can lose.

s/lose/be lost/

Quoted 79 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +54 -0

+	 *
+	 * See commit 37e4b8df27bc ("net: stmmac: fix FPE events losing")
+	 */
+	state->tx_enabled = !!(priv->plat->fpe_cfg->fpe_csr == EFPE);
+
+	/* FPE active if common tx_enabled and verification success or disabled (forced) */
+	state->tx_active = state->tx_enabled &&
+			   (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
+			    state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED);
+
+	state->verify_enabled = priv->plat->fpe_cfg->hs_enable;
+
+	add_frag_size = stmmac_fpe_get_add_frag_size(priv, priv->ioaddr);
+	state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(add_frag_size);
+
+	state->rx_min_frag_size = ETH_ZLEN;
+
+	mutex_unlock(&priv->mm_lock);
+
+	return 0;
+}
+
+static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
+			 struct netlink_ext_ack *extack)
+{
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	u32 add_frag_size;
+	int err;
+
+	if (!priv->dma_cap.fpesel)
+		return -EOPNOTSUPP;
+
+	err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size,
+					      &add_frag_size, extack);
+	if (err)
+		return err;
+
+	mutex_lock(&priv->mm_lock);
+
+	priv->plat->fpe_cfg->verify_time = cfg->verify_time;
+
+	stmmac_fpe_configure(priv, priv->ioaddr, priv->plat->fpe_cfg,
+			     priv->plat->tx_queues_to_use,
+			     priv->plat->rx_queues_to_use,
+			     cfg->tx_enabled);
+
+	stmmac_fpe_set_add_frag_size(priv, priv->ioaddr, add_frag_size);
+
+	stmmac_fpe_handshake(priv, cfg->verify_enabled);
+
+	mutex_unlock(&priv->mm_lock);
+
+	return 0;
+}

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -6

index 12689774d755..9b1cf81c50ea 100644
@@ -7384,7 +7384,6 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
 	enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state;
 	enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state;
 	bool *hs_enable = &fpe_cfg->hs_enable;
-	bool *enable = &fpe_cfg->enable;
 	int retries = 20;
 
 	while (retries-- > 0) {

@@ -7394,11 +7393,6 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
 
 		if (*lo_state == FPE_STATE_ENTERING_ON &&
 		    *lp_state == FPE_STATE_ENTERING_ON) {
-			stmmac_fpe_configure(priv, priv->ioaddr,
-					     fpe_cfg,
-					     priv->plat->tx_queues_to_use,
-					     priv->plat->rx_queues_to_use,
-					     *enable);
 
 			netdev_info(priv->dev, "configured FPE\n");

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c static void stmmac_fpe_lp_task(struct work_struct *work) · line 7415

Quoted 5 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +1 -1

@@ -7418,7 +7412,7 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
 						MPACKET_VERIFY);
 		}
 		/* Sleep then retry */
-		msleep(500);
+		msleep(fpe_cfg->verify_time);

FWIW, if you want to follow the standard, I guess you should modify "retries" to 3 as well - this is the constant that 802.3 uses for verifyLimit. It helps make the verification process fail more predictably (within verifyTime * 3 ms).

Quoted 11 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +1 -0

 	}
 
 	clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state);

@@ -7720,6 +7714,7 @@ int stmmac_dvr_probe(struct device *device,
 	stmmac_napi_add(ndev);
 
 	mutex_init(&priv->lock);
+	mutex_init(&priv->mm_lock);
 
 	/* If a specific clk_csr value is passed from the platform
 	 * this means that the CSR Clock Range selection cannot be

Furong Xu

author

2024-07-31 10:44 UTC #3 lore raw

PATCH 2/5 net: stmmac: support fp parameter of tc-mqprio

In reply to Furong Xu

tc-mqprio can select whether traffic classes are express or preemptible.

Tested on DWMAC CORE 5.10a

Signed-off-by Furong Xu <[email protected]>

---
 .../net/ethernet/stmicro/stmmac/dwmac4_core.c |  2 +
 drivers/net/ethernet/stmicro/stmmac/dwmac5.c  | 12 ++++
 drivers/net/ethernet/stmicro/stmmac/dwmac5.h  |  2 +
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  8 +++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  2 +
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 61 +++++++++++++++++++
 6 files changed, 87 insertions(+)

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c +2 -0

index af871cce767e..68cdfd9e7c8c 100644
@@ -1270,6 +1270,7 @@ const struct stmmac_ops dwmac410_ops = {
 	.fpe_irq_status = dwmac5_fpe_irq_status,
 	.fpe_get_add_frag_size = dwmac5_fpe_get_add_frag_size,
 	.fpe_set_add_frag_size = dwmac5_fpe_set_add_frag_size,
+	.fpe_set_preemptible_tcs = dwmac5_fpe_set_preemptible_tcs,
 	.add_hw_vlan_rx_fltr = dwmac4_add_hw_vlan_rx_fltr,
 	.del_hw_vlan_rx_fltr = dwmac4_del_hw_vlan_rx_fltr,
 	.restore_hw_vlan_rx_fltr = dwmac4_restore_hw_vlan_rx_fltr,

@@ -1324,6 +1325,7 @@ const struct stmmac_ops dwmac510_ops = {
 	.fpe_irq_status = dwmac5_fpe_irq_status,
 	.fpe_get_add_frag_size = dwmac5_fpe_get_add_frag_size,
 	.fpe_set_add_frag_size = dwmac5_fpe_set_add_frag_size,
+	.fpe_set_preemptible_tcs = dwmac5_fpe_set_preemptible_tcs,
 	.add_hw_vlan_rx_fltr = dwmac4_add_hw_vlan_rx_fltr,
 	.del_hw_vlan_rx_fltr = dwmac4_del_hw_vlan_rx_fltr,
 	.restore_hw_vlan_rx_fltr = dwmac4_restore_hw_vlan_rx_fltr,

drivers/net/ethernet/stmicro/stmmac/dwmac5.c +12 -0

index 5d132bada3fe..068859284691 100644
@@ -655,3 +655,15 @@ void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size)
 
 	writel(value, ioaddr + MTL_FPE_CTRL_STS);
 }
+
+void dwmac5_fpe_set_preemptible_tcs(void __iomem *ioaddr, unsigned long tcs)
+{
+	u32 value;
+
+	value = readl(ioaddr + MTL_FPE_CTRL_STS);
+
+	value &= ~PEC;
+	value |= FIELD_PREP(PEC, tcs);
+
+	writel(value, ioaddr + MTL_FPE_CTRL_STS);
+}

drivers/net/ethernet/stmicro/stmmac/dwmac5.h +2 -0

index 6b1c9d2da308..bce7e88418ac 100644
@@ -40,6 +40,7 @@
 #define MAC_PPSx_WIDTH(x)		(0x00000b8c + ((x) * 0x10))
 
 #define MTL_FPE_CTRL_STS		0x00000c90
+#define PEC				GENMASK(15, 8)
 #define AFSZ				GENMASK(1, 0)
 
 #define MTL_RXP_CONTROL_STATUS		0x00000ca0

@@ -114,5 +115,6 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
 int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev);
 int dwmac5_fpe_get_add_frag_size(void __iomem *ioaddr);
 void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size);
+void dwmac5_fpe_set_preemptible_tcs(void __iomem *ioaddr, unsigned long tcs);
 
 #endif /* __DWMAC5_H__ */

drivers/net/ethernet/stmicro/stmmac/hwif.h +8 -0

index f868ffc3e64f..109699d3df0a 100644
@@ -7,6 +7,7 @@
 
 #include <linux/netdevice.h>
 #include <linux/stmmac.h>
+#include <net/pkt_cls.h>
 
 #define stmmac_do_void_callback(__priv, __module, __cname,  __arg0, __args...) \
 ({ \

@@ -427,6 +428,7 @@ struct stmmac_ops {
 	int (*fpe_irq_status)(void __iomem *ioaddr, struct net_device *dev);
 	int (*fpe_get_add_frag_size)(void __iomem *ioaddr);
 	void (*fpe_set_add_frag_size)(void __iomem *ioaddr, u32 add_frag_size);
+	void (*fpe_set_preemptible_tcs)(void __iomem *ioaddr, unsigned long tcs);
 };
 
 #define stmmac_core_init(__priv, __args...) \

@@ -535,6 +537,8 @@ struct stmmac_ops {
 	stmmac_do_callback(__priv, mac, fpe_get_add_frag_size, __args)
 #define stmmac_fpe_set_add_frag_size(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mac, fpe_set_add_frag_size, __args)
+#define stmmac_fpe_set_preemptible_tcs(__priv, __args...) \
+	stmmac_do_void_callback(__priv, mac, fpe_set_preemptible_tcs, __args)
 
 /* PTP and HW Timer helpers */
 struct stmmac_hwtimestamp {

@@ -622,6 +626,8 @@ struct stmmac_tc_ops {
 			 struct tc_etf_qopt_offload *qopt);
 	int (*query_caps)(struct stmmac_priv *priv,
 			  struct tc_query_caps_base *base);
+	int (*setup_mqprio)(struct stmmac_priv *priv,
+			    struct tc_mqprio_qopt_offload *qopt);
 };
 
 #define stmmac_tc_init(__priv, __args...) \

@@ -638,6 +644,8 @@ struct stmmac_tc_ops {
 	stmmac_do_callback(__priv, tc, setup_etf, __args)
 #define stmmac_tc_query_caps(__priv, __args...) \
 	stmmac_do_callback(__priv, tc, query_caps, __args)
+#define stmmac_tc_setup_mqprio(__priv, __args...) \
+	stmmac_do_callback(__priv, tc, setup_mqprio, __args)
 
 struct stmmac_counters;

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +2 -0

index 9b1cf81c50ea..a5e3316bc410 100644
@@ -6256,6 +6256,8 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
 	switch (type) {
 	case TC_QUERY_CAPS:
 		return stmmac_tc_query_caps(priv, priv, type_data);
+	case TC_SETUP_QDISC_MQPRIO:
+		return stmmac_tc_setup_mqprio(priv, priv, type_data);
 	case TC_SETUP_BLOCK:
 		return flow_block_cb_setup_simple(type_data,
 						  &stmmac_block_cb_list,

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +61 -0

77 lines not on this page · raw message

Signature · 1 line

2.34.1

Vladimir Oltean

2024-08-01 23:07 UTC #4 lore raw

Re: [PATCH net-next v1 2/5] net: stmmac: support fp parameter of tc-mqprio

Quoted 24 lines · Furong Xu

tc-mqprio can select whether traffic classes are express or preemptible.

Tested on DWMAC CORE 5.10a

Signed-off-by Furong Xu <[email protected]>

---
 .../net/ethernet/stmicro/stmmac/dwmac4_core.c |  2 +
 drivers/net/ethernet/stmicro/stmmac/dwmac5.c  | 12 ++++
 drivers/net/ethernet/stmicro/stmmac/dwmac5.h  |  2 +
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  8 +++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  2 +
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 61 +++++++++++++++++++
 6 files changed, 87 insertions(+)

drivers/net/ethernet/stmicro/stmmac/dwmac5.c +2 -0

index 5d132bada3fe..068859284691 100644
@@ -655,3 +655,15 @@ void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size)
 
 	writel(value, ioaddr + MTL_FPE_CTRL_STS);
 }
+
+void dwmac5_fpe_set_preemptible_tcs(void __iomem *ioaddr, unsigned long tcs)

drivers/net/ethernet/stmicro/stmmac/dwmac5.c void dwmac5_fpe_set_add_frag_size(void __iomem *ioaddr, u32 add_frag_size) · line 669

Quoted 10 lines

drivers/net/ethernet/stmicro/stmmac/dwmac5.c +10 -0

+{
+	u32 value;
+
+	value = readl(ioaddr + MTL_FPE_CTRL_STS);
+
+	value &= ~PEC;
+	value |= FIELD_PREP(PEC, tcs);
+
+	writel(value, ioaddr + MTL_FPE_CTRL_STS);
+}

Watch out here. I think the MTL_FPE_CTRL_STS[PEC] field is per TXQ, but input from user space is per TC. There's a difference between the 2, that I'll try to clarify below. But even ignoring the case of multiple TXQs per TC, there's also the case of reverse TC:TXQ mappings: "num_tc 4 map 0 1 2 3 queues 1@3 1@2 1@1 1@0". When the user then says he wants TC 0 to be preemptible, he really means TXQ 3. That's how this should be treated.

Quoted 63 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +2 -0

index 9b1cf81c50ea..a5e3316bc410 100644
@@ -6256,6 +6256,8 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
 	switch (type) {
 	case TC_QUERY_CAPS:
 		return stmmac_tc_query_caps(priv, priv, type_data);
+	case TC_SETUP_QDISC_MQPRIO:
+		return stmmac_tc_setup_mqprio(priv, priv, type_data);
 	case TC_SETUP_BLOCK:
 		return flow_block_cb_setup_simple(type_data,
 						  &stmmac_block_cb_list,

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +35 -0

index 996f2bcd07a2..494fe2f68300 100644
@@ -1198,6 +1198,13 @@ static int tc_query_caps(struct stmmac_priv *priv,
 			 struct tc_query_caps_base *base)
 {
 	switch (base->type) {
+	case TC_SETUP_QDISC_MQPRIO: {
+		struct tc_mqprio_caps *caps = base->caps;
+
+		caps->validate_queue_counts = true;
+
+		return 0;
+	}
 	case TC_SETUP_QDISC_TAPRIO: {
 		struct tc_taprio_caps *caps = base->caps;

@@ -1214,6 +1221,59 @@ static int tc_query_caps(struct stmmac_priv *priv,
 	}
 }
 
+static void stmmac_reset_tc_mqprio(struct net_device *ndev)
+{
+	struct stmmac_priv *priv = netdev_priv(ndev);
+
+	netdev_reset_tc(ndev);
+	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
+
+	stmmac_fpe_set_preemptible_tcs(priv, priv->ioaddr, 0);
+}
+
+static int tc_setup_mqprio(struct stmmac_priv *priv,
+			   struct tc_mqprio_qopt_offload *mqprio)
+{
+	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
+	struct net_device *ndev = priv->dev;
+	int num_stack_tx_queues = 0;
+	int num_tc = qopt->num_tc;
+	int offset, count;
+	int tc, err;
+
+	if (!num_tc) {
+		stmmac_reset_tc_mqprio(ndev);
+		return 0;
+	}
+
+	err = netdev_set_num_tc(ndev, num_tc);
+	if (err)
+		return err;

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c static int tc_query_caps(struct stmmac_priv *priv, · line 1261

Quoted 10 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +10 -0

+
+	for (tc = 0; tc < num_tc; tc++) {
+		offset = qopt->offset[tc];
+		count = qopt->count[tc];
+		num_stack_tx_queues += count;
+
+		err = netdev_set_tc_queue(ndev, tc, count, offset);
+		if (err)
+			goto err_reset_tc;
+	}

We might have a problem here, and I don't know if I'm well enough equipped with DWMAC knowledge to help you solve it.

The way I understand mqprio is that it groups TX queues into traffic classes. All traffic classes are in strict priority relative to each other (TC 0 having the smallest priority). If multiple TX queues go to the same traffic class, it is expected that the NIC performs round robin dequeuing out of them. Then there's a prio_tc_map[], which maps skb->priority values to traffic classes. On xmit, netdev_pick_tx() chooses a random TX queue out of those assigned to the computed traffic class for the packet. This skb_tx_hash() is the software enqueue counterpart to what the NIC is expected to do in terms of scheduling. Much of this is said in newer versions of "man tc-mqprio".
https://man7.org/linux/man-pages/man8/tc-mqprio.8.html

Where I was trying to get is that you aren't programming the TC to TXQ mapping to hardware in any way, and you are accepting any mapping that the user requests. This isn't okay.

I believe that the DWMAC TX scheduling algorithm is strict priority by default, with plat->tx_sched_algorithm being configurable through device tree properties to other values. Then, individual TX queues have the "snps,priority" device tree property for configuring their scheduling priority. All of that can go out of sync with what the user thinks he configures through tc-mqprio, and badly.

Consider "num_tc 2 queues 3@0 2@3". The stack will think that TXQs 0, 1, 2 have one priority, and TXQs 3 and 4 another. But in reality, each TXQ (say by default) has a priority equal to its index. netdev_pick_tx() will think it's okay, for a packet belonging to TC0, to select a TXQ based on hashing between indices 0, 1, 2. But in hardware, the packets will get sent through TXQs with different priorities, based on nothing more than pure chance. That is a disaster, and especially noticeable when the mqprio mapping is applied through taprio, and there is a Qbv schedule on the port. Some packets will be scheduled on the right time slots, and some won't.

So the idea here is that you'll either have to experiment with reprogramming the scheduling algorithm and TXQ priorities on mqprio offload, or refuse offloading anything that doesn't match what the hardware was pre-programmed with (through device tree, likely).

Quoted 14 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +14 -0

+
+	err = netif_set_real_num_tx_queues(ndev, num_stack_tx_queues);
+	if (err)
+		goto err_reset_tc;
+
+	stmmac_fpe_set_preemptible_tcs(priv, priv->ioaddr, mqprio->preemptible_tcs);
+
+	return 0;
+
+err_reset_tc:
+	stmmac_reset_tc_mqprio(ndev);
+
+	return err;
+}

Furong Xu

author

2024-07-31 10:44 UTC #5 lore raw

PATCH 3/5 net: stmmac: support fp parameter of tc-taprio

In reply to Furong Xu

tc-taprio can select whether traffic classes are express or preemptible.

After some traffic tests, MAC merge layer statistics are all good.

Local device:
ethtool --include-statistics --json --show-mm eth1

[ {
        "ifname": "eth1",
        "pmac-enabled": true,
        "tx-enabled": true,
        "tx-active": true,
        "tx-min-frag-size": 60,
        "rx-min-frag-size": 60,
        "verify-enabled": true,
        "verify-time": 100,
        "max-verify-time": 128,
        "verify-status": "SUCCEEDED",
        "statistics": {
            "MACMergeFrameAssErrorCount": 0,
            "MACMergeFrameSmdErrorCount": 0,
            "MACMergeFrameAssOkCount": 0,
            "MACMergeFragCountRx": 0,
            "MACMergeFragCountTx": 1398,
            "MACMergeHoldCount": 15783
        }
    } ]

Remote device:
ethtool --include-statistics --json --show-mm eth1

[ {
        "ifname": "eth1",
        "pmac-enabled": true,
        "tx-enabled": true,
        "tx-active": true,
        "tx-min-frag-size": 60,
        "rx-min-frag-size": 60,
        "verify-enabled": true,
        "verify-time": 100,
        "max-verify-time": 128,
        "verify-status": "SUCCEEDED",
        "statistics": {
            "MACMergeFrameAssErrorCount": 0,
            "MACMergeFrameSmdErrorCount": 0,
            "MACMergeFrameAssOkCount": 1388,
            "MACMergeFragCountRx": 1398,
            "MACMergeFragCountTx": 0,
            "MACMergeHoldCount": 0
        }
    } ]

Tested on DWMAC CORE 5.10a

Signed-off-by Furong Xu <[email protected]>

---
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 34 ++-----------------
 1 file changed, 3 insertions(+), 31 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +3 ~~-31~~

index 494fe2f68300..eeb5eb453b98 100644
@@ -943,7 +943,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	u32 size, wid = priv->dma_cap.estwid, dep = priv->dma_cap.estdep;
 	struct timespec64 time, current_time, qopt_time;
 	ktime_t current_time_ns;
-	bool fpe = false;
 	int i, ret = 0;
 	u64 ctr;

@@ -1028,16 +1027,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 
 		switch (qopt->entries[i].command) {
 		case TC_TAPRIO_CMD_SET_GATES:
-			if (fpe)
-				return -EINVAL;
-			break;
-		case TC_TAPRIO_CMD_SET_AND_HOLD:
-			gates |= BIT(0);
-			fpe = true;
-			break;
-		case TC_TAPRIO_CMD_SET_AND_RELEASE:
-			gates &= ~BIT(0);
-			fpe = true;
 			break;
 		default:
 			return -EOPNOTSUPP;

@@ -1068,16 +1057,11 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 
 	tc_taprio_map_maxsdu_txq(priv, qopt);
 
-	if (fpe && !priv->dma_cap.fpesel) {
+	if (qopt->mqprio.preemptible_tcs && !priv->dma_cap.fpesel) {
 		mutex_unlock(&priv->est_lock);
 		return -EOPNOTSUPP;
 	}
 
-	/* Actual FPE register configuration will be done after FPE handshake
-	 * is success.
-	 */
-	priv->plat->fpe_cfg->enable = fpe;
-
 	ret = stmmac_est_configure(priv, priv, priv->est,
 				   priv->plat->clk_ptp_rate);
 	mutex_unlock(&priv->est_lock);

@@ -1088,10 +1072,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 
 	netdev_info(priv->dev, "configured EST\n");
 
-	if (fpe) {
-		stmmac_fpe_handshake(priv, true);
-		netdev_info(priv->dev, "start FPE handshake\n");
-	}
+	stmmac_fpe_set_preemptible_tcs(priv, priv->ioaddr, qopt->mqprio.preemptible_tcs);
 
 	return 0;

@@ -1109,16 +1090,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 		mutex_unlock(&priv->est_lock);
 	}
 
-	priv->plat->fpe_cfg->enable = false;
-	stmmac_fpe_configure(priv, priv->ioaddr,
-			     priv->plat->fpe_cfg,
-			     priv->plat->tx_queues_to_use,
-			     priv->plat->rx_queues_to_use,
-			     false);
-	netdev_info(priv->dev, "disabled FPE\n");
-
-	stmmac_fpe_handshake(priv, false);
-	netdev_info(priv->dev, "stop FPE handshake\n");
+	stmmac_fpe_set_preemptible_tcs(priv, priv->ioaddr, 0);
 
 	return ret;
 }

Signature · 1 line

2.34.1

Vladimir Oltean

2024-08-01 23:16 UTC #6 lore raw

Re: [PATCH net-next v1 3/5] net: stmmac: support fp parameter of tc-taprio

Quoted 73 lines · Furong Xu

tc-taprio can select whether traffic classes are express or preemptible.

After some traffic tests, MAC merge layer statistics are all good.

Local device:
ethtool --include-statistics --json --show-mm eth1

[ {
        "ifname": "eth1",
        "pmac-enabled": true,
        "tx-enabled": true,
        "tx-active": true,
        "tx-min-frag-size": 60,
        "rx-min-frag-size": 60,
        "verify-enabled": true,
        "verify-time": 100,
        "max-verify-time": 128,
        "verify-status": "SUCCEEDED",
        "statistics": {
            "MACMergeFrameAssErrorCount": 0,
            "MACMergeFrameSmdErrorCount": 0,
            "MACMergeFrameAssOkCount": 0,
            "MACMergeFragCountRx": 0,
            "MACMergeFragCountTx": 1398,
            "MACMergeHoldCount": 15783
        }
    } ]

Remote device:
ethtool --include-statistics --json --show-mm eth1

[ {
        "ifname": "eth1",
        "pmac-enabled": true,
        "tx-enabled": true,
        "tx-active": true,
        "tx-min-frag-size": 60,
        "rx-min-frag-size": 60,
        "verify-enabled": true,
        "verify-time": 100,
        "max-verify-time": 128,
        "verify-status": "SUCCEEDED",
        "statistics": {
            "MACMergeFrameAssErrorCount": 0,
            "MACMergeFrameSmdErrorCount": 0,
            "MACMergeFrameAssOkCount": 1388,
            "MACMergeFragCountRx": 1398,
            "MACMergeFragCountTx": 0,
            "MACMergeHoldCount": 0
        }
    } ]

Tested on DWMAC CORE 5.10a

Signed-off-by Furong Xu <[email protected]>

---
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 34 ++-----------------
 1 file changed, 3 insertions(+), 31 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +0 -1

index 494fe2f68300..eeb5eb453b98 100644
@@ -943,7 +943,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	u32 size, wid = priv->dma_cap.estwid, dep = priv->dma_cap.estdep;
 	struct timespec64 time, current_time, qopt_time;
 	ktime_t current_time_ns;
-	bool fpe = false;
 	int i, ret = 0;
 	u64 ctr;

@@ -1028,16 +1027,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 
 		switch (qopt->entries[i].command) {
 		case TC_TAPRIO_CMD_SET_GATES:

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c static int tc_taprio_configure(struct stmmac_priv *priv, · line 1040

Quoted 10 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +0 ~~-10~~

-			if (fpe)
-				return -EINVAL;
-			break;
-		case TC_TAPRIO_CMD_SET_AND_HOLD:
-			gates |= BIT(0);
-			fpe = true;
-			break;
-		case TC_TAPRIO_CMD_SET_AND_RELEASE:
-			gates &= ~BIT(0);
-			fpe = true;

I don't think these can go.

In the DWMAC5 manual, I see:
"To enable the support of hold and release operations, the format of the
GCL is slightly changed while configuring and enabling the FPE. The first Queue (Q0) is always programmed to carry preemption traffic and therefore it is always Open. The bit corresponding to Q0 is renamed as Release/Hold MAC control. The TX Queues whose packets are preemptable are indicated by setting the PEC field of the MTL_FPE_CTRL_STS register. The GCL bit of the corresponding Queue are ignored and considered as always "Open". So, even if the software writes a "0" ("C"), it is ignored for such queues."

It's relatively clear to me that this is what the "gates" variable is doing here - it's modulating when preemptible traffic begins to be "held", and when it is "released".

Now, the "fpe" variable - that can definitely go.

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c static int tc_taprio_configure(struct stmmac_priv *priv, · line 1032

Quoted 3 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +0 -0
 			break;
 		default:
 			return -EOPNOTSUPP;

Also, this is more general advice. If TC_TAPRIO_CMD_SET_AND_HOLD and TC_TAPRIO_CMD_SET_AND_RELEASE used to work as UAPI input into this driver, you don't want to break that now by letting those fall into the default -EOPNOTSUPP case. What worked must continue to work... somehow.

Quoted 18 lines

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +1 -6

@@ -1068,16 +1057,11 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 
 	tc_taprio_map_maxsdu_txq(priv, qopt);
 
-	if (fpe && !priv->dma_cap.fpesel) {
+	if (qopt->mqprio.preemptible_tcs && !priv->dma_cap.fpesel) {
 		mutex_unlock(&priv->est_lock);
 		return -EOPNOTSUPP;
 	}
 
-	/* Actual FPE register configuration will be done after FPE handshake
-	 * is success.
-	 */
-	priv->plat->fpe_cfg->enable = fpe;
-
 	ret = stmmac_est_configure(priv, priv, priv->est,
 				   priv->plat->clk_ptp_rate);
 	mutex_unlock(&priv->est_lock);

Vladimir Oltean

2024-08-01 23:29 UTC #7 lore raw

Re: [PATCH net-next v1 3/5] net: stmmac: support fp parameter of tc-taprio

In reply to Furong Xu

Quoted 16 lines · Furong Xu

tc-taprio can select whether traffic classes are express or preemptible.
After some traffic tests, MAC merge layer statistics are all good.
Local device:
ethtool --include-statistics --json --show-mm eth1
[ {
        "ifname": "eth1",
        "pmac-enabled": true,
        "tx-enabled": true,
        "tx-active": true,
        "tx-min-frag-size": 60,
        "rx-min-frag-size": 60,
        "verify-enabled": true,
        "verify-time": 100,
        "max-verify-time": 128,

Quoted 8 lines

        "verify-status": "SUCCEEDED",
        "statistics": {
            "MACMergeFrameAssErrorCount": 0,
            "MACMergeFrameSmdErrorCount": 0,
            "MACMergeFrameAssOkCount": 0,
            "MACMergeFragCountRx": 0,
            "MACMergeFragCountTx": 1398,
            "MACMergeHoldCount": 15783

In order for readers to really understand this output (including me), could you also post the associated tc-taprio command, please?

You deleted the code that treated the Set-And-Hold-MAC GCL command - and according to 802.1Q, that is the only source of Hold requests. I _think_ that as a side effect of your reimplementation, every time the gate for TC 0 opens, the HoldCount bumps by one. Would that be a correct description?

The more unfortunate part is that I haven't yet come across a NIC hardware design that would behave completely as you'd expect w.r.t. Hold requests. In the case of DWMAC, I would expect that with a taprio schedule that lacks any Set-And-Hold-MAC command, the HoldCount would stay at zero. I'm not sure, given the way they piggy back onto gate 0 for Hold/Release, that this is possible :(

At least HoldCount stays constant with a tc-mqprio offload, right?

Quoted 2 lines

        }
    } ]

Furong Xu

author

2024-07-31 10:44 UTC #8 lore raw

PATCH 4/5 net: stmmac: drop unneeded FPE handshake code

In reply to Furong Xu

FPE is configured via ethtool-mm, the hardcoded way shall be no more.

Signed-off-by Furong Xu <[email protected]>

---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +-----
 include/linux/stmmac.h                            | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +1 -5

index a5e3316bc410..fba44bd1990a 100644
@@ -3532,13 +3532,9 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
 
 	stmmac_set_hw_vlan_mode(priv, priv->hw);
 
-	if (priv->dma_cap.fpesel) {
+	if (priv->dma_cap.fpesel)
 		stmmac_fpe_start_wq(priv);
 
-		if (priv->plat->fpe_cfg->enable)
-			stmmac_fpe_handshake(priv, true);
-	}
-
 	return 0;
 }

include/linux/stmmac.h +0 -1

index 707a6916e51a..66eb4627bd47 100644
@@ -157,7 +157,6 @@ enum stmmac_fpe_task_state_t {
 };
 
 struct stmmac_fpe_cfg {
-	bool enable;				/* FPE enable */
 	bool hs_enable;				/* FPE handshake enable */
 	enum stmmac_fpe_state lp_fpe_state;	/* Link Partner FPE state */
 	enum stmmac_fpe_state lo_fpe_state;	/* Local station FPE state */

Signature · 1 line

2.34.1

Furong Xu

author

2024-07-31 10:44 UTC #9 lore raw

PATCH 5/5 net: stmmac: silence FPE kernel logs

In reply to Furong Xu

ethtool --show-mm can get real-time state of FPE.
Those kernel logs should keep quiet.

Signed-off-by Furong Xu <[email protected]>

---
 drivers/net/ethernet/stmicro/stmmac/dwmac5.c      | 8 ++++----
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

drivers/net/ethernet/stmicro/stmmac/dwmac5.c +4 -4

index 068859284691..3abacd863fe4 100644
@@ -605,22 +605,22 @@ int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
 
 	if (value & TRSP) {
 		status |= FPE_EVENT_TRSP;
-		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
+		netdev_dbg(dev, "FPE: Respond mPacket is transmitted\n");
 	}
 
 	if (value & TVER) {
 		status |= FPE_EVENT_TVER;
-		netdev_info(dev, "FPE: Verify mPacket is transmitted\n");
+		netdev_dbg(dev, "FPE: Verify mPacket is transmitted\n");
 	}
 
 	if (value & RRSP) {
 		status |= FPE_EVENT_RRSP;
-		netdev_info(dev, "FPE: Respond mPacket is received\n");
+		netdev_dbg(dev, "FPE: Respond mPacket is received\n");
 	}
 
 	if (value & RVER) {
 		status |= FPE_EVENT_RVER;
-		netdev_info(dev, "FPE: Verify mPacket is received\n");
+		netdev_dbg(dev, "FPE: Verify mPacket is received\n");
 	}
 
 	return status;

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +4 -4

index fba44bd1990a..5531c26cba34 100644
@@ -7392,19 +7392,19 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
 		if (*lo_state == FPE_STATE_ENTERING_ON &&
 		    *lp_state == FPE_STATE_ENTERING_ON) {
 
-			netdev_info(priv->dev, "configured FPE\n");
+			netdev_dbg(priv->dev, "configured FPE\n");
 
 			*lo_state = FPE_STATE_ON;
 			*lp_state = FPE_STATE_ON;
-			netdev_info(priv->dev, "!!! BOTH FPE stations ON\n");
+			netdev_dbg(priv->dev, "!!! BOTH FPE stations ON\n");
 			break;
 		}
 
 		if ((*lo_state == FPE_STATE_CAPABLE ||
 		     *lo_state == FPE_STATE_ENTERING_ON) &&
 		     *lp_state != FPE_STATE_ON) {
-			netdev_info(priv->dev, SEND_VERIFY_MPAKCET_FMT,
-				    *lo_state, *lp_state);
+			netdev_dbg(priv->dev, SEND_VERIFY_MPAKCET_FMT,
+				   *lo_state, *lp_state);
 			stmmac_fpe_send_mpacket(priv, priv->ioaddr,
 						fpe_cfg,
 						MPACKET_VERIFY);

Signature · 1 line

2.34.1

Vladimir Oltean

2024-08-01 16:02 UTC #10 lore raw

In reply to Furong Xu

Hi Furong,

Quoted 2 lines · Furong Xu

Move the Frame Preemption(FPE) over to the new standard API which uses ethtool-mm/tc-mqprio/tc-taprio.

Thanks for working on this! I will review it soon.

On the DWMAC 5.10a that you've tested, were other patches also necessary? What is the status of the kselftest? Does it pass? Can you post its output as part of the cover letter?

Vladimir Oltean

2024-08-01 16:17 UTC #11 lore raw

Quoted 11 lines · Vladimir Oltean

Hi Furong,
Quoted 2 lines · Furong Xu
Move the Frame Preemption(FPE) over to the new standard API which uses ethtool-mm/tc-mqprio/tc-taprio.
Thanks for working on this! I will review it soon.
On the DWMAC 5.10a that you've tested, were other patches also necessary? What is the status of the kselftest? Does it pass? Can you post its output as part of the cover letter?

Can you additionally test FPE across a suspend/resume cycle, in 2 cases:
- FPE was enabled before suspend, make sure it runs again automatically after resume, and that there are no deadlock issues (to be confirmed with CONFIG_PROVE_LOCKING)
- FPE was disabled before suspend, make sure it can be enabled successfully after resume

Serge Semin

2024-08-05 17:11 UTC #12 lore raw

In reply to Furong Xu

Hi Furong

Quoted 2 lines · Furong Xu

Move the Frame Preemption(FPE) over to the new standard API which uses ethtool-mm/tc-mqprio/tc-taprio.

Thank you very much for the series. I am not that much aware of the FPE and ethtool MAC Merge guts. But I had a thoughtful glance to the FPE-handshaking algo and got to a realization that all the FPE-related data defined in the include/linux/stmmac.h weren't actually platform-data. All of that are the run-time settings utilized during the handshaking algo execution.

So could you please move the fpe_cfg field to the stmmac_priv data and move the FPE-related declarations from the include/linux/stmmac.h header file to the drivers/net/ethernet/stmicro/stmmac/stmmac.h file? It's better to be done in a pre-requisite (preparation) patch of your series.

Another useful cleanup would be moving the entire FPE-implementation from stmmac_main.c to a separate module. Thus the main driver code would be simplified a bit. I guess it could be moved to the stmmac_tc.c file since FPE is the TC-related feature. Right?

Vladimir, what do you think about the suggestions above?

-Serge(y)

Quoted 23 lines

 .../net/ethernet/stmicro/stmmac/dwmac4_core.c |   6 +
 drivers/net/ethernet/stmicro/stmmac/dwmac5.c  |  37 +++++-
 drivers/net/ethernet/stmicro/stmmac/dwmac5.h  |   7 ++
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  14 +++
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |   3 +
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 111 ++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  25 ++--
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   |  95 ++++++++++-----
 include/linux/stmmac.h                        |   2 +-
 9 files changed, 248 insertions(+), 52 deletions(-)

Signature · 1 line

2.34.1

Furong Xu

author

2024-08-06 04:55 UTC #13 lore raw

Hi Serge

Quoted 9 lines · Serge Semin

Hi Furong
Thank you very much for the series. I am not that much aware of the FPE and ethtool MAC Merge guts. But I had a thoughtful glance to the FPE-handshaking algo and got to a realization that all the FPE-related data defined in the include/linux/stmmac.h weren't actually platform-data. All of that are the run-time settings utilized during the handshaking algo execution.

Quoted 5 lines

So could you please move the fpe_cfg field to the stmmac_priv data and move the FPE-related declarations from the include/linux/stmmac.h header file to the drivers/net/ethernet/stmicro/stmmac/stmmac.h file? It's better to be done in a pre-requisite (preparation) patch of your series.

This will be included in V2 of this patchset.

Quoted 5 lines

Another useful cleanup would be moving the entire FPE-implementation from stmmac_main.c to a separate module. Thus the main driver code would be simplified a bit. I guess it could be moved to the stmmac_tc.c file since FPE is the TC-related feature. Right?

Thanks for your advice.

A few weeks ago, I sent a patchset to refactor FPE implementation:
https://lore.kernel.org/all/[email protected]/

Vladimir suggested me to move the FPE over to the new standard API, then this patchset comes.

I am working on V2 of this patchset, once this patchset get merged, a new FPE implementation will be sent to review.

Serge Semin

2024-08-06 09:16 UTC #14 lore raw

Quoted 27 lines · Furong Xu

Hi Serge
Quoted 9 lines · Serge Semin
Hi Furong
Thank you very much for the series. I am not that much aware of the FPE and ethtool MAC Merge guts. But I had a thoughtful glance to the FPE-handshaking algo and got to a realization that all the FPE-related data defined in the include/linux/stmmac.h weren't actually platform-data. All of that are the run-time settings utilized during the handshaking algo execution.
Quoted 5 lines
So could you please move the fpe_cfg field to the stmmac_priv data and move the FPE-related declarations from the include/linux/stmmac.h header file to the drivers/net/ethernet/stmicro/stmmac/stmmac.h file? It's better to be done in a pre-requisite (preparation) patch of your series.
This will be included in V2 of this patchset.
Quoted 5 lines
Another useful cleanup would be moving the entire FPE-implementation from stmmac_main.c to a separate module. Thus the main driver code would be simplified a bit. I guess it could be moved to the stmmac_tc.c file since FPE is the TC-related feature. Right?
Thanks for your advice.

Quoted 8 lines

A few weeks ago, I sent a patchset to refactor FPE implementation:
https://lore.kernel.org/all/[email protected]/
Vladimir suggested me to move the FPE over to the new standard API, then this patchset comes.
I am working on V2 of this patchset, once this patchset get merged, a new FPE implementation will be sent to review.

If the new FPE-implementation includes the FPE-hanshaking stuff moved out from the stmmac_main.c it will be just wonderful. Thanks!

-Serge(y)