|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. **Release notes entry could be clearer**
   
   The release notes say "now unconditional" but don't mention that MAC addresses, multicast lists, VLAN, and RSS were always restored regardless of the devarg. Consider adding a note that this change makes the documentation match the actual behavior (only promiscuous/allmulticast were ever controlled by the devarg).

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

### Errors

1. **`vf->in_reset_recovery` flag may not be set correctly**

   The patch relies on `vf->in_reset_recovery` to distinguish whether `iavf_dev_configure()` is being called during reset recovery. However, I do not see where this flag is set in the provided patch context. If this flag is not correctly set/cleared in the reset handler, the logic could fail:
   - If `in_reset_recovery` is never set, the settings would be restored twice (once in configure, once in the reset handler)
   - If `in_reset_recovery` is never cleared, the settings would never be restored after the first reset

   Verify that:
   - `vf->in_reset_recovery` is set at the start of `iavf_handle_hw_reset()`
   - It is cleared after `iavf_post_reset_reconfig()` completes
   - It is properly initialized (likely to `false`) during adapter creation

2. **Unused variable `status` removed but logic may have changed unintentionally (from Patch 1)**

   In `iavf_post_reset_reconfig()`, patch 1 removed the `status` variable and changed:
   ```c
   status |= ret;
   return status;
   ```
   to:
   ```c
   return ret;
   ```

   This changes the function's behavior: previously it would OR all error codes together and return the combined result, allowing multiple errors to be accumulated. Now it returns immediately after the first failure. If the original behavior was intentional (to collect multiple error statuses), this is a semantic change that should be noted in the commit message. If the loop is expected to return on first failure, this is fine but should be confirmed.

### Warnings

1. **Missing field definition in reviewed code**

   The patch references `vf->in_reset_recovery` but this field is not added in either patch. If this is a new field, it should be added to the `struct iavf_info` or `struct iavf_vf` definition (in `iavf.h` or `iavf_ethdev.c`). If it already exists, disregard this warning.

2. **`get_restore_flags` callback pattern should be documented**

   The new `iavf_get_restore_flags()` callback implements driver-specific control over ethdev's default restore behavior. Consider adding a comment in the function explaining why promiscuous settings persist in hardware (unlike other NICs where they may be cleared on stop) and therefore don't need ethdev's restore mechanism.

   Suggested comment:
   ```c
   /*
    * For iavf, promiscuous settings persist in the VF hardware across
    * stop/start and are only lost on VF reset. Unlike drivers where
    * these settings are volatile, we don't need ethdev to restore them
    * on every start - only after a reset (handled by the driver).
    */
   ```

### Info

1. **Consider consolidating reset logic**

   With these changes, promiscuous/allmulticast restore happens in three places:
   - `iavf_post_reset_reconfig()` (after VF reset in `iavf_handle_hw_reset()`)
   - `iavf_dev_configure()` (after queue reconfiguration reset)
   - Skipped on normal start (via `get_restore_flags()`)

   While this is correct, it may be worth noting in the commit message that the reset-triggered paths are intentionally separated from the normal start path to avoid redundant PF requests.

2. **Patch ordering**

   The patch series is well-ordered: patch 1 removes the devarg and makes restore unconditional, then patch 2 optimizes the restore path. No issues with compilation order or dependencies.

---

## Summary

**Critical findings:**
- Verify `vf->in_reset_recovery` flag lifecycle (set/clear/init)
- Confirm the intentional behavior change in `iavf_post_reset_reconfig()` error handling (OR-accumulation vs early-return)

**Other notes:**
- Documentation and code comments could be enhanced to explain the hardware behavior and restore logic
- Patch 1 changes error handling semantics (possibly intentionally)