|WARNING| net/iavf: harden reset recovery and data path on link flap

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-17

# DPDK Patch Review: net/iavf reset recovery and link flap hardening

## Summary

This patch adds hardening for Intel AVF driver reset recovery and link flap handling. The changes address reset detection, data-plane gating, Tx descriptor draining, and deferred start on PF VSI inactivity. Several critical correctness issues and style violations were identified.

---

## Errors

### 1. Tx drain may spin indefinitely if HW stalls

**File:** `drivers/net/intel/iavf/iavf_rxtx.c`, function `iavf_dev_tx_drain()`

**Issue:** The drain loop breaks after `IAVF_TX_DRAIN_IDLE_MAX` iterations of no progress, but continues if *any* queue makes progress. If one queue advances while another is stalled, the timeout-based exit (`deadline`) becomes the only bound. If `rte_get_timer_hz()` returns an incorrect frequency or the timer wraps, the loop could spin far longer than intended, blocking the control path.

**Fix:** Add an absolute iteration limit as a safety net independent of the timer:

```c
#define IAVF_TX_DRAIN_MAX_ITERS 500  /* ~25 s at 50 us per iteration */

void
iavf_dev_tx_drain(struct rte_eth_dev *dev)
{
	/* ... existing locals ... */
	unsigned int total_iters = 0;

	/* ... settle delay ... */

	while (rte_get_timer_cycles() < deadline &&
	       ++total_iters < IAVF_TX_DRAIN_MAX_ITERS) {
		/* ... existing loop body ... */
	}

	if (total_iters >= IAVF_TX_DRAIN_MAX_ITERS)
		PMD_DRV_LOG(WARNING, "Tx drain hit max iterations");
}
```

### 2. Boolean implicit comparison on `link_change` parameter

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

**Issue:** `link_change & !vf->link_up` uses implicit truthiness on the `link_change` parameter. DPDK style requires explicit comparison for non-`bool` types. The parameter is `bool`, but the bitwise `&` is suspicious (likely should be logical `&&`).

**Fix:**

```c
no_poll = (link_change && !vf->link_up) ||
	vf->vf_reset || vf->in_reset_recovery;
```

(If `&` was intentional for some reason, it should be documented.)

### 3. Missing error check on `rte_get_timer_hz()`

**File:** `drivers/net/intel/iavf/iavf_rxtx.c`, function `iavf_dev_tx_drain()`

**Issue:** `rte_get_timer_hz()` can return 0 if the timer subsystem is not initialized, leading to division by zero in the deadline calculation.

**Fix:**

```c
hz = rte_get_timer_hz();
if (hz == 0) {
	PMD_DRV_LOG(ERR, "Timer subsystem not initialized; skipping Tx drain");
	return;
}
deadline = rte_get_timer_cycles() + (hz * IAVF_TX_DRAIN_TIMEOUT_US) / 1000000ULL;
```

### 4. Opcode zero discard may hide real zero-opcode messages

**File:** `drivers/net/intel/iavf/iavf_vchnl.c`, function `iavf_handle_virtchnl_msg()`

**Issue:** The patch silently discards all descriptors with `opcode == 0` without checking other fields (cookie, datalen). If a legitimate zero-opcode message exists (or is added in the future), it will be silently dropped.

**Fix:** Log at DEBUG level on first occurrence per reset window to aid debugging:

```c
if (aq_opc == 0) {
	PMD_DRV_LOG(DEBUG,
		    "Discarding zeroed descriptor (likely PF reset artifact)");
	continue;
}
```

---

## Warnings

### 1. Magic numbers for Tx drain not documented

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

**Issue:** `IAVF_TX_DRAIN_IDLE_MAX` is set to 20, but the comment "~1 ms of no RS write-back" is approximate. The actual time is `20 * 50 us = 1 ms`, which is clear, but the relationship to the overall 10 ms budget is not explicit.

**Suggestion:** Clarify the comment:

```c
#define IAVF_TX_DRAIN_IDLE_MAX		20	/* 1 ms of no RS progress (20 * 50us) */
```

