[PATCH] net/i40e: support FEC feature

[PATCH] net/i40e: support FEC feature

[Qiming Yang]

This patch enabled FEC set and get functions.

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 183 +++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 3ca226156b..1eb49176d1 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -405,6 +405,11 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
+			   unsigned int num);
+static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
+
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -519,6 +524,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.tm_ops_get                   = i40e_tm_ops_get,
 	.tx_done_cleanup              = i40e_tx_done_cleanup,
 	.get_monitor_addr             = i40e_get_monitor_addr,
+	.fec_get_capability           = i40e_fec_get_capability,
+	.fec_get                      = i40e_fec_get,
+	.fec_set                      = i40e_fec_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -12159,6 +12167,181 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
+			   unsigned int num)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	num = 0;
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return 0;
+	}
+
+	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
+	    hw->device_id == I40E_DEV_ID_25G_B) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		num++;
+	}
+
+	if (hw->device_id == I40E_DEV_ID_KX_X722) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		num++;
+	}
+
+	return num;
+}
+
+static int
+i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities;
+	u32 temp_fec_capa = 0;
+	u8 fec_cfg;
+	int ret = 0;
+
+	/* Get the current phy config */
+	memset(&abilities, 0, sizeof(abilities));
+	ret = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+						  NULL);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	fec_cfg = abilities.fec_cfg_curr_mod_ext_info;
+	if (fec_cfg & I40E_AQ_SET_FEC_AUTO)
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+
+	if (fec_cfg & I40E_AQ_SET_FEC_REQUEST_KR ||
+		fec_cfg & I40E_AQ_SET_FEC_ABILITY_KR)
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+	if (fec_cfg & I40E_AQ_SET_FEC_REQUEST_RS ||
+		fec_cfg & I40E_AQ_SET_FEC_ABILITY_RS)
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+
+	if (fec_cfg == 0)
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+
+	*fec_capa = temp_fec_capa;
+
+	return 0;
+}
+
+static int
+i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities;
+	struct i40e_aq_set_phy_config config = { 0 };
+	enum i40e_status_code status;
+	u8 req_fec;
+	int ret = 0;
+
+	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
+	    hw->device_id != I40E_DEV_ID_25G_B &&
+	    hw->device_id != I40E_DEV_ID_KX_X722) {
+		ret = -ENOTSUP;
+		goto done;
+	}
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	/* Copy the current user PHY configuration. The current user PHY
+	 * configuration is initialized during probe from PHY capabilities
+	 * software mode, and updated on set PHY configuration.
+	 */
+	switch (fec_capa) {
+	case RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC):
+		req_fec = 0;
+		break;
+	case RTE_ETH_FEC_MODE_CAPA_MASK(AUTO):
+		if (hw->mac.type == I40E_MAC_X722) {
+			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
+			ret = -EINVAL;
+			goto done;
+		} else {
+			req_fec = I40E_AQ_SET_FEC_AUTO;
+		}
+		break;
+	case RTE_ETH_FEC_MODE_CAPA_MASK(BASER):
+		req_fec = (I40E_AQ_SET_FEC_REQUEST_KR |
+			     I40E_AQ_SET_FEC_ABILITY_KR);
+		break;
+	case RTE_ETH_FEC_MODE_CAPA_MASK(RS):
+		if (hw->mac.type == I40E_MAC_X722) {
+			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+			ret = -EINVAL;
+			goto done;
+		} else {
+			req_fec = (I40E_AQ_SET_FEC_REQUEST_RS |
+				   I40E_AQ_SET_FEC_ABILITY_RS);
+		}
+		break;
+	default:
+		PMD_DRV_LOG(ERR, "Unsupported FEC mode: %d\n", fec_capa);
+		return -EINVAL;
+	}
+
+	/* Get the current phy config */
+	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+					      NULL);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				status);
+		ret = -ENOTSUP;
+		goto done;
+	}
+
+	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
+		config.phy_type = abilities.phy_type;
+		config.abilities = abilities.abilities |
+				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
+		config.phy_type_ext = abilities.phy_type_ext;
+		config.link_speed = abilities.link_speed;
+		config.eee_capability = abilities.eee_capability;
+		config.eeer = abilities.eeer_val;
+		config.low_power_ctrl = abilities.d3_lpan;
+		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
+		status = i40e_aq_set_phy_config(hw, &config, NULL);
+		if (status) {
+			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+			ret = -ENOTSUP;
+			goto done;
+		}
+	}
+
+	status = i40e_update_link_info(hw);
+	if (status)
+		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+
+done:
+	return ret;
+}
+
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
 #ifdef RTE_ETHDEV_DEBUG_RX
-- 
2.25.1

[PATCH v4] net/i40e: support FEC feature

[Zhichao Zeng]

This patch enabled querying Forward Error Correction(FEC) capabilities,
set FEC mode and get current FEC mode functions.

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

---
v4: fix some logic
v3: optimize code details
v2: update NIC feature document
---
 doc/guides/nics/features/i40e.ini      |   1 +
 doc/guides/rel_notes/release_24_03.rst |   5 +
 drivers/net/i40e/i40e_ethdev.c         | 192 +++++++++++++++++++++++++
 3 files changed, 198 insertions(+)

diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
index e241dad047..aac2c1a6a1 100644
--- a/doc/guides/nics/features/i40e.ini
+++ b/doc/guides/nics/features/i40e.ini
@@ -30,6 +30,7 @@ Flow control         = Y
 CRC offload          = Y
 VLAN offload         = Y
 QinQ offload         = P
+FEC                  = Y
 L3 checksum offload  = P
 L4 checksum offload  = P
 Inner L3 checksum    = P
diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
index 161f77112b..862a5f8fb8 100644
--- a/doc/guides/rel_notes/release_24_03.rst
+++ b/doc/guides/rel_notes/release_24_03.rst
@@ -110,6 +110,11 @@ New Features
 
   * Added support for 5760X device family.
 
+* **Updated Intel i40e driver.**
+
+  * Added support for configuring the Forward Error Correction(FEC) mode, querying
+  * FEC capabilities and current FEC mode from a device.
+
 * **Updated Marvell cnxk net driver.**
 
   * Added support for port representors.
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 380ce1a720..2bc6675a04 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
+static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.tm_ops_get                   = i40e_tm_ops_get,
 	.tx_done_cleanup              = i40e_tx_done_cleanup,
 	.get_monitor_addr             = i40e_get_monitor_addr,
+	.fec_get_capability           = i40e_fec_get_capability,
+	.fec_get                      = i40e_fec_get,
+	.fec_set                      = i40e_fec_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -12297,6 +12304,191 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
+	    hw->device_id == I40E_DEV_ID_25G_B) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+
+		/* since HW only supports 25G */
+		return 1;
+	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		return 1;
+	}
+
+	return -ENOTSUP;
+}
+
+static int
+i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_link_status link_status = {0};
+	uint8_t configured_fec_cfg = 0, current_fec_cfg;
+	uint32_t temp_fec_capa = 0;
+	bool link_up, enable_lse;
+	int ret = 0;
+
+	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+	/* Get FEC info */
+	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
+	if (ret != I40E_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get link information");
+		return -ENOTSUP;
+	}
+
+	link_up = link_status.link_info & I40E_AQ_LINK_UP;
+
+	/**
+	 * If link is down and AUTO is enabled, AUTO is returned,
+	 * otherwise, configured FEC mode is returned.
+	 * If link is up, current FEC mode is returned.
+	 */
+	configured_fec_cfg = link_status.req_fec_info;
+	current_fec_cfg = link_status.fec_info;
+
+	if (!link_up) {
+		if (current_fec_cfg & (I40E_AQ_ENABLE_FEC_KR | I40E_AQ_ENABLE_FEC_RS)) {
+			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+		} else {
+			if (configured_fec_cfg == (I40E_AQ_REQUEST_FEC_KR | I40E_AQ_REQUEST_FEC_RS))
+				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+			else if (configured_fec_cfg & I40E_AQ_REQUEST_FEC_KR)
+				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+			else if (configured_fec_cfg & I40E_AQ_REQUEST_FEC_RS)
+				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+			else
+				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+		}
+	} else {
+		if (current_fec_cfg & (I40E_AQ_ENABLE_FEC_KR | I40E_AQ_ENABLE_FEC_RS))
+			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+		else if (current_fec_cfg & I40E_AQ_ENABLE_FEC_KR)
+			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+		else if (current_fec_cfg & I40E_AQ_ENABLE_FEC_RS)
+			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		else
+			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+	}
+
+	*fec_capa = temp_fec_capa;
+
+	return 0;
+}
+
+static int
+i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_aq_set_phy_config config = {0};
+	enum i40e_status_code status;
+	uint8_t req_fec = 0;
+
+	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
+	    hw->device_id != I40E_DEV_ID_25G_B &&
+	    hw->device_id != I40E_DEV_ID_KX_X722) {
+		return -ENOTSUP;
+	}
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	/**
+	 * Copy the current user PHY configuration. The current user PHY
+	 * configuration is initialized during probe from PHY capabilities
+	 * software mode, and updated on set PHY configuration.
+	 */
+	if (fec_capa != 0)
+		return -EINVAL;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)) {
+		if (hw->mac.type == I40E_MAC_X722) {
+			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
+			return -EINVAL;
+		}
+		req_fec = I40E_AQ_SET_FEC_AUTO;
+		goto set_fec;
+	}
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC))
+		req_fec = 0;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
+		req_fec |= I40E_AQ_SET_FEC_REQUEST_KR;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS)) {
+		if (hw->mac.type == I40E_MAC_X722) {
+			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+			return -EINVAL;
+		}
+		req_fec |= I40E_AQ_SET_FEC_REQUEST_RS;
+	}
+
+set_fec:
+	/* Get the current phy config */
+	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+					      NULL);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				status);
+		return -ENOTSUP;
+	}
+
+	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
+		config.phy_type = abilities.phy_type;
+		config.abilities = abilities.abilities |
+				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
+		config.phy_type_ext = abilities.phy_type_ext;
+		config.link_speed = abilities.link_speed;
+		config.eee_capability = abilities.eee_capability;
+		config.eeer = abilities.eeer_val;
+		config.low_power_ctrl = abilities.d3_lpan;
+		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
+		status = i40e_aq_set_phy_config(hw, &config, NULL);
+		if (status) {
+			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+			return -ENOTSUP;
+		}
+	}
+
+	status = i40e_update_link_info(hw);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+		return -EAGAIN;
+	}
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
 #ifdef RTE_ETHDEV_DEBUG_RX
-- 
2.34.1

Re: [PATCH v4] net/i40e: support FEC feature

[Medvedkin, Vladimir]

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

Hi Zhicha,

It would be good to reflect FEC feature here:

https://doc.dpdk.org/guides/nics/overview.html

in "/Table 1.1 //Features availability in networking drivers/"

please find the rest comments inline

