[PATCH v6 2/2] net/iavf: fix duplicate MAC addresses install

[Loftus, Ciara]

> Subject: [PATCH v6 2/2] 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: stable at dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand at redhat.com>
> ---
> Changes since v4:
> - rebased on next-net-intel,
> 
> Changes since v4:
> - moved mac restoration in iavf_post_reset_reconfig,
> 
> ---
>  drivers/net/intel/iavf/iavf_ethdev.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c
> b/drivers/net/intel/iavf/iavf_ethdev.c
> index bbd1f08ff0..bec7b3b6d7 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);
> 
> @@ -3434,6 +3434,11 @@ 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, restore them. */
> +	iavf_add_del_all_mac_addr(adapter, true);

Could this lead to a double install of the primary on the reset path? In
iavf_handle_hw_reset, dev_start can run before
iavf_post_reset_reconfig and can install the primary MAC and set
mac_primary_set. iavf_post_reset_reconfig then calls
iavf_add_del_all_mac_addr which can install the primary again as it
doesn't check mac_primary_set.

> +	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)
> --
> 2.54.0