|SUCCESS| [v2,5/5] net/iavf: fix duplicate MAC addresses install

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163685

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-06

Reviewing patch series from David Marchand <david.marchand at redhat.com>
Commit: ethdev: skip VMDq pools unless configured

This series modifies VMDq handling in ethdev and drivers.

> diff --git a/drivers/net/cnxk/cnxk_ethdev_ops.c b/drivers/net/cnxk/cnxk_ethdev_ops.c
> index 49e77e49a6..75decf7098 100644
> --- a/drivers/net/cnxk/cnxk_ethdev_ops.c
> +++ b/drivers/net/cnxk/cnxk_ethdev_ops.c
> @@ -1240,7 +1240,6 @@ cnxk_nix_mc_addr_list_configure(struct rte_eth_dev *eth_dev, struct rte_ether_ad
>  		/* Update address in NIC data structure */
>  		rte_ether_addr_copy(&mc_addr_set[i], &data->mac_addrs[j]);
>  		rte_ether_addr_copy(&mc_addr_set[i], &dev->dmac_addrs[j]);
> -		data->mac_pool_sel[j] = RTE_BIT64(0);
>  	}

Does the removal of mac_pool_sel assignment break multicast handling
when VMDq is configured? The code unconditionally processes all
mc_addr_set entries but now skips pool bitmap updates.

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 2edc7a362e..9577b7d848 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c

[ ... ]

> @@ -1680,7 +1680,10 @@ eth_dev_mac_restore(struct rte_eth_dev *dev,
>  				continue;
>  
>  			pool = 0;
> -			pool_mask = dev->data->mac_pool_sel[i];
> +			if ((dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_FLAG) != 0)
> +				pool_mask = dev->data->mac_pool_sel[i];
> +			else
> +				pool_mask = 1;

Does the implicit comparison against zero in the pool_mask & UINT64_C(1)
test below match coding style? The style guide recommends explicit
comparisons: (pool_mask & UINT64_C(1)) != 0.

[ ... ]

> @@ -5390,8 +5393,9 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  			uint32_t pool)
>  {
>  	struct rte_eth_dev *dev;
> -	int index;
>  	uint64_t pool_mask;
> +	bool vmdq;
> +	int index;

Regarding whitespace style: Does mixing int and bool in the declarations
match DPDK patterns? The style guide shows grouping by type then
declaration order.

[ ... ]

> @@ -5416,6 +5420,12 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  		RTE_ETHDEV_LOG_LINE(ERR, "Pool ID must be 0-%d", RTE_ETH_64_POOLS - 1);
>  		return -EINVAL;
>  	}
> +	vmdq = (dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_FLAG) != 0;
> +	if (!vmdq && pool != 0) {
> +		RTE_ETHDEV_LOG_LINE(ERR, "Port %u: VMDq is not configured (pool %d)",
> +			port_id, pool);
> +		return -EINVAL;
> +	}

Does the implicit comparison on vmdq comply with the coding style
requirement for explicit comparisons? Should be if (vmdq == false).

[ ... ]

