Date: Sat, 13 May 2023 13:18:14 +0200
From: Michal Schmidt <mschmidt@...hat.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>, david.m.ertman@...el.com
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org, 
	aleksander.lobakin@...el.com
Subject: Re: [PATCH iwl-next] ice: Remove LAG+SRIOV mutual exclusion

On Fri, May 12, 2023 at 11:08 AM Wojciech Drewek
<wojciech.drewek@...el.com> wrote:
>
> From: Dave Ertman <david.m.ertman@...el.com>
>
> There was a change previously to stop SR-IOV and LAG from existing on the
> same interface.  This was to prevent the violation of LACP (Link
> Aggregation Control Protocol).  The method to achieve this was to add a
> no-op Rx handler onto the netdev when SR-IOV VFs were present, thus
> blocking bonding, bridging, etc from claiming the interface by adding
> its own Rx handler.  Also, when an interface was added into a aggregate,
> then the SR-IOV capability was set to false.
>
> There are some users that have in house solutions using both SR-IOV and
> bridging/bonding that this method interferes with (e.g. creating duplicate
> VFs on the bonded interfaces and failing between them when the interface
> fails over).
>
> It makes more sense to provide the most functionality
> possible, the restriction on co-existence of these features will be
> removed.  No additional functionality is currently being provided beyond
> what existed before the co-existence restriction was put into place.  It is
> up to the end user to not implement a solution that would interfere with
> existing network protocols.
>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> Signed-off-by: Dave Ertman <david.m.ertman@...el.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> ---
>  .../device_drivers/ethernet/intel/ice.rst     | 18 -------
>  drivers/net/ethernet/intel/ice/ice.h          | 19 -------
>  drivers/net/ethernet/intel/ice/ice_lag.c      | 12 -----
>  drivers/net/ethernet/intel/ice/ice_lag.h      | 53 -------------------
>  drivers/net/ethernet/intel/ice/ice_lib.c      |  2 -
>  drivers/net/ethernet/intel/ice/ice_sriov.c    |  4 --
>  6 files changed, 108 deletions(-)

I see that both Alexander Lobakin's and my feedback about the previous
version has been accomodated.
And thanks for updating the Documentation too.

Reviewed-by: Michal Schmidt <mschmidt@...hat.com>

