[PATCH] net/ice: support FEC feature

[PATCH] net/ice: support FEC feature

[Mingjin Ye]

This patch enable three Forward Error Correction(FEC) related ops
in ice driver. As no speed information can get from HW, this patch
only show FEC capability.

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 doc/guides/nics/features/ice.ini |   1 +
 doc/guides/nics/ice.rst          |   5 +
 drivers/net/ice/ice_ethdev.c     | 176 +++++++++++++++++++++++++++++++
 3 files changed, 182 insertions(+)

diff --git a/doc/guides/nics/features/ice.ini b/doc/guides/nics/features/ice.ini
index 62869ef0a0..a9be394696 100644
--- a/doc/guides/nics/features/ice.ini
+++ b/doc/guides/nics/features/ice.ini
@@ -9,6 +9,7 @@
 [Features]
 Speed capabilities   = Y
 Link speed configuration = Y
+FEC                  = Y
 Link status          = Y
 Link status event    = Y
 Rx interrupt         = Y
diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 3deeea9e6c..3d7e4ed7f1 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -323,6 +323,11 @@ The DCF PMD needs to advertise and acquire DCF capability which allows DCF to
 send AdminQ commands that it would like to execute over to the PF and receive
 responses for the same from PF.
 
+Forward Error Correction (FEC)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Supports get/set FEC mode and get FEC capability.
+
 Generic Flow Support
 ~~~~~~~~~~~~~~~~~~~~
 
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 87385d2649..56d0f2bb28 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -181,6 +181,10 @@ static int ice_timesync_read_time(struct rte_eth_dev *dev,
 static int ice_timesync_write_time(struct rte_eth_dev *dev,
 				   const struct timespec *timestamp);
 static int ice_timesync_disable(struct rte_eth_dev *dev);
+static int ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
+			   unsigned int num);
+static int ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
 static const uint32_t *ice_buffer_split_supported_hdr_ptypes_get(struct rte_eth_dev *dev,
 						size_t *no_of_elements);
 
@@ -298,6 +302,9 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
 	.timesync_write_time          = ice_timesync_write_time,
 	.timesync_disable             = ice_timesync_disable,
 	.tm_ops_get                   = ice_tm_ops_get,
+	.fec_get_capability           = ice_fec_get_capability,
+	.fec_get                      = ice_fec_get,
+	.fec_set                      = ice_fec_set,
 	.buffer_split_supported_hdr_ptypes_get = ice_buffer_split_supported_hdr_ptypes_get,
 };
 
@@ -6644,6 +6651,175 @@ ice_buffer_split_supported_hdr_ptypes_get(struct rte_eth_dev *dev __rte_unused,
 	return ptypes;
 }
 
+static int
+ice_fec_get_capa_num(struct ice_aqc_get_phy_caps_data *pcaps,
+			   struct rte_eth_fec_capa *speed_fec_capa)
+{
+	int num = 0;
+
+	if (!pcaps)
+		return ICE_ERR_NO_MEMORY;
+
+	if (pcaps->caps & ICE_AQC_PHY_EN_AUTO_FEC) {
+		if (speed_fec_capa)
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+		num++;
+	}
+
+	if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN ||
+	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ ||
+	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN ||
+	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_REQ) {
+		if (speed_fec_capa)
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+		num++;
+	}
+
+	if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_528_REQ ||
+	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_544_REQ ||
+	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN) {
+		if (speed_fec_capa)
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		num++;
+	}
+
+	if (pcaps->link_fec_options == 0) {
+		if (speed_fec_capa)
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+		num++;
+	}
+
+	return num;
+}
+
+static int
+ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
+			   unsigned int num)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_aqc_get_phy_caps_data *pcaps;
+	unsigned int capa_num;
+	int ret;
+
+	pcaps = (struct ice_aqc_get_phy_caps_data *)
+			ice_malloc(hw, sizeof(*pcaps));
+	if (!pcaps)
+		return ICE_ERR_NO_MEMORY;
+
+	ret = ice_aq_get_phy_caps(hw->port_info, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
+				  pcaps, NULL);
+	if (ret)
+		goto done;
+
+	/* first time to get capa_num */
+	capa_num = ice_fec_get_capa_num(pcaps, NULL);
+	if (!speed_fec_capa || num < capa_num) {
+		ret = capa_num;
+		goto done;
+	}
+
+	ret = ice_fec_get_capa_num(pcaps, speed_fec_capa);
+
+done:
+	ice_free(hw, pcaps);
+	return ret;
+}
+
+static int
+ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_port_info *pi = hw->port_info;
+	u32 temp_fec_capa = 0;
+	int ret = 0;
+
+	if (!pi)
+		return -ENOTSUP;
+
+	/* Get current FEC mode from port info */
+	switch (pi->phy.curr_user_fec_req) {
+	case ICE_FEC_NONE:
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+		break;
+	case ICE_FEC_AUTO:
+	case ICE_FEC_DIS_AUTO:
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+		break;
+	case ICE_FEC_BASER:
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+		break;
+	case ICE_FEC_RS:
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		break;
+	default:
+		ret = -ENOTSUP;
+		break;
+	}
+
+	*fec_capa = temp_fec_capa;
+	return ret;
+}
+
+static int
+ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_port_info *pi = hw->port_info;
+	struct ice_aqc_set_phy_cfg_data config = { 0 };
+	enum ice_fec_mode req_fec;
+	int ret = 0;
+
+	if (!pi)
+		return -ENOTSUP;
+
+	switch (fec_capa) {
+	case RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC):
+		req_fec = ICE_FEC_NONE;
+		break;
+	case RTE_ETH_FEC_MODE_CAPA_MASK(AUTO):
+		if (ice_fw_supports_fec_dis_auto(hw))
+			req_fec = ICE_FEC_DIS_AUTO;
+		else
+			req_fec = ICE_FEC_AUTO;
+		break;
+	case RTE_ETH_FEC_MODE_CAPA_MASK(BASER):
+		req_fec = ICE_FEC_BASER;
+		break;
+	case RTE_ETH_FEC_MODE_CAPA_MASK(RS):
+		req_fec = ICE_FEC_RS;
+		break;
+	default:
+		PMD_DRV_LOG(ERR, "Unsupported FEC mode: %d\n", fec_capa);
+		return -EINVAL;
+	}
+
+	/* Proceed only if requesting different FEC mode */
+	if (pi->phy.curr_user_fec_req == req_fec)
+		return 0;
+
+	/* 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.
+	 */
+	memcpy(&config, &pi->phy.curr_user_phy_cfg, sizeof(config));
+
+	ret = ice_cfg_phy_fec(pi, &config, req_fec);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "Failed to set FEC mode");
+		return -EINVAL;
+	}
+
+	config.caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
+
+	if (ice_aq_set_phy_cfg(pi->hw, pi, &config, NULL))
+		return -EAGAIN;
+
+	/* Save requested FEC config */
+	pi->phy.curr_user_fec_req = req_fec;
+
+	return 0;
+}
+
 static int
 ice_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	      struct rte_pci_device *pci_dev)
