Re: [PATCH iwl-next v4 06/10] ice: Flesh out implementation of support for SRIOV on bonded interface

[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 in the functions that will allow a VF created on the primary interface
> of a bond to "fail-over" to another PF interface in the bond and continue
> to Tx and Rx.
> 
> Add in an ordered take-down path for the bonded interface.
> 
> Reviewed-by: Daniel Machon <daniel.machon@microchip.com>
> Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_lag.c | 821 ++++++++++++++++++++++-
>   1 file changed, 811 insertions(+), 10 deletions(-)
> 

[...]

> +/**
> + * ice_lag_qbuf_recfg - generate a buffer of queues for a reconfigure command
> + * @hw: HW struct that contains the queue contexts
> + * @qbuf: pointer to buffer to populate
> + * @vsi_num: index of the VSI in PF space
> + * @numq: number of queues to search for
> + * @tc: traffic class that contains the queues
> + *
> + * function returns the number of valid queues in buffer
> + */
> +static u16
> +ice_lag_qbuf_recfg(struct ice_hw *hw, struct ice_aqc_cfg_txqs_buf *qbuf,
> +                  u16 vsi_num, u16 numq, u8 tc)
> +{
> +       struct ice_q_ctx *q_ctx;
> +       u16 qid, count = 0;
> +       struct ice_pf *pf;
> +       int i;
> +
> +       pf = hw->back;
> +       for (i = 0; i < numq; i++) {
> +               q_ctx = ice_get_lan_q_ctx(hw, vsi_num, tc, i);
> +               if (!q_ctx || q_ctx->q_handle == ICE_INVAL_Q_HANDLE) {

Should q_ctx->q_teid be checked against ICE_INVAL_TEID as well? If so, 
the dev_dbg() should also be updated.

> +                       dev_dbg(ice_hw_to_dev(hw), "%s queue %d %s\n", __func__,
> +                               i, q_ctx ? "INVAL Q HANDLE" : "NO Q CONTEXT");
> +                       continue;
> +               }
> +
> +               qid = pf->vsi[vsi_num]->txq_map[q_ctx->q_handle];
> +               qbuf->queue_info[count].q_handle = cpu_to_le16(qid);
> +               qbuf->queue_info[count].tc = tc;
> +               qbuf->queue_info[count].q_teid = cpu_to_le32(q_ctx->q_teid);
> +               count++;
> +       }
> +
> +       return count;
> +}
> +
> +/**
> + * ice_lag_get_sched_parent - locate or create a sched node parent
> + * @hw: HW struct for getting parent in
> + * @tc: traffic class on parent/node
> + */
> +static struct ice_sched_node *
> +ice_lag_get_sched_parent(struct ice_hw *hw, u8 tc)
> +{
> +       struct ice_sched_node *tc_node, *aggnode, *parent = NULL;
> +       u16 num_nodes[ICE_AQC_TOPO_MAX_LEVEL_NUM] = { 0 };
> +       struct ice_port_info *pi = hw->port_info;
> +       struct device *dev;
> +       u8 aggl, vsil;
> +       int n;
> +
> +       dev = ice_hw_to_dev(hw);
> +
> +       tc_node = ice_sched_get_tc_node(pi, tc);
> +       if (!tc_node) {
> +               dev_warn(dev, "Failure to find TC node in for LAG move\n");

Nit, but seems like there's a stray "in" in the log message?

> +               return parent;
> +       }
> +
> +       aggnode = ice_sched_get_agg_node(pi, tc_node, ICE_DFLT_AGG_ID);
> +       if (!aggnode) {
> +               dev_warn(dev, "Failure to find aggregate node for LAG move\n");
> +               return parent;
> +       }
> +
> +       aggl = ice_sched_get_agg_layer(hw);
> +       vsil = ice_sched_get_vsi_layer(hw);
> +
> +       for (n = aggl + 1; n < vsil; n++)
> +               num_nodes[n] = 1;
> +
> +       for (n = 0; n < aggnode->num_children; n++) {
> +               parent = ice_sched_get_free_vsi_parent(hw, aggnode->children[n],
> +                                                      num_nodes);
> +               if (parent)
> +                       return parent;
> +       }
> +
> +       /* if free parent not found - add one */
> +       parent = aggnode;
> +       for (n = aggl + 1; n < vsil; n++) {
> +               u16 num_nodes_added;
> +               u32 first_teid;
> +               int err;
> +
> +               err = ice_sched_add_nodes_to_layer(pi, tc_node, parent, n,
> +                                                  num_nodes[n], &first_teid,
> +                                                  &num_nodes_added);
> +               if (err || num_nodes[n] != num_nodes_added)
> +                       return NULL;
> +
> +               if (num_nodes_added)
> +                       parent = ice_sched_find_node_by_teid(tc_node,
> +                                                            first_teid);
> +               else
> +                       parent = parent->children[0];
> +               if (!parent) {
> +                       dev_warn(dev, "Failure to add new parent for LAG move\n");
> +                       return parent;
> +               }
> +       }
> +
> +       return parent;
> +}
> +

[...]

>   /**
> @@ -625,6 +1321,73 @@ static void ice_lag_monitor_active(struct ice_lag *lag, void *ptr)
>   static bool
>   ice_lag_chk_comp(struct ice_lag *lag, void *ptr)
>   {
> +       struct net_device *event_netdev, *event_upper;
> +       struct netdev_notifier_bonding_info *info;
> +       struct netdev_bonding_info *bonding_info;
> +       struct list_head *tmp;
> +       int count = 0;
> +
> +       if (!lag->primary)
> +               return true;
> +
> +       event_netdev = netdev_notifier_info_to_dev(ptr);
> +       rcu_read_lock();
> +       event_upper = netdev_master_upper_dev_get_rcu(event_netdev);
> +       rcu_read_unlock();
> +       if (event_upper != lag->upper_netdev)
> +               return true;
> +
> +       info = (struct netdev_notifier_bonding_info *)ptr;
> +       bonding_info = &info->bonding_info;
> +       lag->bond_mode = bonding_info->master.bond_mode;
> +       if (lag->bond_mode != BOND_MODE_ACTIVEBACKUP) {
> +               netdev_info(lag->netdev, "Bond Mode not ACTIVE-BACKUP\n");
> +               return false;
> +       }
> +
> +       list_for_each(tmp, lag->netdev_head) {
> +#if !defined(NO_DCB_SUPPORT) || defined(ADQ_SUPPORT)
> +               struct ice_dcbx_cfg *dcb_cfg, *peer_dcb_cfg;
> +#endif /* !NO_DCB_SUPPORT || ADQ_SUPPORT */

These #ifdefs don't belong here.

> +               struct ice_lag_netdev_list *entry;
> +               struct ice_netdev_priv *peer_np;
> +               struct net_device *peer_netdev;
> +               struct ice_vsi *vsi, *peer_vsi;
> +
> +               entry = list_entry(tmp, struct ice_lag_netdev_list, node);
> +               peer_netdev = entry->netdev;
> +               if (!netif_is_ice(peer_netdev)) {
> +                       netdev_info(lag->netdev, "Found non-ice netdev in LAG\n");
> +                       return false;
> +               }
> +
> +               count++;
> +               if (count > 2) {
> +                       netdev_info(lag->netdev, "Found more than two netdevs in LAG\n");
> +                       return false;
> +               }
> +
> +               peer_np = netdev_priv(peer_netdev);
> +               vsi = ice_get_main_vsi(lag->pf);
> +               peer_vsi = peer_np->vsi;
> +               if (lag->pf->pdev->bus != peer_vsi->back->pdev->bus ||
> +                   lag->pf->pdev->slot != peer_vsi->back->pdev->slot) {
> +                       netdev_info(lag->netdev, "Found netdev on different device in LAG\n");
> +                       return false;
> +               }
> +
> +#if !defined(NO_DCB_SUPPORT) || defined(ADQ_SUPPORT)
> +               dcb_cfg = &vsi->port_info->qos_cfg.local_dcbx_cfg;
> +               peer_dcb_cfg = &peer_vsi->port_info->qos_cfg.local_dcbx_cfg;
> +               if (memcmp(dcb_cfg, peer_dcb_cfg,
> +                          sizeof(struct ice_dcbx_cfg))) {
> +                       netdev_info(lag->netdev, "Found netdev with different DCB config in LAG\n");
> +                       return false;
> +               }
> +
> +#endif /* !NO_DCB_SUPPORT || ADQ_SUPPORT */

Ditto on the #ifdefs

> +       }
> +
>          return true;
>   }
> 

[...]