> Subject: [PATCH v7 2/4] net/iavf: fix duplicate MAC addresses install
>
> On port restart, all MAC addresses get pushed *twice* to the hardware,
> once by the driver and once by the eth_dev_mac_restore() in ethdev.
>
> On the other hand, MAC address filters are reset in the hardware
> by the PF only when a VF reset is triggered.
>
> Strictly speaking, the mac restore on port (re)start is unneeded,
> if no VF reset happened, so we can announce to ethdev that no mac
> restoration is needed via a get_restore_flags callback.
>
> Then, move the mac restoration to the VF reset handler.
>
> Fixes: 3d42086def30 ("net/iavf: preserve MAC address with i40e PF Linux
> driver")
> Cc: [email protected]
>
> Signed-off-by: David Marchand <[email protected]>
> ---
> Changes since v6:
> - renamed iavf_add_del_all_mac_addr and removed primary mac address
> handling out of this helper (avoids double primary mac installation),
>
> Changes since v4:
> - rebased on next-net-intel,
>
> Changes since v4:
> - moved mac restoration in iavf_post_reset_reconfig,
>
> ---
> drivers/net/intel/iavf/iavf.h | 2 +-
> drivers/net/intel/iavf/iavf_ethdev.c | 30 ++++++++++++++++++----------
> drivers/net/intel/iavf/iavf_vchnl.c | 11 +++++-----
> 3 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index 1d52b3d113..34b9b4ad94 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -482,7 +482,7 @@ int iavf_add_del_vlan_v2(struct iavf_adapter
> *adapter, uint16_t vlanid,
> int iavf_get_vlan_offload_caps_v2(struct iavf_adapter *adapter);
> int iavf_config_irq_map(struct iavf_adapter *adapter);
> int iavf_config_irq_map_lv(struct iavf_adapter *adapter, uint16_t num);
> -void iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add);
> +void iavf_add_del_secondary_mac_addr(struct iavf_adapter *adapter, bool
> add);
> int iavf_dev_link_update(struct rte_eth_dev *dev,
> __rte_unused int wait_to_complete);
> void iavf_dev_alarm_handler(void *param);
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c
> b/drivers/net/intel/iavf/iavf_ethdev.c
> index b498258d58..f7aeac8c83 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> @@ -292,11 +292,12 @@ iavf_get_restore_flags(__rte_unused struct
> rte_eth_dev *dev,
> __rte_unused enum rte_eth_dev_operation op)
> {
> /*
> - * The unicast and multicast promiscuous settings persist across a
> + * The mac addresses, unicast and multicast promiscuous settings
> persist across a
> * stop/start; they are only cleared by a VF reset, which the driver
> * restores itself. So ethdev does not need to restore them on start.
> */
> - return RTE_ETH_RESTORE_ALL & ~(RTE_ETH_RESTORE_PROMISC |
> + return RTE_ETH_RESTORE_ALL & ~(RTE_ETH_RESTORE_MAC_ADDR |
> + RTE_ETH_RESTORE_PROMISC |
> RTE_ETH_RESTORE_ALLMULTI);
> }
>
> @@ -1095,15 +1096,14 @@ iavf_dev_start(struct rte_eth_dev *dev)
> rte_intr_enable(intr_handle);
> }
>
> - /* Set all mac addrs */
> - iavf_add_del_all_mac_addr(adapter, true);
> -
> - if (!adapter->mac_primary_set)
> - adapter->mac_primary_set = true;
> -
> - /* Set all multicast addresses */
> - iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf-
> >mc_addrs_num,
> - true);
> + if (!adapter->mac_primary_set) {
> + if (iavf_add_del_eth_addr(adapter, &dev->data-
> >mac_addrs[0], true,
> + VIRTCHNL_ETHER_ADDR_PRIMARY) != 0)
> + PMD_DRV_LOG(ERR, "failed to add primary MAC:"
> RTE_ETHER_ADDR_PRT_FMT,
> + RTE_ETHER_ADDR_BYTES(&dev->data-
> >mac_addrs[0]));
> + else
> + adapter->mac_primary_set = true;
> + }
>
> rte_spinlock_init(&vf->phc_time_aq_lock);
>
> @@ -3428,6 +3428,14 @@ iavf_post_reset_reconfig(struct rte_eth_dev
> *dev)
> int ret = 0;
> bool allmulti = false, allunicast = false;
> struct iavf_adapter *adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev-
> >data->dev_private);
> + struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data-
> >dev_private);
> +
> + /*
> + * After a VF reset, all MAC addresses got flushed.
> + * The primary MAC should have been set in iavf_dev_start, restore
iavf_dev_start is not guaranteed to have been executed before this handler.
So I would change this comment to something like:
"The primary MAC has been or will be restored by iavf_dev_start".
Other than that:
Acked-by: Ciara Loftus <[email protected]>
> the rest.
> + */
> + iavf_add_del_secondary_mac_addr(adapter, true);
> + (void)iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf-
> >mc_addrs_num, true);
>
> /* Restore pre-reset unicast promiscuous and multicast promiscuous
> states */
> if (dev->data->promiscuous)
> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c
> b/drivers/net/intel/iavf/iavf_vchnl.c
> index df97ff2052..a918db5443 100644
> --- a/drivers/net/intel/iavf/iavf_vchnl.c
> +++ b/drivers/net/intel/iavf/iavf_vchnl.c
> @@ -1672,7 +1672,7 @@ iavf_config_irq_map_lv(struct iavf_adapter
> *adapter, uint16_t num)
> }
>
> void
> -iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
> +iavf_add_del_secondary_mac_addr(struct iavf_adapter *adapter, bool add)
> {
> struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> struct {
> @@ -1685,7 +1685,7 @@ iavf_add_del_all_mac_addr(struct iavf_adapter
> *adapter, bool add)
> int err;
> size_t buf_len;
>
> - for (unsigned int i = 0; i < RTE_DIM(vf->mac_addrs); i++) {
> + for (unsigned int i = 1; i < RTE_DIM(vf->mac_addrs); i++) {
> struct rte_ether_addr *addr = &vf->mac_addrs[i];
> struct virtchnl_ether_addr *vc_addr = &list->list[list-
> >num_elements];
>
> @@ -1695,11 +1695,12 @@ iavf_add_del_all_mac_addr(struct iavf_adapter
> *adapter, bool add)
> list->num_elements++;
>
> memcpy(vc_addr->addr, addr->addr_bytes, sizeof(addr-
> >addr_bytes));
> - vc_addr->type = (list->num_elements == 1) ?
> - VIRTCHNL_ETHER_ADDR_PRIMARY :
> - VIRTCHNL_ETHER_ADDR_EXTRA;
> + vc_addr->type = VIRTCHNL_ETHER_ADDR_EXTRA;
> }
>
> + if (list->num_elements == 0)
> + return;
> +
> /* for some reason PF side checks for buffer being too big, so adjust it
> down */
> buf_len = sizeof(struct virtchnl_ether_addr_list) +
> sizeof(struct virtchnl_ether_addr) * list->num_elements;
> --
> 2.54.0