On 06/03/2024 10:41, Zhichao Zeng wrote:
> This patch enabled querying Forward Error Correction(FEC) capabilities,
> set FEC mode and get current FEC mode functions.
>
> Signed-off-by: Qiming Yang<qiming.yang@intel.com>
> Signed-off-by: Zhichao Zeng<zhichaox.zeng@intel.com>
>
> ---
> v4: fix some logic
> v3: optimize code details
> v2: update NIC feature document
> ---
>   doc/guides/nics/features/i40e.ini      |   1 +
>   doc/guides/rel_notes/release_24_03.rst |   5 +
>   drivers/net/i40e/i40e_ethdev.c         | 192 +++++++++++++++++++++++++
>   3 files changed, 198 insertions(+)
>
> diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
> index e241dad047..aac2c1a6a1 100644
> --- a/doc/guides/nics/features/i40e.ini
> +++ b/doc/guides/nics/features/i40e.ini
> @@ -30,6 +30,7 @@ Flow control         = Y
>   CRC offload          = Y
>   VLAN offload         = Y
>   QinQ offload         = P
> +FEC                  = Y
>   L3 checksum offload  = P
>   L4 checksum offload  = P
>   Inner L3 checksum    = P
> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
> index 161f77112b..862a5f8fb8 100644
> --- a/doc/guides/rel_notes/release_24_03.rst
> +++ b/doc/guides/rel_notes/release_24_03.rst
> @@ -110,6 +110,11 @@ New Features
>   
>     * Added support for 5760X device family.
>   
> +* **Updated Intel i40e driver.**
> +
> +  * Added support for configuring the Forward Error Correction(FEC) mode, querying
> +  * FEC capabilities and current FEC mode from a device.
> +
>   * **Updated Marvell cnxk net driver.**
>   
>     * Added support for port representors.
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 380ce1a720..2bc6675a04 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
>   static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
>   static void i40e_filter_restore(struct i40e_pf *pf);
>   static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
> +static int i40e_fec_get_capability(struct rte_eth_dev *dev,
> +	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
> +static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
> +static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
>   
>   static const char *const valid_keys[] = {
>   	ETH_I40E_FLOATING_VEB_ARG,
> @@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
>   	.tm_ops_get                   = i40e_tm_ops_get,
>   	.tx_done_cleanup              = i40e_tx_done_cleanup,
>   	.get_monitor_addr             = i40e_get_monitor_addr,
> +	.fec_get_capability           = i40e_fec_get_capability,
> +	.fec_get                      = i40e_fec_get,
> +	.fec_set                      = i40e_fec_set,
>   };
>   
>   /* store statistics names and its offset in stats structure */
> @@ -12297,6 +12304,191 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
>   	return ret;
>   }
>   
> +static int
> +i40e_fec_get_capability(struct rte_eth_dev *dev,
> +	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +	if (hw->mac.type == I40E_MAC_X722 &&
> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
> +			 " firmware. Please update the NVM image.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
> +	    hw->device_id == I40E_DEV_ID_25G_B) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +
> +		/* since HW only supports 25G */
> +		return 1;
> +	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +		return 1;
> +	}
> +
> +	return -ENOTSUP;
> +}
> +
> +static int
> +i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct i40e_link_status link_status = {0};
> +	uint8_t configured_fec_cfg = 0, current_fec_cfg;
> +	uint32_t temp_fec_capa = 0;
> +	bool link_up, enable_lse;
> +	int ret = 0;
> +
> +	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
> +	/* Get FEC info */
> +	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
> +	if (ret != I40E_SUCCESS) {
> +		PMD_DRV_LOG(ERR, "Failed to get link information");
> +		return -ENOTSUP;
> +	}
> +
> +	link_up = link_status.link_info & I40E_AQ_LINK_UP;
> +
> +	/**
> +	 * If link is down and AUTO is enabled, AUTO is returned,
> +	 * otherwise, configured FEC mode is returned.
> +	 * If link is up, current FEC mode is returned.
> +	 */
> +	configured_fec_cfg = link_status.req_fec_info;

here we need to better understand the difference between [FC,RS]-FEC 
ability bit (aka I40E_AQ_ENABLE_FEC_[KR,RS]) from [FC,RS]-FEC Request 
bit (aka I40E_AQ_REQUEST_FEC_[KR,RS]), since link_status.req_fec_info 
has only "Request" bits for each FEC algo (see i40e_update_link_info()).

 From what I found on the internet, it seems that we don't need to use 
them at all, because, for example for FC-FEC (aka Clause 74), from:

https://www.ieee802.org/3/25GSG/public/Nov14/baden_25GE_02_1114.pdf

Key phrase: "If both LPs advertise the FEC Ability, and EITHER LP 
requests the FEC, it is enabled."

So I'd suggest not to use these "Request" bits here.
For configured fec (case when link is DOWN) please use struct 
i40e_aq_get_phy_abilities_resp abilities.

> +	current_fec_cfg = link_status.fec_info;
> +
> +	if (!link_up) {
This section should be rewritten according to bit flags from 
abilities.fec_cfg_curr_mod_ext_info.
> +		if (current_fec_cfg & (I40E_AQ_ENABLE_FEC_KR | I40E_AQ_ENABLE_FEC_RS)) {
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> +		} else {
> +			if (configured_fec_cfg == (I40E_AQ_REQUEST_FEC_KR | I40E_AQ_REQUEST_FEC_RS))
> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> +			else if (configured_fec_cfg & I40E_AQ_REQUEST_FEC_KR)
> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +			else if (configured_fec_cfg & I40E_AQ_REQUEST_FEC_RS)
> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +			else
> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> +		}
> +	} else {
> +		if (current_fec_cfg & (I40E_AQ_ENABLE_FEC_KR | I40E_AQ_ENABLE_FEC_RS))
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
In case when FEC was successfully negotiated current_fec_cfg is 
containing only single bit (since 2 algos can not be enabled at the same 
time), so this part is meaningless. Also here and below in this else 
section (i.e. in section if link is UP), for consistency please use 
corresponding macros defined for struct i40e_aqc_get_link_status - 
I40E_AQ_CONFIG_FEC_KR_ENA and I40E_AQ_CONFIG_FEC_RS_ENA instead of 
I40E_AQ_ENABLE_FEC_ .
> +		else if (current_fec_cfg & I40E_AQ_ENABLE_FEC_KR)
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +		else if (current_fec_cfg & I40E_AQ_ENABLE_FEC_RS)
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		else
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> +	}
> +
> +	*fec_capa = temp_fec_capa;
> +
> +	return 0;
> +}
> +
> +static int
> +i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct i40e_aq_get_phy_abilities_resp abilities = {0};
> +	struct i40e_aq_set_phy_config config = {0};
> +	enum i40e_status_code status;
> +	uint8_t req_fec = 0;
> +
> +	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
> +	    hw->device_id != I40E_DEV_ID_25G_B &&
> +	    hw->device_id != I40E_DEV_ID_KX_X722) {
> +		return -ENOTSUP;
> +	}
> +
> +	if (hw->mac.type == I40E_MAC_X722 &&
> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
> +			 " firmware. Please update the NVM image.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	/**
> +	 * Copy the current user PHY configuration. The current user PHY
> +	 * configuration is initialized during probe from PHY capabilities
> +	 * software mode, and updated on set PHY configuration.
> +	 */
> +	if (fec_capa != 0)
> +		return -EINVAL;
Did you mean if (fec_capa == 0)?
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)) {
> +		if (hw->mac.type == I40E_MAC_X722) {
> +			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
> +			return -EINVAL;
> +		}
> +		req_fec = I40E_AQ_SET_FEC_AUTO;
> +		goto set_fec;
> +	}
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC))
> +		req_fec = 0;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
> +		req_fec |= I40E_AQ_SET_FEC_REQUEST_KR;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS)) {
> +		if (hw->mac.type == I40E_MAC_X722) {
> +			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
> +			return -EINVAL;
> +		}
> +		req_fec |= I40E_AQ_SET_FEC_REQUEST_RS;
> +	}

This part is not looking correct to me:

- only I40E_AQ_SET_FEC_AUTO bit is set w/o enabling specific protocols

- "if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC))" makes no sense if 
other bitfields are set

- only I40E_AQ_SET_FEC_REQUEST_* is set without I40E_AQ_SET_FEC_ABILITY_*

  According to API : "If only the AUTO bit is set, the decision on which 
FEC mode to use will be made by HW/FW or driver. If the AUTO bit is set 
with some FEC modes, only specified FEC modes can be set. If AUTO bit is 
clear, specify FEC mode to be used (only one valid mode per speed may be 
set)." I'd suggest:

- In case when AUTO bit is set we need to check for other bit flags. In 
case of absence let's decide to  add all supported by mac type 
algorithms (i.e. set corresponding _enable_ and _request_ bit fields for 
each supported algo).

- In case if there are more bits apart from AUTO - check for their 
validity and support and specify corresponding _enable_ and _request_ flags.

- In case if AUTO is not set check that only single mode was specified, 
check its validity + support and so on.

> +
> +set_fec:
> +	/* Get the current phy config */
> +	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
> +					      NULL);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
> +				status);
> +		return -ENOTSUP;
> +	}
> +
> +	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
> +		config.phy_type = abilities.phy_type;
> +		config.abilities = abilities.abilities |
> +				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> +		config.phy_type_ext = abilities.phy_type_ext;
> +		config.link_speed = abilities.link_speed;
> +		config.eee_capability = abilities.eee_capability;
> +		config.eeer = abilities.eeer_val;
> +		config.low_power_ctrl = abilities.d3_lpan;
> +		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
> +		status = i40e_aq_set_phy_config(hw, &config, NULL);
> +		if (status) {
> +			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
> +			status);
> +			return -ENOTSUP;
> +		}
> +	}
> +
> +	status = i40e_update_link_info(hw);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
> +			status);
> +		return -EAGAIN;
This is a new return status of this API. It needs to be added in doxygen 
documentation for this function and be reflected in release notes
> +	}
> +
> +	return 0;
> +}
> +
>   RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
>   RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
>   #ifdef RTE_ETHDEV_DEBUG_RX

-- 
Regards,
Vladimir

[-- Attachment #2: Type: text/html, Size: 85889 bytes --]

RE: [PATCH v4] net/i40e: support FEC feature

[Zeng, ZhichaoX]

Hi Vladimir:

Thanks for your comments, some responses and questions are inline.

> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com> 
> Sent: Monday, March 11, 2024 11:59 PM
> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>; dev@dpdk.org
> Cc: Cui, KaixinX <kaixinx.cui@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> Subject: Re: [PATCH v4] net/i40e: support FEC feature
> 
> Hi Zhicha,
> It would be good to reflect FEC feature here:
> https://doc.dpdk.org/guides/nics/overview.html 
> in " Table 1.1 Features availability in networking drivers "

After modifying "doc/guides/nics/features/i40e.ini", the corresponding "overview.html" will be generated after compiling the document.

> please find the rest comments inline
> On 06/03/2024 10:41, Zhichao Zeng wrote:
> This patch enabled querying Forward Error Correction(FEC) capabilities,
> set FEC mode and get current FEC mode functions.
> 
> Signed-off-by: Qiming Yang mailto:qiming.yang@intel.com
> Signed-off-by: Zhichao Zeng mailto:zhichaox.zeng@intel.com
> 
> ---
> v4: fix some logic
> v3: optimize code details
> v2: update NIC feature document
> ---
>  doc/guides/nics/features/i40e.ini      |   1 +
>  doc/guides/rel_notes/release_24_03.rst |   5 +
>  drivers/net/i40e/i40e_ethdev.c         | 192 +++++++++++++++++++++++++
>  3 files changed, 198 insertions(+)
> 
> diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
> index e241dad047..aac2c1a6a1 100644
> --- a/doc/guides/nics/features/i40e.ini
> +++ b/doc/guides/nics/features/i40e.ini
> @@ -30,6 +30,7 @@ Flow control         = Y
>  CRC offload          = Y
>  VLAN offload         = Y
>  QinQ offload         = P
> +FEC                  = Y
>  L3 checksum offload  = P
>  L4 checksum offload  = P
>  Inner L3 checksum    = P
> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
> index 161f77112b..862a5f8fb8 100644
> --- a/doc/guides/rel_notes/release_24_03.rst
> +++ b/doc/guides/rel_notes/release_24_03.rst
> @@ -110,6 +110,11 @@ New Features
>  
>    * Added support for 5760X device family.
>  
> +* **Updated Intel i40e driver.**
> +
> +  * Added support for configuring the Forward Error Correction(FEC) mode, querying
> +  * FEC capabilities and current FEC mode from a device.
> +
>  * **Updated Marvell cnxk net driver.**
>  
>    * Added support for port representors.
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 380ce1a720..2bc6675a04 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
>  static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
>  static void i40e_filter_restore(struct i40e_pf *pf);
>  static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
> +static int i40e_fec_get_capability(struct rte_eth_dev *dev,
> +	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
> +static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
> +static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
>  
>  static const char *const valid_keys[] = {
>  	ETH_I40E_FLOATING_VEB_ARG,
> @@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
>  	.tm_ops_get                   = i40e_tm_ops_get,
>  	.tx_done_cleanup              = i40e_tx_done_cleanup,
>  	.get_monitor_addr             = i40e_get_monitor_addr,
> +	.fec_get_capability           = i40e_fec_get_capability,
> +	.fec_get                      = i40e_fec_get,
> +	.fec_set                      = i40e_fec_set,
>  };
>  
>  /* store statistics names and its offset in stats structure */
> @@ -12297,6 +12304,191 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
>  	return ret;
>  }
>  
> +static int
> +i40e_fec_get_capability(struct rte_eth_dev *dev,
> +	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +	if (hw->mac.type == I40E_MAC_X722 &&
> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
> +			 " firmware. Please update the NVM image.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
> +	    hw->device_id == I40E_DEV_ID_25G_B) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +
> +		/* since HW only supports 25G */
> +		return 1;
> +	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +		return 1;
> +	}
> +
> +	return -ENOTSUP;
> +}
> +
> +static int
> +i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct i40e_link_status link_status = {0};
> +	uint8_t configured_fec_cfg = 0, current_fec_cfg;
> +	uint32_t temp_fec_capa = 0;
> +	bool link_up, enable_lse;
> +	int ret = 0;
> +
> +	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
> +	/* Get FEC info */
> +	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
> +	if (ret != I40E_SUCCESS) {
> +		PMD_DRV_LOG(ERR, "Failed to get link information");
> +		return -ENOTSUP;
> +	}
> +
> +	link_up = link_status.link_info & I40E_AQ_LINK_UP;
> +
> +	/**
> +	 * If link is down and AUTO is enabled, AUTO is returned,
> +	 * otherwise, configured FEC mode is returned.
> +	 * If link is up, current FEC mode is returned.
> +	 */
> +	configured_fec_cfg = link_status.req_fec_info;
> here we need to better understand the difference between [FC,RS]-FEC ability bit (aka I40E_AQ_ENABLE_FEC_[KR,RS]) from [FC,RS]-FEC Request bit (aka I40E_AQ_REQUEST_FEC_[KR,RS]), since link_status.req_fec_info has only "Request" bits for each FEC algo (see i40e_update_link_info()).
> From what I found on the internet, it seems that we don't need to use them at all, because, for example for FC-FEC (aka Clause 74), from:
> https://www.ieee802.org/3/25GSG/public/Nov14/baden_25GE_02_1114.pdf
> Key phrase: "If both LPs advertise the FEC Ability, and EITHER LP requests the FEC, it is enabled."
> So I'd suggest not to use these "Request" bits here.
> For configured fec (case when link is DOWN) please use struct i40e_aq_get_phy_abilities_resp abilities.

