Re: [PATCH v6 2/3] ethdev: skip VMDq pools unless configured

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 24 Aug 2026 13:42:06 +0200
David Marchand <david.marchand@redhat.com> wrote:

> The mac_addr_add API describes that only the 0 pool should be passed
> unless VMDq has been enabled, though there was no validation so far.
> Add such a check, then cleanup the MAC related operations (adding,
> removing, restoring).
> 
> As a side effect, the net/cnxk does not need to manually reset the
> mac_pool_sel[] array.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---

Claude Fable AI review has some warnings.

Review of [PATCH v6 0/3] ethdev: VMDq cleanups

Applied cleanly on current main (c1a46b9). Build testing not done
here per your note; findings below are from reading the applied tree.

Patches 1/3 and 3/3: no findings.

Patch 2/3 - ethdev: skip VMDq pools unless configured
-----------------------------------------------------

Error: MAC removal no longer reaches hardware on i40e and bnxt when
VMDq is not configured.

Both drivers read dev->data->mac_pool_sel[index] inside their
mac_addr_remove callback to decide which VSI / VNIC to delete the
filter from:

  drivers/net/intel/i40e/i40e_ethdev.c:4516  i40e_macaddr_remove()
  drivers/net/bnxt/bnxt_ethdev.c:2027        bnxt_mac_addr_remove_op()

Before this patch rte_eth_dev_mac_addr_add() always did
mac_pool_sel[index] |= RTE_BIT64(pool), so a non-VMDq add left bit 0
set and the drivers deleted from pool 0 (main VSI / VNIC 0). After
this patch the bitmap update is guarded by "if (vmdq)", so in RSS or
NONE mode mac_pool_sel[index] stays 0. rte_eth_dev_mac_addr_remove()
then calls dev_ops->mac_addr_remove() while the bitmap is still 0,
both drivers iterate an empty mask, and the hardware filter is never
removed. The software copy in mac_addrs[] is cleared, so the address
looks gone from the API but still receives traffic.

bnxt_restore_mac_filters() (bnxt_ethdev.c:4566) has the same
dependency on the recovery path: with a zero mask it replays nothing.

A second, less common consequence of the guards: after a VMDq ->
non-VMDq reconfigure, rte_eth_dev_mac_addr_remove() no longer clears
mac_pool_sel[index], so stale pool bits survive. A later VMDq
reconfigure that places a new address at the same index does |= on
top of the stale bits and eth_dev_mac_restore() replays the address
into pools it was never added to.

Since ethdev_driver.h documents mac_pool_sel as "Bitmap associating
MAC addresses to VMDq pools" and drivers are entitled to consume it,
ethdev should keep it coherent in all modes. The minimal fix is to
drop the two guards and keep the bitmap always up to date (pool is
already forced to 0 when !vmdq, so bit 0 is recorded):

  In rte_eth_dev_mac_addr_add():

	if (ret == 0) {
		/* 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);
	}

  In rte_eth_dev_mac_addr_remove():

	/* reset pool bitmap */
	dev->data->mac_pool_sel[index] = 0;

The new "if (!vmdq) return 0;" early-return for an already-present
address, the !vmdq branch in eth_dev_mac_restore(), and the cnxk
cleanup all remain valid with this change.

The alternative is to make i40e and bnxt stop depending on
mac_pool_sel[] in non-VMDq mode, but that would need to land in the
same series and is more invasive.

Info: "Port %u: VMDq is not configured (pool %d)" - pool is uint32_t,
use %u.