|WARNING| [v6,2/2] net/iavf: fix duplicate MAC addresses install

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/169101

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-04

# DPDK Patch Review

## PATCH 1/2: net/iavf: accept up to 32k unicast MAC addresses

### Errors

1. **Resource leak on error path in `iavf_add_del_uc_addr_bulk()`**

   The function can return an error after partially adding MAC addresses, but does not clean up the successfully added addresses before returning. If the PF accepts some addresses and then rejects one, the function returns an error, leaving the port in an inconsistent state (some addresses added, function reported failure).

   **Fix:** On error, attempt to remove the successfully added addresses before returning:
   ```c
   err = iavf_execute_vf_cmd_safe(adapter, &args);
   if (err != 0) {
       PMD_DRV_LOG(ERR, "fail to execute command %s for %u macs",
           add ? "VIRTCHNL_OP_ADD_ETH_ADDR" : "VIRTCHNL_OP_DEL_ETH_ADDR",
           list->num_elements);
       
       /* Clean up previously added addresses on failure */
       if (add && i > 0) {
           iavf_add_del_uc_addr_bulk(adapter, addrs, i, false);
       }
       return err;
   }
   ```

2. **Missing error propagation in `iavf_add_del_all_mac_addr()`**

   The function calls `iavf_add_del_uc_addr_bulk()` which can fail, but ignores the return value. Errors from adding/deleting MAC addresses are silently dropped.

   **Fix:** Check the return value and log errors:
   ```c
   if (start != -1) {
       int ret = iavf_add_del_uc_addr_bulk(adapter, &adapter->dev_data->mac_addrs[start],
           i - start, add);
       if (ret != 0)
           PMD_DRV_LOG(ERR, "Failed to %s %u MAC addresses", 
               add ? "add" : "delete", i - start);
   }
   ```

### Warnings

1. **`rte_calloc()` instead of `rte_zmalloc_socket()` for MAC address table**

   In `iavf_dev_init()`, the MAC address table is allocated with `rte_calloc()` instead of `rte_zmalloc_socket()`. This allocation should use `rte_zmalloc_socket()` to ensure NUMA-local allocation and hugepage backing (required for secondary process access and DMA).

   **Fix:**
   ```c
   eth_dev->data->mac_addrs = rte_zmalloc_socket(
       "iavf_mac", RTE_ETHER_ADDR_LEN * IAVF_UC_MACADDR_MAX, 0,
       SOCKET_ID_ANY);
   ```

2. **Missing bounds check on `nb_addrs` parameter**

   `iavf_add_del_uc_addr_bulk()` does not validate that `nb_addrs` is within the valid range (1 to `IAVF_UC_MACADDR_MAX`). While current callers appear to pass valid values, defensive programming suggests adding a check.

   **Fix:** Add validation at function entry:
   ```c
   if (nb_addrs == 0 || nb_addrs > IAVF_UC_MACADDR_MAX)
       return -EINVAL;
   ```

3. **Unclear loop exit condition**

   In `iavf_add_del_all_mac_addr()`, the loop continues to `i < IAVF_UC_MACADDR_MAX` even after finding a zero address. With the new 32k limit, this means scanning all 32k addresses even if only a few are populated. Consider breaking early after a sufficiently long run of zeros, or maintaining a count of valid addresses.

---

## PATCH 2/2: net/iavf: fix duplicate MAC addresses install

### Errors

1. **Incorrect return value check in `iavf_dev_start()`**

   The code checks `if (iavf_add_del_eth_addr(...) != 0)` but does not propagate the error. The function continues execution after logging the error, potentially leaving the device in a partially configured state. The caller expects `iavf_dev_start()` to fail if the primary MAC cannot be installed.

   **Fix:** Propagate the error:
   ```c
   if (!adapter->mac_primary_set) {
       ret = iavf_add_del_eth_addr(adapter, &dev->data->mac_addrs[0], true,
           VIRTCHNL_ETHER_ADDR_PRIMARY);
       if (ret != 0) {
           PMD_DRV_LOG(ERR, "failed to add primary MAC:" RTE_ETHER_ADDR_PRT_FMT,
               RTE_ETHER_ADDR_BYTES(&dev->data->mac_addrs[0]));
           return ret;
       }
       adapter->mac_primary_set = true;
   }
   ```

2. **Missing error checks in `iavf_post_reset_reconfig()`**

   Both `iavf_add_del_all_mac_addr()` and `iavf_add_del_mc_addr_list()` can fail (as evidenced by patch 1's error handling), but their return values are not checked in `iavf_post_reset_reconfig()`. If MAC address restoration fails after a VF reset, the device will be in an inconsistent state.

   **Fix:** Check return values and propagate errors:
   ```c
   /* After a VF reset, all MAC addresses got flushed, restore them. */
   iavf_add_del_all_mac_addr(adapter, true);
   ret = iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num, true);
   if (ret != 0) {
       PMD_DRV_LOG(ERR, "Failed to restore multicast addresses after reset");
       return ret;
   }
   ```

   Note: `iavf_add_del_all_mac_addr()` returns void in current code, which should be changed to return `int` (see patch 1 findings).

### Warnings

None.

---

## Summary

**PATCH 1:**
- 2 Errors (resource leak, missing error propagation)
- 3 Warnings (allocation API, bounds check, loop optimization)

**PATCH 2:**
- 2 Errors (missing error propagation in two locations)
- 0 Warnings

The primary concerns are correctness: error paths that leak resources or silently drop errors, leaving the device in an inconsistent state. These must be fixed before merging.