Will fix in next version.

> 
> +	current_fec_cfg = link_status.fec_info;
> +
> +	if (!link_up) {
> This section should be rewritten according to bit flags from abilities.fec_cfg_curr_mod_ext_info. 
> +		if (current_fec_cfg & (I40E_AQ_ENABLE_FEC_KR | I40E_AQ_ENABLE_FEC_RS)) {
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> +		} else {
> +			if (configured_fec_cfg == (I40E_AQ_REQUEST_FEC_KR | I40E_AQ_REQUEST_FEC_RS))
> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> +			else if (configured_fec_cfg & I40E_AQ_REQUEST_FEC_KR)
> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +			else if (configured_fec_cfg & I40E_AQ_REQUEST_FEC_RS)
> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +			else
> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> +		}

Should current_fec_cfg (link_status.fec_info) be rewritten this way since it only has KR or RS bit and no AUTO bit? any suggestion please?

		if (configured_fec_cfg & I40E_AQ_ENABLE_FEC_AUTO)
			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
		else if (configured_fec_cfg & I40E_AQ_ENABLE_FEC_KR)
			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
		else if (configured_fec_cfg & I40E_AQ_ENABLE_FEC_RS)
			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
		else
			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);

> +	} else {
> +		if (current_fec_cfg & (I40E_AQ_ENABLE_FEC_KR | I40E_AQ_ENABLE_FEC_RS))
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> In case when FEC was successfully negotiated current_fec_cfg is containing only single bit (since 2 algos can not be enabled at the same time), so this part is meaningless. Also here and below in this else section (i.e. in section if link is UP), for consistency please use corresponding macros defined for struct i40e_aqc_get_link_status - I40E_AQ_CONFIG_FEC_KR_ENA and I40E_AQ_CONFIG_FEC_RS_ENA instead of I40E_AQ_ENABLE_FEC_ . 
> +		else if (current_fec_cfg & I40E_AQ_ENABLE_FEC_KR)
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +		else if (current_fec_cfg & I40E_AQ_ENABLE_FEC_RS)
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		else
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);

Is there any issue with this change?

		if (current_fec_cfg & I40E_AQ_CONFIG_FEC_KR_ENA)
			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
		else if (current_fec_cfg & I40E_AQ_CONFIG_FEC_RS_ENA)
			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
		else
			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);

> +	}
> +
> +	*fec_capa = temp_fec_capa;
> +
> +	return 0;
> +}
> +
> +static int
> +i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct i40e_aq_get_phy_abilities_resp abilities = {0};
> +	struct i40e_aq_set_phy_config config = {0};
> +	enum i40e_status_code status;
> +	uint8_t req_fec = 0;
> +
> +	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
> +	    hw->device_id != I40E_DEV_ID_25G_B &&
> +	    hw->device_id != I40E_DEV_ID_KX_X722) {
> +		return -ENOTSUP;
> +	}
> +
> +	if (hw->mac.type == I40E_MAC_X722 &&
> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
> +			 " firmware. Please update the NVM image.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	/**
> +	 * Copy the current user PHY configuration. The current user PHY
> +	 * configuration is initialized during probe from PHY capabilities
> +	 * software mode, and updated on set PHY configuration.
> +	 */
> +	if (fec_capa != 0)
> +		return -EINVAL;
> Did you mean if (fec_capa == 0)?

Yes...sorry for mistake, will fix in next version.

> 
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)) {
> +		if (hw->mac.type == I40E_MAC_X722) {
> +			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
> +			return -EINVAL;
> +		}
> +		req_fec = I40E_AQ_SET_FEC_AUTO;
> +		goto set_fec;
> +	}
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC))
> +		req_fec = 0;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
> +		req_fec |= I40E_AQ_SET_FEC_REQUEST_KR;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS)) {
> +		if (hw->mac.type == I40E_MAC_X722) {
> +			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
> +			return -EINVAL;
> +		}
> +		req_fec |= I40E_AQ_SET_FEC_REQUEST_RS;
> +	}
> This part is not looking correct to me:
> - only I40E_AQ_SET_FEC_AUTO bit is set w/o enabling specific protocols
> - "if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC))" makes no sense if other bitfields are set
> - only I40E_AQ_SET_FEC_REQUEST_* is set without I40E_AQ_SET_FEC_ABILITY_*
>  According to API : "If only the AUTO bit is set, the decision on which FEC mode to use will be made by HW/FW or driver. If the AUTO bit is set with some FEC modes, only specified FEC modes can be set. If AUTO bit is clear, specify FEC mode to be used (only one valid mode per speed may be set)." I'd suggest:
> - In case when AUTO bit is set we need to check for other bit flags. In case of absence let's decide to  add all supported by mac type algorithms (i.e. set corresponding _enable_ and _request_ bit fields for each supported algo).
> - In case if there are more bits apart from AUTO - check for their validity and support and specify corresponding _enable_ and _request_ flags.
> - In case if AUTO is not set check that only single mode was specified, check its validity + support and so on.

Based on your suggestions, any other suggestion for the following rework?

	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
		fec_auto = 1;

	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
		fec_kr = 1;

	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
		fec_rs = 1;

	if (fec_auto) {
		if (hw->mac.type == I40E_MAC_X722) {
			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
			return -EINVAL;
		}
		if (fec_kr ^ fec_rs) {
			if (fec_kr)
				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
							I40E_AQ_SET_FEC_REQUEST_KR;
			else {
				if (hw->mac.type == I40E_MAC_X722) {
					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
					return -EINVAL;
				}
				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
							I40E_AQ_SET_FEC_REQUEST_RS;
			}
		} else {
			if (hw->mac.type == I40E_MAC_X722) {
				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
						  I40E_AQ_SET_FEC_REQUEST_KR;
			} else {
				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
						  I40E_AQ_SET_FEC_REQUEST_KR |
						  I40E_AQ_SET_FEC_ABILITY_RS |
						  I40E_AQ_SET_FEC_REQUEST_RS;
			}
		}
	} else {
		if (fec_kr ^ fec_rs) {
			if (fec_kr)
				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
							I40E_AQ_SET_FEC_REQUEST_KR;
			else {
				if (hw->mac.type == I40E_MAC_X722) {
					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
					return -EINVAL;
				}
				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
							I40E_AQ_SET_FEC_REQUEST_RS;
			}
		} else {
			return -EINVAL;
		}
	}

> +
> +set_fec:
> +	/* Get the current phy config */
> +	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
> +					      NULL);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
> +				status);
> +		return -ENOTSUP;
> +	}
> +
> +	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
> +		config.phy_type = abilities.phy_type;
> +		config.abilities = abilities.abilities |
> +				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> +		config.phy_type_ext = abilities.phy_type_ext;
> +		config.link_speed = abilities.link_speed;
> +		config.eee_capability = abilities.eee_capability;
> +		config.eeer = abilities.eeer_val;
> +		config.low_power_ctrl = abilities.d3_lpan;
> +		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
> +		status = i40e_aq_set_phy_config(hw, &config, NULL);
> +		if (status) {
> +			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
> +			status);
> +			return -ENOTSUP;
> +		}
> +	}
> +
> +	status = i40e_update_link_info(hw);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
> +			status);
> +		return -EAGAIN;
> This is a new return status of this API. It needs to be added in doxygen documentation for this function and be reflected in release notes

My bad, will fix in next version.

> 
> +	}
> +
> +	return 0;
> +}
> +
>  RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
>  RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
>  #ifdef RTE_ETHDEV_DEBUG_RX
> -- 
> Regards,
> Vladimir
>

Regards,
Zhichao

Re: [PATCH v4] net/i40e: support FEC feature

[Medvedkin, Vladimir]

Hi Zhichao,

