NACK/Cmnt: [PATCH 1/1][SRU][J] ice: alter feature support check for SRIOV and LAG
Robert Malz
robert.malz at canonical.com
Wed Jan 10 11:15:02 UTC 2024
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 at canonical.com>
wrote:
> On 04.01.24 13:19, Robert Malz wrote:
> > From: Dave Ertman <david.m.ertman at 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 at intel.com>
> > Signed-off-by: Dave Ertman <david.m.ertman at intel.com>
> > Reviewed-by: Simon Horman <horms at kernel.org>
> > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha at intel.com>
> (A Contingent worker at Intel)
> > Signed-off-by: Tony Nguyen <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>
> > ---
> > 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20240110/f54aa619/attachment.html>
More information about the kernel-team
mailing list