|WARNING| [v2] net/iavf: fix reset race condition

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This patch converts `in_reset_recovery` from a plain `bool` to an atomic variable to prevent concurrent reset handling. The implementation is generally sound, but there are several issues with error path cleanup and one incorrect memory ordering choice.

---

## Errors

### 1. Resource leak on reset failure path

In `iavf_handle_hw_reset()`, when the atomic compare-exchange succeeds (taking ownership of the reset), but a later operation fails (e.g., `iavf_dev_stop()`, `iavf_init_vf()`, `iavf_dev_configure()`, or `iavf_dev_start()`), the function jumps to the `done` label and clears `in_reset_recovery`. However, if `ret != 0`, the driver is left in an inconsistent state: `in_reset_recovery` is cleared, but the hardware may not be fully operational. On the next reset attempt, the atomic guard will allow entry, but resources allocated during the failed reset (e.g., partially initialized queues) may leak or cause corruption.

**Fix**: Add explicit cleanup on the error path before the `done` label. Consider adding a label like `reset_error:` that cleans up partial state before clearing `in_reset_recovery`.

```c
done:
	if (ret != 0) {
		/* TODO: Add cleanup here - reset queues, free resources, etc. */
		PMD_DRV_LOG(ERR, "Reset failed on port %u, driver may be inconsistent", dev->data->port_id);
	}

	vf->pf_reset_in_progress = false;
	rte_atomic_store_explicit(&vf->in_reset_recovery, false, rte_memory_order_release);
```

Alternatively, document that the driver is left in an unrecoverable state and mark the port as unusable.

---

### 2. Incorrect memory ordering in `iavf_resume_pending_start()`

```c
if (vf->vf_reset || rte_atomic_load_explicit(&vf->in_reset_recovery,
		rte_memory_order_acquire))
	return;
```

This acquire load is incorrect. The function reads `in_reset_recovery` to decide whether to start pending I/O. The publishing store (the release in `iavf_handle_hw_reset()`) does not guard any data that `iavf_resume_pending_start()` subsequently accesses. The acquire ordering synchronizes with the release store, but there is no dependent read of shared data after the load. The only purpose is to read the flag itself.

**Fix**: Use `rte_memory_order_relaxed`. There is no acquire/release pair here -- the flag is polled, not used to guard access to other data.

```c
if (vf->vf_reset || rte_atomic_load_explicit(&vf->in_reset_recovery,
		rte_memory_order_relaxed))
	return;
```

---

## Warnings

### 3. Non-atomic access to `vf->pf_reset_in_progress`

`vf->pf_reset_in_progress` is read in `iavf_set_no_poll()` without atomic operations, but it is written in `iavf_handle_hw_reset()` (which can now run concurrently with other threads reading it in `iavf_set_no_poll()`). This is a data race.

**Fix**: Either make `pf_reset_in_progress` atomic, or ensure it is only accessed under `in_reset_recovery` serialization. Since the flag is set/cleared inside the reset critical section (after the atomic compare-exchange succeeds), relaxed loads outside the critical section should be safe -- but the non-atomic write still races with concurrent reads.

Suggested fix:

```c
/* In iavf.h */
RTE_ATOMIC(bool) pf_reset_in_progress;

/* In iavf_ethdev.c, iavf_handle_hw_reset() */
rte_atomic_store_explicit(&vf->pf_reset_in_progress, !vf_initiated_reset, rte_memory_order_relaxed);
/* ... later ... */
rte_atomic_store_explicit(&vf->pf_reset_in_progress, false, rte_memory_order_relaxed);

/* In iavf_set_no_poll() */
no_poll = (link_change & !vf->link_up) ||
	rte_atomic_load_explicit(&vf->vf_reset, rte_memory_order_relaxed) ||
	rte_atomic_load_explicit(&vf->in_reset_recovery, rte_memory_order_relaxed);
```

Note: `vf->vf_reset` also appears to be a plain `bool` read in a racy context and should be reviewed separately (out of scope for this patch).

---

### 4. Non-atomic access to `vf->reset_pending`

`vf->reset_pending` is a plain `bool` but is read in multiple contexts without atomics. It should be reviewed for concurrent access patterns. Not caused by this patch, but the same pattern applies.

---

### 5. Missing release notes

This patch fixes a race condition that could cause undefined behavior (concurrent resets). It should have a release notes entry under the "Bug Fixes" section of the current release notes file.

**Fix**: Add an entry to `doc/guides/rel_notes/release_XX_YY.rst`:

```rst
* **net/iavf: Fixed race condition in VF reset handling.**

  Fixed a data race where concurrent reset requests (from RTE_ETH_EVENT_INTR_RESET
  and rte_pmd_iavf_reinit()) could run simultaneously, causing undefined behavior.
```

---

## Info

### 6. Style: atomic variable spacing

```c
RTE_ATOMIC(bool)in_reset_recovery;
```

Missing space after `RTE_ATOMIC(bool)`. Should be:

```c
RTE_ATOMIC(bool) in_reset_recovery;
```

This is consistent with other atomic declarations in DPDK.

---

### 7. Return value from `rte_pmd_iavf_reinit()` may confuse callers

The function now returns:
- `0` on success
- `-EINVAL` if port is invalid
- `-EBUSY` if a reset is already in progress (new)
- Other negative error codes from `iavf_handle_hw_reset()`

The `-EBUSY` return is not documented in the function's public API header. If this function has a Doxygen comment, update it to document the new return value. If it does not, consider adding one, or at minimum ensure the calling code in applications handles `-EBUSY` correctly.

---

## Clean Patches

None -- this is a single-patch submission.

---

## Conclusion

The core approach (using an atomic flag to serialize resets) is correct. The critical issues are:

1. **Error**: Missing cleanup on reset failure paths (resource leak / inconsistent state)
2. **Error**: Wrong memory ordering in `iavf_resume_pending_start()` (acquire should be relaxed)

The warnings about non-atomic access to `pf_reset_in_progress` and missing release notes should also be addressed before merging.