On 12/03/2024 08:44, Zeng, ZhichaoX wrote:
> Hi Vladimir:
>
> Thanks for your comments, some responses and questions are inline.
>
>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>> Sent: Monday, March 11, 2024 11:59 PM
>> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>; dev@dpdk.org
>> Cc: Cui, KaixinX <kaixinx.cui@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>> Subject: Re: [PATCH v4] net/i40e: support FEC feature
>>
>> Hi Zhicha,
>> It would be good to reflect FEC feature here:
>> https://doc.dpdk.org/guides/nics/overview.html
>> in " Table 1.1 Features availability in networking drivers "
> After modifying "doc/guides/nics/features/i40e.ini", the corresponding "overview.html" will be generated after compiling the document.
Ah sure, my bad, thanks!
>> please find the rest comments inline
>> On 06/03/2024 10:41, Zhichao Zeng wrote:
>> This patch enabled querying Forward Error Correction(FEC) capabilities,
>> set FEC mode and get current FEC mode functions.
>>
>> Signed-off-by: Qiming Yang mailto:qiming.yang@intel.com
>> Signed-off-by: Zhichao Zeng mailto:zhichaox.zeng@intel.com
>>
>> ---
>> v4: fix some logic
>> v3: optimize code details
>> v2: update NIC feature document
>> ---
>>   doc/guides/nics/features/i40e.ini      |   1 +
>>   doc/guides/rel_notes/release_24_03.rst |   5 +
>>   drivers/net/i40e/i40e_ethdev.c         | 192 +++++++++++++++++++++++++
>>   3 files changed, 198 insertions(+)
>>
>> diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
>> index e241dad047..aac2c1a6a1 100644
>> --- a/doc/guides/nics/features/i40e.ini
>> +++ b/doc/guides/nics/features/i40e.ini
>> @@ -30,6 +30,7 @@ Flow control         = Y
>>   CRC offload          = Y
>>   VLAN offload         = Y
>>   QinQ offload         = P
>> +FEC                  = Y
>>   L3 checksum offload  = P
>>   L4 checksum offload  = P
>>   Inner L3 checksum    = P
>> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
>> index 161f77112b..862a5f8fb8 100644
>> --- a/doc/guides/rel_notes/release_24_03.rst
>> +++ b/doc/guides/rel_notes/release_24_03.rst
>> @@ -110,6 +110,11 @@ New Features
>>   
>>     * Added support for 5760X device family.
>>   
>> +* **Updated Intel i40e driver.**
>> +
>> +  * Added support for configuring the Forward Error Correction(FEC) mode, querying
>> +  * FEC capabilities and current FEC mode from a device.
>> +
>>   * **Updated Marvell cnxk net driver.**
>>   
>>     * Added support for port representors.
>> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
>> index 380ce1a720..2bc6675a04 100644
>> --- a/drivers/net/i40e/i40e_ethdev.c
>> +++ b/drivers/net/i40e/i40e_ethdev.c
>> @@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
>>   static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
>>   static void i40e_filter_restore(struct i40e_pf *pf);
>>   static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
>> +static int i40e_fec_get_capability(struct rte_eth_dev *dev,
>> +	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
>> +static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
>> +static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
>>   
>>   static const char *const valid_keys[] = {
>>   	ETH_I40E_FLOATING_VEB_ARG,
>> @@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
>>   	.tm_ops_get                   = i40e_tm_ops_get,
>>   	.tx_done_cleanup              = i40e_tx_done_cleanup,
>>   	.get_monitor_addr             = i40e_get_monitor_addr,
>> +	.fec_get_capability           = i40e_fec_get_capability,
>> +	.fec_get                      = i40e_fec_get,
>> +	.fec_set                      = i40e_fec_set,
>>   };
>>   
>>   /* store statistics names and its offset in stats structure */
>> @@ -12297,6 +12304,191 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
>>   	return ret;
>>   }
>>   
>> +static int
>> +i40e_fec_get_capability(struct rte_eth_dev *dev,
>> +	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
>> +{
>> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>> +
>> +	if (hw->mac.type == I40E_MAC_X722 &&
>> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
>> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
>> +			 " firmware. Please update the NVM image.\n");
>> +		return -ENOTSUP;
>> +	}
>> +
>> +	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
>> +	    hw->device_id == I40E_DEV_ID_25G_B) {
>> +		if (speed_fec_capa) {
>> +			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
>> +			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
>> +					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
>> +					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
>> +					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
>> +		}
>> +
>> +		/* since HW only supports 25G */
>> +		return 1;
>> +	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
>> +		if (speed_fec_capa) {
>> +			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
>> +			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
>> +					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
>> +		}
>> +		return 1;
>> +	}
>> +
>> +	return -ENOTSUP;
>> +}
>> +
>> +static int
>> +i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
>> +{
>> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>> +	struct i40e_link_status link_status = {0};
>> +	uint8_t configured_fec_cfg = 0, current_fec_cfg;
>> +	uint32_t temp_fec_capa = 0;
>> +	bool link_up, enable_lse;
>> +	int ret = 0;
>> +
>> +	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
>> +	/* Get FEC info */
>> +	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
>> +	if (ret != I40E_SUCCESS) {
>> +		PMD_DRV_LOG(ERR, "Failed to get link information");
>> +		return -ENOTSUP;
>> +	}
>> +
>> +	link_up = link_status.link_info & I40E_AQ_LINK_UP;
>> +
>> +	/**
>> +	 * If link is down and AUTO is enabled, AUTO is returned,
>> +	 * otherwise, configured FEC mode is returned.
>> +	 * If link is up, current FEC mode is returned.
>> +	 */
>> +	configured_fec_cfg = link_status.req_fec_info;
>> here we need to better understand the difference between [FC,RS]-FEC ability bit (aka I40E_AQ_ENABLE_FEC_[KR,RS]) from [FC,RS]-FEC Request bit (aka I40E_AQ_REQUEST_FEC_[KR,RS]), since link_status.req_fec_info has only "Request" bits for each FEC algo (see i40e_update_link_info()).
>>  From what I found on the internet, it seems that we don't need to use them at all, because, for example for FC-FEC (aka Clause 74), from:
>> https://www.ieee802.org/3/25GSG/public/Nov14/baden_25GE_02_1114.pdf
>> Key phrase: "If both LPs advertise the FEC Ability, and EITHER LP requests the FEC, it is enabled."
>> So I'd suggest not to use these "Request" bits here.
>> For configured fec (case when link is DOWN) please use struct i40e_aq_get_phy_abilities_resp abilities.
> Will fix in next version.
>
>> +	current_fec_cfg = link_status.fec_info;
>> +
>> +	if (!link_up) {
>> This section should be rewritten according to bit flags from abilities.fec_cfg_curr_mod_ext_info.
>> +		if (current_fec_cfg & (I40E_AQ_ENABLE_FEC_KR | I40E_AQ_ENABLE_FEC_RS)) {
>> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
>> +		} else {
>> +			if (configured_fec_cfg == (I40E_AQ_REQUEST_FEC_KR | I40E_AQ_REQUEST_FEC_RS))
>> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
>> +			else if (configured_fec_cfg & I40E_AQ_REQUEST_FEC_KR)
>> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
>> +			else if (configured_fec_cfg & I40E_AQ_REQUEST_FEC_RS)
>> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
>> +			else
>> +				temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
>> +		}
> Should current_fec_cfg (link_status.fec_info) be rewritten this way since it only has KR or RS bit and no AUTO bit? any suggestion please?
>
> 		if (configured_fec_cfg & I40E_AQ_ENABLE_FEC_AUTO)
> 			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> 		else if (configured_fec_cfg & I40E_AQ_ENABLE_FEC_KR)
> 			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> 		else if (configured_fec_cfg & I40E_AQ_ENABLE_FEC_RS)
> 			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> 		else
> 			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);

Probably you confused "current_fec_cfg" with "configured_fec_cfg". Let's 
rename those variables, for example use current_fec_mode for 
"link_status.fec_info" which will be used in case when link is UP, and 
fec_config for "abilities.fec_cfg_curr_mod_ext_info" which will be used 
in case when link is DOWN.

So, logic here should be like:

if (link is UP) {

/* check current_fec_mode */

if (popcnt(current_fec_mode) > 1)

     return -EINVAL;


switch (current_fec_mode) {

case I40E_AQ_CONFIG_FEC_KR_ENA:

     *fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);

     break;

case I40E_AQ_CONFIG_FEC_RS_ENA:

     *fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);

     break;

case 0:

     *fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);

     break;

default:

     return -EINVAL;

}

return 0;

}

/*port is UP, check fec_config */

else {

if (fec_config & I40E_AQ_ENABLE_FEC_AUTO ) {

     *fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);

     return 0;

}

uint32_t ret_fec = 0;

if (fec_config & I40E_AQ_ENABLE_FEC_KR)

     ret_fec |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);

if (fec_config & I40E_AQ_ENABLE_FEC_RS)

     ret_fec |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);

if (ret_fec == 0)

     ret_fec = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);


*fec_capa = ret_fec;

return 0;

}

>> +	} else {
>> +		if (current_fec_cfg & (I40E_AQ_ENABLE_FEC_KR | I40E_AQ_ENABLE_FEC_RS))
>> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
>> In case when FEC was successfully negotiated current_fec_cfg is containing only single bit (since 2 algos can not be enabled at the same time), so this part is meaningless. Also here and below in this else section (i.e. in section if link is UP), for consistency please use corresponding macros defined for struct i40e_aqc_get_link_status - I40E_AQ_CONFIG_FEC_KR_ENA and I40E_AQ_CONFIG_FEC_RS_ENA instead of I40E_AQ_ENABLE_FEC_ .
>> +		else if (current_fec_cfg & I40E_AQ_ENABLE_FEC_KR)
>> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
>> +		else if (current_fec_cfg & I40E_AQ_ENABLE_FEC_RS)
>> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
>> +		else
>> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> Is there any issue with this change?
>
> 		if (current_fec_cfg & I40E_AQ_CONFIG_FEC_KR_ENA)
> 			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> 		else if (current_fec_cfg & I40E_AQ_CONFIG_FEC_RS_ENA)
> 			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> 		else
> 			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
It should be fine if there is only one single bit in the 
current_fec_cfg. Please refer to my previous comment above.
>> +	}
>> +
>> +	*fec_capa = temp_fec_capa;
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
>> +{
>> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>> +	struct i40e_aq_get_phy_abilities_resp abilities = {0};
>> +	struct i40e_aq_set_phy_config config = {0};
>> +	enum i40e_status_code status;
>> +	uint8_t req_fec = 0;
>> +
>> +	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
>> +	    hw->device_id != I40E_DEV_ID_25G_B &&
>> +	    hw->device_id != I40E_DEV_ID_KX_X722) {
>> +		return -ENOTSUP;
>> +	}
>> +
>> +	if (hw->mac.type == I40E_MAC_X722 &&
>> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
>> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
>> +			 " firmware. Please update the NVM image.\n");
>> +		return -ENOTSUP;
>> +	}
>> +
>> +	/**
>> +	 * Copy the current user PHY configuration. The current user PHY
>> +	 * configuration is initialized during probe from PHY capabilities
>> +	 * software mode, and updated on set PHY configuration.
>> +	 */
>> +	if (fec_capa != 0)
>> +		return -EINVAL;
>> Did you mean if (fec_capa == 0)?
> Yes...sorry for mistake, will fix in next version.
>
>> +
>> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)) {
>> +		if (hw->mac.type == I40E_MAC_X722) {
>> +			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
>> +			return -EINVAL;
>> +		}
>> +		req_fec = I40E_AQ_SET_FEC_AUTO;
>> +		goto set_fec;
>> +	}
>> +
>> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC))
>> +		req_fec = 0;
>> +
>> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
>> +		req_fec |= I40E_AQ_SET_FEC_REQUEST_KR;
>> +
>> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS)) {
>> +		if (hw->mac.type == I40E_MAC_X722) {
>> +			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
>> +			return -EINVAL;
>> +		}
>> +		req_fec |= I40E_AQ_SET_FEC_REQUEST_RS;
>> +	}
>> This part is not looking correct to me:
>> - only I40E_AQ_SET_FEC_AUTO bit is set w/o enabling specific protocols
>> - "if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC))" makes no sense if other bitfields are set
>> - only I40E_AQ_SET_FEC_REQUEST_* is set without I40E_AQ_SET_FEC_ABILITY_*
>>   According to API : "If only the AUTO bit is set, the decision on which FEC mode to use will be made by HW/FW or driver. If the AUTO bit is set with some FEC modes, only specified FEC modes can be set. If AUTO bit is clear, specify FEC mode to be used (only one valid mode per speed may be set)." I'd suggest:
>> - In case when AUTO bit is set we need to check for other bit flags. In case of absence let's decide to  add all supported by mac type algorithms (i.e. set corresponding _enable_ and _request_ bit fields for each supported algo).
>> - In case if there are more bits apart from AUTO - check for their validity and support and specify corresponding _enable_ and _request_ flags.
>> - In case if AUTO is not set check that only single mode was specified, check its validity + support and so on.
> Based on your suggestions, any other suggestion for the following rework?
>
> 	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
> 		fec_auto = 1;
>
> 	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
> 		fec_kr = 1;
>
> 	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
> 		fec_rs = 1;
>
> 	if (fec_auto) {
> 		if (hw->mac.type == I40E_MAC_X722) {
> 			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
> 			return -EINVAL;
> 		}
> 		if (fec_kr ^ fec_rs) {
did you mean OR instead of XOR?
> 			if (fec_kr)
> 				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> 							I40E_AQ_SET_FEC_REQUEST_KR;
> 			else {
here instead of else should be if (fec_rs)

                                             req_fec |= ...

> 				if (hw->mac.type == I40E_MAC_X722) {
> 					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
> 					return -EINVAL;
> 				}
> 				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
> 							I40E_AQ_SET_FEC_REQUEST_RS;
> 			}
> 		} else {
> 			if (hw->mac.type == I40E_MAC_X722) {
> 				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> 						  I40E_AQ_SET_FEC_REQUEST_KR;
> 			} else {
> 				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> 						  I40E_AQ_SET_FEC_REQUEST_KR |
> 						  I40E_AQ_SET_FEC_ABILITY_RS |
> 						  I40E_AQ_SET_FEC_REQUEST_RS;
> 			}
> 		}
> 	} else {
> 		if (fec_kr ^ fec_rs) {
> 			if (fec_kr)
> 				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> 							I40E_AQ_SET_FEC_REQUEST_KR;
> 			else {
> 				if (hw->mac.type == I40E_MAC_X722) {
> 					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
> 					return -EINVAL;
> 				}
> 				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
> 							I40E_AQ_SET_FEC_REQUEST_RS;
> 			}
> 		} else {
> 			return -EINVAL;
> 		}
> 	}
It's better to rewrite this section
>> +
>> +set_fec:
>> +	/* Get the current phy config */
>> +	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
>> +					      NULL);
>> +	if (status) {
>> +		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
>> +				status);
>> +		return -ENOTSUP;
>> +	}
>> +
>> +	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
>> +		config.phy_type = abilities.phy_type;
>> +		config.abilities = abilities.abilities |
>> +				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
>> +		config.phy_type_ext = abilities.phy_type_ext;
>> +		config.link_speed = abilities.link_speed;
>> +		config.eee_capability = abilities.eee_capability;
>> +		config.eeer = abilities.eeer_val;
>> +		config.low_power_ctrl = abilities.d3_lpan;
>> +		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
>> +		status = i40e_aq_set_phy_config(hw, &config, NULL);
>> +		if (status) {
>> +			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
>> +			status);
>> +			return -ENOTSUP;
>> +		}
>> +	}
>> +
>> +	status = i40e_update_link_info(hw);
>> +	if (status) {
>> +		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
>> +			status);
>> +		return -EAGAIN;
>> This is a new return status of this API. It needs to be added in doxygen documentation for this function and be reflected in release notes
> My bad, will fix in next version.
>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
>>   RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
>>   #ifdef RTE_ETHDEV_DEBUG_RX
>> -- 
>> Regards,
>> Vladimir
>>
> Regards,
> Zhichao

