On Tue, 12 May 2026 at 16:42, Stephen Hemminger <[email protected]> wrote: > > On Sun, 10 May 2026 19:03:04 +0200 > David Marchand <[email protected]> wrote: > > > E810 hardware provides 32k switch lookups. > > Thanks to this, it is possible to allow a lot more secondary mac > > addresses than what is possible today. > > > > In practice, the maximum number of macs available per port may be lower > > and depends on usage by other (trusted?) VFs on the same PF. > > There is no way to figure out this limit but to try adding a mac address > > and get an error from the PF driver. > > > > Mailbox exchanges are limited to IAVF_AQ_BUF_SZ, segment messages > > accordingly. > > > > Signed-off > > AI review feedback; nothing major but worth a look
In the first revision, it was interesting, but now, that's a lot of noise. > > Series: ethdev: VMDq cleanup and iavf: 32k MACs / duplicate install fix > Tree: dpdk main (DPDK 26.07) > > Patch 2 - ethdev: skip VMDq pools unless configured > Info > Doxygen of rte_eth_dev_mac_addr_add() is not updated to describe the > new constraint that pool must be 0 when VMDq is not configured. The > function-level @param documentation and @return list should mention > that -EINVAL is now returned when VMDq is disabled and pool != 0. * @param pool * VMDq pool index to associate address with (if VMDq is enabled). If VMDq is * not enabled, this should be set to 0. It has been documented since the start that a 0 pool should be passed. I can update the return code EINVAL. > > Subtle behavior change: when VMDq is not configured, mac_pool_sel[index] > is no longer set to RTE_BIT64(0) on add. Before this patch every added > MAC ended up with bit 0 set; afterwards the array stays at 0. This is > consistent with the new restore path (which now uses the VMDq flag > rather than walking pool_mask in the !vmdq case), but the change in > observable state of dev->data->mac_pool_sel[] could surprise an out-of- > tree consumer. Worth a sentence in the API change note. This is really internal stuff, only drivers would access this, and we don't care about such out of tree consumer. > > Patch 4 - net/iavf: accept up to 32k unicast MAC addresses > Info > Release notes wording: "secondary MAC addresses from 64 to 32k" is > slightly imprecise. IAVF_NUM_MACADDR_MAX (64) was the total cap including > the primary MAC; IAVF_UC_MACADDR_MAX (32768) is also a total cap > including index 0. Either drop "secondary" or say "secondary MAC > addresses from 63 to 32767". > > iavf_add_del_uc_addr_bulk() returns an int but the only caller > (iavf_add_del_all_mac_addr) discards it. The pre-existing function was > already void so this is no regression, but having the helper return an > error code that is unconditionally dropped is misleading. Either > propagate the error to the caller (and make iavf_add_del_all_mac_addr > return int) or make the helper void. > > eth_dev_mac_restore() now iterates dev_info->max_mac_addrs which for > iavf becomes 32768. With patch 5 applied this path is skipped via > get_restore_flags, but any driver that increases max_mac_addrs into the > thousands without setting RTE_ETH_RESTORE_MAC_ADDR off will pay a 32k > rte_is_zero_ether_addr() scan on every dev_start. Not a bug here, but > worth keeping in mind. > > sizeof(struct virtchnl_ether_addr_list) already accounts for the > embedded list[1] slot, so > in_args_size = sizeof(struct virtchnl_ether_addr_list) + > sizeof(struct virtchnl_ether_addr) * list->num_elements > overcounts by sizeof(struct virtchnl_ether_addr). This matches what > iavf_add_del_eth_addr() already does, and the buffer is sized to hold > it, so it's not a bug -- noting it for completeness. Those points do reflect the pre-existing implementations, but that's it, I won't fix all bugs in the world... for now. > Patch 5 - net/iavf: fix duplicate MAC addresses install > Info > Recovery edge case: if iavf_add_del_eth_addr() for the primary fails > during the first iavf_dev_start(), mac_primary_set stays false. On a > subsequent VF reset, iavf_dev_start() (called from iavf_handle_hw_reset) > will re-attempt the primary add, and the new > iavf_add_del_all_mac_addr() call right after will add the primary again > via VIRTCHNL_ETHER_ADDR_PRIMARY. This re-introduces a single-MAC > double-install in a narrow failure path. Could be avoided by skipping > the primary in iavf_add_del_all_mac_addr() when mac_primary_set is > already true, or by having iavf_dev_start() skip the primary add when > in_reset_recovery is set. > > No release notes entry. With Fixes: + Cc: stable this is acceptable as > a bug fix, but the patch also introduces a new dev_op > (get_restore_flags) for the driver and changes when MC addresses are > re-pushed (only on VF reset, not on every dev_start). A one-line note > in the iavf section of the release notes would help users tracking > behaviour changes between stop/start cycles. Mac addresses are installed only once and restored on reset, that's all we need. IOW, this patch should have no visible impact on an application, and nothing to write about in the RN. -- David Marchand