> diff --git a/Documentation/networking/device_drivers/ethernet/intel/ice.rst b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
> index 69695e5511f4..e4d065c55ea8 100644
> --- a/Documentation/networking/device_drivers/ethernet/intel/ice.rst
> +++ b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
> @@ -84,24 +84,6 @@ Once the VM shuts down, or otherwise releases the VF, the command will
>  complete.
>
>
> -Important notes for SR-IOV and Link Aggregation
> ------------------------------------------------
> -Link Aggregation is mutually exclusive with SR-IOV.
> -
> -- If Link Aggregation is active, SR-IOV VFs cannot be created on the PF.
> -- If SR-IOV is active, you cannot set up Link Aggregation on the interface.
> -
> -Bridging and MACVLAN are also affected by this. If you wish to use bridging or
> -MACVLAN with SR-IOV, you must set up bridging or MACVLAN before enabling
> -SR-IOV. If you are using bridging or MACVLAN in conjunction with SR-IOV, and
> -you want to remove the interface from the bridge or MACVLAN, you must follow
> -these steps:
> -
> -1. Destroy SR-IOV VFs if they exist
> -2. Remove the interface from the bridge or MACVLAN
> -3. Recreate SRIOV VFs as needed
> -
> -
>  Additional Features and Configurations
>  ======================================
>
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 8b016511561f..b4bca1d964a9 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -814,25 +814,6 @@ static inline bool ice_is_switchdev_running(struct ice_pf *pf)
>         return pf->switchdev.is_running;
>  }
>
> -/**
> - * ice_set_sriov_cap - enable SRIOV in PF flags
> - * @pf: PF struct
> - */
> -static inline void ice_set_sriov_cap(struct ice_pf *pf)
> -{
> -       if (pf->hw.func_caps.common_cap.sr_iov_1_1)
> -               set_bit(ICE_FLAG_SRIOV_CAPABLE, pf->flags);
> -}
> -
> -/**
> - * ice_clear_sriov_cap - disable SRIOV in PF flags
> - * @pf: PF struct
> - */
> -static inline void ice_clear_sriov_cap(struct ice_pf *pf)
> -{
> -       clear_bit(ICE_FLAG_SRIOV_CAPABLE, pf->flags);
> -}
> -
>  #define ICE_FD_STAT_CTR_BLOCK_COUNT    256
>  #define ICE_FD_STAT_PF_IDX(base_idx) \
>                         ((base_idx) * ICE_FD_STAT_CTR_BLOCK_COUNT)
> diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
> index ee5b36941ba3..5a7753bda324 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lag.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lag.c
> @@ -6,15 +6,6 @@
>  #include "ice.h"
>  #include "ice_lag.h"
>
> -/**
> - * ice_lag_nop_handler - no-op Rx handler to disable LAG
> - * @pskb: pointer to skb pointer
> - */
> -rx_handler_result_t ice_lag_nop_handler(struct sk_buff __always_unused **pskb)
> -{
> -       return RX_HANDLER_PASS;
> -}
> -
>  /**
>   * ice_lag_set_primary - set PF LAG state as Primary
>   * @lag: LAG info struct
> @@ -158,7 +149,6 @@ ice_lag_link(struct ice_lag *lag, struct netdev_notifier_changeupper_info *info)
>                 lag->upper_netdev = upper;
>         }
>
> -       ice_clear_sriov_cap(pf);
>         ice_clear_rdma_cap(pf);
>
>         lag->bonded = true;
> @@ -205,7 +195,6 @@ ice_lag_unlink(struct ice_lag *lag,
>         }
>
>         lag->peer_netdev = NULL;
> -       ice_set_sriov_cap(pf);
>         ice_set_rdma_cap(pf);
>         lag->bonded = false;
>         lag->role = ICE_LAG_NONE;
> @@ -229,7 +218,6 @@ static void ice_lag_unregister(struct ice_lag *lag, struct net_device *netdev)
>         if (lag->upper_netdev) {
>                 dev_put(lag->upper_netdev);
>                 lag->upper_netdev = NULL;
> -               ice_set_sriov_cap(pf);
>                 ice_set_rdma_cap(pf);
>         }
>         /* perform some cleanup in case we come back */
> diff --git a/drivers/net/ethernet/intel/ice/ice_lag.h b/drivers/net/ethernet/intel/ice/ice_lag.h
> index 51b5cf467ce2..54d6663fe586 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lag.h
> +++ b/drivers/net/ethernet/intel/ice/ice_lag.h
> @@ -26,62 +26,9 @@ struct ice_lag {
>         u8 bonded:1; /* currently bonded */
>         u8 primary:1; /* this is primary */
>         u8 handler:1; /* did we register a rx_netdev_handler */
> -       /* each thing blocking bonding will increment this value by one.
> -        * If this value is zero, then bonding is allowed.
> -        */
> -       u16 dis_lag;
>         u8 role;
>  };
>
>  int ice_init_lag(struct ice_pf *pf);
>  void ice_deinit_lag(struct ice_pf *pf);
> -rx_handler_result_t ice_lag_nop_handler(struct sk_buff **pskb);
> -
> -/**
> - * ice_disable_lag - increment LAG disable count
> - * @lag: LAG struct
> - */
> -static inline void ice_disable_lag(struct ice_lag *lag)
> -{
> -       /* If LAG this PF is not already disabled, disable it */
> -       rtnl_lock();
> -       if (!netdev_is_rx_handler_busy(lag->netdev)) {
> -               if (!netdev_rx_handler_register(lag->netdev,
> -                                               ice_lag_nop_handler,
> -                                               NULL))
> -                       lag->handler = true;
> -       }
> -       rtnl_unlock();
> -       lag->dis_lag++;
> -}
> -
> -/**
> - * ice_enable_lag - decrement disable count for a PF
> - * @lag: LAG struct
> - *
> - * Decrement the disable counter for a port, and if that count reaches
> - * zero, then remove the no-op Rx handler from that netdev
> - */
> -static inline void ice_enable_lag(struct ice_lag *lag)
> -{
> -       if (lag->dis_lag)
> -               lag->dis_lag--;
> -       if (!lag->dis_lag && lag->handler) {
> -               rtnl_lock();
> -               netdev_rx_handler_unregister(lag->netdev);
> -               rtnl_unlock();
> -               lag->handler = false;
> -       }
> -}
> -
> -/**
> - * ice_is_lag_dis - is LAG disabled
> - * @lag: LAG struct
> - *
> - * Return true if bonding is disabled
> - */
> -static inline bool ice_is_lag_dis(struct ice_lag *lag)
> -{
> -       return !!(lag->dis_lag);
> -}
>  #endif /* _ICE_LAG_H_ */
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index d9731476cd7f..5ddb95d1073a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2712,8 +2712,6 @@ ice_vsi_setup(struct ice_pf *pf, struct ice_vsi_cfg_params *params)
>         return vsi;
>
>  err_vsi_cfg:
> -       if (params->type == ICE_VSI_VF)
> -               ice_enable_lag(pf->lag);
>         ice_vsi_free(vsi);
>
>         return NULL;
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 9788f363e9dc..a222cd702fd5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -960,8 +960,6 @@ int ice_sriov_configure(struct pci_dev *pdev, int num_vfs)
>         if (!num_vfs) {
>                 if (!pci_vfs_assigned(pdev)) {
>                         ice_free_vfs(pf);
> -                       if (pf->lag)
> -                               ice_enable_lag(pf->lag);
>                         return 0;
>                 }
>
> @@ -973,8 +971,6 @@ int ice_sriov_configure(struct pci_dev *pdev, int num_vfs)
>         if (err)
>                 return err;
>
> -       if (pf->lag)
> -               ice_disable_lag(pf->lag);
>         return num_vfs;
>  }
>
> --
> 2.39.2
>

Powered by blists - more mailing lists