-- 
Regards,
Vladimir

[PATCH v5] net/i40e: support FEC feature

[Zhichao Zeng]

This patch enabled querying Forward Error Correction(FEC) capabilities,
set FEC mode and get current FEC mode functions.

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

---
v5: fix some judgments
v4: fix some logic
v3: optimize code details
v2: update NIC feature document
---
 doc/guides/nics/features/i40e.ini      |   1 +
 doc/guides/rel_notes/release_24_07.rst |   4 +
 drivers/net/i40e/i40e_ethdev.c         | 237 +++++++++++++++++++++++++
 3 files changed, 242 insertions(+)

diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
index ef7514c44b..4610444ace 100644
--- a/doc/guides/nics/features/i40e.ini
+++ b/doc/guides/nics/features/i40e.ini
@@ -32,6 +32,7 @@ Traffic manager      = Y
 CRC offload          = Y
 VLAN offload         = Y
 QinQ offload         = P
+FEC                  = Y
 L3 checksum offload  = P
 L4 checksum offload  = P
 Inner L3 checksum    = P
diff --git a/doc/guides/rel_notes/release_24_07.rst b/doc/guides/rel_notes/release_24_07.rst
index a69f24cf99..1e65f70d6c 100644
--- a/doc/guides/rel_notes/release_24_07.rst
+++ b/doc/guides/rel_notes/release_24_07.rst
@@ -55,6 +55,10 @@ New Features
      Also, make sure to start the actual text at the margin.
      =======================================================
 
+* **Updated Intel i40e driver.**
+
+  * Added support for configuring the Forward Error Correction(FEC) mode, querying
+  * FEC capabilities and current FEC mode from a device.
 
 Removed Items
 -------------
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 380ce1a720..2235bbefda 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
+static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.tm_ops_get                   = i40e_tm_ops_get,
 	.tx_done_cleanup              = i40e_tx_done_cleanup,
 	.get_monitor_addr             = i40e_get_monitor_addr,