-- 
2.25.1

Re: [PATCH] net/ice: support FEC feature

[Medvedkin, Vladimir]

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

Hi Mingjin,

On 11/04/2024 10:45, Mingjin Ye wrote:
> This patch enable three Forward Error Correction(FEC) related ops
> in ice driver. As no speed information can get from HW, this patch
> only show FEC capability.
>
> Signed-off-by: Mingjin Ye<mingjinx.ye@intel.com>
> ---
>   doc/guides/nics/features/ice.ini |   1 +
>   doc/guides/nics/ice.rst          |   5 +
>   drivers/net/ice/ice_ethdev.c     | 176 +++++++++++++++++++++++++++++++
>   3 files changed, 182 insertions(+)
>
> diff --git a/doc/guides/nics/features/ice.ini b/doc/guides/nics/features/ice.ini
> index 62869ef0a0..a9be394696 100644
> --- a/doc/guides/nics/features/ice.ini
> +++ b/doc/guides/nics/features/ice.ini
> @@ -9,6 +9,7 @@
>   [Features]
>   Speed capabilities   = Y
>   Link speed configuration = Y
> +FEC                  = Y
>   Link status          = Y
>   Link status event    = Y
>   Rx interrupt         = Y
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index 3deeea9e6c..3d7e4ed7f1 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -323,6 +323,11 @@ The DCF PMD needs to advertise and acquire DCF capability which allows DCF to
>   send AdminQ commands that it would like to execute over to the PF and receive
>   responses for the same from PF.
>   
> +Forward Error Correction (FEC)
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Supports get/set FEC mode and get FEC capability.
> +
>   Generic Flow Support
>   ~~~~~~~~~~~~~~~~~~~~
>   
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 87385d2649..56d0f2bb28 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -181,6 +181,10 @@ static int ice_timesync_read_time(struct rte_eth_dev *dev,
>   static int ice_timesync_write_time(struct rte_eth_dev *dev,
>   				   const struct timespec *timestamp);
>   static int ice_timesync_disable(struct rte_eth_dev *dev);
> +static int ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
> +			   unsigned int num);
> +static int ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
> +static int ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
>   static const uint32_t *ice_buffer_split_supported_hdr_ptypes_get(struct rte_eth_dev *dev,
>   						size_t *no_of_elements);
>   
> @@ -298,6 +302,9 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
>   	.timesync_write_time          = ice_timesync_write_time,
>   	.timesync_disable             = ice_timesync_disable,
>   	.tm_ops_get                   = ice_tm_ops_get,
> +	.fec_get_capability           = ice_fec_get_capability,
> +	.fec_get                      = ice_fec_get,
> +	.fec_set                      = ice_fec_set,
>   	.buffer_split_supported_hdr_ptypes_get = ice_buffer_split_supported_hdr_ptypes_get,
>   };
>   
> @@ -6644,6 +6651,175 @@ ice_buffer_split_supported_hdr_ptypes_get(struct rte_eth_dev *dev __rte_unused,
>   	return ptypes;
>   }
>   
> +static int
> +ice_fec_get_capa_num(struct ice_aqc_get_phy_caps_data *pcaps,
> +			   struct rte_eth_fec_capa *speed_fec_capa)
> +{
> +	int num = 0;
> +
> +	if (!pcaps)
> +		return ICE_ERR_NO_MEMORY;

no need to check since it was checked before in ice_fec_get_capability

> +
> +	if (pcaps->caps & ICE_AQC_PHY_EN_AUTO_FEC) {
> +		if (speed_fec_capa)
> +			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> +		num++;
> +	}
> +
> +	if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN ||
> +	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ ||
> +	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN ||
> +	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_REQ) {
> +		if (speed_fec_capa)
> +			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +		num++;
> +	}
> +
> +	if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_528_REQ ||
> +	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_544_REQ ||
> +	    pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN) {
> +		if (speed_fec_capa)
> +			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		num++;
> +	}
> +
> +	if (pcaps->link_fec_options == 0) {
> +		if (speed_fec_capa)
> +			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> +		num++;
> +	}
> +
> +	return num;
> +}

here in this function above I see a number of problems:

1. according to API returning speed_fec_capa must have capabilities 
associated with corresponding link speed.

2. RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) is not an unique fec capability, if 
it is supported then it should be presented in capability bitmask for 
every speed

