Hi Vladimir: Thanks for your comments, some responses and questions are inline.
> From: Medvedkin, Vladimir <vladimir.medved...@intel.com> > Sent: Monday, March 11, 2024 11:59 PM > To: Zeng, ZhichaoX <zhichaox.z...@intel.com>; dev@dpdk.org > Cc: Cui, KaixinX <kaixinx....@intel.com>; Yang, Qiming > <qiming.y...@intel.com>; Zhang, Yuying <yuying.zh...@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.y...@intel.com > Signed-off-by: Zhichao Zeng mailto:zhichaox.z...@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