+	.fec_get_capability           = i40e_fec_get_capability,
+	.fec_get                      = i40e_fec_get,
+	.fec_set                      = i40e_fec_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -12297,6 +12304,236 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
+	    hw->device_id == I40E_DEV_ID_25G_B) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+
+		/* since HW only supports 25G */
+		return 1;
+	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		return 1;
+	}
+
+	return -ENOTSUP;
+}
+
+static int
+i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_link_status link_status = {0};
+	uint8_t current_fec_mode = 0, fec_config = 0;
+	bool link_up, enable_lse;
+	int ret = 0;
+
+	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+	/* Get link info */
+	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
+	if (ret != I40E_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	link_up = link_status.link_info & I40E_AQ_LINK_UP;
+
+	ret = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+						  NULL);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	/**
+	 * If link is down and AUTO is enabled, AUTO is returned,
+	 * otherwise, configured FEC mode is returned.
+	 * If link is up, current FEC mode is returned.
+	 */
+	fec_config = abilities.fec_cfg_curr_mod_ext_info;
+	current_fec_mode = link_status.fec_info;
+
+	if (link_up) {
+		switch (current_fec_mode) {
+		case I40E_AQ_CONFIG_FEC_KR_ENA:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
+			break;
+		case I40E_AQ_CONFIG_FEC_RS_ENA:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
+			break;
+		case 0:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
+			break;
+		default:
+			return -EINVAL;
+		}
+		return 0;
+	}
+
+	if (fec_config & I40E_AQ_ENABLE_FEC_AUTO) {
+		*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
+		return 0;
+	}
+
+	uint32_t temp_fec_capa = 0;
+	if (fec_config & I40E_AQ_ENABLE_FEC_KR)
+		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
+	if (configured_fec_cfg & I40E_AQ_ENABLE_FEC_RS)
+		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
+	if (configured_fec_cfg == 0)
+		temp_fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
+
+	*fec_capa = temp_fec_capa;
+	return 0;
+}
+
+static int
+i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_aq_set_phy_config config = {0};
+	enum i40e_status_code status;
+	uint8_t req_fec = 0, fec_auto = 0, fec_kr = 0, fec_rs = 0;
+
+	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
+	    hw->device_id != I40E_DEV_ID_25G_B &&
+	    hw->device_id != I40E_DEV_ID_KX_X722) {
+		return -ENOTSUP;
+	}
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	/**
+	 * Copy the current user PHY configuration. The current user PHY
+	 * configuration is initialized during probe from PHY capabilities
+	 * software mode, and updated on set PHY configuration.
+	 */
+	if (fec_capa == 0)
+		return -EINVAL;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
+		fec_auto = 1;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
+		fec_kr = 1;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
+		fec_rs = 1;
+
+	if (fec_auto) {
+		if (hw->mac.type == I40E_MAC_X722) {
+			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
+			return -EINVAL;
+		}
+		if (fec_kr || fec_rs) {
+			if (fec_kr)
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+							I40E_AQ_SET_FEC_REQUEST_KR;
+			if (fec_rs) {
+				if (hw->mac.type == I40E_MAC_X722) {
+					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+					return -EINVAL;
+				}
+				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
+							I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		} else {
+			if (hw->mac.type == I40E_MAC_X722) {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+						  I40E_AQ_SET_FEC_REQUEST_KR;
+			} else {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+						  I40E_AQ_SET_FEC_REQUEST_KR |
+						  I40E_AQ_SET_FEC_ABILITY_RS |
+						  I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		}
+	} else {
+		if (fec_kr ^ fec_rs) {
+			if (fec_kr) {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+							I40E_AQ_SET_FEC_REQUEST_KR;
+			} else {
+				if (hw->mac.type == I40E_MAC_X722) {
+					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+					return -EINVAL;
+				}
+				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
+							I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	/* Get the current phy config */
+	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+					      NULL);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				status);
+		return -ENOTSUP;
+	}
+
+	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
+		config.phy_type = abilities.phy_type;
+		config.abilities = abilities.abilities |
+				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
+		config.phy_type_ext = abilities.phy_type_ext;
+		config.link_speed = abilities.link_speed;
+		config.eee_capability = abilities.eee_capability;
+		config.eeer = abilities.eeer_val;
+		config.low_power_ctrl = abilities.d3_lpan;
+		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
+		status = i40e_aq_set_phy_config(hw, &config, NULL);
+		if (status) {
+			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+			return -ENOTSUP;
+		}
+	}
+
+	status = i40e_update_link_info(hw);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
 #ifdef RTE_ETHDEV_DEBUG_RX
-- 
2.34.1

[PATCH v5] net/i40e: support FEC feature

[Zhichao Zeng]

This patch enabled querying Forward Error Correction(FEC) capabilities,
set FEC mode and get current FEC mode functions.

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

---
v5: fix some judgments
v4: fix some logic
v3: optimize code details
v2: update NIC feature document
---
 doc/guides/nics/features/i40e.ini      |   1 +
 doc/guides/rel_notes/release_24_07.rst |   4 +
 drivers/net/i40e/i40e_ethdev.c         | 237 +++++++++++++++++++++++++
 3 files changed, 242 insertions(+)

diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
index ef7514c44b..4610444ace 100644
--- a/doc/guides/nics/features/i40e.ini
+++ b/doc/guides/nics/features/i40e.ini
@@ -32,6 +32,7 @@ Traffic manager      = Y
 CRC offload          = Y
 VLAN offload         = Y
 QinQ offload         = P
+FEC                  = Y
 L3 checksum offload  = P
 L4 checksum offload  = P
 Inner L3 checksum    = P
diff --git a/doc/guides/rel_notes/release_24_07.rst b/doc/guides/rel_notes/release_24_07.rst
index a69f24cf99..1e65f70d6c 100644
--- a/doc/guides/rel_notes/release_24_07.rst
+++ b/doc/guides/rel_notes/release_24_07.rst
@@ -55,6 +55,10 @@ New Features
      Also, make sure to start the actual text at the margin.
      =======================================================
 
+* **Updated Intel i40e driver.**
+
+  * Added support for configuring the Forward Error Correction(FEC) mode, querying
+  * FEC capabilities and current FEC mode from a device.
 
 Removed Items
 -------------
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 380ce1a720..bc4a62f64b 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
+static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.tm_ops_get                   = i40e_tm_ops_get,
 	.tx_done_cleanup              = i40e_tx_done_cleanup,
 	.get_monitor_addr             = i40e_get_monitor_addr,
+	.fec_get_capability           = i40e_fec_get_capability,
+	.fec_get                      = i40e_fec_get,
+	.fec_set                      = i40e_fec_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -12297,6 +12304,236 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
+	    hw->device_id == I40E_DEV_ID_25G_B) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+
+		/* since HW only supports 25G */
+		return 1;
+	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		return 1;
+	}
+
+	return -ENOTSUP;
+}
+
+static int
+i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_link_status link_status = {0};
+	uint8_t current_fec_mode = 0, fec_config = 0;
+	bool link_up, enable_lse;
+	int ret = 0;
+
+	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+	/* Get link info */
+	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
+	if (ret != I40E_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	link_up = link_status.link_info & I40E_AQ_LINK_UP;
+
+	ret = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+						  NULL);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	/**
+	 * If link is down and AUTO is enabled, AUTO is returned,
+	 * otherwise, configured FEC mode is returned.
+	 * If link is up, current FEC mode is returned.
+	 */
+	fec_config = abilities.fec_cfg_curr_mod_ext_info;
+	current_fec_mode = link_status.fec_info;
+
+	if (link_up) {
+		switch (current_fec_mode) {
+		case I40E_AQ_CONFIG_FEC_KR_ENA:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
+			break;
+		case I40E_AQ_CONFIG_FEC_RS_ENA:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
+			break;
+		case 0:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
+			break;
+		default:
+			return -EINVAL;
+		}
+		return 0;
+	}
+
+	if (fec_config & I40E_AQ_ENABLE_FEC_AUTO) {
+		*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
+		return 0;
+	}
+
+	uint32_t temp_fec_capa = 0;
+	if (fec_config & I40E_AQ_ENABLE_FEC_KR)
+		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
+	if (fec_config & I40E_AQ_ENABLE_FEC_RS)
+		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
+	if (temp_fec_capa == 0)
+		temp_fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
+
+	*fec_capa = temp_fec_capa;
+	return 0;
+}
+
+static int
+i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_aq_set_phy_config config = {0};
+	enum i40e_status_code status;
+	uint8_t req_fec = 0, fec_auto = 0, fec_kr = 0, fec_rs = 0;
+
+	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
+	    hw->device_id != I40E_DEV_ID_25G_B &&
+	    hw->device_id != I40E_DEV_ID_KX_X722) {
+		return -ENOTSUP;
+	}
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	/**
+	 * Copy the current user PHY configuration. The current user PHY
+	 * configuration is initialized during probe from PHY capabilities
+	 * software mode, and updated on set PHY configuration.
+	 */
+	if (fec_capa == 0)
+		return -EINVAL;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
+		fec_auto = 1;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
+		fec_kr = 1;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
+		fec_rs = 1;
+
+	if (fec_auto) {
+		if (hw->mac.type == I40E_MAC_X722) {
+			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
+			return -EINVAL;
+		}
+		if (fec_kr || fec_rs) {
+			if (fec_kr)
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+							I40E_AQ_SET_FEC_REQUEST_KR;
+			if (fec_rs) {
+				if (hw->mac.type == I40E_MAC_X722) {
+					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+					return -EINVAL;
+				}
+				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
+							I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		} else {
+			if (hw->mac.type == I40E_MAC_X722) {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+						  I40E_AQ_SET_FEC_REQUEST_KR;
+			} else {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+						  I40E_AQ_SET_FEC_REQUEST_KR |
+						  I40E_AQ_SET_FEC_ABILITY_RS |
+						  I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		}
+	} else {
+		if (fec_kr ^ fec_rs) {
+			if (fec_kr) {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+							I40E_AQ_SET_FEC_REQUEST_KR;
+			} else {
+				if (hw->mac.type == I40E_MAC_X722) {
+					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+					return -EINVAL;
+				}
+				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
+							I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	/* Get the current phy config */
+	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+					      NULL);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				status);
+		return -ENOTSUP;
+	}
+
+	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
+		config.phy_type = abilities.phy_type;
+		config.abilities = abilities.abilities |
+				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
+		config.phy_type_ext = abilities.phy_type_ext;
+		config.link_speed = abilities.link_speed;
+		config.eee_capability = abilities.eee_capability;
+		config.eeer = abilities.eeer_val;
+		config.low_power_ctrl = abilities.d3_lpan;
+		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
+		status = i40e_aq_set_phy_config(hw, &config, NULL);
+		if (status) {
+			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+			return -ENOTSUP;
+		}
+	}
+
+	status = i40e_update_link_info(hw);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
 #ifdef RTE_ETHDEV_DEBUG_RX
-- 
2.34.1

RE: [PATCH v5] net/i40e: support FEC feature

[Zeng, ZhichaoX]

Hi Bruce,

Kindly ping for review~ Thanks.

Regards
Zhichao

> -----Original Message-----
> From: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Sent: Thursday, April 11, 2024 5:30 PM
> To: dev@dpdk.org
> Cc: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>; Cui, KaixinX
> <kaixinx.cui@intel.com>; Zeng, ZhichaoX <zhichaox.zeng@intel.com>; Yang,
> Qiming <qiming.yang@intel.com>; Yuying Zhang <Yuying.Zhang@intel.com>
> Subject: [PATCH v5] net/i40e: support FEC feature
> 
> This patch enabled querying Forward Error Correction(FEC) capabilities, set
> FEC mode and get current FEC mode functions.
> 
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> ---
> v5: fix some judgments
> v4: fix some logic
> v3: optimize code details
> v2: update NIC feature document
> ---
>  doc/guides/nics/features/i40e.ini      |   1 +
>  doc/guides/rel_notes/release_24_07.rst |   4 +
>  drivers/net/i40e/i40e_ethdev.c         | 237 +++++++++++++++++++++++++
>  3 files changed, 242 insertions(+)
> 
> diff --git a/doc/guides/nics/features/i40e.ini
> b/doc/guides/nics/features/i40e.ini
> index ef7514c44b..4610444ace 100644
> --- a/doc/guides/nics/features/i40e.ini
> +++ b/doc/guides/nics/features/i40e.ini
> @@ -32,6 +32,7 @@ Traffic manager      = Y
>  CRC offload          = Y
>  VLAN offload         = Y
>  QinQ offload         = P
> +FEC                  = Y
>  L3 checksum offload  = P
>  L4 checksum offload  = P
>  Inner L3 checksum    = P
> diff --git a/doc/guides/rel_notes/release_24_07.rst
> b/doc/guides/rel_notes/release_24_07.rst
> index a69f24cf99..1e65f70d6c 100644
> --- a/doc/guides/rel_notes/release_24_07.rst
> +++ b/doc/guides/rel_notes/release_24_07.rst
> @@ -55,6 +55,10 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
> 
> +* **Updated Intel i40e driver.**
> +
> +  * Added support for configuring the Forward Error Correction(FEC)
> + mode, querying
> +  * FEC capabilities and current FEC mode from a device.
> 
>  Removed Items
>  -------------
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 380ce1a720..bc4a62f64b 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct
> i40e_pf *pf);  static void i40e_tunnel_filter_restore(struct i40e_pf *pf);  static
> void i40e_filter_restore(struct i40e_pf *pf);  static void
> i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
> +static int i40e_fec_get_capability(struct rte_eth_dev *dev,
> +	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num); static int
> +i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa); static int
> +i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
> 
>  static const char *const valid_keys[] = {
>  	ETH_I40E_FLOATING_VEB_ARG,
> @@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
>  	.tm_ops_get                   = i40e_tm_ops_get,
>  	.tx_done_cleanup              = i40e_tx_done_cleanup,
>  	.get_monitor_addr             = i40e_get_monitor_addr,
> +	.fec_get_capability           = i40e_fec_get_capability,
> +	.fec_get                      = i40e_fec_get,
> +	.fec_set                      = i40e_fec_set,
>  };
> 
>  /* store statistics names and its offset in stats structure */ @@ -12297,6
> +12304,236 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
>  	return ret;
>  }
> 
> +static int
> +i40e_fec_get_capability(struct rte_eth_dev *dev,
> +	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int
> +num) {
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +
> +	if (hw->mac.type == I40E_MAC_X722 &&
> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported
> by"
> +			 " firmware. Please update the NVM image.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
> +	    hw->device_id == I40E_DEV_ID_25G_B) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa->speed =
> RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa->capa =
> RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +
> RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> +
> RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +
> RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +
> +		/* since HW only supports 25G */
> +		return 1;
> +	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa->speed =
> RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa->capa =
> RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +
> RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +		return 1;
> +	}
> +
> +	return -ENOTSUP;
> +}
> +
> +static int
> +i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa) {
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct i40e_aq_get_phy_abilities_resp abilities = {0};
> +	struct i40e_link_status link_status = {0};
> +	uint8_t current_fec_mode = 0, fec_config = 0;
> +	bool link_up, enable_lse;
> +	int ret = 0;
> +
> +	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
> +	/* Get link info */
> +	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
> +	if (ret != I40E_SUCCESS) {
> +		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
> +				ret);
> +		return -ENOTSUP;
> +	}
> +
> +	link_up = link_status.link_info & I40E_AQ_LINK_UP;
> +
> +	ret = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
> +						  NULL);
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
> +				ret);
> +		return -ENOTSUP;
> +	}
> +
> +	/**
> +	 * If link is down and AUTO is enabled, AUTO is returned,
> +	 * otherwise, configured FEC mode is returned.
> +	 * If link is up, current FEC mode is returned.
> +	 */
> +	fec_config = abilities.fec_cfg_curr_mod_ext_info;
> +	current_fec_mode = link_status.fec_info;
> +
> +	if (link_up) {
> +		switch (current_fec_mode) {
> +		case I40E_AQ_CONFIG_FEC_KR_ENA:
> +			*fec_capa =
> RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
> +			break;
> +		case I40E_AQ_CONFIG_FEC_RS_ENA:
> +			*fec_capa =
> RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
> +			break;
> +		case 0:
> +			*fec_capa =
> RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		return 0;
> +	}
> +
> +	if (fec_config & I40E_AQ_ENABLE_FEC_AUTO) {
> +		*fec_capa =
> RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
> +		return 0;
> +	}
> +
> +	uint32_t temp_fec_capa = 0;
> +	if (fec_config & I40E_AQ_ENABLE_FEC_KR)
> +		temp_fec_capa |=
> RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
> +	if (fec_config & I40E_AQ_ENABLE_FEC_RS)
> +		temp_fec_capa |=
> RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
> +	if (temp_fec_capa == 0)
> +		temp_fec_capa =
> RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
> +
> +	*fec_capa = temp_fec_capa;
> +	return 0;
> +}
> +
> +static int
> +i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa) {
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct i40e_aq_get_phy_abilities_resp abilities = {0};
> +	struct i40e_aq_set_phy_config config = {0};
> +	enum i40e_status_code status;
> +	uint8_t req_fec = 0, fec_auto = 0, fec_kr = 0, fec_rs = 0;
> +
> +	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
> +	    hw->device_id != I40E_DEV_ID_25G_B &&
> +	    hw->device_id != I40E_DEV_ID_KX_X722) {
> +		return -ENOTSUP;
> +	}
> +
> +	if (hw->mac.type == I40E_MAC_X722 &&
> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported
> by"
> +			 " firmware. Please update the NVM image.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	/**
> +	 * Copy the current user PHY configuration. The current user PHY
> +	 * configuration is initialized during probe from PHY capabilities
> +	 * software mode, and updated on set PHY configuration.
> +	 */
> +	if (fec_capa == 0)
> +		return -EINVAL;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
> +		fec_auto = 1;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
> +		fec_kr = 1;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
> +		fec_rs = 1;
> +
> +	if (fec_auto) {
> +		if (hw->mac.type == I40E_MAC_X722) {
> +			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode:
> AUTO");
> +			return -EINVAL;
> +		}
> +		if (fec_kr || fec_rs) {
> +			if (fec_kr)
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> +
> 	I40E_AQ_SET_FEC_REQUEST_KR;
> +			if (fec_rs) {
> +				if (hw->mac.type == I40E_MAC_X722) {
> +					PMD_DRV_LOG(ERR, "X722
> Unsupported FEC mode: RS");
> +					return -EINVAL;
> +				}
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
> +
> 	I40E_AQ_SET_FEC_REQUEST_RS;
> +			}
> +		} else {
> +			if (hw->mac.type == I40E_MAC_X722) {
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> +
> I40E_AQ_SET_FEC_REQUEST_KR;
> +			} else {
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> +
> I40E_AQ_SET_FEC_REQUEST_KR |
> +
> I40E_AQ_SET_FEC_ABILITY_RS |
> +
> I40E_AQ_SET_FEC_REQUEST_RS;
> +			}
> +		}
> +	} else {
> +		if (fec_kr ^ fec_rs) {
> +			if (fec_kr) {
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> +
> 	I40E_AQ_SET_FEC_REQUEST_KR;
> +			} else {
> +				if (hw->mac.type == I40E_MAC_X722) {
> +					PMD_DRV_LOG(ERR, "X722
> Unsupported FEC mode: RS");
> +					return -EINVAL;
> +				}
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
> +
> 	I40E_AQ_SET_FEC_REQUEST_RS;
> +			}
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Get the current phy config */
> +	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
> +					      NULL);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
> +				status);
> +		return -ENOTSUP;
> +	}
> +
> +	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
> +		config.phy_type = abilities.phy_type;
> +		config.abilities = abilities.abilities |
> +				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> +		config.phy_type_ext = abilities.phy_type_ext;
> +		config.link_speed = abilities.link_speed;
> +		config.eee_capability = abilities.eee_capability;
> +		config.eeer = abilities.eeer_val;
> +		config.low_power_ctrl = abilities.d3_lpan;
> +		config.fec_config = req_fec &
> I40E_AQ_PHY_FEC_CONFIG_MASK;
> +		status = i40e_aq_set_phy_config(hw, &config, NULL);
> +		if (status) {
> +			PMD_DRV_LOG(ERR, "Failed to set PHY
> capabilities: %d\n",
> +			status);
> +			return -ENOTSUP;
> +		}
> +	}
> +
> +	status = i40e_update_link_info(hw);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
> +			status);
> +		return -ENOTSUP;
> +	}
> +
> +	return 0;
> +}
> +
>  RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
> RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);  #ifdef
> RTE_ETHDEV_DEBUG_RX
> --
> 2.34.1

Re: [PATCH v5] net/i40e: support FEC feature

[Medvedkin, Vladimir]

Hi Zhichao,

