On Mon, 24 Aug 2026 13:42:06 +0200 David Marchand <[email protected]> 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 <[email protected]> > Acked-by: Andrew Rybchenko <[email protected]> > --- 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.