3. Same applied for RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC)

My suggestions here:

-  check for available speed in pcaps->eee_cap

- look at Table 3-20. Supported Electrical Modes to get supported FEC 
modes for a given speed

something like:

int auto = (pcaps->caps & ICE_AQC_PHY_EN_AUTO_FEC) ? 
RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) : 0;

int nofec = (pcaps->caps & ICE_AQC_PHY_FEC_DIS) ? 
RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) : 0;

if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_100BASE_TX) {

     speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_100M;

     speed_fec_capa[num++].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);

}

if (pcaps->eee_cap & 
(ICE_AQC_PHY_EEE_EN_1000BASE_T|ICE_AQC_PHY_EEE_EN_1000BASE_KX)) {

     speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_1G;

     speed_fec_capa[num++].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);

}

if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_10GBASE_T) {

     speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_10G;

     speed_fec_capa[num].capa = auto|nofec;

     if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN)

         speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);

     num++;

}

if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_25GBASE_KR) {

     speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_25G;

     speed_fec_capa[num].capa = auto|nofec;

     if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN)

         speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);

     if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)

         speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);

     num++;

}

if (pcaps->eee_cap & 
(ICE_AQC_PHY_EEE_EN_50GBASE_KR2|ICE_AQC_PHY_EEE_EN_50GBASE_KR_PAM4)) {

     speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_50G;

     speed_fec_capa[num].capa = auto|nofec;

     if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)

         speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);

     num++;

}

if (pcaps->eee_cap & 
(ICE_AQC_PHY_EEE_EN_100GBASE_KR4|ICE_AQC_PHY_EEE_EN_100GBASE_KR2_PAM4)) {

     speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_100G;

     speed_fec_capa[num].capa = auto|nofec;

     if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)

         speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);

     num++;

}

> +
> +static int
> +ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
> +			   unsigned int num)
> +{
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ice_aqc_get_phy_caps_data *pcaps;
> +	unsigned int capa_num;
> +	int ret;
> +
> +	pcaps = (struct ice_aqc_get_phy_caps_data *)
> +			ice_malloc(hw, sizeof(*pcaps));
> +	if (!pcaps)
> +		return ICE_ERR_NO_MEMORY;
wouldn't it be easier just to have this struct on the stack?
> +
> +	ret = ice_aq_get_phy_caps(hw->port_info, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
> +				  pcaps, NULL);
> +	if (ret)
should be (ret != ICE_SUCCESS) since this function returns enum ice_status
> +		goto done;
> +
> +	/* first time to get capa_num */
> +	capa_num = ice_fec_get_capa_num(pcaps, NULL);
> +	if (!speed_fec_capa || num < capa_num) {
> +		ret = capa_num;
> +		goto done;
> +	}
> +
> +	ret = ice_fec_get_capa_num(pcaps, speed_fec_capa);
> +
> +done:
> +	ice_free(hw, pcaps);
> +	return ret;
> +}
> +
> +static int
> +ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
> +{
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ice_port_info *pi = hw->port_info;
> +	u32 temp_fec_capa = 0;
> +	int ret = 0;
> +
> +	if (!pi)
> +		return -ENOTSUP;
> +
> +	/* Get current FEC mode from port info */
> +	switch (pi->phy.curr_user_fec_req) {
> +	case ICE_FEC_NONE:
> +		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> +		break;
> +	case ICE_FEC_AUTO:
> +	case ICE_FEC_DIS_AUTO:
> +		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> +		break;
> +	case ICE_FEC_BASER:
> +		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +		break;
> +	case ICE_FEC_RS:
> +		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +		break;
> +	default:
> +		ret = -ENOTSUP;
> +		break;
> +	}
> +
> +	*fec_capa = temp_fec_capa;
> +	return ret;
> +}

Thisfunctionneedsto berewrittentomeetAPIrequirements: "Get current 
Forward Error Correction(FEC) mode. 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."

So, if link is down - return AUTO or bitmask with supported capabilities 
(from ice_aqc_get_phy_caps_data)

If link is up - use your logic with switch (pi->phy.curr_user_fec_req) 
{} (but without case ICE_FEC_AUTO/ICE_FEC_DIS_AUTO, because we need to 
return current mode)

Also, check plz that curr_user_fec_req has relevant data. As I can see 
it is not updated anywhere in the code, since ice_cache_phy_user_req() 
is called only with ICE_FC_MODE. So probably you need to use 
corresponding AQ command to retrieve current fec mode from the HW.

Or it is probably better to read directly content of the "Table 3-40. 
Get Link Status Command Response Data Structure" instead of cached 
"pi->phy.curr_user_fec_req"