On 11/04/2024 10:29, Zhichao Zeng wrote:
> This patch enabled querying Forward Error Correction(FEC) capabilities,
> set FEC mode and get current FEC mode functions.
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
>
> ---
> v5: fix some judgments
> v4: fix some logic
> v3: optimize code details
> v2: update NIC feature document
> ---
>   doc/guides/nics/features/i40e.ini      |   1 +
>   doc/guides/rel_notes/release_24_07.rst |   4 +
>   drivers/net/i40e/i40e_ethdev.c         | 237 +++++++++++++++++++++++++
>   3 files changed, 242 insertions(+)
>
> diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
> index ef7514c44b..4610444ace 100644
> --- a/doc/guides/nics/features/i40e.ini
> +++ b/doc/guides/nics/features/i40e.ini
> @@ -32,6 +32,7 @@ Traffic manager      = Y
>   CRC offload          = Y
>   VLAN offload         = Y
>   QinQ offload         = P
> +FEC                  = Y
>   L3 checksum offload  = P
>   L4 checksum offload  = P
>   Inner L3 checksum    = P
> diff --git a/doc/guides/rel_notes/release_24_07.rst b/doc/guides/rel_notes/release_24_07.rst
> index a69f24cf99..1e65f70d6c 100644
> --- a/doc/guides/rel_notes/release_24_07.rst
> +++ b/doc/guides/rel_notes/release_24_07.rst
> @@ -55,6 +55,10 @@ New Features
>        Also, make sure to start the actual text at the margin.
>        =======================================================
>   
> +* **Updated Intel i40e driver.**
> +
> +  * Added support for configuring the Forward Error Correction(FEC) mode, querying
> +  * FEC capabilities and current FEC mode from a device.
>   
>   Removed Items
>   -------------
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 380ce1a720..bc4a62f64b 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
>   static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
>   static void i40e_filter_restore(struct i40e_pf *pf);
>   static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
> +static int i40e_fec_get_capability(struct rte_eth_dev *dev,
> +	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
> +static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
> +static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
>   
>   static const char *const valid_keys[] = {
>   	ETH_I40E_FLOATING_VEB_ARG,
> @@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
>   	.tm_ops_get                   = i40e_tm_ops_get,
>   	.tx_done_cleanup              = i40e_tx_done_cleanup,
>   	.get_monitor_addr             = i40e_get_monitor_addr,
> +	.fec_get_capability           = i40e_fec_get_capability,
> +	.fec_get                      = i40e_fec_get,
> +	.fec_set                      = i40e_fec_set,
>   };
>   
>   /* store statistics names and its offset in stats structure */
> @@ -12297,6 +12304,236 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
>   	return ret;
>   }
>   
> +static int
> +i40e_fec_get_capability(struct rte_eth_dev *dev,
> +	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +	if (hw->mac.type == I40E_MAC_X722 &&
> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
> +			 " firmware. Please update the NVM image.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
> +	    hw->device_id == I40E_DEV_ID_25G_B) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +
> +		/* since HW only supports 25G */
> +		return 1;
> +	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
> +		if (speed_fec_capa) {
> +			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
> +			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		}
> +		return 1;
> +	}
> +
> +	return -ENOTSUP;
> +}
> +
> +static int
> +i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct i40e_aq_get_phy_abilities_resp abilities = {0};
> +	struct i40e_link_status link_status = {0};
> +	uint8_t current_fec_mode = 0, fec_config = 0;
> +	bool link_up, enable_lse;
> +	int ret = 0;
> +
> +	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
> +	/* Get link info */
> +	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
> +	if (ret != I40E_SUCCESS) {
> +		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
> +				ret);
> +		return -ENOTSUP;
> +	}
> +
> +	link_up = link_status.link_info & I40E_AQ_LINK_UP;
> +
> +	ret = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
> +						  NULL);
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
> +				ret);
> +		return -ENOTSUP;
> +	}
> +
> +	/**
> +	 * If link is down and AUTO is enabled, AUTO is returned,
> +	 * otherwise, configured FEC mode is returned.
> +	 * If link is up, current FEC mode is returned.
> +	 */
> +	fec_config = abilities.fec_cfg_curr_mod_ext_info;
needs masking with I40E_AQ_PHY_FEC_CONFIG_MASK
> +	current_fec_mode = link_status.fec_info;
> +
> +	if (link_up) {
> +		switch (current_fec_mode) {
> +		case I40E_AQ_CONFIG_FEC_KR_ENA:
> +			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
> +			break;
> +		case I40E_AQ_CONFIG_FEC_RS_ENA:
> +			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
> +			break;
> +		case 0:
> +			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		return 0;
> +	}
> +
> +	if (fec_config & I40E_AQ_ENABLE_FEC_AUTO) {
> +		*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
> +		return 0;
> +	}
> +
> +	uint32_t temp_fec_capa = 0;
> +	if (fec_config & I40E_AQ_ENABLE_FEC_KR)
> +		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
> +	if (fec_config & I40E_AQ_ENABLE_FEC_RS)
> +		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
> +	if (temp_fec_capa == 0)
> +		temp_fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
> +
> +	*fec_capa = temp_fec_capa;
> +	return 0;
> +}
> +
> +static int
> +i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
> +{
> +	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct i40e_aq_get_phy_abilities_resp abilities = {0};
> +	struct i40e_aq_set_phy_config config = {0};
> +	enum i40e_status_code status;
> +	uint8_t req_fec = 0, fec_auto = 0, fec_kr = 0, fec_rs = 0;
> +
> +	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
> +	    hw->device_id != I40E_DEV_ID_25G_B &&
> +	    hw->device_id != I40E_DEV_ID_KX_X722) {
> +		return -ENOTSUP;
> +	}
> +
> +	if (hw->mac.type == I40E_MAC_X722 &&
> +	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
> +		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
> +			 " firmware. Please update the NVM image.\n");
> +		return -ENOTSUP;
> +	}
> +
> +	/**
> +	 * Copy the current user PHY configuration. The current user PHY
> +	 * configuration is initialized during probe from PHY capabilities
> +	 * software mode, and updated on set PHY configuration.
> +	 */
> +	if (fec_capa == 0)
> +		return -EINVAL;

not necessary since it was checked in rte_eth_fec_set().

But it's worth checking out if (fec_capa & 
~(RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)|RTE_ETH_FEC_MODE_CAPA_MASK(BASER)|RTE_ETH_FEC_MODE_CAPA_MASK(RS)) 
)

> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
> +		fec_auto = 1;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
> +		fec_kr = 1;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
> +		fec_rs = 1;
> +
> +	if (fec_auto) {
> +		if (hw->mac.type == I40E_MAC_X722) {
> +			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
> +			return -EINVAL;
> +		}
> +		if (fec_kr || fec_rs) {
> +			if (fec_kr)
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> +							I40E_AQ_SET_FEC_REQUEST_KR;
> +			if (fec_rs) {
> +				if (hw->mac.type == I40E_MAC_X722) {
> +					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
> +					return -EINVAL;
> +				}
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
> +							I40E_AQ_SET_FEC_REQUEST_RS;
shouldn't here be "req_fec |=" instead of "rec_fec ="? In case of both 
KR and RS, the latter rewrites the former.
> +			}
> +		} else {
> +			if (hw->mac.type == I40E_MAC_X722) {
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> +						  I40E_AQ_SET_FEC_REQUEST_KR;
> +			} else {
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> +						  I40E_AQ_SET_FEC_REQUEST_KR |
> +						  I40E_AQ_SET_FEC_ABILITY_RS |
> +						  I40E_AQ_SET_FEC_REQUEST_RS;
> +			}
> +		}
> +	} else {
> +		if (fec_kr ^ fec_rs) {
> +			if (fec_kr) {
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
> +							I40E_AQ_SET_FEC_REQUEST_KR;
> +			} else {
> +				if (hw->mac.type == I40E_MAC_X722) {
> +					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
> +					return -EINVAL;
> +				}
> +				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
> +							I40E_AQ_SET_FEC_REQUEST_RS;
> +			}
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Get the current phy config */
> +	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
> +					      NULL);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
> +				status);
> +		return -ENOTSUP;
> +	}
> +
> +	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
> +		config.phy_type = abilities.phy_type;
> +		config.abilities = abilities.abilities |
> +				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
> +		config.phy_type_ext = abilities.phy_type_ext;
> +		config.link_speed = abilities.link_speed;
> +		config.eee_capability = abilities.eee_capability;
> +		config.eeer = abilities.eeer_val;
> +		config.low_power_ctrl = abilities.d3_lpan;
> +		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
> +		status = i40e_aq_set_phy_config(hw, &config, NULL);
> +		if (status) {
> +			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
> +			status);
> +			return -ENOTSUP;
> +		}
> +	}
> +
> +	status = i40e_update_link_info(hw);
> +	if (status) {
> +		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
> +			status);
> +		return -ENOTSUP;
> +	}
> +
> +	return 0;
> +}
> +
>   RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
>   RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
>   #ifdef RTE_ETHDEV_DEBUG_RX

-- 
Regards,
Vladimir

[PATCH v6] net/i40e: support FEC feature

[Zhichao Zeng]

This patch enabled querying Forward Error Correction(FEC) capabilities,
set FEC mode and get current FEC mode functions.

Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

---
v6: fix some judgments
v5: fix some judgments
v4: fix some logic
v3: optimize code details
v2: update NIC feature document
---
 doc/guides/nics/features/i40e.ini      |   1 +
 doc/guides/rel_notes/release_24_07.rst |   5 +
 drivers/net/i40e/i40e_ethdev.c         | 239 +++++++++++++++++++++++++
 3 files changed, 245 insertions(+)

diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
index ef7514c44b..4610444ace 100644
--- a/doc/guides/nics/features/i40e.ini
+++ b/doc/guides/nics/features/i40e.ini
@@ -32,6 +32,7 @@ Traffic manager      = Y
 CRC offload          = Y
 VLAN offload         = Y
 QinQ offload         = P
+FEC                  = Y
 L3 checksum offload  = P
 L4 checksum offload  = P
 Inner L3 checksum    = P
diff --git a/doc/guides/rel_notes/release_24_07.rst b/doc/guides/rel_notes/release_24_07.rst
index 56c03ed6c9..9f63d77673 100644
--- a/doc/guides/rel_notes/release_24_07.rst
+++ b/doc/guides/rel_notes/release_24_07.rst
@@ -83,6 +83,11 @@ New Features
     without having to do a full handshake over a Unix Domain Socket
     with the Device Plugin.
 
+* **Updated Intel i40e driver.**
+
+  * Added support for configuring the Forward Error Correction(FEC) mode, querying
+  * FEC capabilities and current FEC mode from a device.
+
 * **Updated Intel ixgbe driver.**
 
   * Updated base code with E610 device family support.
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index c38515a758..f513ab1220 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
+static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.tm_ops_get                   = i40e_tm_ops_get,
 	.tx_done_cleanup              = i40e_tx_done_cleanup,
 	.get_monitor_addr             = i40e_get_monitor_addr,
