|WARNING| [v3] net/iavf: fix duplicate VF reset during PF reset recovery

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-08

# DPDK Patch Review

## Summary
This patch fixes duplicate VF reset issues during PF-initiated reset recovery in the iavf driver. The changes prevent sending VIRTCHNL_OP_RESET_VF when a PF-triggered reset is already in progress, avoiding virtchnl state corruption.

## Findings

### Errors

**1. Use-after-free risk in `iavf_handle_link_change_event()`**
```c
static void
iavf_handle_link_change_event(struct rte_eth_dev *dev,
                              struct virtchnl_pf_event *vpe)
{
    // ... NULL checks ...
    
    adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
    vf = &adapter->vf;

    adv_link_speed = (vf->vf_res != NULL) &&
        ((vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_ADV_LINK_SPEED) != 0);
```
The code checks `dev->data->dev_private != NULL` but then immediately uses it without an `adapter == NULL` check. If `IAVF_DEV_PRIVATE_TO_ADAPTER()` macro could return NULL (depending on implementation), this would dereference a NULL pointer. Verify the macro always returns a valid pointer or add: `if (adapter == NULL) { PMD_DRV_LOG(ERR, "Failed to get adapter"); return; }`

**2. Race condition on `vf->vf_res` access**
```c
adv_link_speed = (vf->vf_res != NULL) &&
    ((vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_ADV_LINK_SPEED) != 0);
```
Then later in the same function:
```c
if (adv_link_speed) {
    vf->link_up = vpe->event_data.link_event_adv.link_status;
    vf->link_speed = vpe->event_data.link_event_adv.link_speed;
}
```
If `vf->vf_res` can be modified by another thread between the NULL check and the capability flag read, this is a TOCTOU race. The pointer should be loaded once atomically or protected by a lock. Consider: `struct virtchnl_vf_resource *vf_res_local = vf->vf_res; if (vf_res_local != NULL && (vf_res_local->vf_cap_flags & ...))` to avoid repeated dereferences.

**3. Missing NULL check before calling `iavf_handle_link_change_event()` in `iavf_handle_pf_event_msg()`**
```c
case VIRTCHNL_EVENT_LINK_CHANGE:
    PMD_DRV_LOG(DEBUG, "VIRTCHNL_EVENT_LINK_CHANGE event");
    iavf_handle_link_change_event(dev, pf_msg);
    break;
```
The function `iavf_handle_link_change_event()` performs NULL checks on `dev` and `vpe`, but `iavf_handle_pf_event_msg()` does not verify `pf_msg != NULL` before calling it in this case. While the NULL check exists inside the handler, it would be safer to verify `pf_msg` is non-NULL at the call site, consistent with defensive programming. Add: `if (pf_msg == NULL) break;` before the call.

### Warnings

**1. Release notes describe feature addition, not bug fix**
```rst
+  * Prevented duplicate 'VIRTCHNL_OP_RESET_VF' during a PF-initiated
+    reset recovery, which earlier caused virtchnl state corruption
+    and connection loss after a top-of-rack (ToR) link flap/power-cycle.
```
This is under "New Features" in `release_26_07.rst`. A bug fix belongs under "**Fixed Issues**" section, not "New Features". The release notes should document this as a fix to existing behavior, not a new feature addition.

**2. Inconsistent link-down forcing on RESET_IMPENDING**
```c
case VIRTCHNL_EVENT_RESET_IMPENDING:
    /*
     * Force link down on impending reset to drop
     * the cached link-up state; a fresh LSC up
     * event will be re-issued by the PF once the
     * VF is reinitialised.
     */
    vf->link_up = false;
```
The code forces `vf->link_up = false` but does not call `iavf_dev_link_update(dev, 0)` as done in the link-change path. This means the driver's internal link state is updated but the `rte_eth_dev` link state may remain stale until the next link update. Consider calling `iavf_dev_link_update(dev, 0)` after forcing link down for consistency.

