[PATCH net-next v1 0/5] net: stmmac: FPE via ethtool + tc

[PATCH net-next v1 0/5] net: stmmac: FPE via ethtool + tc

[Furong Xu]

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(-)

-- 
2.34.1

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

[Furong Xu]

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 <0x1207@gmail.com>
---
 .../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(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index f98741d2607e..af871cce767e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -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,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
index e02cebc3f1b7..5d132bada3fe 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
@@ -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);
+}
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
index bf33a51d229e..6b1c9d2da308 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
@@ -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__ */
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index e53c32362774..f868ffc3e64f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -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 {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index b23b920eedb1..5228493bc68c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -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;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 7008219fd88d..ca85e8694285 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -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
@@ -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;
+	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;
+
+	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)
+		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.
+	 *
+	 * 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;
+}
+
+static void stmmac_get_mm_stats(struct net_device *ndev,
+				struct ethtool_mm_stats *s)
+{
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	struct stmmac_counters *mmc = &priv->mmc;
+
+	if (!priv->dma_cap.rmon)
+		return;
+
+	stmmac_mmc_read(priv, priv->mmcaddr, mmc);
+
+	s->MACMergeFrameAssErrorCount = mmc->mmc_rx_packet_assembly_err_cntr;
+	s->MACMergeFrameSmdErrorCount = mmc->mmc_rx_packet_smd_err_cntr;
+	s->MACMergeFrameAssOkCount = mmc->mmc_rx_packet_assembly_ok_cntr;
+	s->MACMergeFragCountRx = mmc->mmc_rx_fpe_fragment_cntr;
+	s->MACMergeFragCountTx = mmc->mmc_tx_fpe_fragment_cntr;
+	s->MACMergeHoldCount = mmc->mmc_tx_hold_req_cntr;
+}
+
 static const struct ethtool_ops stmmac_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES,
@@ -1309,6 +1417,9 @@ static const struct ethtool_ops stmmac_ethtool_ops = {
 	.set_tunable = stmmac_set_tunable,
 	.get_link_ksettings = stmmac_ethtool_get_link_ksettings,
 	.set_link_ksettings = stmmac_ethtool_set_link_ksettings,
+	.get_mm = stmmac_get_mm,
+	.set_mm = stmmac_set_mm,
+	.get_mm_stats = stmmac_get_mm_stats,
 };
 
 void stmmac_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 12689774d755..9b1cf81c50ea 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -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
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 84e13bd5df28..707a6916e51a 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -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 {
-- 
2.34.1

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

[Vladimir Oltean]

On Wed, Jul 31, 2024 at 06:43:12PM +0800, Furong Xu wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index b23b920eedb1..5228493bc68c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -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.

> +
>  	/* TC Handling */
>  	unsigned int tc_entries_max;
>  	unsigned int tc_off_max;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 7008219fd88d..ca85e8694285 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -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
> @@ -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.

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

> +
> +	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)
> +		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/

> +	 *
> +	 * 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;
> +}
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 12689774d755..9b1cf81c50ea 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -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);

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

>  	}
>  
>  	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

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

[Furong Xu]

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

Tested on DWMAC CORE 5.10a

Signed-off-by: Furong Xu <0x1207@gmail.com>
---
 .../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(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index af871cce767e..68cdfd9e7c8c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -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,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
index 5d132bada3fe..068859284691 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
@@ -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);
+}
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
index 6b1c9d2da308..bce7e88418ac 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
@@ -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__ */
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index f868ffc3e64f..109699d3df0a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -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;
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9b1cf81c50ea..a5e3316bc410 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -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,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 996f2bcd07a2..494fe2f68300 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -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;
+
+	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;
+	}
+
+	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;
+}
+
 const struct stmmac_tc_ops dwmac510_tc_ops = {
 	.init = tc_init,
 	.setup_cls_u32 = tc_setup_cls_u32,
@@ -1222,4 +1282,5 @@ const struct stmmac_tc_ops dwmac510_tc_ops = {
 	.setup_taprio = tc_setup_taprio,
 	.setup_etf = tc_setup_etf,
 	.query_caps = tc_query_caps,
+	.setup_mqprio = tc_setup_mqprio,
 };
-- 
2.34.1

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

[Vladimir Oltean]

On Wed, Jul 31, 2024 at 06:43:13PM +0800, Furong Xu wrote:
> tc-mqprio can select whether traffic classes are express or preemptible.
> 
> Tested on DWMAC CORE 5.10a
> 
> Signed-off-by: Furong Xu <0x1207@gmail.com>
> ---
>  .../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(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> index 5d132bada3fe..068859284691 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> @@ -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);
> +}

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.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9b1cf81c50ea..a5e3316bc410 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -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,
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 996f2bcd07a2..494fe2f68300 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -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;
> +
> +	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).