> +
> +static int
> +ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
> +{
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ice_port_info *pi = hw->port_info;
> +	struct ice_aqc_set_phy_cfg_data config = { 0 };
> +	enum ice_fec_mode req_fec;
> +	int ret = 0;
> +
> +	if (!pi)
> +		return -ENOTSUP;
> +
> +	switch (fec_capa) {
> +	case RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC):
> +		req_fec = ICE_FEC_NONE;
> +		break;
> +	case RTE_ETH_FEC_MODE_CAPA_MASK(AUTO):
> +		if (ice_fw_supports_fec_dis_auto(hw))
> +			req_fec = ICE_FEC_DIS_AUTO;
I'm not sure why it is here, could you clarify plz? Shouldn't 
ICE_FEC_DIS_AUTO be specified if RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)?
> +		else
> +			req_fec = ICE_FEC_AUTO;
> +		break;
> +	case RTE_ETH_FEC_MODE_CAPA_MASK(BASER):
> +		req_fec = ICE_FEC_BASER;
> +		break;
> +	case RTE_ETH_FEC_MODE_CAPA_MASK(RS):
> +		req_fec = ICE_FEC_RS;
> +		break;
> +	default:
> +		PMD_DRV_LOG(ERR, "Unsupported FEC mode: %d\n", fec_capa);
> +		return -EINVAL;
> +	}
> +
> +	/* Proceed only if requesting different FEC mode */
> +	if (pi->phy.curr_user_fec_req == req_fec)
> +		return 0;
> +
> +	/* 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.
> +	 */
> +	memcpy(&config, &pi->phy.curr_user_phy_cfg, sizeof(config));
> +
> +	ret = ice_cfg_phy_fec(pi, &config, req_fec);
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "Failed to set FEC mode");
> +		return -EINVAL;
> +	}
> +
> +	config.caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
> +
> +	if (ice_aq_set_phy_cfg(pi->hw, pi, &config, NULL))
> +		return -EAGAIN;
> +
> +	/* Save requested FEC config */
> +	pi->phy.curr_user_fec_req = req_fec;
> +
> +	return 0;
> +}

 From API documentation: "fec_capa    A bitmask of allowed FEC modes. If 
AUTO bit is set, other bits specify FEC modes which may be negotiated. 
If AUTO bit is clear, specify FEC modes to be used (only one valid mode 
per speed may be set)."

I think logic of this function should be rewritten to meet API requirements.

The goal of this function to set proper PHY config (see Table 3-28. Set 
PHY Config Command Data Structure), particularily:

Auto FEC Enable bit (if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)))

and Link FEC Options bits.

I'm not sure if current implementation of ice_cfg_phy_fec() fits 
perfectly here since it doesn't allow you to have different FEC modes 
(RS and BASER at the same time)

> +
>   static int
>   ice_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>   	      struct rte_pci_device *pci_dev)

-- 
Regards,
Vladimir

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

[PATCH v2] net/ice: support FEC feature

[Mingjin Ye]

This patch enable three Forward Error Correction(FEC) related ops
in ice driver. As no speed information can get from HW, this patch
only show FEC capability.

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v2: fix some logic
---
 doc/guides/nics/features/ice.ini |   1 +
 doc/guides/nics/ice.rst          |   5 +
 drivers/net/ice/ice_ethdev.c     | 292 +++++++++++++++++++++++++++++++
 3 files changed, 298 insertions(+)

diff --git a/doc/guides/nics/features/ice.ini b/doc/guides/nics/features/ice.ini
index 62869ef0a0..9c8569740a 100644
--- a/doc/guides/nics/features/ice.ini
+++ b/doc/guides/nics/features/ice.ini
@@ -11,6 +11,7 @@ Speed capabilities   = Y
 Link speed configuration = Y
 Link status          = Y
 Link status event    = Y
+FEC                  = Y
 Rx interrupt         = Y
 Fast mbuf free       = P
 Queue start/stop     = Y
diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 3deeea9e6c..3d7e4ed7f1 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -323,6 +323,11 @@ The DCF PMD needs to advertise and acquire DCF capability which allows DCF to
 send AdminQ commands that it would like to execute over to the PF and receive
 responses for the same from PF.
 
+Forward Error Correction (FEC)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Supports get/set FEC mode and get FEC capability.
+
 Generic Flow Support
 ~~~~~~~~~~~~~~~~~~~~
 
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 194109b0f6..3caacfa48a 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -181,6 +181,10 @@ static int ice_timesync_read_time(struct rte_eth_dev *dev,
 static int ice_timesync_write_time(struct rte_eth_dev *dev,
 				   const struct timespec *timestamp);
 static int ice_timesync_disable(struct rte_eth_dev *dev);
+static int ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
+			   unsigned int num);
+static int ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
 static const uint32_t *ice_buffer_split_supported_hdr_ptypes_get(struct rte_eth_dev *dev,
 						size_t *no_of_elements);
 
@@ -298,6 +302,9 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
 	.timesync_write_time          = ice_timesync_write_time,
 	.timesync_disable             = ice_timesync_disable,
 	.tm_ops_get                   = ice_tm_ops_get,
+	.fec_get_capability           = ice_fec_get_capability,
+	.fec_get                      = ice_fec_get,
+	.fec_set                      = ice_fec_set,
 	.buffer_split_supported_hdr_ptypes_get = ice_buffer_split_supported_hdr_ptypes_get,
 };
 
@@ -6677,6 +6684,291 @@ ice_buffer_split_supported_hdr_ptypes_get(struct rte_eth_dev *dev __rte_unused,
 	return ptypes;
 }
 