+	.fec_get_capability           = i40e_fec_get_capability,
+	.fec_get                      = i40e_fec_get,
+	.fec_set                      = i40e_fec_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -12301,6 +12308,238 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
+	    hw->device_id == I40E_DEV_ID_25G_B) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+
+		/* since HW only supports 25G */
+		return 1;
+	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		return 1;
+	}
+
+	return -ENOTSUP;
+}
+
+static int
+i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_link_status link_status = {0};
+	uint8_t current_fec_mode = 0, fec_config = 0;
+	bool link_up, enable_lse;
+	int ret = 0;
+
+	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+	/* Get link info */
+	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
+	if (ret != I40E_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	link_up = link_status.link_info & I40E_AQ_LINK_UP;
+
+	ret = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+						  NULL);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	/**
+	 * If link is down and AUTO is enabled, AUTO is returned,
+	 * otherwise, configured FEC mode is returned.
+	 * If link is up, current FEC mode is returned.
+	 */
+	fec_config = abilities.fec_cfg_curr_mod_ext_info
+					& I40E_AQ_PHY_FEC_CONFIG_MASK;
+	current_fec_mode = link_status.fec_info;
+
+	if (link_up) {
+		switch (current_fec_mode) {
+		case I40E_AQ_CONFIG_FEC_KR_ENA:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
+			break;
+		case I40E_AQ_CONFIG_FEC_RS_ENA:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
+			break;
+		case 0:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
+			break;
+		default:
+			return -EINVAL;
+		}
+		return 0;
+	}
+
+	if (fec_config & I40E_AQ_ENABLE_FEC_AUTO) {
+		*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
+		return 0;
+	}
+
+	uint32_t temp_fec_capa = 0;
+	if (fec_config & I40E_AQ_ENABLE_FEC_KR)
+		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
+	if (configured_fec_cfg & I40E_AQ_ENABLE_FEC_RS)
+		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
+	if (configured_fec_cfg == 0)
+		temp_fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
+
+	*fec_capa = temp_fec_capa;
+	return 0;
+}
+
+static int
+i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_aq_set_phy_config config = {0};
+	enum i40e_status_code status;
+	uint8_t req_fec = 0, fec_auto = 0, fec_kr = 0, fec_rs = 0;
+
+	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
+	    hw->device_id != I40E_DEV_ID_25G_B &&
+	    hw->device_id != I40E_DEV_ID_KX_X722) {
+		return -ENOTSUP;
+	}
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	/**
+	 * Copy the current user PHY configuration. The current user PHY
+	 * configuration is initialized during probe from PHY capabilities
+	 * software mode, and updated on set PHY configuration.
+	 */
+	if (fec_capa & ~(RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+		RTE_ETH_FEC_MODE_CAPA_MASK(BASER) | RTE_ETH_FEC_MODE_CAPA_MASK(RS)))
+		return -EINVAL;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
+		fec_auto = 1;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
+		fec_kr = 1;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
+		fec_rs = 1;
+
+	if (fec_auto) {
+		if (hw->mac.type == I40E_MAC_X722) {
+			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
+			return -EINVAL;
+		}
+		if (fec_kr || fec_rs) {
+			if (fec_kr)
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+							I40E_AQ_SET_FEC_REQUEST_KR;
+			if (fec_rs) {
+				if (hw->mac.type == I40E_MAC_X722) {
+					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+					return -EINVAL;
+				}
+				req_fec |= I40E_AQ_SET_FEC_ABILITY_RS |
+							I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		} else {
+			if (hw->mac.type == I40E_MAC_X722) {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+						  I40E_AQ_SET_FEC_REQUEST_KR;
+			} else {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+						  I40E_AQ_SET_FEC_REQUEST_KR |
+						  I40E_AQ_SET_FEC_ABILITY_RS |
+						  I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		}
+	} else {
+		if (fec_kr ^ fec_rs) {
+			if (fec_kr) {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+							I40E_AQ_SET_FEC_REQUEST_KR;
+			} else {
+				if (hw->mac.type == I40E_MAC_X722) {
+					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+					return -EINVAL;
+				}
+				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
+							I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	/* Get the current phy config */
+	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+					      NULL);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				status);
+		return -ENOTSUP;
+	}
+
+	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
+		config.phy_type = abilities.phy_type;
+		config.abilities = abilities.abilities |
+				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
+		config.phy_type_ext = abilities.phy_type_ext;
+		config.link_speed = abilities.link_speed;
+		config.eee_capability = abilities.eee_capability;
+		config.eeer = abilities.eeer_val;
+		config.low_power_ctrl = abilities.d3_lpan;
+		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
+		status = i40e_aq_set_phy_config(hw, &config, NULL);
+		if (status) {
+			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+			return -ENOTSUP;
+		}
+	}
+
+	status = i40e_update_link_info(hw);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
 #ifdef RTE_ETHDEV_DEBUG_RX
-- 
2.34.1

[PATCH v6] net/i40e: support FEC feature

[Zhichao Zeng]

This patch enabled querying Forward Error Correction(FEC) capabilities,
set FEC mode and get current FEC mode functions.

Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

---
v6: fix some judgments
v5: fix some judgments
v4: fix some logic
v3: optimize code details
v2: update NIC feature document
---
 doc/guides/nics/features/i40e.ini      |   1 +
 doc/guides/rel_notes/release_24_07.rst |   5 +
 drivers/net/i40e/i40e_ethdev.c         | 239 +++++++++++++++++++++++++
 3 files changed, 245 insertions(+)

diff --git a/doc/guides/nics/features/i40e.ini b/doc/guides/nics/features/i40e.ini
index ef7514c44b..4610444ace 100644
--- a/doc/guides/nics/features/i40e.ini
+++ b/doc/guides/nics/features/i40e.ini
@@ -32,6 +32,7 @@ Traffic manager      = Y
 CRC offload          = Y
 VLAN offload         = Y
 QinQ offload         = P
+FEC                  = Y
 L3 checksum offload  = P
 L4 checksum offload  = P
 Inner L3 checksum    = P
diff --git a/doc/guides/rel_notes/release_24_07.rst b/doc/guides/rel_notes/release_24_07.rst
index 56c03ed6c9..9f63d77673 100644
--- a/doc/guides/rel_notes/release_24_07.rst
+++ b/doc/guides/rel_notes/release_24_07.rst
@@ -83,6 +83,11 @@ New Features
     without having to do a full handshake over a Unix Domain Socket
     with the Device Plugin.
 
+* **Updated Intel i40e driver.**
+
+  * Added support for configuring the Forward Error Correction(FEC) mode, querying
+  * FEC capabilities and current FEC mode from a device.
+
 * **Updated Intel ixgbe driver.**
 
   * Updated base code with E610 device family support.
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index c38515a758..f847bf82bc 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
+static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
 
 static const char *const valid_keys[] = {
 	ETH_I40E_FLOATING_VEB_ARG,
@@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.tm_ops_get                   = i40e_tm_ops_get,
 	.tx_done_cleanup              = i40e_tx_done_cleanup,
 	.get_monitor_addr             = i40e_get_monitor_addr,
+	.fec_get_capability           = i40e_fec_get_capability,
+	.fec_get                      = i40e_fec_get,
+	.fec_set                      = i40e_fec_set,
 };
 
 /* store statistics names and its offset in stats structure */
@@ -12301,6 +12308,238 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_fec_get_capability(struct rte_eth_dev *dev,
+	struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
+	    hw->device_id == I40E_DEV_ID_25G_B) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+
+		/* since HW only supports 25G */
+		return 1;
+	} else if (hw->device_id == I40E_DEV_ID_KX_X722) {
+		if (speed_fec_capa) {
+			speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+					     RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		return 1;
+	}
+
+	return -ENOTSUP;
+}
+
+static int
+i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_link_status link_status = {0};
+	uint8_t current_fec_mode = 0, fec_config = 0;
+	bool link_up, enable_lse;
+	int ret = 0;
+
+	enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+	/* Get link info */
+	ret = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
+	if (ret != I40E_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	link_up = link_status.link_info & I40E_AQ_LINK_UP;
+
+	ret = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+						  NULL);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	/**
+	 * If link is down and AUTO is enabled, AUTO is returned,
+	 * otherwise, configured FEC mode is returned.
+	 * If link is up, current FEC mode is returned.
+	 */
+	fec_config = abilities.fec_cfg_curr_mod_ext_info
+					& I40E_AQ_PHY_FEC_CONFIG_MASK;
+	current_fec_mode = link_status.fec_info;
+
+	if (link_up) {
+		switch (current_fec_mode) {
+		case I40E_AQ_CONFIG_FEC_KR_ENA:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
+			break;
+		case I40E_AQ_CONFIG_FEC_RS_ENA:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
+			break;
+		case 0:
+			*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
+			break;
+		default:
+			return -EINVAL;
+		}
+		return 0;
+	}
+
+	if (fec_config & I40E_AQ_ENABLE_FEC_AUTO) {
+		*fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_AUTO);
+		return 0;
+	}
+
+	uint32_t temp_fec_capa = 0;
+	if (fec_config & I40E_AQ_ENABLE_FEC_KR)
+		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_BASER);
+	if (fec_config & I40E_AQ_ENABLE_FEC_RS)
+		temp_fec_capa |= RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_RS);
+	if (temp_fec_capa == 0)
+		temp_fec_capa = RTE_ETH_FEC_MODE_TO_CAPA(RTE_ETH_FEC_NOFEC);
+
+	*fec_capa = temp_fec_capa;
+	return 0;
+}
+
+static int
+i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_aq_get_phy_abilities_resp abilities = {0};
+	struct i40e_aq_set_phy_config config = {0};
+	enum i40e_status_code status;
+	uint8_t req_fec = 0, fec_auto = 0, fec_kr = 0, fec_rs = 0;
+
+	if (hw->device_id != I40E_DEV_ID_25G_SFP28 &&
+	    hw->device_id != I40E_DEV_ID_25G_B &&
+	    hw->device_id != I40E_DEV_ID_KX_X722) {
+		return -ENOTSUP;
+	}
+
+	if (hw->mac.type == I40E_MAC_X722 &&
+	    !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+		PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+			 " firmware. Please update the NVM image.\n");
+		return -ENOTSUP;
+	}
+
+	/**
+	 * Copy the current user PHY configuration. The current user PHY
+	 * configuration is initialized during probe from PHY capabilities
+	 * software mode, and updated on set PHY configuration.
+	 */
+	if (fec_capa & ~(RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+		RTE_ETH_FEC_MODE_CAPA_MASK(BASER) | RTE_ETH_FEC_MODE_CAPA_MASK(RS)))
+		return -EINVAL;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
+		fec_auto = 1;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
+		fec_kr = 1;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
+		fec_rs = 1;
+
+	if (fec_auto) {
+		if (hw->mac.type == I40E_MAC_X722) {
+			PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: AUTO");
+			return -EINVAL;
+		}
+		if (fec_kr || fec_rs) {
+			if (fec_kr)
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+							I40E_AQ_SET_FEC_REQUEST_KR;
+			if (fec_rs) {
+				if (hw->mac.type == I40E_MAC_X722) {
+					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+					return -EINVAL;
+				}
+				req_fec |= I40E_AQ_SET_FEC_ABILITY_RS |
+							I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		} else {
+			if (hw->mac.type == I40E_MAC_X722) {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+						  I40E_AQ_SET_FEC_REQUEST_KR;
+			} else {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+						  I40E_AQ_SET_FEC_REQUEST_KR |
+						  I40E_AQ_SET_FEC_ABILITY_RS |
+						  I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		}
+	} else {
+		if (fec_kr ^ fec_rs) {
+			if (fec_kr) {
+				req_fec = I40E_AQ_SET_FEC_ABILITY_KR |
+							I40E_AQ_SET_FEC_REQUEST_KR;
+			} else {
+				if (hw->mac.type == I40E_MAC_X722) {
+					PMD_DRV_LOG(ERR, "X722 Unsupported FEC mode: RS");
+					return -EINVAL;
+				}
+				req_fec = I40E_AQ_SET_FEC_ABILITY_RS |
+							I40E_AQ_SET_FEC_REQUEST_RS;
+			}
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	/* Get the current phy config */
+	status = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
+					      NULL);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to get PHY capabilities: %d\n",
+				status);
+		return -ENOTSUP;
+	}
+
+	if (abilities.fec_cfg_curr_mod_ext_info != req_fec) {
+		config.phy_type = abilities.phy_type;
+		config.abilities = abilities.abilities |
+				   I40E_AQ_PHY_ENABLE_ATOMIC_LINK;
+		config.phy_type_ext = abilities.phy_type_ext;
+		config.link_speed = abilities.link_speed;
+		config.eee_capability = abilities.eee_capability;
+		config.eeer = abilities.eeer_val;
+		config.low_power_ctrl = abilities.d3_lpan;
+		config.fec_config = req_fec & I40E_AQ_PHY_FEC_CONFIG_MASK;
+		status = i40e_aq_set_phy_config(hw, &config, NULL);
+		if (status) {
+			PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+			return -ENOTSUP;
+		}
+	}
+
+	status = i40e_update_link_info(hw);
+	if (status) {
+		PMD_DRV_LOG(ERR, "Failed to set PHY capabilities: %d\n",
+			status);
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
 #ifdef RTE_ETHDEV_DEBUG_RX
-- 
2.34.1

Re: [PATCH v6] net/i40e: support FEC feature

[Medvedkin, Vladimir]

Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

On 02/07/2024 09:40, Zhichao Zeng wrote:
> This patch enabled querying Forward Error Correction(FEC) capabilities,
> set FEC mode and get current FEC mode functions.
>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
>
> ---
> v6: fix some judgments
> v5: fix some judgments
> v4: fix some logic
> v3: optimize code details
> v2: update NIC feature document
> ---
>   doc/guides/nics/features/i40e.ini      |   1 +
>   doc/guides/rel_notes/release_24_07.rst |   5 +
>   drivers/net/i40e/i40e_ethdev.c         | 239 +++++++++++++++++++++++++
>   3 files changed, 245 insertions(+)
>
-- 
Regards,
Vladimir

Re: [PATCH v6] net/i40e: support FEC feature

[Bruce Richardson]

On Wed, Jul 03, 2024 at 06:37:12PM +0100, Medvedkin, Vladimir wrote:
> 
> On 02/07/2024 09:40, Zhichao Zeng wrote:
> > This patch enabled querying Forward Error Correction(FEC) capabilities,
> > set FEC mode and get current FEC mode functions.
> > 
> > Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> > 
> Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

Applied to dpdk-next-net-intel.

Thanks,
/Bruce