> @@ -5425,7 +5435,7 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  				port_id);
>  			return -ENOSPC;
>  		}
> -	} else {
> +	} else if (vmdq) {

Does this condition change the behavior when vmdq is false? The original
code entered this branch when index >= 0, now it only enters when
vmdq is true. Is this correct?

[ ... ]

> @@ -5440,8 +5450,10 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  		/* Update address in NIC data structure */
>  		rte_ether_addr_copy(addr, &dev->data->mac_addrs[index]);
>  
> -		/* Update pool bitmap in NIC data structure */
> -		dev->data->mac_pool_sel[index] |= RTE_BIT64(pool);
> +		if (vmdq) {
> +			/* Update pool bitmap in NIC data structure */
> +			dev->data->mac_pool_sel[index] |= RTE_BIT64(pool);
> +		}

Does adding this conditional bracket match the style guide section 3.1
which discourages single statement conditionals with braces?

[ ... ]

Commit: ethdev: announce VMDq capability

> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index b677f9491d..0f783b9e98 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -1214,7 +1214,8 @@ static int bnxt_dev_info_get_op(struct rte_eth_dev *eth_dev,
>  
>  	dev_info->speed_capa = bnxt_get_speed_capabilities(bp);
>  	dev_info->dev_capa = RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP |
> -			     RTE_ETH_DEV_CAPA_RUNTIME_TX_QUEUE_SETUP;
> +			     RTE_ETH_DEV_CAPA_RUNTIME_TX_QUEUE_SETUP |
> +			     RTE_ETH_DEV_CAPA_VMDQ;

Does the alignment of the RTE_ETH_DEV_CAPA_VMDQ line match the
surrounding lines? Appears correct.

[ ... ]

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 9577b7d848..7ba539e796 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c

[ ... ]

> @@ -1581,6 +1582,22 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  		goto rollback;
>  	}
>  
> +	if (!(dev_info.dev_capa & RTE_ETH_DEV_CAPA_VMDQ)) {
> +		if ((dev_conf->rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_FLAG) != 0) {
> +			RTE_ETHDEV_LOG_LINE(ERR, "Ethdev port_id=%u does not support VMDq rx mode",
> +				port_id);
> +			ret = -EINVAL;
> +			goto rollback;
> +		}
> +		if (dev_conf->txmode.mq_mode == RTE_ETH_MQ_TX_VMDQ_DCB ||
> +				dev_conf->txmode.mq_mode == RTE_ETH_MQ_TX_VMDQ_ONLY) {
> +			RTE_ETHDEV_LOG_LINE(ERR, "Ethdev port_id=%u does not support VMDq tx mode",
> +				port_id);
> +			ret = -EINVAL;
> +			goto rollback;
> +		}
> +	}

Does the implicit comparison in the outer if statement follow the coding
style? Should be: if ((dev_info.dev_capa & RTE_ETH_DEV_CAPA_VMDQ) == 0).

[ ... ]

Commit: net/iavf: accept up to 32k unicast MAC addresses

> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index 403c61e2e8..f1dede0694 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -31,7 +31,8 @@
>  #define IAVF_IRQ_MAP_NUM_PER_BUF	 128
>  #define IAVF_RXTX_QUEUE_CHUNKS_NUM	 2
>  
> -#define IAVF_NUM_MACADDR_MAX      64
> +#define IAVF_UC_MACADDR_MAX      32768
> +#define IAVF_MC_MACADDR_MAX      64

Does changing IAVF_NUM_MACADDR_MAX to IAVF_UC_MACADDR_MAX maintain API
compatibility? Are there external users of this define?

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c b/drivers/net/intel/iavf/iavf_vchnl.c
> index 08dd6f2d7f..144968db35 100644
> --- a/drivers/net/intel/iavf/iavf_vchnl.c
> +++ b/drivers/net/intel/iavf/iavf_vchnl.c

[ ... ]

> @@ -1437,48 +1437,101 @@ iavf_config_irq_map_lv(struct iavf_adapter *adapter, uint16_t num)
>  	return 0;
>  }
>  
> -void
> -iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
> +static int
> +iavf_add_del_uc_addr_bulk(struct iavf_adapter *adapter, struct rte_ether_addr *addrs,
> +			  uint32_t nb_addrs, bool add)
>  {
> +#define IAVF_ETH_ADDR_PER_REQ \
> +	((IAVF_AQ_BUF_SZ - sizeof(struct virtchnl_ether_addr_list)) / \
> +	 sizeof(struct virtchnl_ether_addr))
>  	struct {
>  		struct virtchnl_ether_addr_list list;
> -		struct virtchnl_ether_addr addr[IAVF_NUM_MACADDR_MAX];
> -	} list_req = {0};
> -	struct virtchnl_ether_addr_list *list = &list_req.list;
> +		struct virtchnl_ether_addr addr[IAVF_ETH_ADDR_PER_REQ];
> +	} cmd_buffer;

Does the cmd_buffer lack initialization? The original list_req had = {0}.
Does this code leak uninitialized padding in the struct to the hardware?

> +#undef IAVF_ETH_ADDR_PER_REQ
> +	struct virtchnl_ether_addr_list *list = &cmd_buffer.list;
>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> -	struct iavf_cmd_info args = {0};
> -	int err, i;
> -	size_t buf_len;
>  
> -	for (i = 0; i < IAVF_NUM_MACADDR_MAX; i++) {
> -		struct rte_ether_addr *addr = &adapter->dev_data->mac_addrs[i];
> -		struct virtchnl_ether_addr *vc_addr = &list->list[list->num_elements];
> +	for (uint32_t i = 0; i < nb_addrs; i++) {
> +		size_t buf_len = sizeof(struct virtchnl_ether_addr_list) +
> +			sizeof(struct virtchnl_ether_addr) * list->num_elements;
> +		struct iavf_cmd_info args;
> +		uint32_t batch;
> +		int err;
>  
> -		/* ignore empty addresses */
> -		if (rte_is_zero_ether_addr(addr))
> -			continue;
> +		batch = i % RTE_DIM(cmd_buffer.addr);
> +
> +		if (batch == 0) {
> +			memset(&cmd_buffer, 0, sizeof(cmd_buffer));
> +			list->vsi_id = vf->vsi_res->vsi_id;
> +			list->num_elements = 0;
> +		}

Does this logic properly handle the case when nb_addrs is 0? Would it
dereference list->num_elements before initialization?

[ ... ]

> @@ -2060,7 +2113,7 @@ iavf_add_del_mc_addr_list(struct iavf_adapter *adapter,
>  {
>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
>  	uint8_t cmd_buffer[sizeof(struct virtchnl_ether_addr_list) +
> -		(IAVF_NUM_MACADDR_MAX * sizeof(struct virtchnl_ether_addr))];
> +		(IAVF_MC_MACADDR_MAX * sizeof(struct virtchnl_ether_addr))];

Does reducing the buffer size from 128 to 64 entries match the function
signature which still accepts up to mc_addr_num entries? Could this
overflow cmd_buffer?

[ ... ]

Commit: net/iavf: fix duplicate MAC addresses install

> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index c69a012d50..5871ed3539 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c

[ ... ]

> @@ -1039,15 +1049,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 */