+static unsigned int
+ice_fec_get_capa_num(struct ice_aqc_get_phy_caps_data *pcaps,
+			   struct rte_eth_fec_capa *speed_fec_capa)
+{
+	unsigned int num = 0;
+	int auto_fec = (pcaps->caps & ICE_AQC_PHY_EN_AUTO_FEC) ?
+		RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) : 0;
+	int link_nofec = (pcaps->link_fec_options & ICE_AQC_PHY_FEC_DIS) ?
+		RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) : 0;
+
+	if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_100BASE_TX) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_100M;
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & (ICE_AQC_PHY_EEE_EN_1000BASE_T |
+		ICE_AQC_PHY_EEE_EN_1000BASE_KX)) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_1G;
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & (ICE_AQC_PHY_EEE_EN_10GBASE_T |
+		ICE_AQC_PHY_EEE_EN_10GBASE_KR)) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_10G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_25GBASE_KR) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_40GBASE_KR4) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_40G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & (ICE_AQC_PHY_EEE_EN_50GBASE_KR2 |
+		ICE_AQC_PHY_EEE_EN_50GBASE_KR_PAM4)) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_50G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & (ICE_AQC_PHY_EEE_EN_100GBASE_KR4 |
+		ICE_AQC_PHY_EEE_EN_100GBASE_KR2_PAM4)) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_100G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		num++;
+	}
+
+	return num;
+}
+
+static int
+ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
+			   unsigned int num)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_aqc_get_phy_caps_data pcaps = {0};
+	unsigned int capa_num;
+	int ret;
+
+	ret = ice_aq_get_phy_caps(hw->port_info, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
+				  &pcaps, NULL);
+	if (ret != ICE_SUCCESS)
+		goto done;
+
+	/* first time to get capa_num */
+	capa_num = ice_fec_get_capa_num(&pcaps, NULL);
+	if (!speed_fec_capa || num < capa_num) {
+		ret = capa_num;
+		goto done;
+	}
+
+	ret = ice_fec_get_capa_num(&pcaps, speed_fec_capa);
+
+done:
+	return ret;
+}
+
+static int
+ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+#define FEC_CAPA_NUM 10
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+	bool link_up;
+	u32 temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+	struct ice_link_status link_status = {0};
+	struct ice_aqc_get_phy_caps_data pcaps = {0};
+	struct ice_port_info *pi = hw->port_info;
+	u8 fec_config;
+	int ret;
+
+	if (!pi)
+		return -ENOTSUP;
+
+	ret = ice_get_link_info_safe(pf, enable_lse, &link_status);
+	if (ret != ICE_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
+			ret);
+		goto done;
+	}
+
+	link_up = link_status.link_info & ICE_AQ_LINK_UP;
+
+	ret = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
+				  &pcaps, NULL);
+	if (ret != ICE_SUCCESS)
+		goto done;
+
+	/* Get current FEC mode from port info */
+	if (link_up) {
+		switch (link_status.fec_info) {
+		case ICE_AQ_LINK_25G_KR_FEC_EN:
+			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+			break;
+		case ICE_AQ_LINK_25G_RS_528_FEC_EN:
+			/* fall-through */
+		case ICE_AQ_LINK_25G_RS_544_FEC_EN:
+			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+			break;
+		default:
+			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+			break;
+		}
+		goto done;
+	}
+
+	if (pcaps.caps & ICE_AQC_PHY_EN_AUTO_FEC) {
+		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+		return 0;
+	}
+
+	fec_config = pcaps.link_fec_options & ICE_AQC_PHY_FEC_MASK;
+
+	if (fec_config & (ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
+				ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
+				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
+				ICE_AQC_PHY_FEC_25G_KR_REQ))
+		temp_fec_capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+	if (fec_config & (ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
+				ICE_AQC_PHY_FEC_25G_RS_528_REQ |
+				ICE_AQC_PHY_FEC_25G_RS_544_REQ))
+		temp_fec_capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+
+done:
+	*fec_capa = temp_fec_capa;
+	return ret;
+}
+
+static int
+ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_port_info *pi = hw->port_info;
+	struct ice_aqc_set_phy_cfg_data cfg = { 0 };
+	bool fec_auto = false, fec_kr = false, fec_rs = false;
+
+	if (!pi)
+		return -ENOTSUP;
+
+	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;
+	/* 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.
+	 */
+	memcpy(&cfg, &pi->phy.curr_user_phy_cfg, sizeof(cfg));
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
+		fec_auto = true;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
+		fec_kr = true;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
+		fec_rs = true;
+
+	if (fec_auto) {
+		if (fec_kr || fec_rs) {
+			if (fec_rs) {
+				cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
+					ICE_AQC_PHY_FEC_25G_RS_528_REQ |
+					ICE_AQC_PHY_FEC_25G_RS_544_REQ;
+			}
+			if (fec_kr) {
+				cfg.link_fec_opt |= ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
+					ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
+					ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
+					ICE_AQC_PHY_FEC_25G_KR_REQ;
+			}
+		} else {
+			cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
+				ICE_AQC_PHY_FEC_25G_RS_528_REQ |
+				ICE_AQC_PHY_FEC_25G_RS_544_REQ |
+				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
+				ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
+				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
+				ICE_AQC_PHY_FEC_25G_KR_REQ;
+		}
+	} else {
+		if (fec_kr ^ fec_rs) {
+			if (fec_rs) {
+				cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
+					ICE_AQC_PHY_FEC_25G_RS_528_REQ |
+					ICE_AQC_PHY_FEC_25G_RS_544_REQ;
+			} else {
+				cfg.link_fec_opt = ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
+					ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
+					ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
+					ICE_AQC_PHY_FEC_25G_KR_REQ;
+			}
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	/* Recovery of accidentally rewritten bit */
+	if (pi->phy.curr_user_phy_cfg.link_fec_opt &
+		~ICE_AQC_PHY_FEC_MASK)
+		cfg.link_fec_opt |= ICE_AQC_PHY_FEC_DIS;
+	else
+		cfg.link_fec_opt &= ICE_AQC_PHY_FEC_MASK;
+
+	/* Proceed only if requesting different FEC mode */
+	if (pi->phy.curr_user_phy_cfg.link_fec_opt == cfg.link_fec_opt)
+		return 0;
+
+	cfg.caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
+
+	if (ice_aq_set_phy_cfg(pi->hw, pi, &cfg, NULL))
+		return -EAGAIN;
+
+	return 0;
+}
+
 static int
 ice_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	      struct rte_pci_device *pci_dev)
-- 
2.25.1

Re: [PATCH v2] net/ice: support FEC feature

[Medvedkin, Vladimir]

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

Hi Minjin,

- please update release notes

- see comments inline

