On Mon, 13 Jul 2026 at 15:14, Loftus, Ciara <[email protected]> wrote: > > > 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: [email protected] > > > > Signed-off-by: David Marchand <[email protected]> > > --- > > 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.
I was not aware of this knob. It seems crazy to me to have such specific flag to disable something that I take foregranted during the life of a DPDK port. I really hope no application relies on the mac addresses list being flushed during a VF reset... While moving the code to iavf_post_reset_reconfig kind of makes sense to me, I honesly don't know what to do wrt restore_flags. It seems orthogonal to me. If ethdev decides to restore the mac addresses, I don't see why some devargs would have something to say about it. -- David Marchand
