|WARNING| [RFC, 2/2] net/iavf: restore promiscuous mode only after VF reset

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-21

# DPDK Patch Review

## PATCH 1/2: net/iavf: remove auto reconfig devarg

### Errors

None identified.

### Warnings

None identified.

### Info

1. **Devarg removal follows deprecation process**
   The patch removes the `auto_reconfig` devarg. Verify that this removal was announced in a prior deprecation notice (check `doc/guides/rel_notes/deprecation.rst` in earlier releases). The release notes document the removal correctly.

2. **Behavior change is properly documented**
   The release notes clearly state that promiscuous/all-multicast restoration is now unconditional. This is a behavior change that applications relying on `auto_reconfig=0` will observe.

---

## PATCH 2/2: net/iavf: restore promiscuous mode only after VF reset

### Errors

1. **Potential double-restore on queue reconfiguration during reset recovery**
   In `iavf_dev_configure()`, the code adds:
   ```c
   if (reset_done && !vf->in_reset_recovery) {
       ret = iavf_post_reset_reconfig(dev);
       if (ret)
           return ret;
   }
   ```
   However, `vf->in_reset_recovery` is not defined in the provided context. This field does not exist in the `struct iavf_info` shown in `iavf.h` from patch 1. Unless this field is added in an earlier patch in the series (which is not shown), this will cause a compilation failure.

   **Fix:** Either ensure `in_reset_recovery` exists in `struct iavf_info`, or use a different mechanism to detect whether `configure` is being called from the reset recovery path.

2. **Missing error propagation in iavf_post_reset_reconfig()**
   In patch 1, the function was changed from:
   ```c
   int ret = 0;
   ...
   ret = iavf_restore_promisc(dev, &allunicast, &allmulti);
   if (ret)
       PMD_DRV_LOG(DEBUG, ...);
       status |= ret;
   }
   return status;
   ```
   to:
   ```c
   int ret = 0;
   ...
   ret = iavf_restore_promisc(dev, &allunicast, &allmulti);
   if (ret)
       PMD_DRV_LOG(DEBUG, ...);
   }
   return ret;
   ```
   This changes the error behavior: previously, a failure in `iavf_restore_promisc()` was accumulated in `status` and returned; now `ret` is returned directly. However, the `if (ret)` block logs but does not `return ret;`, so the function always returns the last `ret` value even if earlier restoration steps failed. The patch does not show the full function body, but if there are other restoration calls after the promiscuous restore, their errors will overwrite `ret` and the promiscuous restore failure will be lost.

   **Fix:** Either return immediately on error (`if (ret) { PMD_DRV_LOG(...); return ret; }`), or ensure all error paths are checked and the first failure is returned.

### Warnings

1. **Inconsistent error handling in iavf_post_reset_reconfig() vs configure()**
   In `iavf_dev_configure()`, the new call site checks the return value and propagates errors:
   ```c
   ret = iavf_post_reset_reconfig(dev);
   if (ret)
       return ret;
   ```
   But in `iavf_handle_hw_reset()` (from patch 1), a failure in `iavf_post_reset_reconfig()` logs an error and jumps to the error label:
   ```c
   ret = iavf_post_reset_reconfig(dev);
   if (ret) {
       PMD_DRV_LOG(ERR, "Failed to restore VF settings after reset");
       goto error;
   }
   ```
   In `iavf_dev_configure()`, no error message is logged before returning the error. Consider adding a log message for consistency, or document why logging differs between these two paths.

2. **get_restore_flags() callback compatibility**
   The patch introduces a new `get_restore_flags()` callback. Verify that this callback is defined in the ethdev API and is supported in the target DPDK version (26.07). If this is a new API addition, it should be marked `__rte_experimental` and documented in the release notes under "New Features" (not just "Removed Items"). The patch does not show the ethdev header changes or experimental marking.

3. **Missing verification of queue reconfiguration reset completion**
   The `reset_done` flag is set after `iavf_queues_req_reset()` returns successfully, but the patch does not show whether `iavf_queues_req_reset()` waits for the reset to complete or merely initiates it. If the reset is asynchronous, calling `iavf_post_reset_reconfig()` immediately after `iavf_queues_req_reset()` may race with the hardware reset and fail to restore settings correctly.

   **Verify:** Does `iavf_queues_req_reset()` block until the reset finishes, or does it return before the hardware is ready?

### Info

1. **Comment clarity**
   The comment in `iavf_get_restore_flags()` is clear and explains why promiscuous/all-multicast restoration is skipped.

2. **Redundant restoration eliminated**
   The stated goal of the patch (eliminate double restore after reset+start) is achieved by the `get_restore_flags()` callback. This is a sensible optimization.

---

## Summary

**PATCH 1/2:** No correctness issues identified. Verify deprecation process was followed.

**PATCH 2/2:** Contains at least one compilation error (`vf->in_reset_recovery` undefined) and a potential error propagation bug in `iavf_post_reset_reconfig()`. The `get_restore_flags()` callback introduction needs verification of API availability and experimental status.