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

[Stefan Bader]

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 --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xE8675DEECBEECEA3.asc
Type: application/pgp-keys
Size: 48643 bytes
Desc: OpenPGP public key
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20240110/92bb98e8/attachment-0001.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20240110/92bb98e8/attachment-0001.sig>