NAK: [PATCH 1/1][SRU][J] ice: alter feature support check for SRIOV and LAG

[Tim Gardner]

On 1/4/24 5:19 AM, 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")
> 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;

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