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