|WARNING| [v1,2/2] net/iavf: keep device stopped if start fails

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-15

# DPDK Patch Review

## Patch 1/2: net/iavf: fix memory leaks around IRQ resources

### ERRORS

**Missing `rte_free()` on one error path:**

Line 987 in the error path calls `rte_intr_vec_list_free(intr_handle)` and `rte_intr_efd_disable(intr_handle)`, but does not free `vf->qv_map` which was allocated earlier at line 933 (`vf->qv_map = rte_zmalloc("qv_map", ...)`.

The patch correctly frees it in the `iavf_dev_start()` error path (line 1133) and in `iavf_dev_stop()` (line 1168), but the allocation happens in `iavf_config_rx_queues_irqs()` itself. If the function fails after allocating `qv_map`, it must free it before returning.

Add `rte_free(vf->qv_map); vf->qv_map = NULL;` before the `return -1;` at line 989.

```c
qv_map_alloc_err:
	rte_intr_vec_list_free(intr_handle);
efd_alloc_err:
	rte_intr_efd_disable(intr_handle);
	rte_free(vf->qv_map);
	vf->qv_map = NULL;
	return -1;
```

**Incorrect goto label in line 865:**

The code at line 865 does `goto efd_alloc_err;` but `rte_intr_efd_enable()` failure means eventfds were NOT allocated, so calling `rte_intr_efd_disable()` on a failed enable is semantically wrong (though likely harmless). The function should return directly or use a label that does not call `rte_intr_efd_disable()`.

More importantly, if `qv_map` was already allocated at line 933 before reaching this failure, it leaks. However, re-reading the flow: `rte_intr_efd_enable()` is called before `qv_map` allocation (line 864 vs line 933), so at this point `qv_map` is NULL and there's no leak. The label name is misleading but the jump is to the wrong cleanup -- it should just `return -1;` because no resources have been allocated yet.

Change line 865 to `return -1;` instead of `goto efd_alloc_err;`.

**Incomplete cleanup on startup failure -- LSC interrupt:**

Line 1101 in `iavf_dev_start()` enables the link status change interrupt if `RTE_ETH_DEV_INTR_LSC` is configured. If startup then fails at line 1117 (`iavf_start_queues()` returns non-zero), the error path at line 1129 disables the Rx interrupt (`if (!wb_on_itr && dev->data->dev_conf.intr_conf.rxq != 0)`) but does NOT disable the LSC interrupt. This leaves the LSC interrupt enabled after a failed start, which can cause use-after-free when the interrupt fires and tries to access cleaned-up queue state.

Add `if (dev->data->dev_conf.intr_conf.lsc != 0) rte_intr_callback_unregister(intr_handle, iavf_dev_interrupt_handler, dev);` to the error path before line 1133.

### WARNINGS

**Missing release notes entry:**

This patch fixes resource leaks (memory and interrupt vector list) which can accumulate across start/stop cycles or on startup failure. It should have a corresponding entry in `doc/guides/rel_notes/release_XX_XX.rst` (where XX_XX is the current release) under **Fixed Issues in the iavf PMD** or similar section.

## Patch 2/2: net/iavf: keep device stopped if start fails

### ERRORS

**Incorrect early return in `iavf_dev_stop()`:**

The check `if (adapter->stopped == 1)` is moved from line 1159 (after disabling the interrupt) to line 1153 (before disabling the interrupt). However, the comment and the code structure indicate that `iavf_dev_stop()` is idempotent and should be safe to call multiple times. The original code disables the Rx interrupt even if already stopped, which may be cleaning up from a failed start.

With this patch, if `iavf_dev_start()` fails after enabling the interrupt at line 1103 but before setting `adapter->stopped = 0` at line 1122, then `adapter->stopped` is still 1 (it was never cleared). A subsequent `iavf_dev_stop()` will now return early at line 1153 WITHOUT disabling the interrupt that was enabled at line 1103.

This creates an interrupt leak: the Rx interrupt is enabled but never disabled.

The early return must come AFTER the interrupt disable, as in the original code. The patch inverts the correct order. Revert lines 1153-1154 to their original position after line 1158 (after the interrupt disable).

**`adapter->stopped` logic incorrect with the fix as written:**

Even if the early return is moved back, the core issue is that `adapter->stopped = 0` is now set only at line 1122 (after `iavf_start_queues()` succeeds), but several operations before that can fail and jump to the error path. The error path does not set `adapter->stopped = 1`, so after a failed start, `adapter->stopped` remains at its previous value (likely 1 from the last stop).

If `adapter->stopped` is 1 and start fails, `iavf_dev_stop()` will see `stopped == 1` and return early, skipping cleanup. If `adapter->stopped` was 0 (fresh init or multiple start attempts), stop will proceed.

The correct fix is: set `adapter->stopped = 1` in the error path at line 1134 before `return -1;`. This ensures that after a failed start, the adapter is marked as stopped and stop() is a no-op.

### WARNINGS

**Missing release notes entry:**

This patch fixes a state management bug where a failed start leaves the device in an inconsistent state, potentially causing interrupt leaks or repeated cleanup on stop. It should be documented in the release notes.

## FINAL CHECK

Re-reviewing for evidence and correctness of findings:

1. **Patch 1, qv_map leak in iavf_config_rx_queues_irqs error path**: Line 933 allocates `vf->qv_map = rte_zmalloc(...)`. Lines 986-989 are the error path labels. No `rte_free(vf->qv_map)` appears there. The leak is real. KEEP.

2. **Patch 1, goto efd_alloc_err on line 865**: Line 864 calls `rte_intr_efd_enable()`. Line 865 does `goto efd_alloc_err;` which is line 987. Line 988 calls `rte_intr_efd_disable()`. However, at line 865, `qv_map` has NOT been allocated yet (line 933 is later). So there is no `qv_map` leak here. The issue is calling `rte_intr_efd_disable()` when `efd_enable()` failed -- but that is not a leak, just semantically odd. The real problem: if `rte_intr_efd_enable()` fails, no cleanup is needed except returning -1. The current goto is wrong but does not leak because `qv_map` is not yet allocated. However, the error path at `efd_alloc_err` now (after this patch) calls `rte_free(vf->qv_map)` (per my first finding). If we goto there when `qv_map` is NULL, `rte_free(NULL)` is safe. So this is not a leak. However, jumping to a label that frees `qv_map` when `qv_map` is not allocated is confusing. Better to return -1 directly. This is a style/clarity issue, not a leak. DEMOTE to suggestion or om
 it. Actually, re-reading: the patch does NOT yet add `rte_free(vf->qv_map)` to the error labels -- that was MY suggestion in finding #1. So currently (in the patch as written), line 865 goto efd_alloc_err does NOT free qv_map. The patch is missing that free (finding #1). Line 865 is fine as-is because qv_map is not allocated. DROP this finding.

3. **Patch 1, LSC interrupt not unregistered on error**: Line 1101 does `if (dev->data->dev_conf.intr_conf.lsc != 0) rte_intr_callback_register(...)`. Line 1117 can fail. The error path at line 1129 does NOT unregister the LSC callback. This is a resource leak. KEEP.

4. **Patch 2, early return before interrupt disable**: Original code line 1159 checks `if (adapter->stopped == 1) return 0;` AFTER line 1158 disables interrupt. Patch moves it to line 1153, BEFORE line 1158. If start fails after enabling interrupt but before `adapter->stopped = 0`, stop will return early without disabling. Interrupt leak. KEEP.

5. **Patch 2, adapter->stopped not set in error path**: Patch sets `adapter->stopped = 0` at line 1122, after all failures. Error path does not set it back to 1. If it was 1 before start, and start fails, it remains 1, and stop returns early. But stop should proceed to clean up partial init. Actually, re-reading: if `adapter->stopped` is 1 and start fails, the early return in stop (line 1153) means no cleanup. But startup allocated IRQ resources (lines 1129-1133 in error path do clean them up). So the error path does its own cleanup; stop does not need to. The issue is: if start is called again after a failure, and then fails again, and then stop is called, what happens? Depends on whether `adapter->stopped` is 0 or 1. If start never sets it to 0 on failure, it stays 1, and stop is a no-op. But the error path in start cleaned up. So this is consistent. However, the guideline is: after a failed start, stop should be a no-op. The patch achieves that if `adapter->stopped` remains 1. But
  the error path at line 1153 checks `if (adapter->stopped == 1) return 0;`. If start never set it to 0, stop returns early, which is correct. But if start was previously successful, `adapter->stopped` was set to 0, and then another start is attempted and fails, `adapter->stopped` is still 0 (never set back to 1), so stop will proceed. That's wrong -- stop should be no-op after failed start. So the error path needs `adapter->stopped = 1;`. Re-reading original code: line 1048 sets `adapter->stopped = 0;` immediately. If start fails, `adapter->stopped` is 0, and stop will run full cleanup. Patch delays setting to line 1122. If start fails, `adapter->stopped` is still 1 (from previous stop), and stop returns early. But what if this is the first start (fresh init)? `adapter->stopped` initial value is set at init to... checking `iavf_dev_init()`: I don't have the full source, but typically it is 1. So if first start fails, `adapter->stopped` is 1, stop returns early. If second start (afte
 r successful first start and stop) fails, `adapter->stopped` is 1 from stop, and stop returns early. Seems OK. But the early return is now BEFORE interrupt disable (finding #4). So the real bug is the reordering of the early return. The `adapter->stopped` not set in error path is actually not a bug with the current patch logic, assuming `stopped` starts at 1. However, it's good practice to set it in error path for clarity. This is marginal; the real bug is #4. KEEP #4, DROP #5 (or demote to INFO as a style suggestion).

6. **Missing release notes**: Both patches fix bugs. KEEP as Warning for both.

Re-reviewing: Findings #1, #3, #4 are errors. #2 is dropped. #5 is dropped. #6 is warning.

Review-Result: ERROR