| Message ID | 20220823150438.3613327-3-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: support FEC automatic disable | expand |
On Tue, 23 Aug 2022 08:04:38 -0700 Jacob Keller wrote: > The default Link Establishment State Machine (LESM) behavior does not LESM is the algo as specified by the IEEE standard? If so could you add the citation (section of the spec where it's defined)? Is disabling the only customization we may want? > allow the use of FEC disabled if the media does not support FEC > disabled. However users may want to override this behavior. > > To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a request > to automatically select an appropriate FEC mode including potentially > disabling FEC. > > This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM > to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because > FEC_OFF will always disable FEC without any LESM automatic selection. > > This *does* mean that ice is now accepting one "bitwise OR" set for FEC > configuration, which is somewhat against the recommendations made in > 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not > sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make any > sense here. > > With this change, users can opt to allow automatic FEC disable via > > ethtool --set-fec ethX encoding auto off
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Tuesday, August 23, 2022 3:18 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Greenwalt, Paul <paul.greenwalt@intel.com> > Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled > via ETHTOOL_SFECPARAM > > On Tue, 23 Aug 2022 08:04:38 -0700 Jacob Keller wrote: > > The default Link Establishment State Machine (LESM) behavior does not > > LESM is the algo as specified by the IEEE standard? If so could you add > the citation (section of the spec where it's defined)? > Hm. I am not sure if its part of IEEE standard or if its just the name we used for that section of our firmware. I'll get back on this and if its not part of a standard I'll rephrase this a bit so thats clear. > Is disabling the only customization we may want? > Of that, I'm not sure. > > allow the use of FEC disabled if the media does not support FEC > > disabled. However users may want to override this behavior. > > > > To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a > request > > to automatically select an appropriate FEC mode including potentially > > disabling FEC. > > > > This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM > > to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because > > FEC_OFF will always disable FEC without any LESM automatic selection. > > > > This *does* mean that ice is now accepting one "bitwise OR" set for FEC > > configuration, which is somewhat against the recommendations made in > > 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not > > sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make > any > > sense here. > > > > With this change, users can opt to allow automatic FEC disable via > > > > ethtool --set-fec ethX encoding auto off
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Tuesday, August 23, 2022 3:18 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Greenwalt, Paul <paul.greenwalt@intel.com> > Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled > via ETHTOOL_SFECPARAM > > On Tue, 23 Aug 2022 08:04:38 -0700 Jacob Keller wrote: > > The default Link Establishment State Machine (LESM) behavior does not > > LESM is the algo as specified by the IEEE standard? If so could you add > the citation (section of the spec where it's defined)? > > Is disabling the only customization we may want? > Ok I got information from the other folks here. LESM is not a standard its just the name we used internally for how the firmware establishes link. I'll rephrase this whole section and clarify it. I also clarified whats going on here in https://lore.kernel.org/netdev/CO1PR11MB50895C42C04A408CF6151023D6739@CO1PR11MB5089.namprd11.prod.outlook.com/ Basically: the firmware has a process for automatically selecting FEC mode. On older firmware, this process didn't include the possibility of selecting "No FEC", or in other words of disabling FEC. The process firmware uses is a state machine that goes through the FEC modes known to be supported by the media type. Some of our customers were confused about this and have asked if it was possible to allow disabling FEC. This is distinct from manually setting FEC_OFF, because it lets the firmwares existing state machine determine of the disabled mode is suitable or not. I understand it as the goal of being able to say "automatically select FEC for me, but if No FEC is suitable allow that". The new firmware requires manually opting in with a new bit, and we don't want to change existing behavior, hence the new approach to using both AUTO and OFF together. If we instead go with "use the new mode when its available" then there is no way for users to know this easily since it would just depend on the firmware version. I've got a proposed reword to the commit message as follows: Users can request automatic selection of FEC mode via the ETHTOOL_MSG_FEC_SET netlink message (or ETHTOOL_SFECPARAM ioctl). The ice driver implements this by asking firmware to select a suitable mode. The firmware selects a FEC mode automatically based on a table of supported FEC modes for the media type. Older versions of firmware will never select "No FEC" (i.e. disabling FEC). Newer versions of firmware support an automatic mode which also allows selecting the "No FEC" mode. To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a request to automatically select an appropriate FEC mode including potentially disabling FEC. This is done so that the existing behavior of ETHTOOL_FEC_AUTO remains unchanged, and users must actively select the new behavior. This is important since we do not want to change the behavior purely based on the firmware version. Additionally, this allows reporting an error if the mode is requested on a device still operating the older firmware version. This is distinct from ETHTOOL_FEC_OFF because that selection will always simply disable FEC without going through the firmware automatic selection state machine. This *does* mean that ice is now accepting one "bitwise OR" set for FEC configuration, which is somewhat against the recommendations made in 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make any sense here. Is that a better explanation of the reasoning? Thanks, Jake > > allow the use of FEC disabled if the media does not support FEC > > disabled. However users may want to override this behavior. > > > > To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a > request > > to automatically select an appropriate FEC mode including potentially > > disabling FEC. > > > > This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM > > to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because > > FEC_OFF will always disable FEC without any LESM automatic selection. > > > > This *does* mean that ice is now accepting one "bitwise OR" set for FEC > > configuration, which is somewhat against the recommendations made in > > 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not > > sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make > any > > sense here. > > > > With this change, users can opt to allow automatic FEC disable via > > > > ethtool --set-fec ethX encoding auto off
On Wed, 24 Aug 2022 21:29:31 +0000 Keller, Jacob E wrote: > Ok I got information from the other folks here. LESM is not a > standard its just the name we used internally for how the firmware > establishes link. I'll rephrase this whole section and clarify it. Hold up, I'm pretty sure this is in the standard.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Wednesday, August 24, 2022 3:47 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Greenwalt, Paul <paul.greenwalt@intel.com> > Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled > via ETHTOOL_SFECPARAM > > On Wed, 24 Aug 2022 21:29:31 +0000 Keller, Jacob E wrote: > > Ok I got information from the other folks here. LESM is not a > > standard its just the name we used internally for how the firmware > > establishes link. I'll rephrase this whole section and clarify it. > > Hold up, I'm pretty sure this is in the standard. According to the folks I talked to what we have here, we didn't understand this as being from a standard, but if it is I'd love to read on it. Thanks, Jake
On Wed, 24 Aug 2022 22:53:44 +0000 Keller, Jacob E wrote: > > On Wed, 24 Aug 2022 21:29:31 +0000 Keller, Jacob E wrote: > > > Ok I got information from the other folks here. LESM is not a > > > standard its just the name we used internally for how the firmware > > > establishes link. I'll rephrase this whole section and clarify it. > > > > Hold up, I'm pretty sure this is in the standard. > > According to the folks I talked to what we have here, we didn't > understand this as being from a standard, but if it is I'd love to > read on it. Table 110C–1—Host and cable assembly combinations in IEEE 802.3 2018, that's what I was thinking of.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Wednesday, August 24, 2022 4:03 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Greenwalt, Paul <paul.greenwalt@intel.com> > Subject: Re: [PATCH net-next 2/2] ice: add support for Auto FEC with FEC disabled > via ETHTOOL_SFECPARAM > > On Wed, 24 Aug 2022 22:53:44 +0000 Keller, Jacob E wrote: > > > On Wed, 24 Aug 2022 21:29:31 +0000 Keller, Jacob E wrote: > > > > Ok I got information from the other folks here. LESM is not a > > > > standard its just the name we used internally for how the firmware > > > > establishes link. I'll rephrase this whole section and clarify it. > > > > > > Hold up, I'm pretty sure this is in the standard. > > > > According to the folks I talked to what we have here, we didn't > > understand this as being from a standard, but if it is I'd love to > > read on it. > > Table 110C–1—Host and cable assembly combinations > in IEEE 802.3 2018, that's what I was thinking of. Ah. I am not sure if the state machine in firmware uses this table or not, but my guess is that it does. Thanks, Jake
On Wed, 24 Aug 2022 23:13:47 +0000 Keller, Jacob E wrote: > > > According to the folks I talked to what we have here, we didn't > > > understand this as being from a standard, but if it is I'd love to > > > read on it. > > > > Table 110C–1—Host and cable assembly combinations > > in IEEE 802.3 2018, that's what I was thinking of. > > Ah. I am not sure if the state machine in firmware uses this table or > not, but my guess is that it does. Yeah, I thought that's what AUTO means, otherwise if everyone did their own thing we wouldn't get link up without real autoneg enabled, right? Now the table only specifies minimum FEC requirements, in your case (and perhaps for others) No FEC does not get used, presumably the selection is only done between R and RS?
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h index 9d2fa67d873e..5c5ebd70483c 100644 --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h @@ -1123,6 +1123,7 @@ struct ice_aqc_get_phy_caps_data { #define ICE_AQC_PHY_FEC_25G_RS_528_REQ BIT(2) #define ICE_AQC_PHY_FEC_25G_KR_REQ BIT(3) #define ICE_AQC_PHY_FEC_25G_RS_544_REQ BIT(4) +#define ICE_AQC_PHY_FEC_DIS BIT(5) #define ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN BIT(6) #define ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN BIT(7) #define ICE_AQC_PHY_FEC_MASK ICE_M(0xdf, 0) diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 2c8c3fcc3dcd..96f51b7c9898 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -3253,8 +3253,11 @@ enum ice_fc_mode ice_caps_to_fc_mode(u8 caps) */ enum ice_fec_mode ice_caps_to_fec_mode(u8 caps, u8 fec_options) { - if (caps & ICE_AQC_PHY_EN_AUTO_FEC) + if (caps & ICE_AQC_PHY_EN_AUTO_FEC) { + if (fec_options & ICE_AQC_PHY_FEC_DIS) + return ICE_FEC_DIS_AUTO; return ICE_FEC_AUTO; + } if (fec_options & (ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN | ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ | @@ -3513,6 +3516,12 @@ ice_cfg_phy_fec(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg, /* Clear all FEC option bits. */ cfg->link_fec_opt &= ~ICE_AQC_PHY_FEC_MASK; break; + case ICE_FEC_DIS_AUTO: + /* Set No FEC and auto FEC */ + if (!ice_fw_supports_fec_dis_auto(hw)) + return -EOPNOTSUPP; + cfg->link_fec_opt |= ICE_AQC_PHY_FEC_DIS; + fallthrough; case ICE_FEC_AUTO: /* AND auto FEC bit, and all caps bits. */ cfg->caps &= ICE_AQC_PHY_CAPS_MASK; @@ -5289,6 +5298,35 @@ ice_aq_get_gpio(struct ice_hw *hw, u16 gpio_ctrl_handle, u8 pin_idx, return 0; } +/** + * ice_is_fw_min_ver + * @hw: pointer to the hardware structure + * @branch: branch version + * @maj: major version + * @min: minor version + * @patch: patch version + * + * Checks if the firmware is minimum version + */ +static bool ice_is_fw_min_ver(struct ice_hw *hw, u8 branch, u8 maj, u8 min, + u8 patch) +{ + if (hw->fw_branch == branch) { + if (hw->fw_maj_ver > maj) + return true; + if (hw->fw_maj_ver == maj) { + if (hw->fw_min_ver > min) + return true; + if (hw->fw_min_ver == min && hw->fw_patch >= patch) + return true; + } + } else if (hw->fw_branch > branch) { + return true; + } + + return false; +} + /** * ice_fw_supports_link_override * @hw: pointer to the hardware structure @@ -5510,3 +5548,17 @@ bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw) } return false; } + +/** + * ice_fw_supports_fec_dis_auto + * @hw: pointer to the hardware structure + * + * Checks if the firmware supports FEC disable in Auto FEC mode + */ +bool ice_fw_supports_fec_dis_auto(struct ice_hw *hw) +{ + return ice_is_fw_min_ver(hw, ICE_FW_FEC_DIS_AUTO_BRANCH, + ICE_FW_FEC_DIS_AUTO_MAJ, + ICE_FW_FEC_DIS_AUTO_MIN, + ICE_FW_FEC_DIS_AUTO_PATCH); +} diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h index f339bdc48062..a0baf80df356 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.h +++ b/drivers/net/ethernet/intel/ice/ice_common.h @@ -110,6 +110,7 @@ int ice_aq_set_phy_cfg(struct ice_hw *hw, struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg, struct ice_sq_cd *cd); bool ice_fw_supports_link_override(struct ice_hw *hw); +bool ice_fw_supports_fec_dis_auto(struct ice_hw *hw); int ice_get_link_default_override(struct ice_link_default_override_tlv *ldo, struct ice_port_info *pi); diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c index 770101577a94..a83b127a00b0 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -1020,9 +1020,17 @@ ice_set_fecparam(struct net_device *netdev, struct ethtool_fecparam *fecparam, { struct ice_netdev_priv *np = netdev_priv(netdev); struct ice_vsi *vsi = np->vsi; + struct ice_pf *pf = vsi->back; enum ice_fec_mode fec; switch (fecparam->fec) { + case ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF: + if (!ice_fw_supports_fec_dis_auto(&pf->hw)) { + NL_SET_ERR_MSG_MOD(extack, "FEC automatic disable is not supported by current firmware\n"); + return -EOPNOTSUPP; + } + fec = ICE_FEC_DIS_AUTO; + break; case ETHTOOL_FEC_AUTO: fec = ICE_FEC_AUTO; break; @@ -1037,8 +1045,7 @@ ice_set_fecparam(struct net_device *netdev, struct ethtool_fecparam *fecparam, fec = ICE_FEC_NONE; break; default: - dev_warn(ice_pf_to_dev(vsi->back), "Unsupported FEC mode: %d\n", - fecparam->fec); + NL_SET_ERR_MSG_MOD(extack, "Requested FEC mode is not supported\n"); return -EINVAL; } diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 3dbd91d8d83e..98fc40f20abd 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -2190,7 +2190,8 @@ static int ice_configure_phy(struct ice_vsi *vsi) ice_cfg_phy_fec(pi, cfg, phy->curr_user_fec_req); /* Can't provide what was requested; use PHY capabilities */ - if (cfg->link_fec_opt != + if (phy->curr_user_fec_req != ICE_FEC_DIS_AUTO && + cfg->link_fec_opt != (cfg->link_fec_opt & pcaps->link_fec_options)) { cfg->caps |= pcaps->caps & ICE_AQC_PHY_EN_AUTO_FEC; cfg->link_fec_opt = pcaps->link_fec_options; diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index e1abfcee96dc..0b71c40e881b 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -107,7 +107,8 @@ enum ice_fec_mode { ICE_FEC_NONE = 0, ICE_FEC_RS, ICE_FEC_BASER, - ICE_FEC_AUTO + ICE_FEC_AUTO, + ICE_FEC_DIS_AUTO }; struct ice_phy_cache_mode_data { @@ -1145,4 +1146,10 @@ struct ice_aq_get_set_rss_lut_params { #define ICE_FW_API_REPORT_DFLT_CFG_MIN 7 #define ICE_FW_API_REPORT_DFLT_CFG_PATCH 3 +/* FW version for FEC disable in Auto FEC mode */ +#define ICE_FW_FEC_DIS_AUTO_BRANCH 1 +#define ICE_FW_FEC_DIS_AUTO_MAJ 7 +#define ICE_FW_FEC_DIS_AUTO_MIN 0 +#define ICE_FW_FEC_DIS_AUTO_PATCH 5 + #endif /* _ICE_TYPE_H_ */
The default Link Establishment State Machine (LESM) behavior does not allow the use of FEC disabled if the media does not support FEC disabled. However users may want to override this behavior. To support this, accept the ETHTOOL_FEC_AUTO | ETHTOOL_FEC_OFF as a request to automatically select an appropriate FEC mode including potentially disabling FEC. This is distinct from ETHTOOL_FEC_AUTO because that will not allow the LESM to select FEC disabled. It is distinct from ETHTOOL_FEC_OFF because FEC_OFF will always disable FEC without any LESM automatic selection. This *does* mean that ice is now accepting one "bitwise OR" set for FEC configuration, which is somewhat against the recommendations made in 6dbf94b264e6 ("ethtool: clarify the ethtool FEC interface"), but I am not sure if the addition of an entirely new ETHTOOL_FEC_AUTO_DIS would make any sense here. With this change, users can opt to allow automatic FEC disable via ethtool --set-fec ethX encoding auto off Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Cc: Jakub Kicinski <kuba@kernel.org> --- .../net/ethernet/intel/ice/ice_adminq_cmd.h | 1 + drivers/net/ethernet/intel/ice/ice_common.c | 54 ++++++++++++++++++- drivers/net/ethernet/intel/ice/ice_common.h | 1 + drivers/net/ethernet/intel/ice/ice_ethtool.c | 11 +++- drivers/net/ethernet/intel/ice/ice_main.c | 3 +- drivers/net/ethernet/intel/ice/ice_type.h | 9 +++- 6 files changed, 74 insertions(+), 5 deletions(-)