> +
> +	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;
> +}

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

[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 <0x1207@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 34 ++-----------------
 1 file changed, 3 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 494fe2f68300..eeb5eb453b98 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -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;
 }
-- 
2.34.1

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

[Vladimir Oltean]

On Wed, Jul 31, 2024 at 06:43:14PM +0800, Furong Xu wrote:
> 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 <0x1207@gmail.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 34 ++-----------------
>  1 file changed, 3 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 494fe2f68300..eeb5eb453b98 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -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;

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.

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

> @@ -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);

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

[Vladimir Oltean]

On Wed, Jul 31, 2024 at 06:43:14PM +0800, Furong Xu wrote:
> 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

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?

>         }
>     } ]

[PATCH net-next v1 4/5] net: stmmac: drop unneeded FPE handshake code

[Furong Xu]

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

Signed-off-by: Furong Xu <0x1207@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +-----
 include/linux/stmmac.h                            | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a5e3316bc410..fba44bd1990a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -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;
 }
 
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 707a6916e51a..66eb4627bd47 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -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 */
-- 
2.34.1

[PATCH net-next v1 5/5] net: stmmac: silence FPE kernel logs

[Furong Xu]

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

Signed-off-by: Furong Xu <0x1207@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac5.c      | 8 ++++----
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
index 068859284691..3abacd863fe4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
@@ -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;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fba44bd1990a..5531c26cba34 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -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);
-- 
2.34.1

Re: [PATCH net-next v1 0/5] net: stmmac: FPE via ethtool + tc

[Vladimir Oltean]

Hi Furong,

On Wed, Jul 31, 2024 at 06:43:11PM +0800, Furong Xu wrote:
> 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?

Re: [PATCH net-next v1 0/5] net: stmmac: FPE via ethtool + tc

[Vladimir Oltean]

On Thu, Aug 01, 2024 at 07:02:24PM +0300, Vladimir Oltean wrote:
> Hi Furong,
> 
> On Wed, Jul 31, 2024 at 06:43:11PM +0800, Furong Xu wrote:
> > 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

Re: [PATCH net-next v1 0/5] net: stmmac: FPE via ethtool + tc

[Serge Semin]

Hi Furong

On Wed, Jul 31, 2024 at 06:43:11PM +0800, Furong Xu wrote:
> 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)

> 
> 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(-)
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH net-next v1 0/5] net: stmmac: FPE via ethtool + tc

[Furong Xu]

Hi Serge

On Mon, 5 Aug 2024 20:11:10 +0300, Serge Semin <fancer.lancer@gmail.com> wrote:
> 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.
> 
> 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.

> 
> 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/cover.1720512888.git.0x1207@gmail.com/

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.

Re: [PATCH net-next v1 0/5] net: stmmac: FPE via ethtool + tc

[Serge Semin]

On Tue, Aug 06, 2024 at 12:55:24PM +0800, Furong Xu wrote:
> Hi Serge
> 
> On Mon, 5 Aug 2024 20:11:10 +0300, Serge Semin <fancer.lancer@gmail.com> wrote:
> > 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.
> > 
> > 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.
> 
> > 
> > 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/cover.1720512888.git.0x1207@gmail.com/
> 
> 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)