| Message ID | 20240104121936.1436946-2-robert.malz@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Intel E810 transmit hang with bonding enabled | expand |
On 1/4/24 5:19 AM, Robert Malz wrote: > From: Dave Ertman <david.m.ertman@intel.com> > > BugLink: https://bugs.launchpad.net/bugs/2036239 > > Previously, the ice driver had support for using a handler for bonding > netdev events to ensure that conflicting features were not allowed to be > activated at the same time. While this was still in place, additional > support was added to specifically support SRIOV and LAG together. These > both utilized the netdev event handler, but the SRIOV and LAG feature was > behind a capabilities feature check to make sure the current NVM has > support. > > The exclusion part of the event handler should be removed since there are > users who have custom made solutions that depend on the non-exclusion of > features. > > Wrap the creation/registration and cleanup of the event handler and > associated structs in the probe flow with a feature check so that the > only systems that support the full implementation of LAG features will > initialize support. This will leave other systems unhindered with > functionality as it existed before any LAG code was added. > > Fixes: bb52f42acef6 ("ice: Add driver support for firmware changes for LAG") > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > Reviewed-by: Simon Horman <horms@kernel.org> > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel) > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > (backported from commit 4d50fcdc2476eef94c14c6761073af5667bb43b6) > [rmalz: additionally backported necessary changes from bb52f42] > Signed-off-by: Robert Malz <robert.malz@canonical.com> > --- > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 3 +++ > drivers/net/ethernet/intel/ice/ice_common.c | 8 ++++++++ > drivers/net/ethernet/intel/ice/ice_lag.c | 3 +++ > drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ > 4 files changed, 16 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > index 21b4c7cd6f05..9a721f9d38ee 100644 > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > @@ -118,6 +118,9 @@ struct ice_aqc_list_caps_elem { > #define ICE_AQC_CAPS_PENDING_NET_VER 0x004D > #define ICE_AQC_CAPS_RDMA 0x0051 > #define ICE_AQC_CAPS_NVM_MGMT 0x0080 > +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 > +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 > +#define ICE_AQC_BIT_SRIOV_LAG 0x02 > > u8 major_ver; > u8 minor_ver; > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c > index 3de6f16f985a..f8d9b2be26c8 100644 > --- a/drivers/net/ethernet/intel/ice/ice_common.c > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > @@ -2022,6 +2022,14 @@ ice_parse_common_caps(struct ice_hw *hw, struct ice_hw_common_caps *caps, > ice_debug(hw, ICE_DBG_INIT, "%s: max_mtu = %d\n", > prefix, caps->max_mtu); > break; > + case ICE_AQC_CAPS_FW_LAG_SUPPORT: > + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); > + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", > + prefix, caps->roce_lag); > + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); > + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", > + prefix, caps->sriov_lag); > + break; > default: > /* Not one of the recognized common capabilities */ > found = false; > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c > index 4f954db01b92..01030346398d 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lag.c > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c > @@ -391,6 +391,9 @@ int ice_init_lag(struct ice_pf *pf) > struct ice_vsi *vsi; > int err; > > + if (!pf->hw.dev_caps.common_cap.sriov_lag) > + return 0; > + > pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL); > if (!pf->lag) > return -ENOMEM; > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h > index d33d1906103c..fcee84c6fa6e 100644 > --- a/drivers/net/ethernet/intel/ice/ice_type.h > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > @@ -268,6 +268,8 @@ struct ice_hw_common_caps { > u8 dcb; > u8 ieee_1588; > u8 rdma; > + u8 roce_lag; > + u8 sriov_lag; > > bool nvm_update_pending_nvm; > bool nvm_update_pending_orom; I'm having some trouble with your explanation of the backports. I can find no vestige of commit 4d50fcdc2476eef94c14c6761073af5667bb43b6 ('ice: alter feature support check for SRIOV and LAG') in your patch. In fact, it appears that only pieces of commit bb52f42acef6ac317ee298d39909ce17bbaddb82 ('ice: Add driver support for firmware changes for LAG') have been backported. This would be much clearer if you made this into 2 patches and clearly explained what was going on with each backport.
Hey Tim, + if (!pf->hw.dev_caps.common_cap.sriov_lag) + return 0; is doing exactly the same thing as + if (!ice_is_feature_supported(pf, ICE_F_SRIOV_LAG)) + return 0; Only without defined ice_is_feature_supported function. Intention was to backport as little changes as possible to make logic the same. Since the patch is already small I assumed it doesn't need an additional separation. Let me know if that clarification is enough. Thanks, Robert On Mon, Jan 8, 2024 at 3:59 PM Tim Gardner <tim.gardner@canonical.com> wrote: > On 1/4/24 5:19 AM, Robert Malz wrote: > > From: Dave Ertman <david.m.ertman@intel.com> > > > > BugLink: https://bugs.launchpad.net/bugs/2036239 > > > > Previously, the ice driver had support for using a handler for bonding > > netdev events to ensure that conflicting features were not allowed to be > > activated at the same time. While this was still in place, additional > > support was added to specifically support SRIOV and LAG together. These > > both utilized the netdev event handler, but the SRIOV and LAG feature was > > behind a capabilities feature check to make sure the current NVM has > > support. > > > > The exclusion part of the event handler should be removed since there are > > users who have custom made solutions that depend on the non-exclusion of > > features. > > > > Wrap the creation/registration and cleanup of the event handler and > > associated structs in the probe flow with a feature check so that the > > only systems that support the full implementation of LAG features will > > initialize support. This will leave other systems unhindered with > > functionality as it existed before any LAG code was added. > > > > Fixes: bb52f42acef6 ("ice: Add driver support for firmware changes for > LAG") > > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > > Reviewed-by: Simon Horman <horms@kernel.org> > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> > (A Contingent worker at Intel) > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > > > (backported from commit 4d50fcdc2476eef94c14c6761073af5667bb43b6) > > [rmalz: additionally backported necessary changes from bb52f42] > > Signed-off-by: Robert Malz <robert.malz@canonical.com> > > --- > > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 3 +++ > > drivers/net/ethernet/intel/ice/ice_common.c | 8 ++++++++ > > drivers/net/ethernet/intel/ice/ice_lag.c | 3 +++ > > drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ > > 4 files changed, 16 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > index 21b4c7cd6f05..9a721f9d38ee 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > @@ -118,6 +118,9 @@ struct ice_aqc_list_caps_elem { > > #define ICE_AQC_CAPS_PENDING_NET_VER 0x004D > > #define ICE_AQC_CAPS_RDMA 0x0051 > > #define ICE_AQC_CAPS_NVM_MGMT 0x0080 > > +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 > > +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 > > +#define ICE_AQC_BIT_SRIOV_LAG 0x02 > > > > u8 major_ver; > > u8 minor_ver; > > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c > b/drivers/net/ethernet/intel/ice/ice_common.c > > index 3de6f16f985a..f8d9b2be26c8 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_common.c > > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > > @@ -2022,6 +2022,14 @@ ice_parse_common_caps(struct ice_hw *hw, struct > ice_hw_common_caps *caps, > > ice_debug(hw, ICE_DBG_INIT, "%s: max_mtu = %d\n", > > prefix, caps->max_mtu); > > break; > > + case ICE_AQC_CAPS_FW_LAG_SUPPORT: > > + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); > > + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", > > + prefix, caps->roce_lag); > > + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); > > + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", > > + prefix, caps->sriov_lag); > > + break; > > default: > > /* Not one of the recognized common capabilities */ > > found = false; > > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c > b/drivers/net/ethernet/intel/ice/ice_lag.c > > index 4f954db01b92..01030346398d 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_lag.c > > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c > > @@ -391,6 +391,9 @@ int ice_init_lag(struct ice_pf *pf) > > struct ice_vsi *vsi; > > int err; > > > > + if (!pf->hw.dev_caps.common_cap.sriov_lag) > > + return 0; > > + > > pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL); > > if (!pf->lag) > > return -ENOMEM; > > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h > b/drivers/net/ethernet/intel/ice/ice_type.h > > index d33d1906103c..fcee84c6fa6e 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_type.h > > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > > @@ -268,6 +268,8 @@ struct ice_hw_common_caps { > > u8 dcb; > > u8 ieee_1588; > > u8 rdma; > > + u8 roce_lag; > > + u8 sriov_lag; > > > > bool nvm_update_pending_nvm; > > bool nvm_update_pending_orom; > > I'm having some trouble with your explanation of the backports. I can > find no vestige of commit 4d50fcdc2476eef94c14c6761073af5667bb43b6 > ('ice: alter feature support check for SRIOV and LAG') in your patch. In > fact, it appears that only pieces of commit > bb52f42acef6ac317ee298d39909ce17bbaddb82 ('ice: Add driver support for > firmware changes for LAG') have been backported. > > This would be much clearer if you made this into 2 patches and clearly > explained what was going on with each backport. > -- > ----------- > Tim Gardner > Canonical, Inc > >
On 1/9/24 2:13 AM, Robert Malz wrote: > Hey Tim, > + if (!pf->hw.dev_caps.common_cap.sriov_lag) > + return 0; > > is doing exactly the same thing as > + if (!ice_is_feature_supported(pf, ICE_F_SRIOV_LAG)) > + return 0; > Only without defined ice_is_feature_supported function. Intention was to > backport as little changes as possible to make logic the same. > Since the patch is already small I assumed it doesn't need an additional > separation. Let me know if that clarification is enough. > > Thanks, > Robert > > On Mon, Jan 8, 2024 at 3:59 PM Tim Gardner <tim.gardner@canonical.com > <mailto:tim.gardner@canonical.com>> wrote: > > On 1/4/24 5:19 AM, Robert Malz wrote: > > From: Dave Ertman <david.m.ertman@intel.com > <mailto:david.m.ertman@intel.com>> > > > > BugLink: https://bugs.launchpad.net/bugs/2036239 > <https://bugs.launchpad.net/bugs/2036239> > > > > Previously, the ice driver had support for using a handler for > bonding > > netdev events to ensure that conflicting features were not > allowed to be > > activated at the same time. While this was still in place, > additional > > support was added to specifically support SRIOV and LAG > together. These > > both utilized the netdev event handler, but the SRIOV and LAG > feature was > > behind a capabilities feature check to make sure the current NVM has > > support. > > > > The exclusion part of the event handler should be removed since > there are > > users who have custom made solutions that depend on the > non-exclusion of > > features. > > > > Wrap the creation/registration and cleanup of the event handler and > > associated structs in the probe flow with a feature check so that the > > only systems that support the full implementation of LAG features > will > > initialize support. This will leave other systems unhindered with > > functionality as it existed before any LAG code was added. > > > > Fixes: bb52f42acef6 ("ice: Add driver support for firmware > changes for LAG") > > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com > <mailto:jesse.brandeburg@intel.com>> > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com > <mailto:david.m.ertman@intel.com>> > > Reviewed-by: Simon Horman <horms@kernel.org > <mailto:horms@kernel.org>> > > Tested-by: Pucha Himasekhar Reddy > <himasekharx.reddy.pucha@intel.com > <mailto:himasekharx.reddy.pucha@intel.com>> (A Contingent worker at > Intel) > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com > <mailto:anthony.l.nguyen@intel.com>> > > > > (backported from commit 4d50fcdc2476eef94c14c6761073af5667bb43b6) > > [rmalz: additionally backported necessary changes from bb52f42] > > Signed-off-by: Robert Malz <robert.malz@canonical.com > <mailto:robert.malz@canonical.com>> > > --- > > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 3 +++ > > drivers/net/ethernet/intel/ice/ice_common.c | 8 ++++++++ > > drivers/net/ethernet/intel/ice/ice_lag.c | 3 +++ > > drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ > > 4 files changed, 16 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > index 21b4c7cd6f05..9a721f9d38ee 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > @@ -118,6 +118,9 @@ struct ice_aqc_list_caps_elem { > > #define ICE_AQC_CAPS_PENDING_NET_VER 0x004D > > #define ICE_AQC_CAPS_RDMA 0x0051 > > #define ICE_AQC_CAPS_NVM_MGMT 0x0080 > > +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 > > +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 > > +#define ICE_AQC_BIT_SRIOV_LAG 0x02 > > > > u8 major_ver; > > u8 minor_ver; > > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c > b/drivers/net/ethernet/intel/ice/ice_common.c > > index 3de6f16f985a..f8d9b2be26c8 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_common.c > > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > > @@ -2022,6 +2022,14 @@ ice_parse_common_caps(struct ice_hw *hw, > struct ice_hw_common_caps *caps, > > ice_debug(hw, ICE_DBG_INIT, "%s: max_mtu = %d\n", > > prefix, caps->max_mtu); > > break; > > + case ICE_AQC_CAPS_FW_LAG_SUPPORT: > > + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); > > + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", > > + prefix, caps->roce_lag); > > + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); > > + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", > > + prefix, caps->sriov_lag); > > + break; > > default: > > /* Not one of the recognized common capabilities */ > > found = false; > > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c > b/drivers/net/ethernet/intel/ice/ice_lag.c > > index 4f954db01b92..01030346398d 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_lag.c > > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c > > @@ -391,6 +391,9 @@ int ice_init_lag(struct ice_pf *pf) > > struct ice_vsi *vsi; > > int err; > > > > + if (!pf->hw.dev_caps.common_cap.sriov_lag) > > + return 0; > > + > > pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL); > > if (!pf->lag) > > return -ENOMEM; > > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h > b/drivers/net/ethernet/intel/ice/ice_type.h > > index d33d1906103c..fcee84c6fa6e 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_type.h > > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > > @@ -268,6 +268,8 @@ struct ice_hw_common_caps { > > u8 dcb; > > u8 ieee_1588; > > u8 rdma; > > + u8 roce_lag; > > + u8 sriov_lag; > > > > bool nvm_update_pending_nvm; > > bool nvm_update_pending_orom; > > I'm having some trouble with your explanation of the backports. I can > find no vestige of commit 4d50fcdc2476eef94c14c6761073af5667bb43b6 > ('ice: alter feature support check for SRIOV and LAG') in your > patch. In > fact, it appears that only pieces of commit > bb52f42acef6ac317ee298d39909ce17bbaddb82 ('ice: Add driver support for > firmware changes for LAG') have been backported. > > This would be much clearer if you made this into 2 patches and clearly > explained what was going on with each backport. > -- > ----------- > Tim Gardner > Canonical, Inc > Acked-by: Tim Gardner <tim.gardner@canonical.com> Perhaps this patch should be labeled as SAUCE since it so distorted. I'm only ACKing this based on the test results.
On 04.01.24 13:19, Robert Malz wrote: > From: Dave Ertman <david.m.ertman@intel.com> > > BugLink: https://bugs.launchpad.net/bugs/2036239 > > Previously, the ice driver had support for using a handler for bonding > netdev events to ensure that conflicting features were not allowed to be > activated at the same time. While this was still in place, additional > support was added to specifically support SRIOV and LAG together. These > both utilized the netdev event handler, but the SRIOV and LAG feature was > behind a capabilities feature check to make sure the current NVM has > support. > > The exclusion part of the event handler should be removed since there are > users who have custom made solutions that depend on the non-exclusion of > features. > > Wrap the creation/registration and cleanup of the event handler and > associated structs in the probe flow with a feature check so that the > only systems that support the full implementation of LAG features will > initialize support. This will leave other systems unhindered with > functionality as it existed before any LAG code was added. > > Fixes: bb52f42acef6 ("ice: Add driver support for firmware changes for LAG") Rejected for the following reasons: - As this stands this would be totally confusing. The patch claims to fix some change which is not present in Jammy right now. - If I read this right you try to fix some problem which needs the driver support added and that has to be fixed. We rather prefer to use the proper upstream sequence of things. So this should be a 2 patch set (at least). This is important also because various processes check for certain patches being applied to decide whether a fix is needed. This would break badly when mushing patches together. -Stefan > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > Reviewed-by: Simon Horman <horms@kernel.org> > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel) > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > (backported from commit 4d50fcdc2476eef94c14c6761073af5667bb43b6) > [rmalz: additionally backported necessary changes from bb52f42] > Signed-off-by: Robert Malz <robert.malz@canonical.com> > --- > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 3 +++ > drivers/net/ethernet/intel/ice/ice_common.c | 8 ++++++++ > drivers/net/ethernet/intel/ice/ice_lag.c | 3 +++ > drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ > 4 files changed, 16 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > index 21b4c7cd6f05..9a721f9d38ee 100644 > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > @@ -118,6 +118,9 @@ struct ice_aqc_list_caps_elem { > #define ICE_AQC_CAPS_PENDING_NET_VER 0x004D > #define ICE_AQC_CAPS_RDMA 0x0051 > #define ICE_AQC_CAPS_NVM_MGMT 0x0080 > +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 > +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 > +#define ICE_AQC_BIT_SRIOV_LAG 0x02 > > u8 major_ver; > u8 minor_ver; > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c > index 3de6f16f985a..f8d9b2be26c8 100644 > --- a/drivers/net/ethernet/intel/ice/ice_common.c > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > @@ -2022,6 +2022,14 @@ ice_parse_common_caps(struct ice_hw *hw, struct ice_hw_common_caps *caps, > ice_debug(hw, ICE_DBG_INIT, "%s: max_mtu = %d\n", > prefix, caps->max_mtu); > break; > + case ICE_AQC_CAPS_FW_LAG_SUPPORT: > + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); > + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", > + prefix, caps->roce_lag); > + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); > + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", > + prefix, caps->sriov_lag); > + break; > default: > /* Not one of the recognized common capabilities */ > found = false; > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c > index 4f954db01b92..01030346398d 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lag.c > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c > @@ -391,6 +391,9 @@ int ice_init_lag(struct ice_pf *pf) > struct ice_vsi *vsi; > int err; > > + if (!pf->hw.dev_caps.common_cap.sriov_lag) > + return 0; > + > pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL); > if (!pf->lag) > return -ENOMEM; > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h > index d33d1906103c..fcee84c6fa6e 100644 > --- a/drivers/net/ethernet/intel/ice/ice_type.h > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > @@ -268,6 +268,8 @@ struct ice_hw_common_caps { > u8 dcb; > u8 ieee_1588; > u8 rdma; > + u8 roce_lag; > + u8 sriov_lag; > > bool nvm_update_pending_nvm; > bool nvm_update_pending_orom;
Thanks Stefan for comments, I mostly agree. Problem with upstream 4d50fcdc2476eef94c14c6761073af5667bb43b6 is that it doesn't really fix bb52f42acef6 (even though the commit message says so). Proper commit should be Fixes: df006dd (this is the patch which introduced LAG handler without NVM caps check) That's why I approached the path set this way but I understand that it was not clearly clarified. I'll properly backport all patches required for 4d50fcdc. Regards, Robert On Wed, Jan 10, 2024 at 9:38 AM Stefan Bader <stefan.bader@canonical.com> wrote: > On 04.01.24 13:19, Robert Malz wrote: > > From: Dave Ertman <david.m.ertman@intel.com> > > > > BugLink: https://bugs.launchpad.net/bugs/2036239 > > > > Previously, the ice driver had support for using a handler for bonding > > netdev events to ensure that conflicting features were not allowed to be > > activated at the same time. While this was still in place, additional > > support was added to specifically support SRIOV and LAG together. These > > both utilized the netdev event handler, but the SRIOV and LAG feature was > > behind a capabilities feature check to make sure the current NVM has > > support. > > > > The exclusion part of the event handler should be removed since there are > > users who have custom made solutions that depend on the non-exclusion of > > features. > > > > Wrap the creation/registration and cleanup of the event handler and > > associated structs in the probe flow with a feature check so that the > > only systems that support the full implementation of LAG features will > > initialize support. This will leave other systems unhindered with > > functionality as it existed before any LAG code was added. > > > > Fixes: bb52f42acef6 ("ice: Add driver support for firmware changes for > LAG") > > Rejected for the following reasons: > - As this stands this would be totally confusing. The patch claims to > fix some > change which is not present in Jammy right now. > - If I read this right you try to fix some problem which needs the > driver support > added and that has to be fixed. We rather prefer to use the proper > upstream > sequence of things. So this should be a 2 patch set (at least). > This is important also because various processes check for certain > patches > being applied to decide whether a fix is needed. This would break > badly when > mushing patches together. > > -Stefan > > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > > Reviewed-by: Simon Horman <horms@kernel.org> > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> > (A Contingent worker at Intel) > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > > > (backported from commit 4d50fcdc2476eef94c14c6761073af5667bb43b6) > > [rmalz: additionally backported necessary changes from bb52f42] > > Signed-off-by: Robert Malz <robert.malz@canonical.com> > > --- > > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 3 +++ > > drivers/net/ethernet/intel/ice/ice_common.c | 8 ++++++++ > > drivers/net/ethernet/intel/ice/ice_lag.c | 3 +++ > > drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ > > 4 files changed, 16 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > index 21b4c7cd6f05..9a721f9d38ee 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > @@ -118,6 +118,9 @@ struct ice_aqc_list_caps_elem { > > #define ICE_AQC_CAPS_PENDING_NET_VER 0x004D > > #define ICE_AQC_CAPS_RDMA 0x0051 > > #define ICE_AQC_CAPS_NVM_MGMT 0x0080 > > +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 > > +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 > > +#define ICE_AQC_BIT_SRIOV_LAG 0x02 > > > > u8 major_ver; > > u8 minor_ver; > > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c > b/drivers/net/ethernet/intel/ice/ice_common.c > > index 3de6f16f985a..f8d9b2be26c8 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_common.c > > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > > @@ -2022,6 +2022,14 @@ ice_parse_common_caps(struct ice_hw *hw, struct > ice_hw_common_caps *caps, > > ice_debug(hw, ICE_DBG_INIT, "%s: max_mtu = %d\n", > > prefix, caps->max_mtu); > > break; > > + case ICE_AQC_CAPS_FW_LAG_SUPPORT: > > + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); > > + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", > > + prefix, caps->roce_lag); > > + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); > > + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", > > + prefix, caps->sriov_lag); > > + break; > > default: > > /* Not one of the recognized common capabilities */ > > found = false; > > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c > b/drivers/net/ethernet/intel/ice/ice_lag.c > > index 4f954db01b92..01030346398d 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_lag.c > > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c > > @@ -391,6 +391,9 @@ int ice_init_lag(struct ice_pf *pf) > > struct ice_vsi *vsi; > > int err; > > > > + if (!pf->hw.dev_caps.common_cap.sriov_lag) > > + return 0; > > + > > pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL); > > if (!pf->lag) > > return -ENOMEM; > > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h > b/drivers/net/ethernet/intel/ice/ice_type.h > > index d33d1906103c..fcee84c6fa6e 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_type.h > > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > > @@ -268,6 +268,8 @@ struct ice_hw_common_caps { > > u8 dcb; > > u8 ieee_1588; > > u8 rdma; > > + u8 roce_lag; > > + u8 sriov_lag; > > > > bool nvm_update_pending_nvm; > > bool nvm_update_pending_orom; > > -- > - Stefan > >
On 10.01.24 12:15, Robert Malz wrote: > Thanks Stefan for comments, I mostly agree. > Problem with upstream 4d50fcdc2476eef94c14c6761073af5667bb43b6 is that > it doesn't really fix bb52f42acef6 (even though the commit message says so). > Proper commit should be Fixes: df006dd (this is the patch which > introduced LAG handler without NVM caps check) > That's why I approached the path set this way but I understand that it > was not clearly clarified. > > I'll properly backport all patches required for 4d50fcdc. Hi Robert, that is normal procedure. The whole set will be bundled in the changelog by BugLink (the bug report). Sometimes it is a challenge to decide which prerequisites to include or when to adjust a backport to work around further requirements. As a rule of thumb I would say it is acceptable to adjust or drop things partially on backport but nor to add code from a previous patch. -Stefan PS: If you could consider subscribing to the mailing list you might not miss general replies and I would not have to moderate each submission. ;-) > > Regards, > Robert > > > On Wed, Jan 10, 2024 at 9:38 AM Stefan Bader <stefan.bader@canonical.com > <mailto:stefan.bader@canonical.com>> wrote: > > On 04.01.24 13:19, Robert Malz wrote: > > From: Dave Ertman <david.m.ertman@intel.com > <mailto:david.m.ertman@intel.com>> > > > > BugLink: https://bugs.launchpad.net/bugs/2036239 > <https://bugs.launchpad.net/bugs/2036239> > > > > Previously, the ice driver had support for using a handler for > bonding > > netdev events to ensure that conflicting features were not > allowed to be > > activated at the same time. While this was still in place, > additional > > support was added to specifically support SRIOV and LAG > together. These > > both utilized the netdev event handler, but the SRIOV and LAG > feature was > > behind a capabilities feature check to make sure the current NVM has > > support. > > > > The exclusion part of the event handler should be removed since > there are > > users who have custom made solutions that depend on the > non-exclusion of > > features. > > > > Wrap the creation/registration and cleanup of the event handler and > > associated structs in the probe flow with a feature check so that the > > only systems that support the full implementation of LAG features > will > > initialize support. This will leave other systems unhindered with > > functionality as it existed before any LAG code was added. > > > > Fixes: bb52f42acef6 ("ice: Add driver support for firmware > changes for LAG") > > Rejected for the following reasons: > - As this stands this would be totally confusing. The patch claims to > fix some > change which is not present in Jammy right now. > - If I read this right you try to fix some problem which needs the > driver support > added and that has to be fixed. We rather prefer to use the proper > upstream > sequence of things. So this should be a 2 patch set (at least). > This is important also because various processes check for certain > patches > being applied to decide whether a fix is needed. This would break > badly when > mushing patches together. > > -Stefan > > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com > <mailto:jesse.brandeburg@intel.com>> > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com > <mailto:david.m.ertman@intel.com>> > > Reviewed-by: Simon Horman <horms@kernel.org > <mailto:horms@kernel.org>> > > Tested-by: Pucha Himasekhar Reddy > <himasekharx.reddy.pucha@intel.com > <mailto:himasekharx.reddy.pucha@intel.com>> (A Contingent worker at > Intel) > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com > <mailto:anthony.l.nguyen@intel.com>> > > > > (backported from commit 4d50fcdc2476eef94c14c6761073af5667bb43b6) > > [rmalz: additionally backported necessary changes from bb52f42] > > Signed-off-by: Robert Malz <robert.malz@canonical.com > <mailto:robert.malz@canonical.com>> > > --- > > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 3 +++ > > drivers/net/ethernet/intel/ice/ice_common.c | 8 ++++++++ > > drivers/net/ethernet/intel/ice/ice_lag.c | 3 +++ > > drivers/net/ethernet/intel/ice/ice_type.h | 2 ++ > > 4 files changed, 16 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > index 21b4c7cd6f05..9a721f9d38ee 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > > @@ -118,6 +118,9 @@ struct ice_aqc_list_caps_elem { > > #define ICE_AQC_CAPS_PENDING_NET_VER 0x004D > > #define ICE_AQC_CAPS_RDMA 0x0051 > > #define ICE_AQC_CAPS_NVM_MGMT 0x0080 > > +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 > > +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 > > +#define ICE_AQC_BIT_SRIOV_LAG 0x02 > > > > u8 major_ver; > > u8 minor_ver; > > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c > b/drivers/net/ethernet/intel/ice/ice_common.c > > index 3de6f16f985a..f8d9b2be26c8 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_common.c > > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > > @@ -2022,6 +2022,14 @@ ice_parse_common_caps(struct ice_hw *hw, > struct ice_hw_common_caps *caps, > > ice_debug(hw, ICE_DBG_INIT, "%s: max_mtu = %d\n", > > prefix, caps->max_mtu); > > break; > > + case ICE_AQC_CAPS_FW_LAG_SUPPORT: > > + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); > > + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", > > + prefix, caps->roce_lag); > > + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); > > + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", > > + prefix, caps->sriov_lag); > > + break; > > default: > > /* Not one of the recognized common capabilities */ > > found = false; > > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c > b/drivers/net/ethernet/intel/ice/ice_lag.c > > index 4f954db01b92..01030346398d 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_lag.c > > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c > > @@ -391,6 +391,9 @@ int ice_init_lag(struct ice_pf *pf) > > struct ice_vsi *vsi; > > int err; > > > > + if (!pf->hw.dev_caps.common_cap.sriov_lag) > > + return 0; > > + > > pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL); > > if (!pf->lag) > > return -ENOMEM; > > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h > b/drivers/net/ethernet/intel/ice/ice_type.h > > index d33d1906103c..fcee84c6fa6e 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_type.h > > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > > @@ -268,6 +268,8 @@ struct ice_hw_common_caps { > > u8 dcb; > > u8 ieee_1588; > > u8 rdma; > > + u8 roce_lag; > > + u8 sriov_lag; > > > > bool nvm_update_pending_nvm; > > bool nvm_update_pending_orom; > > -- > - Stefan >
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h index 21b4c7cd6f05..9a721f9d38ee 100644 --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h @@ -118,6 +118,9 @@ struct ice_aqc_list_caps_elem { #define ICE_AQC_CAPS_PENDING_NET_VER 0x004D #define ICE_AQC_CAPS_RDMA 0x0051 #define ICE_AQC_CAPS_NVM_MGMT 0x0080 +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 +#define ICE_AQC_BIT_SRIOV_LAG 0x02 u8 major_ver; u8 minor_ver; diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 3de6f16f985a..f8d9b2be26c8 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -2022,6 +2022,14 @@ ice_parse_common_caps(struct ice_hw *hw, struct ice_hw_common_caps *caps, ice_debug(hw, ICE_DBG_INIT, "%s: max_mtu = %d\n", prefix, caps->max_mtu); break; + case ICE_AQC_CAPS_FW_LAG_SUPPORT: + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", + prefix, caps->roce_lag); + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", + prefix, caps->sriov_lag); + break; default: /* Not one of the recognized common capabilities */ found = false; diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c index 4f954db01b92..01030346398d 100644 --- a/drivers/net/ethernet/intel/ice/ice_lag.c +++ b/drivers/net/ethernet/intel/ice/ice_lag.c @@ -391,6 +391,9 @@ int ice_init_lag(struct ice_pf *pf) struct ice_vsi *vsi; int err; + if (!pf->hw.dev_caps.common_cap.sriov_lag) + return 0; + pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL); if (!pf->lag) return -ENOMEM; diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index d33d1906103c..fcee84c6fa6e 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -268,6 +268,8 @@ struct ice_hw_common_caps { u8 dcb; u8 ieee_1588; u8 rdma; + u8 roce_lag; + u8 sriov_lag; bool nvm_update_pending_nvm; bool nvm_update_pending_orom;