[PATCH v4 05/10] net/iavf: fix duplicate MAC addresses install

[Loftus, Ciara]

> Subject: [PATCH v4 05/10] 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>
> ---
>  drivers/net/intel/iavf/iavf_ethdev.c | 31 ++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c
> b/drivers/net/intel/iavf/iavf_ethdev.c
> index 179d90ec55..fe54df4b9f 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> @@ -134,6 +134,8 @@ static int iavf_dev_vlan_filter_set(struct rte_eth_dev
> *dev,
>  static int iavf_vlan_tpid_set(struct rte_eth_dev *dev,
>  			     enum rte_vlan_type vlan_type, uint16_t tpid);
>  static int iavf_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask);
> +static uint64_t iavf_get_restore_flags(struct rte_eth_dev *dev,
> +				       enum rte_eth_dev_operation op);
>  static int iavf_dev_rss_reta_update(struct rte_eth_dev *dev,
>  				   struct rte_eth_rss_reta_entry64 *reta_conf,
>  				   uint16_t reta_size);
> @@ -264,6 +266,7 @@ static const struct eth_dev_ops iavf_eth_dev_ops = {
>  	.tx_done_cleanup	    = iavf_dev_tx_done_cleanup,
>  	.get_monitor_addr           = iavf_get_monitor_addr,
>  	.tm_ops_get                 = iavf_tm_ops_get,
> +	.get_restore_flags          = iavf_get_restore_flags,
>  };
> 
>  static int
> @@ -284,6 +287,13 @@ iavf_tm_ops_get(struct rte_eth_dev *dev,
>  	return 0;
>  }
> 
> +static uint64_t
> +iavf_get_restore_flags(__rte_unused struct rte_eth_dev *dev,
> +		       __rte_unused enum rte_eth_dev_operation op)
> +{
> +	return RTE_ETH_RESTORE_ALL & ~RTE_ETH_RESTORE_MAC_ADDR;
> +}
> +
>  __rte_unused
>  static int
>  iavf_vfr_inprogress(struct iavf_hw *hw)
> @@ -1074,15 +1084,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);
> 
> @@ -3424,6 +3433,10 @@ iavf_handle_hw_reset(struct rte_eth_dev *dev,
> bool vf_initiated_reset)
>  		if (ret)
>  			goto error;
> 
> +		/* after a VF reset, all mac addresses got flushed, restore them
> */
> +		iavf_add_del_all_mac_addr(adapter, true);
> +		iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf-
> >mc_addrs_num, true);
> +

This restore block only runs if the device was started when the handler was
entered. It might make more sense to move it into iavf_post_reset_reconfig
which will run for VF-initiated resets regardless of started state, and for
PF-initiated will run if the device was started on entry.
For PF-initiated if the device was not started we return from
iavf_handle_hw_reset() immediately, so there's no opportunity to restore
the MAC addresses at all. It needs to be considered how to restore the macs
in that case.
Need to consider as well how the restore_flags and auto_reconfig features
work with one another. If the auto_reconfig devarg is set to zero it means
the user doesn't want any restoration of settings upon reset.

>  		dev->data->dev_started = 1;
>  	}
> 
> --
> 2.54.0