### 2. Watchdog enable duplicated across reset paths

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

**Issue:** `iavf_dev_watchdog_enable(adapter)` is called in both `iavf_read_msg_from_pf()` (line 347) and `iavf_handle_pf_event_msg()` (line 588). If a single reset event triggers both paths, the watchdog is re-enabled twice. While not a bug (likely idempotent), it is redundant.

**Suggestion:** Audit the call paths to ensure watchdog enable/disable is centralized or add a guard in `iavf_dev_watchdog_enable()` to skip if already enabled.

### 3. Release notes likely needed

**Issue:** This patch fixes correctness bugs (descriptor leaks, MDD events, missed resets) and changes behavior (deferred start, no_poll atomicity, drain on link-down). These are user-visible improvements.

**Suggestion:** Add a release note entry under "**New Features**" or "**Bug Fixes**" (depending on DPDK release policy) summarizing:
- Improved VF reset detection (VFGEN_RSTAT + faster polling)
- Tx descriptor drain on link-down to prevent MDD events
- Deferred dev_start on PF VSI unavailable
- Atomic no_poll for reliable data-plane gating

### 4. Deferred start retry could loop indefinitely

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

**Issue:** If `iavf_dev_start()` fails repeatedly on every link-up event (e.g., PF misconfiguration), the retry continues forever. No limit or backoff is applied.

**Suggestion:** Add a retry counter in `struct iavf_info` and cap retries (e.g., 5 attempts). After exhausting retries, log an error and stop retrying:

```c
if (ret != 0) {
	vf->start_retry_count++;
	if (vf->start_retry_count >= IAVF_START_RETRY_MAX) {
		PMD_DRV_LOG(ERR,
			    "deferred dev_start failed after %u attempts; giving up",
			    vf->start_retry_count);
		vf->start_pending = false;
	} else {
		PMD_DRV_LOG(ERR,
			    "deferred dev_start failed (ret=%d); will retry on next link-up (%u/%u)",
			    ret, vf->start_retry_count, IAVF_START_RETRY_MAX);
	}
}
```

---

## Info

### 1. RTE_ATOMIC macro usage

**File:** `drivers/net/intel/iavf/iavf.h`, line 395

**Observation:** The patch uses `RTE_ATOMIC(bool)no_poll` for the declaration. This is the correct DPDK 25.x+ pattern for tagging atomically-accessed variables. The load/store use `rte_atomic_load_explicit()` / `rte_atomic_store_explicit()` with explicit memory ordering, which is also correct.

**Note:** This is good practice. No change needed.

### 2. Tx drain settle delay may be too short

**File:** `drivers/net/intel/iavf/iavf_rxtx.c`, `IAVF_TX_DRAIN_SETTLE_US = 100`

**Observation:** 100 us is allocated for in-flight bursts to land. On a heavily loaded system or under virtualization, this may be insufficient if a data-plane lcore is preempted mid-burst. Consider increasing to 200-500 us or making it configurable if field data shows descriptor leaks persist.

**Suggestion:** Document the rationale for 100 us or consider a tunable devarg.

### 3. PF VSI readiness detection could be improved

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

**Issue:** When `iavf_dev_start()` fails during reset recovery, the patch defers to the next link-up event. However, there is no explicit check that the failure was due to PF VSI inactivity (`-EIO` from `VIRTCHNL_OP_CONFIG_VSI_QUEUES`). Other failures (e.g., out-of-memory, invalid queue config) also trigger deferral.

**Suggestion:** Inspect the return code and only defer on `-EIO` or `-EAGAIN`. For other errors, log and fail the recovery.

---

## Conclusion

The patch addresses real issues observed in the field (missed resets, descriptor leaks, MDD events). However, three correctness bugs must be fixed:
1. Unbounded Tx drain spin if timer misbehaves
2. Bitwise `&` instead of logical `&&` in no_poll calculation
3. Missing error check on `rte_get_timer_hz()`

Additionally, release notes should be added for user-visible changes, and the deferred start retry mechanism should have a limit to prevent infinite retries.