Re: [PATCH v2] net/iavf: preserve MAC address with i40e PF Linux driver

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Oct 01, 2024 at 11:12:54AM +0200, David Marchand wrote:
> Following two upstream Linux kernel changes (see links), the mac address
> of a iavf port, serviced by a i40e PF driver, is lost when the DPDK iavf
> driver probes the port again (which may be triggered at any point of a
> DPDK application life, like when a reset event is triggered by the PF).
> 
> A first change results in the mac address of the VF port being reset to 0
> during the VIRTCHNL_OP_GET_VF_RESOURCES query.
> The i40e PF driver change is pretty obscure but the iavf Linux driver does
> set VIRTCHNL_VF_OFFLOAD_USO.
> Announcing such a capability in the DPDK driver does not seem to be an
> issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.
> 
> Then a second change in the kernel results in the VF mac address being
> cleared when the VF driver remove its default mac address.
> Removing (unicast or multicast) mac addresses is not done by the kernel VF
> driver in general.
> The reason why the DPDK driver behaves like this is undocumented
> (and lost because the authors are not active anymore).
> Aligning DPDK behavior to the upstream kernel driver is safer in any
> case.
> 

One question inline below.

/Bruce

> Cc: stable@dpdk.org
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ceb29474bbbc
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/iavf/iavf_ethdev.c | 22 +++++-----------------
>  drivers/net/iavf/iavf_vchnl.c  |  1 +
>  2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index 11036bc179..97cdb1cbe0 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -1044,7 +1044,7 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  		if (iavf_configure_queues(adapter,
>  				IAVF_CFG_Q_NUM_PER_BUF, index) != 0) {
>  			PMD_DRV_LOG(ERR, "configure queues failed");
> -			goto err_queue;
> +			goto error;
>  		}
>  		num_queue_pairs -= IAVF_CFG_Q_NUM_PER_BUF;
>  		index += IAVF_CFG_Q_NUM_PER_BUF;
> @@ -1052,12 +1052,12 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  
>  	if (iavf_configure_queues(adapter, num_queue_pairs, index) != 0) {
>  		PMD_DRV_LOG(ERR, "configure queues failed");
> -		goto err_queue;
> +		goto error;
>  	}
>  
>  	if (iavf_config_rx_queues_irqs(dev, intr_handle) != 0) {
>  		PMD_DRV_LOG(ERR, "configure irq failed");
> -		goto err_queue;
> +		goto error;
>  	}
>  	/* re-enable intr again, because efd assign may change */
>  	if (dev->data->dev_conf.intr_conf.rxq != 0) {
> @@ -1077,14 +1077,12 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  
>  	if (iavf_start_queues(dev) != 0) {
>  		PMD_DRV_LOG(ERR, "enable queues failed");
> -		goto err_mac;
> +		goto error;
>  	}
>  
>  	return 0;
>  
> -err_mac:
> -	iavf_add_del_all_mac_addr(adapter, false);
> -err_queue:
> +error:
>  	return -1;
>  }
>  
> @@ -1113,16 +1111,6 @@ iavf_dev_stop(struct rte_eth_dev *dev)
>  	/* Rx interrupt vector mapping free */
>  	rte_intr_vec_list_free(intr_handle);
>  
> -	/* adminq will be disabled when vf is resetting. */
> -	if (!vf->in_reset_recovery) {
> -		/* remove all mac addrs */
> -		iavf_add_del_all_mac_addr(adapter, false);
> -
> -		/* remove all multicast addresses */
> -		iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num,
> -				  false);
> -	}
> -

Question on this: while I understand we don't want to remove the default
mac address, should all other non-default macs not still be removed?