Re: [PATCH iwl-next v4 02/10] ice: Add driver support for firmware changes for LAG

[Brett Creeley <bcreeley@amd.com>]

On 6/9/2023 2:16 PM, Dave Ertman wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> Add the defines, fields, and detection code for FW support of LAG for
> SRIOV.  Also exposes some previously static functions to allow access
> in the lag code.
> 
> Clean up code that is unused or not needed for LAG support.  Also add
> an ordered workqueue for processing LAG events.
> 
> Reviewed-by: Daniel Machon <daniel.machon@microchip.com>
> Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice.h          |  5 ++
>   .../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      | 53 ++++++++++---------
>   drivers/net/ethernet/intel/ice/ice_lib.c      |  2 +-
>   drivers/net/ethernet/intel/ice/ice_lib.h      |  1 +
>   drivers/net/ethernet/intel/ice/ice_main.c     | 12 +++++
>   drivers/net/ethernet/intel/ice/ice_type.h     |  2 +
>   8 files changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 9109006336f0..5ac0ad12f9f1 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -200,6 +200,8 @@ enum ice_feature {
>          ICE_F_PTP_EXTTS,
>          ICE_F_SMA_CTRL,
>          ICE_F_GNSS,
> +       ICE_F_ROCE_LAG,
> +       ICE_F_SRIOV_LAG,
>          ICE_F_MAX
>   };
> 
> @@ -569,6 +571,7 @@ struct ice_pf {
>          struct mutex sw_mutex;          /* lock for protecting VSI alloc flow */
>          struct mutex tc_mutex;          /* lock to protect TC changes */
>          struct mutex adev_mutex;        /* lock to protect aux device access */
> +       struct mutex lag_mutex;         /* protect ice_lag struct in PF */
>          u32 msg_enable;
>          struct ice_ptp ptp;
>          struct gnss_serial *gnss_serial;
> @@ -639,6 +642,8 @@ struct ice_pf {
>          struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
>   };
> 
> +extern struct workqueue_struct *ice_lag_wq;
> +
>   struct ice_netdev_priv {
>          struct ice_vsi *vsi;
>          struct ice_repr *repr;
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 6ea0d4c017f0..6b5ba9a02bad 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -120,6 +120,9 @@ struct ice_aqc_list_caps_elem {
>   #define ICE_AQC_CAPS_PCIE_RESET_AVOIDANCE              0x0076
>   #define ICE_AQC_CAPS_POST_UPDATE_RESET_RESTRICT                0x0077
>   #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 09e2e38d538e..6ddf607d2edf 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -2241,6 +2241,14 @@ ice_parse_common_caps(struct ice_hw *hw, struct ice_hw_common_caps *caps,
>                            "%s: reset_restrict_support = %d\n", prefix,
>                            caps->reset_restrict_support);
>                  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 5a7753bda324..73bfc5cd8b37 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lag.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lag.c
> @@ -4,8 +4,12 @@
>   /* Link Aggregation code */
> 
>   #include "ice.h"
> +#include "ice_lib.h"
>   #include "ice_lag.h"
> 
> +#define ICE_LAG_RES_SHARED     BIT(14)
> +#define ICE_LAG_RES_VALID      BIT(15)
> +
>   /**
>    * ice_lag_set_primary - set PF LAG state as Primary
>    * @lag: LAG info struct
> @@ -225,6 +229,26 @@ static void ice_lag_unregister(struct ice_lag *lag, struct net_device *netdev)
>          lag->role = ICE_LAG_NONE;
>   }
> 
> +/**
> + * ice_lag_check_nvm_support - Check for NVM support for LAG
> + * @pf: PF struct
> + */
> +static void ice_lag_check_nvm_support(struct ice_pf *pf)

Nit, but this name is a bit misleading to me. I would expect it to be 
called something like "ice_lag_init_feature_support_flag()" or something 
similar that better describes what the function is doing.

> +{
> +       struct ice_hw_dev_caps *caps;
> +
> +       caps = &pf->hw.dev_caps;

Nit, but since you are already creating a local variable you could go 
one level further to the common_cap, so it could be:

common_cap->roce_lag and common_cap->sriov_lag

> +       if (caps->common_cap.roce_lag)
> +               ice_set_feature_support(pf, ICE_F_ROCE_LAG);
> +       else
> +               ice_clear_feature_support(pf, ICE_F_ROCE_LAG);
> +
> +       if (caps->common_cap.sriov_lag)
> +               ice_set_feature_support(pf, ICE_F_SRIOV_LAG);
> +       else
> +               ice_clear_feature_support(pf, ICE_F_SRIOV_LAG);
> +}
> +

[...]