|WARNING| [v2, 3/3] net/iavf: fix watchdog clearing reset state during recovery

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/3: net/iavf: fix crash in Tx drain on PF reset

### ERRORS

**Resource leak on error path after queue setup:**
Line in `iavf_dev_tx_drain()` at line 4105 (`if (txq->nb_tx_free >= txq->nb_tx_desc - 1)`):
The patch changes the condition to check if a full RS block is outstanding before calling cleanup helpers.
However, the cleanup helpers (`iavf_xmit_cleanup()` / vector equivalents) free mbufs in the ring.
If fewer than `tx_rs_thresh` descriptors are outstanding when the function exits the loop,
those mbufs are never freed -- the queue is not cleaned up and the drain is incomplete.

The original code attempted cleanup on any non-empty queue.
The fix correctly avoids calling the helper when the DD bit is invalid,
but it does not provide an alternative cleanup path for the <`tx_rs_thresh` case.

```c
/* After the loop exits, partial blocks remain on the ring */
if ((uint16_t)(txq->nb_tx_desc - 1 - txq->nb_tx_free) < txq->tx_rs_thresh) {
    /* mbufs still on ring here are leaked */
    continue;
}
```

The drain function must either wait until a full block is posted (by forcing an RS write)
or implement a separate cleanup path for partial blocks that does not rely on the DD bit.

---

## Patch 2/3: net/iavf: fix event handler leak on init failure

### WARNINGS

**Missing `rte_atomic_load_explicit()` in second error path condition:**
Lines 3189-3190:
The second `iavf_dev_event_handler_fini()` call at the `init_vf_err` label uses the same guard
as the first (line 3092), checking `in_reset_recovery` with relaxed ordering.
This is consistent within the patch.

However, the guard itself (`in_reset_recovery`) is set in `iavf_handle_hw_reset()` without a release fence,
and no acquire fence is used here.
If another thread is setting `in_reset_recovery` concurrently (which should not happen during init,
but the atomic type suggests cross-thread access is intended),
the lack of synchronisation means this thread may not observe the write.

For init-time use where only one thread accesses the device, relaxed ordering is acceptable.
If `in_reset_recovery` can be written by another thread during init (watchdog, event handler),
the load here should use `rte_memory_order_acquire` and the store in the writer should use release.

---

## Patch 3/3: net/iavf: fix watchdog clearing reset state during recovery

### ERRORS

**Race condition: `reset_events_pending` increment vs `vf_reset` read in other threads:**
Lines 359-362 in `iavf_dev_watchdog()`:
The watchdog increments `reset_events_pending`, issues a release fence, then sets `vf_reset = true`.
The intent is to publish the count before the flag is visible.

However, another thread reading `vf_reset` without an acquire fence (e.g., datapath polling, other callbacks)
can observe `vf_reset == true` but not yet see the updated `reset_events_pending`.
That thread may then perform a check or cleanup assuming no events are pending,
creating a TOCTOU (time-of-check-time-of-use) race.

The acquire fence in the watchdog's `if (adapter->vf.vf_reset)` check (line 339)
synchronises only *that thread's* prior writes.
It does not prevent other threads from observing `vf_reset = true` without acquiring the count.

**Suggested fix:**
Make `vf_reset` itself an atomic variable (currently it is plain `bool`).
Write it with `rte_atomic_store_explicit(..., rte_memory_order_release)` after incrementing the count.
Readers must then load it with `rte_atomic_load_explicit(..., rte_memory_order_acquire)`
to ensure they see the updated count.

Alternatively, place both `reset_events_pending` and `vf_reset` under a lock,
or use a single atomic state variable combining the flag and count.

---

**Decrement without checking return value could underflow:**
Line 90 in `iavf_dev_event_handle()`:
`rte_atomic_fetch_sub_explicit(&adapter->vf.reset_events_pending, 1, ...)` is called
unconditionally after handling a reset event.

If an event is posted but `reset_events_pending` was never incremented
(e.g., a race between the event post and the increment, or a failed `rte_malloc()` in `iavf_dev_event_post()`),
the count underflows, wrapping to `UINT32_MAX`.
The watchdog's check for `== 0` at line 344 then never succeeds, blocking reset completion.

**Suggested fix:**
Only decrement if the count is >0, or restructure so that every decrement provably corresponds to an increment.
One option: in `iavf_dev_reset_event_post()`, if `iavf_dev_event_post()` returns `-ENOMEM`,
do not undo the increment (it was already undone by the function itself).

---

### WARNINGS

**Atomics with `rte_memory_order_relaxed` in increment without subsequent fence may not be visible:**
Line 350 in `iavf_read_msg_from_pf()`:
`rte_atomic_fetch_add_explicit(&vf->reset_events_pending, 1, rte_memory_order_relaxed)`
is followed by a release fence (line 353), then `vf->vf_reset = true` is written.

This is correct *if* `vf->vf_reset` is read with acquire semantics.
However, `vf_reset` is a plain `bool` and is read without atomic operations in several places
(e.g., datapath checks, the watchdog's load at line 338 before the acquire fence is applied).

Those reads will not synchronise with the release fence here,
so they may observe `vf_reset = true` without seeing the incremented count.

This is the same issue as the Error above, but applied to a different write site.
Make `vf_reset` atomic or protect all accesses with a lock.

---

**Function return value ignored in iavf_dev_reset_event_post:**
Lines 137-139 in `iavf_dev_reset_event_post()`:
If `iavf_dev_event_post()` returns `-ENOMEM`, the count is decremented.
However, the function has `void` return type, so the caller cannot distinguish success from failure.

The caller (watchdog, virtchnl msg handler) proceeds as if the event was posted,
but if it was not (malloc failed), the reset event is lost.
The application's reset callback is never invoked, and recovery stalls.

**Suggested fix:**
Return `int` from `iavf_dev_reset_event_post()` and propagate the error to the caller,
or retry the post until it succeeds (with a cap to avoid infinite loops).

---

## General (All Patches)

### INFO

**Pre-existing: vf_reset is a plain bool accessed from multiple threads without synchronisation:**
The `vf_reset` field in `struct iavf_info` is a plain `bool`,
but it is written by the watchdog thread and the virtchnl message handler,
and read by the datapath (in various checks for reset state).

This is a data race.
Even with the fences added in patch 3, a plain `bool` load/store is not atomic on all architectures,
and the compiler may optimise accesses in ways that break the synchronisation intent.

This patch series does not introduce `vf_reset`, so this is **Info** severity (pre-existing).

**Recommended fix** (for a future patch):
Change `vf_reset` to `RTE_ATOMIC(bool)` and use `rte_atomic_load_explicit()` / `rte_atomic_store_explicit()`
with appropriate ordering (`acquire` for loads that guard reset logic, `release` for stores that publish it).

---

**Pre-existing: no error propagation in iavf_dev_tx_drain:**
The function `iavf_dev_tx_drain()` calls cleanup helpers and waits in a loop,
but returns `void` and does not report whether the drain completed successfully.

If the drain times out or cleanup fails, the caller (`iavf_dev_close()`, reset recovery)
proceeds as if all descriptors were cleaned, potentially leaving the hardware in an inconsistent state.

This is pre-existing (not introduced by these patches).

---

Review-Result: ERROR