|WARNING| net/iavf: fix reset race condition

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-24

# DPDK Patch Review

## Patch Summary
The patch addresses a race condition in the iavf driver's reset handling by making `in_reset_recovery` atomic and adding synchronization between concurrent reset paths.

---

## Errors

### 1. Missing `RTE_EXPORT_*` macro for new public API return value change

**File:** `drivers/net/intel/iavf/iavf_ethdev.c`

The function `rte_pmd_iavf_reinit()` is an exported experimental symbol (marked with `RTE_EXPORT_EXPERIMENTAL_SYMBOL`), and this patch changes its return semantics (previously always returned 0 or -EINVAL, now propagates `iavf_handle_hw_reset()` return values including -EBUSY).

While the export macro is present, the change in return value behavior is an API modification. The release notes should document this behavior change where the function can now return additional error codes.

**Suggested fix:**
Add to release notes that `rte_pmd_iavf_reinit()` can now return `-EBUSY` if a reset is already in progress.

---

### 2. Inconsistent memory ordering - potential visibility issue

**File:** `drivers/net/intel/iavf/iavf_ethdev.c`, function `iavf_handle_hw_reset()`

```c
if (!rte_atomic_compare_exchange_strong_explicit(&vf->in_reset_recovery,
		&expected, true, rte_memory_order_acquire,
		rte_memory_order_acquire)) {
```

The success ordering is `acquire`, but the store at the end uses `release`:
```c
rte_atomic_store_explicit(&vf->in_reset_recovery, false, rte_memory_order_release);
```

This creates an asymmetry. When acquiring the lock (CAS with `acquire`), you ensure subsequent reads/writes in the critical section are not reordered before the acquisition. When releasing (store with `release`), you ensure prior writes are visible before the release.

However, the CAS failure ordering (second `acquire` parameter) should typically be `relaxed` because on failure, you're not entering the critical section and don't need synchronization.

**Suggested fix:**
```c
if (!rte_atomic_compare_exchange_strong_explicit(&vf->in_reset_recovery,
		&expected, true, rte_memory_order_acquire,
		rte_memory_order_relaxed)) {
```

The success `acquire` pairs with the `release` at the end, which is correct. The failure case can use `relaxed` since no critical section is entered.

---

## Warnings

### 1. All atomic loads use `relaxed` ordering - may miss synchronization

**Files:** Multiple locations throughout the patch

All loads of `in_reset_recovery` use `rte_memory_order_relaxed`:
```c
if (!rte_atomic_load_explicit(&vf->in_reset_recovery, rte_memory_order_relaxed))
```

While `relaxed` is acceptable for simply polling a flag, these loads are often followed by operations that depend on the flag's state. The pattern should use `acquire` when the check gates access to shared state modified by the reset handler.

**Example from `iavf_dev_configure()`:**
```c
if (reset_done && !rte_atomic_load_explicit(&vf->in_reset_recovery,
		rte_memory_order_relaxed)) {
	ret = iavf_post_reset_reconfig(dev);  /* depends on reset state */
```

The load should use `acquire` to ensure visibility of any writes done by the reset handler before it cleared `in_reset_recovery`.

**Suggested fix:**
Change loads that gate access to reset-related state from `relaxed` to `acquire`:
```c
if (!rte_atomic_load_explicit(&vf->in_reset_recovery, rte_memory_order_acquire))
```

Loads where the result is purely observational (e.g., logging, early-exit without accessing shared state) can remain `relaxed`.

---

### 2. `reset_pending` and `pf_reset_in_progress` not made atomic

**File:** `drivers/net/intel/iavf/iavf.h`

The patch makes `in_reset_recovery` atomic but leaves `reset_pending` and `pf_reset_in_progress` as plain `bool`. If these are also accessed from multiple threads (the iavf-event thread and application threads), they should also be atomic.

**Code context:**
```c
struct iavf_info {
	/* ... */
	RTE_ATOMIC(bool) in_reset_recovery;
	bool reset_pending;              /* - not atomic */
	bool pf_reset_in_progress;       /* - not atomic */
```

