> Subject: Re: [PATCH v4 05/10] net/iavf: fix duplicate MAC addresses install > > 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...
I think it's reasonable to keep the MAC restore outside of auto_reconfig, that also preserves the existing behaviour of restoring MACs on reset. But with your patch the restore is skipped whenever the port was stopped at reset entry. Will the MACs be restored on the next dev_start? > > 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. I agree ethdev should take precedence over the devarg. One option is to have iavf_post_reset_reconfig honour the same restore_flags mask the normal start path uses, so the two paths don't diverge. I don't think you need to address that in your set, it's something I can look into. And I agree the flag is quite specific. I think its main use is during a VF initiated reset, the user may want to reset the VF completely and not restore anything. I think instead of a devarg I could address this by adding a parameter to the dev-private rte_pmd_iavf_reinit API. I'll look into this too. Definitely room for improvement here around the reset path. > > > -- > David Marchand