**3. Missing LSC notification on forced link-down**
Following from Warning #2: when `vf->link_up` is forced to `false` during `RESET_IMPENDING`, no LSC event is posted to notify the application. The application may continue to believe the link is up. Consider: `iavf_dev_event_post(vf->eth_dev, RTE_ETH_EVENT_INTR_LSC, NULL, 0);` after forcing link down.

**4. Boolean expression unnecessarily complex**
```c
adv_link_speed = (vf->vf_res != NULL) &&
    ((vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_ADV_LINK_SPEED) != 0);
```
The `!= 0` is redundant for a boolean assignment from a bitwise AND result. Prefer: `adv_link_speed = vf->vf_res != NULL && (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_ADV_LINK_SPEED);`

**5. Conditional nesting could be flattened**
```c
if (!adapter->closed && !vf->in_reset_recovery) {
    if (vf->lv_enabled) {
        ret = iavf_request_queues(dev, IAVF_MAX_NUM_QUEUES_DFLT);
        if (ret)
            PMD_DRV_LOG(ERR, "Reset the num of queues failed");
        vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
    }

    if (vf->promisc_unicast_enabled || vf->promisc_multicast_enabled)
        iavf_config_promisc(adapter, false, false);
}
```
The two inner `if` blocks are independent. For clarity, consider early-return style or keeping them separate to emphasize they are unrelated operations gated by the same outer condition.

**6. Duplicate debug vs info log level**
```c
case VIRTCHNL_EVENT_RESET_IMPENDING:
    // ...
    PMD_DRV_LOG(DEBUG, "VF is resetting");
    break;
```
vs. in `iavf_read_msg_from_pf()`:
```c
case VIRTCHNL_EVENT_RESET_IMPENDING:
    // ...
    PMD_DRV_LOG(INFO, "VF is resetting");
```
The same event is logged at `DEBUG` in one code path and (previously) `INFO` in another. For consistency, choose one log level (suggest `INFO` since it's an important state change).

### Info

**1. Comment style for new function header**
The function header comment for `iavf_handle_link_change_event()` uses a non-standard style:
```c
/*
 * iavf_handle_link_change_event: common handler for VIRTCHNL link change events
 *
 * @dev: pointer to rte_eth_dev for this VF
 * @vpe: pointer to the virtchnl_pf_event payload received from the PF
 */
```
DPDK uses Doxygen. Consider:
```c
/**
 * Handle VIRTCHNL link change events from PF
 *
 * @param dev
 *   Pointer to rte_eth_dev for this VF.
 * @param vpe
 *   Pointer to virtchnl_pf_event payload from PF.
 */
```

**2. Log message phrasing**
```c
PMD_DRV_LOG(INFO, "Link status update:%s",
    vf->link_up ? "up" : "down");
```
Missing space after colon. Prefer: `"Link status update: %s"` for consistency with standard formatting.

**3. Overly defensive NULL checks**
```c
if (dev == NULL || dev->data == NULL ||
    dev->data->dev_private == NULL || vpe == NULL) {
```
These parameters are passed from internal driver code paths where they should always be valid. The checks add runtime overhead. If this is truly an internal-only function, consider removing the checks or converting to assertions for debug builds only.

## Positive Observations

1. **Correctness improvement**: Skipping duplicate VF reset during PF-initiated recovery correctly addresses the root cause of virtchnl state corruption.

2. **Refactoring improves maintainability**: Consolidating link-change handling into `iavf_handle_link_change_event()` eliminates code duplication and ensures consistent behavior across interrupt and polling paths.

3. **AQ drain logic fix**: Treating `IAVF_ERR_ADMIN_QUEUE_NO_WORK` as a normal loop termination condition is correct and reduces log noise.

4. **Watchdog state management**: The fix properly updates watchdog/no_poll state before notifying the app via LSC, preventing race conditions.

## Recommendation

**Do not merge without addressing Errors #1, #2, and the release notes placement (Warning #1).**

The use-after-free risk and TOCTOU race on `vf->vf_res` must be fixed. The release notes should be moved to the "Fixed Issues" section.

Warnings #2 and #3 (missing link update and LSC notification on forced link-down) should be addressed to ensure consistent behavior, but are lower priority than the errors.