>From the patch, `pf_reset_in_progress` is written in `iavf_handle_hw_reset()` which can be called from multiple threads. Without atomics, this is a data race.

**Suggested fix:**
Make these fields atomic if they are accessed from multiple threads:
```c
RTE_ATOMIC(bool) reset_pending;
RTE_ATOMIC(bool) pf_reset_in_progress;
```

And update all accesses accordingly. If they are only accessed from the thread that holds `in_reset_recovery`, document this invariant.

---

### 3. Error path after acquiring `in_reset_recovery` does not restore `no_poll`

**File:** `drivers/net/intel/iavf/iavf_ethdev.c`, function `iavf_handle_hw_reset()`

After the CAS succeeds, the code sets:
```c
vf->pf_reset_in_progress = !vf_initiated_reset;
vf->start_pending = false;
iavf_set_no_poll(adapter, false);
```

Then there are multiple error paths (e.g., `iavf_reset_hw()` fails, `iavf_dev_configure()` fails) that reach the cleanup label, which restores `in_reset_recovery` but does not explicitly restore the `no_poll` state.

The final cleanup does call `iavf_set_no_poll(adapter, false)` again, which may be sufficient. However, if `iavf_set_no_poll()` logic depends on the state of `in_reset_recovery`, there could be a brief inconsistency where `in_reset_recovery` is released but `no_poll` is not recalculated until after the release.

**Suggested fix:**
Verify that `iavf_set_no_poll(adapter, false)` correctly restores state even when called with `in_reset_recovery` already cleared. If the intended semantics are to recalculate based on current state, ensure the order is:
1. Clear `in_reset_recovery`
2. Call `iavf_set_no_poll()` to recalculate

Currently the code does:
```c
rte_atomic_store_explicit(&vf->in_reset_recovery, false, rte_memory_order_release);
vf->pf_reset_in_progress = false;
iavf_set_no_poll(adapter, false);
```

This appears correct, but the comment about what happens on error paths could be clearer.

---

### 4. Missing release notes entry

The patch fixes a race condition (a bug) and changes the return type of `iavf_handle_hw_reset()` (from `void` to `int`) and the behavior of the public `rte_pmd_iavf_reinit()` function.

**Required release notes sections:**
- **Fixes** section: Document the race condition fix
- **API changes** section (if applicable): Note that `rte_pmd_iavf_reinit()` can now return `-EBUSY`

---

## Info

### 1. Consider adding a comment explaining the CAS pattern

**File:** `drivers/net/intel/iavf/iavf_ethdev.c`, function `iavf_handle_hw_reset()`

The CAS pattern here is a mutex-like acquire:
```c
bool expected = false;
if (!rte_atomic_compare_exchange_strong_explicit(&vf->in_reset_recovery,
		&expected, true, ...))
```

A brief comment would help readers understand this is a lock acquisition:
```c
/* Acquire reset lock; if another thread is already resetting, return -EBUSY */
```

---

### 2. Return value of `iavf_handle_hw_reset()` on success path

The function now returns `int`, and on success it returns `ret` which is set by various operations. Trace through to confirm all success paths set `ret = 0` or a meaningful error code.

>From the code:
- `ret` is initialized as uninitialized
- First write is `ret = iavf_reset_vf(dev);`
- On success, `ret` is returned at the end

If all calls succeed, `ret` should be 0. Verify that the intent is to return the error code from the last failing operation, or 0 if all succeed. This appears correct.

---

## Summary

**Errors:** 2 (API behavior change not documented, CAS failure ordering should be `relaxed`)  
**Warnings:** 4 (other flags not atomic, error path state restoration, release notes missing, memory ordering on loads)  
**Info:** 2 (comment suggestion, return value path verification)

The core fix (making `in_reset_recovery` atomic with CAS guarding) is sound. The primary issues are memory ordering refinements and ensuring related flags (`reset_pending`, `pf_reset_in_progress`) receive the same atomic treatment if they are also accessed concurrently.