<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> Hi Minjin, - please update release notes - see comments inline <div class="moz-cite-prefix">On 02/07/2024 09:02, Mingjin Ye wrote: </div> <blockquote type="cite" cite="mid:20240702080244.1190884-1-mingjinx.ye@intel.com"> <pre class="moz-quote-pre" wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:qiming.yang@intel.com"><qiming.yang@intel.com></a> Signed-off-by: Mingjin Ye <a class="moz-txt-link-rfc2396E" href="mailto:mingjinx.ye@intel.com"><mingjinx.ye@intel.com></a> --- 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(+) </pre> </blockquote> <snip> <blockquote type="cite" cite="mid:20240702080244.1190884-1-mingjinx.ye@intel.com"> <pre class="moz-quote-pre" wrap="">+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;</pre> </blockquote> This breaks API. Please redefine with some supported error status And please get rid of goto here, it is not necessary anymore <blockquote type="cite" cite="mid:20240702080244.1190884-1-mingjinx.ye@intel.com"> <pre class="moz-quote-pre" wrap=""> + + /* 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</pre> </blockquote> I believe this macro is not needed <blockquote type="cite" cite="mid:20240702080244.1190884-1-mingjinx.ye@intel.com"> <pre class="moz-quote-pre" wrap=""> + 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;</pre> </blockquote> 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. <blockquote type="cite" cite="mid:20240702080244.1190884-1-mingjinx.ye@intel.com"> <pre class="moz-quote-pre" wrap=""> + } + + 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)))</pre> </blockquote> please use additional tab to indent properly according to dpdk coding style <blockquote type="cite" cite="mid:20240702080244.1190884-1-mingjinx.ye@intel.com"> <pre class="moz-quote-pre" wrap=""> + 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)</pre> </blockquote> 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. <blockquote type="cite" cite="mid:20240702080244.1190884-1-mingjinx.ye@intel.com"> <pre class="moz-quote-pre" wrap=""> + 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) </pre> </blockquote> <pre class="moz-signature" cols="72">-- Regards, Vladimir</pre> </body> </html>