ACK/Cmnt: NAK: [PATCH 1/1][SRU][J] ice: alter feature support check for SRIOV and LAG
Tim Gardner
tim.gardner at canonical.com
Tue Jan 9 14:44:22 UTC 2024
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 at canonical.com
> <mailto:tim.gardner at canonical.com>> wrote:
>
> On 1/4/24 5:19 AM, Robert Malz wrote:
> > From: Dave Ertman <david.m.ertman at intel.com
> <mailto:david.m.ertman at 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 at intel.com
> <mailto:jesse.brandeburg at intel.com>>
> > Signed-off-by: Dave Ertman <david.m.ertman at intel.com
> <mailto:david.m.ertman at intel.com>>
> > Reviewed-by: Simon Horman <horms at kernel.org
> <mailto:horms at kernel.org>>
> > Tested-by: Pucha Himasekhar Reddy
> <himasekharx.reddy.pucha at intel.com
> <mailto:himasekharx.reddy.pucha at intel.com>> (A Contingent worker at
> Intel)
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen at intel.com
> <mailto:anthony.l.nguyen at intel.com>>
> >
> > (backported from commit 4d50fcdc2476eef94c14c6761073af5667bb43b6)
> > [rmalz: additionally backported necessary changes from bb52f42]
> > Signed-off-by: Robert Malz <robert.malz at canonical.com
> <mailto:robert.malz at 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 at canonical.com>
Perhaps this patch should be labeled as SAUCE since it so distorted. I'm
only ACKing this based on the test results.
--
-----------
Tim Gardner
Canonical, Inc
More information about the kernel-team
mailing list