On 02/07/2024 09:02, Mingjin Ye wrote:
> This patch enable three Forward Error Correction(FEC) related ops
> in ice driver. As no speed information can get from HW, this patch
> only show FEC capability.
>
> Signed-off-by: Qiming Yang<qiming.yang@intel.com>
> Signed-off-by: Mingjin Ye<mingjinx.ye@intel.com>
> ---
> v2: fix some logic
> ---
>   doc/guides/nics/features/ice.ini |   1 +
>   doc/guides/nics/ice.rst          |   5 +
>   drivers/net/ice/ice_ethdev.c     | 292 +++++++++++++++++++++++++++++++
>   3 files changed, 298 insertions(+)
>
<snip>
> +static int
> +ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
> +			   unsigned int num)
> +{
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ice_aqc_get_phy_caps_data pcaps = {0};
> +	unsigned int capa_num;
> +	int ret;
> +
> +	ret = ice_aq_get_phy_caps(hw->port_info, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
> +				  &pcaps, NULL);
> +	if (ret != ICE_SUCCESS)
> +		goto done;

This breaks API. Please redefine with some supported error status

And please get rid of goto here, it is not necessary anymore

> +
> +	/* first time to get capa_num */
> +	capa_num = ice_fec_get_capa_num(&pcaps, NULL);
> +	if (!speed_fec_capa || num < capa_num) {
> +		ret = capa_num;
> +		goto done;
> +	}
> +
> +	ret = ice_fec_get_capa_num(&pcaps, speed_fec_capa);
> +
> +done:
> +	return ret;
> +}
> +
> +static int
> +ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
> +{
> +#define FEC_CAPA_NUM 10
I believe this macro is not needed
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
> +	bool link_up;
> +	u32 temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> +	struct ice_link_status link_status = {0};
> +	struct ice_aqc_get_phy_caps_data pcaps = {0};
> +	struct ice_port_info *pi = hw->port_info;
> +	u8 fec_config;
> +	int ret;
> +
> +	if (!pi)
> +		return -ENOTSUP;
> +
> +	ret = ice_get_link_info_safe(pf, enable_lse, &link_status);
> +	if (ret != ICE_SUCCESS) {
> +		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
> +			ret);
> +		goto done;
Same problem here and below with API and returning error statuses. And I 
think it is worth to get rid of goto here in this function as well.
> +	}
> +
> +	link_up = link_status.link_info & ICE_AQ_LINK_UP;
> +
> +	ret = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
> +				  &pcaps, NULL);
> +	if (ret != ICE_SUCCESS)
> +		goto done;
> +
> +	/* Get current FEC mode from port info */
> +	if (link_up) {
> +		switch (link_status.fec_info) {
> +		case ICE_AQ_LINK_25G_KR_FEC_EN:
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +			break;
> +		case ICE_AQ_LINK_25G_RS_528_FEC_EN:
> +			/* fall-through */
> +		case ICE_AQ_LINK_25G_RS_544_FEC_EN:
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +			break;
> +		default:
> +			temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> +			break;
> +		}
> +		goto done;
> +	}
> +
> +	if (pcaps.caps & ICE_AQC_PHY_EN_AUTO_FEC) {
> +		temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> +		return 0;
> +	}
> +
> +	fec_config = pcaps.link_fec_options & ICE_AQC_PHY_FEC_MASK;
> +
> +	if (fec_config & (ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
> +				ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
> +				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
> +				ICE_AQC_PHY_FEC_25G_KR_REQ))
> +		temp_fec_capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +
> +	if (fec_config & (ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
> +				ICE_AQC_PHY_FEC_25G_RS_528_REQ |
> +				ICE_AQC_PHY_FEC_25G_RS_544_REQ))
> +		temp_fec_capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +
> +done:
> +	*fec_capa = temp_fec_capa;
> +	return ret;
> +}
> +
> +static int
> +ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
> +{
> +	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ice_port_info *pi = hw->port_info;
> +	struct ice_aqc_set_phy_cfg_data cfg = { 0 };
> +	bool fec_auto = false, fec_kr = false, fec_rs = false;
> +
> +	if (!pi)
> +		return -ENOTSUP;
> +
> +	if (fec_capa & ~(RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> +		RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> +		RTE_ETH_FEC_MODE_CAPA_MASK(RS)))
please use additional tab to indent properly according to dpdk coding style
> +		return -EINVAL;
> +	/* 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.
> +	 */
> +	memcpy(&cfg, &pi->phy.curr_user_phy_cfg, sizeof(cfg));
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
> +		fec_auto = true;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
> +		fec_kr = true;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
> +		fec_rs = true;
> +
> +	if (fec_auto) {
> +		if (fec_kr || fec_rs) {
> +			if (fec_rs) {
> +				cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
> +					ICE_AQC_PHY_FEC_25G_RS_528_REQ |
> +					ICE_AQC_PHY_FEC_25G_RS_544_REQ;
> +			}
> +			if (fec_kr) {
> +				cfg.link_fec_opt |= ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
> +					ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
> +					ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
> +					ICE_AQC_PHY_FEC_25G_KR_REQ;
> +			}
> +		} else {
> +			cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
> +				ICE_AQC_PHY_FEC_25G_RS_528_REQ |
> +				ICE_AQC_PHY_FEC_25G_RS_544_REQ |
> +				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
> +				ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
> +				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
> +				ICE_AQC_PHY_FEC_25G_KR_REQ;
> +		}
> +	} else {
> +		if (fec_kr ^ fec_rs) {
> +			if (fec_rs) {
> +				cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
> +					ICE_AQC_PHY_FEC_25G_RS_528_REQ |
> +					ICE_AQC_PHY_FEC_25G_RS_544_REQ;
> +			} else {
> +				cfg.link_fec_opt = ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
> +					ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
> +					ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
> +					ICE_AQC_PHY_FEC_25G_KR_REQ;
> +			}
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Recovery of accidentally rewritten bit */
> +	if (pi->phy.curr_user_phy_cfg.link_fec_opt &
> +		~ICE_AQC_PHY_FEC_MASK)

same indentation issue here, please add an extra tab

also it would be a bit more readable if you test link_fec_opt against 
ICE_AQC_PHY_FEC_DIS instead of ~ICE_AQC_PHY_FEC_MASK. Additionally no 
need to clean up this flag in cfg if it wasn't there in the first place.

> +		cfg.link_fec_opt |= ICE_AQC_PHY_FEC_DIS;
> +	else
> +		cfg.link_fec_opt &= ICE_AQC_PHY_FEC_MASK;
> +
> +	/* Proceed only if requesting different FEC mode */
> +	if (pi->phy.curr_user_phy_cfg.link_fec_opt == cfg.link_fec_opt)
> +		return 0;
> +
> +	cfg.caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
> +
> +	if (ice_aq_set_phy_cfg(pi->hw, pi, &cfg, NULL))
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
>   static int
>   ice_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>   	      struct rte_pci_device *pci_dev)

-- 
Regards,
Vladimir

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

[PATCH v3] net/ice: support FEC feature

[Mingjin Ye]

This patch enable three Forward Error Correction(FEC) related ops
in ice driver. As no speed information can get from HW, this patch
only show FEC capability.

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

---
v3: optimize code details
---
v2: fix some logic
---
 doc/guides/nics/features/ice.ini       |   1 +
 doc/guides/nics/ice.rst                |   5 +
 doc/guides/rel_notes/release_24_07.rst |   5 +
 drivers/net/ice/ice_ethdev.c           | 289 +++++++++++++++++++++++++
 4 files changed, 300 insertions(+)

diff --git a/doc/guides/nics/features/ice.ini b/doc/guides/nics/features/ice.ini
index 62869ef0a0..9c8569740a 100644
--- a/doc/guides/nics/features/ice.ini
+++ b/doc/guides/nics/features/ice.ini
@@ -11,6 +11,7 @@ Speed capabilities   = Y
 Link speed configuration = Y
 Link status          = Y
 Link status event    = Y
+FEC                  = Y
 Rx interrupt         = Y
 Fast mbuf free       = P
 Queue start/stop     = Y
diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 3deeea9e6c..3d7e4ed7f1 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -323,6 +323,11 @@ The DCF PMD needs to advertise and acquire DCF capability which allows DCF to
 send AdminQ commands that it would like to execute over to the PF and receive
 responses for the same from PF.
 
+Forward Error Correction (FEC)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Supports get/set FEC mode and get FEC capability.
+
 Generic Flow Support
 ~~~~~~~~~~~~~~~~~~~~
 
diff --git a/doc/guides/rel_notes/release_24_07.rst b/doc/guides/rel_notes/release_24_07.rst
index 56c03ed6c9..4867c5fc4a 100644
--- a/doc/guides/rel_notes/release_24_07.rst
+++ b/doc/guides/rel_notes/release_24_07.rst
@@ -87,6 +87,11 @@ New Features
 
   * Updated base code with E610 device family support.
 
+* **Updated Intel ice 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 disabling custom meta aura
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 194109b0f6..219fbfae30 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -181,6 +181,10 @@ static int ice_timesync_read_time(struct rte_eth_dev *dev,
 static int ice_timesync_write_time(struct rte_eth_dev *dev,
 				   const struct timespec *timestamp);
 static int ice_timesync_disable(struct rte_eth_dev *dev);
+static int ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
+			   unsigned int num);
+static int ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
 static const uint32_t *ice_buffer_split_supported_hdr_ptypes_get(struct rte_eth_dev *dev,
 						size_t *no_of_elements);
 
@@ -298,6 +302,9 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
 	.timesync_write_time          = ice_timesync_write_time,
 	.timesync_disable             = ice_timesync_disable,
 	.tm_ops_get                   = ice_tm_ops_get,
+	.fec_get_capability           = ice_fec_get_capability,
+	.fec_get                      = ice_fec_get,
+	.fec_set                      = ice_fec_set,
 	.buffer_split_supported_hdr_ptypes_get = ice_buffer_split_supported_hdr_ptypes_get,
 };
 
@@ -6677,6 +6684,288 @@ ice_buffer_split_supported_hdr_ptypes_get(struct rte_eth_dev *dev __rte_unused,
 	return ptypes;
 }
 
+static unsigned int
+ice_fec_get_capa_num(struct ice_aqc_get_phy_caps_data *pcaps,
+			   struct rte_eth_fec_capa *speed_fec_capa)
+{
+	unsigned int num = 0;
+	int auto_fec = (pcaps->caps & ICE_AQC_PHY_EN_AUTO_FEC) ?
+		RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) : 0;
+	int link_nofec = (pcaps->link_fec_options & ICE_AQC_PHY_FEC_DIS) ?
+		RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) : 0;
+
+	if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_100BASE_TX) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_100M;
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & (ICE_AQC_PHY_EEE_EN_1000BASE_T |
+		ICE_AQC_PHY_EEE_EN_1000BASE_KX)) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_1G;
+			speed_fec_capa[num].capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & (ICE_AQC_PHY_EEE_EN_10GBASE_T |
+		ICE_AQC_PHY_EEE_EN_10GBASE_KR)) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_10G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_25GBASE_KR) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_25G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & ICE_AQC_PHY_EEE_EN_40GBASE_KR4) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_40G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & (ICE_AQC_PHY_EEE_EN_50GBASE_KR2 |
+		ICE_AQC_PHY_EEE_EN_50GBASE_KR_PAM4)) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_50G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		num++;
+	}
+
+	if (pcaps->eee_cap & (ICE_AQC_PHY_EEE_EN_100GBASE_KR4 |
+		ICE_AQC_PHY_EEE_EN_100GBASE_KR2_PAM4)) {
+		if (speed_fec_capa) {
+			speed_fec_capa[num].speed = RTE_ETH_SPEED_NUM_100G;
+			speed_fec_capa[num].capa = auto_fec | link_nofec;
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+			if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN)
+				speed_fec_capa[num].capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+		}
+		num++;
+	}
+
+	return num;
+}
+
+static int
+ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
+			   unsigned int num)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_aqc_get_phy_caps_data pcaps = {0};
+	unsigned int capa_num;
+	int ret;
+
+	ret = ice_aq_get_phy_caps(hw->port_info, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
+				  &pcaps, NULL);
+	if (ret != ICE_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get capability information: %d\n",
+				ret);
+		return -ENOTSUP;
+	}
+
+	/* first time to get capa_num */
+	capa_num = ice_fec_get_capa_num(&pcaps, NULL);
+	if (!speed_fec_capa || num < capa_num)
+		return capa_num;
+
+	return ice_fec_get_capa_num(&pcaps, speed_fec_capa);
+}
+
+static int
+ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+	bool link_up;
+	u32 temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+	struct ice_link_status link_status = {0};
+	struct ice_aqc_get_phy_caps_data pcaps = {0};
+	struct ice_port_info *pi = hw->port_info;
+	u8 fec_config;
+	int ret;
+
+	if (!pi)
+		return -ENOTSUP;
+
+	ret = ice_get_link_info_safe(pf, enable_lse, &link_status);
+	if (ret != ICE_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
+			ret);
+		return -ENOTSUP;
+	}
+
+	link_up = link_status.link_info & ICE_AQ_LINK_UP;
+
+	ret = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
+				  &pcaps, NULL);
+	if (ret != ICE_SUCCESS) {
+		PMD_DRV_LOG(ERR, "Failed to get capability information: %d\n",
+			ret);
+		return -ENOTSUP;
+	}
+
+	/* Get current FEC mode from port info */
+	if (link_up) {
+		switch (link_status.fec_info) {
+		case ICE_AQ_LINK_25G_KR_FEC_EN:
+			*fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+			break;
+		case ICE_AQ_LINK_25G_RS_528_FEC_EN:
+			/* fall-through */
+		case ICE_AQ_LINK_25G_RS_544_FEC_EN:
+			*fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+			break;
+		default:
+			*fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+			break;
+		}
+		return 0;
+	}
+
+	if (pcaps.caps & ICE_AQC_PHY_EN_AUTO_FEC) {
+		*fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+		return 0;
+	}
+
+	fec_config = pcaps.link_fec_options & ICE_AQC_PHY_FEC_MASK;
+
+	if (fec_config & (ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
+				ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
+				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
+				ICE_AQC_PHY_FEC_25G_KR_REQ))
+		temp_fec_capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+
+	if (fec_config & (ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
+				ICE_AQC_PHY_FEC_25G_RS_528_REQ |
+				ICE_AQC_PHY_FEC_25G_RS_544_REQ))
+		temp_fec_capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+
+	*fec_capa = temp_fec_capa;
+	return 0;
+}
+
+static int
+ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
+{
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ice_port_info *pi = hw->port_info;
+	struct ice_aqc_set_phy_cfg_data cfg = { 0 };
+	bool fec_auto = false, fec_kr = false, fec_rs = false;
+
+	if (!pi)
+		return -ENOTSUP;
+
+	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;
+	/* 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.
+	 */
+	memcpy(&cfg, &pi->phy.curr_user_phy_cfg, sizeof(cfg));
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
+		fec_auto = true;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
+		fec_kr = true;
+
+	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
+		fec_rs = true;
+
+	if (fec_auto) {
+		if (fec_kr || fec_rs) {
+			if (fec_rs) {
+				cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
+					ICE_AQC_PHY_FEC_25G_RS_528_REQ |
+					ICE_AQC_PHY_FEC_25G_RS_544_REQ;
+			}
+			if (fec_kr) {
+				cfg.link_fec_opt |= ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
+					ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
+					ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
+					ICE_AQC_PHY_FEC_25G_KR_REQ;
+			}
+		} else {
+			cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
+				ICE_AQC_PHY_FEC_25G_RS_528_REQ |
+				ICE_AQC_PHY_FEC_25G_RS_544_REQ |
+				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
+				ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
+				ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
+				ICE_AQC_PHY_FEC_25G_KR_REQ;
+		}
+	} else {
+		if (fec_kr ^ fec_rs) {
+			if (fec_rs) {
+				cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
+					ICE_AQC_PHY_FEC_25G_RS_528_REQ |
+					ICE_AQC_PHY_FEC_25G_RS_544_REQ;
+			} else {
+				cfg.link_fec_opt = ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
+					ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
+					ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
+					ICE_AQC_PHY_FEC_25G_KR_REQ;
+			}
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	/* Recovery of accidentally rewritten bit */
+	if (pi->phy.curr_user_phy_cfg.link_fec_opt &
+			ICE_AQC_PHY_FEC_DIS)
+		cfg.link_fec_opt |= ICE_AQC_PHY_FEC_DIS;
+
+	/* Proceed only if requesting different FEC mode */
+	if (pi->phy.curr_user_phy_cfg.link_fec_opt == cfg.link_fec_opt)
+		return 0;
+
+	cfg.caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
+
+	if (ice_aq_set_phy_cfg(pi->hw, pi, &cfg, NULL))
+		return -EAGAIN;
+
+	return 0;
+}
+
 static int
 ice_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	      struct rte_pci_device *pci_dev)
-- 
2.25.1