|WARNING| [v2, 6/6] net/dpaa2: drop the fake software VLAN strip offload

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-16

# DPDK Patch Review

## Patch 1/6: bus/fslmc: move DPCON management from event driver to bus

### Errors

None.

### Warnings

None.

### Info

- Code movement patch appears correct
- Symbol exports properly annotated with `RTE_EXPORT_INTERNAL_SYMBOL`
- Header changes maintain correct include paths

---

## Patch 2/6: bus/fslmc/dpio: make the portal DQRI epoll optional

### Errors

**Error path resource leak in `dpaa2_dpio_intr_init()`:**

In the new error paths after `epoll_create()` succeeds, if `epoll_ctl()` fails, the function closes `dpio_epoll_fd` but does not disable the interrupt that was enabled at the start of the function via `rte_dpaa2_intr_enable()`. The interrupt remains enabled while the epoll fd is closed, leaving the interrupt in an inconsistent state.

```c
ret = epoll_ctl(dpio_epoll_fd, EPOLL_CTL_ADD, eventfd, &epoll_ev);
if (ret < 0) {
    DPAA2_BUS_ERR("epoll_ctl failed");
    rte_dpaa2_intr_disable(dpio_dev->intr_handle, 0);  /* MISSING */
    close(dpio_epoll_fd);
    return -1;
}
```

Also missing after the earlier `epoll_create()` failure path:

```c
dpio_epoll_fd = epoll_create(1);
if (dpio_epoll_fd < 0) {
    DPAA2_BUS_ERR("epoll_create failed");
    rte_dpaa2_intr_disable(dpio_dev->intr_handle, 0);  /* MISSING */
    return -1;
}
```

These are error paths, so the leak is only on failure, but it leaves the VFIO interrupt registration active when the function returns -1.

### Warnings

None.

### Info

- The `build_epoll` parameter design is reasonable for separating event PMD (private epoll) from net PMD (application epoll) use cases
- Setting `epoll_fd = -1` and checking `>= 0` in `intr_deinit()` is correct

---

## Patch 3/6: net/dpaa2: support Rx queue interrupts

### Errors

**Missing error check in `dpaa2_dev_rx_queue_setup()`:**

The `dpaa2_dev_rx_queue_setup()` function calls `dpni_set_queue()` with a stack-allocated `struct dpni_queue cfg` that has been `memset()` to zero but whose `destination.type` is then set to `DPNI_DEST_NONE`. The comment states this clears stale DPIO dest from a prior rx-intr run, but the function does not check the return value of this `dpni_set_queue()` call. If the MC call fails here, the FQ retains its old destination, but the function proceeds to the DPCON allocation and later `dpni_set_queue()` as if the queue were clean. The later DPCON bind may then fail or leave the queue in an inconsistent state.

The goto label `err_free_dpcon` already exists for error cleanup; the unchecked `dpni_set_queue()` should propagate its error.

```c
/* clear any stale DPIO dest left scheduled by a prior rx-intr run */
options |= DPNI_QUEUE_OPT_DEST;
cfg.destination.type = DPNI_DEST_NONE;
/* ... later ... */
ret = dpni_set_queue(dpni, CMD_PRI_LOW, priv->token, DPNI_QUEUE_RX,
        dpaa2_q->tc_index, flow_id, options, &cfg);
if (ret) {
    DPAA2_PMD_ERR("Error in setting the rx flow: = %d", ret);
    goto err_free_dpcon;
}
```

There are two `dpni_set_queue()` calls in this function. The first (the DEST_NONE clear) lacks error checking; the second (later, for cgid) checks `ret` and jumps to `err_free_dpcon`. The first should do the same.

**Potential use-after-free in `dpaa2_dev_rx_queue_intr_enable()`:**

The function reads `dpaa2_q->napi_sub_dpio` with an atomic acquire load and stores the result in a local `old` pointer. It then checks `if (old && old != dpio && dpaa2_q->napi_armed)` and accesses `old->sw_portal` and calls `dpio_remove_static_dequeue_channel(old->dpio, ...)`. However, after the atomic load, another thread (e.g., a concurrent `rx_queue_intr_disable()` or `rx_queue_intr_unbind()`) could have cleared `napi_sub_dpio`, and the old DPIO structure could have been freed or reassigned. The code does not hold a reference or lock on `old` after the atomic load, so dereferencing `old->sw_portal` and `old->dpio` is a potential use-after-free.

The code assumes the queue is quiesced and the old portal is no longer in use ("quiesced; QBMan MMIO unsynced" comment), but there is no locking or refcount to guarantee the `old` pointer remains valid between the atomic load and the derefs. If the old lcore or another thread releases the DPIO between the load and the `qbman_swp_push_set()` call, this is undefined behavior.

A safer pattern would be to either:
- Hold a per-DPIO refcount and increment it while `old` is in use, or
- Require the caller to hold a lock that prevents the old DPIO from being freed

The current code relies on an undocumented quiesce contract that is not enforced.

### Warnings

**`dpaa2_dev_rx_queue_intr_enable()` lacks documentation of quiesce requirement:**

The function's comment states "A re-home reclaims the channel by poking the old portal, so the caller must have quiesced the previous owner and disabled the queue there," but there is no enforcement or assertion of this requirement. If the caller violates the quiesce assumption, the function will race with the old lcore and produce undefined behavior. The code should either document this as a strict API contract (with a clear error return if violated) or add a runtime check (e.g., verify `dpaa2_q->napi_armed == 0` on the re-home path).

**Large NAPI stash size (2 KB per queue):**

Each `struct dpaa2_queue` embeds a 64-entry `struct dpaa2_napi_stash` (64 * ~32 bytes = ~2 KB) for DQRR demux parking. On a device with many queues (e.g., 16+ queues per port), this adds up quickly. The size is justified in the comment as "2x rx burst so the peer port's frames fit before HW backpressure," but this is a per-queue cost paid even when the queue is not using NAPI or is idle. Consider a runtime allocation (only when NAPI is enabled) or a shared pool if memory footprint becomes an issue.

**Hardcoded 100us default for `drv_rx_intr_holdoff_us`:**

The default coalescing holdoff is set to 100us (`DPAA2_RX_INTR_HOLDOFF_US_DEF`) with no justification in the code or documentation. This may be a reasonable default for some workloads, but it is not tunable per-application without recompiling or using devargs. The driver should document the tradeoff (latency vs CPU wakeup rate) and provide guidance on when to adjust this value.

### Info

- The NAPI/DQRR architecture is well-documented in the commit message and code comments
- The demux-by-fqd_ctx stash design is a clever solution for sharing a portal across multiple queues in interrupt mode
- The `rte_atomic_load_explicit()`/`rte_atomic_store_explicit()` usage for `napi_sub_dpio` is correct (acquire/release semantics)
- The `ethrx_intr_refcnt` refcount pattern for the portal DQRI inhibit bit is sound
- The 15% throughput cost vs polling (5.0 vs 5.86 Mpps) is documented, which is helpful for users

---

## Patch 4/6: bus/fslmc/dpio: tune DQRI interrupt coalescing holdoff

### Errors

**`dpaa2_dpio_holdoff_to_itp()` does not check `dpio_get_attributes()` return value:**

The function calls `dpio_get_attributes()` and checks `if (dpio_get_attributes(...) == 0)` to set `qman_mhz`, but if the call fails, `qman_mhz` remains 0 (initialized), and the function falls back to `itp = 0xFF`. However, a failed `dpio_get_attributes()` call indicates a more serious problem (MC command failure, invalid token, etc.), and silently defaulting to `0xFF` may hide the error from the caller. The function should propagate the error or at least log a warning.

### Warnings

**Undocumented return value semantics for `dpaa2_dpio_holdoff_to_itp()`:**

The function returns `int`, but the comment says it returns "QBMan ITP units" (a count, not an error code). The function can return `-1` on MC failure (if `dpio_get_attributes()` returns nonzero and you add error checking as suggested above), but the caller in `dpaa2_dev_rx_queue_intr_enable()` treats the return value as a direct ITP value without checking for error. Either the function should return an error via an out-parameter and an `int` status, or it should document that negative return is impossible and cap the result to `[0, 0xfff]`.

**Environment variable override removed for event PMD without migration guide:**

The event PMD previously allowed `DPAA2_PORTAL_INTR_THRESHOLD` and `DPAA2_PORTAL_INTR_TIMEOUT` environment variable overrides. This patch moves those into an event-PMD-specific block within `dpaa2_configure_stashing()`, so they still work for the event PMD, but the net PMD's new `drv_rx_intr_threshold` and `drv_rx_intr_holdoff_us` devargs do not honor the old env vars. This is a behavior change for users who relied on the env vars. The release notes mention the new devargs but do not call out the env var removal. Add a note if this is intentional.

### Info

- The ITP unit conversion formula (`holdoff_us * clk_MHz / 256`) is correct for QBMan ITP
- The 12-bit field cap (`itp > 0xfff ? 0xfff : itp`) is correct
- The `qbman_swp_dqrr_size()` export is a clean way to get the ring depth

---

## Patch 5/6: net/dpaa2: fix Rx queue count for primary process

### Errors

None.

### Warnings

None.

### Info

- Correct fix: the callback was only assigned on the secondary process path, leaving it NULL for primary
- The fix is minimal and appropriate for a bug fix patch
- Properly tagged with `Fixes:` and `Cc: stable at dpdk.org`

---

## Patch 6/6: net/dpaa2: drop the fake software VLAN strip offload

### Errors

None.

### Warnings

**Behavioral change without deprecation notice:**

Removing `RTE_ETH_RX_OFFLOAD_VLAN_STRIP` from the advertised capabilities is an API change that breaks applications relying on the driver to strip VLAN tags. DPDK's ABI/API policy requires a deprecation notice in one release before removing a feature. This patch should either:
1. Add a deprecation notice in `doc/guides/rel_notes/deprecation.rst` in a prior release, or
2. Keep the offload advertised but document it as "software fallback, not recommended" for one release cycle before removal

The release notes mention the change, but there is no prior deprecation notice. This is acceptable if the feature was never a true hardware offload (it was always software), but the policy should be clarified with the maintainers.

### Info

- The performance justification is strong: +18% throughput by avoiding cold mbuf `buf_addr` and payload reads in the Rx burst
- The patch correctly removes all `rte_vlan_strip()` calls from the Rx paths
- The commit message provides detailed performance data (IPC, L2 refills) to justify the change
- The change shifts VLAN strip responsibility to the application, which already reads the L2 header for classification

---

## Summary

### Critical Issues (Errors)

1. **Patch 2**: Missing `rte_dpaa2_intr_disable()` on `epoll_create()` and `epoll_ctl()` error paths (interrupt left enabled on failure)
2. **Patch 3**: `dpni_set_queue()` return value not checked in `dpaa2_dev_rx_queue_setup()` (DEST_NONE clear)
3. **Patch 3**: Potential use-after-free in `dpaa2_dev_rx_queue_intr_enable()` (old DPIO accessed after atomic load without refcount/lock)
4. **Patch 4**: `dpio_get_attributes()` return value not checked in `dpaa2_dpio_holdoff_to_itp()` (silent fallback on MC failure)

### Warnings

1. **Patch 3**: Undocumented quiesce requirement for re-homing queues (no enforcement or assertion)
2. **Patch 3**: Large per-queue NAPI stash (2 KB) with no runtime allocation strategy
3. **Patch 3**: Hardcoded 100us default holdoff with no tuning guidance
4. **Patch 4**: Undocumented return value semantics for `dpaa2_dpio_holdoff_to_itp()`
5. **Patch 4**: Environment variable override removal without migration note
6. **Patch 6**: Behavioral change (VLAN strip removal) without prior deprecation notice

### Positive Observations

- Well-structured patch series with clear functional separation
- Strong performance justification for Patch 6 (detailed perf data)
- Correct use of atomics (`rte_atomic_load_explicit()`/`rte_atomic_store_explicit()`) in NAPI paths
- Good documentation of NAPI/DQRR demux architecture
- Proper `Fixes:` tag and `Cc: stable` on Patch 5

### Recommendations

1. Add `rte_dpaa2_intr_disable()` to error paths in Patch 2
2. Check `dpni_set_queue()` return in Patch 3 and propagate error
3. Document or enforce quiesce requirement for queue re-homing in Patch 3
4. Check `dpio_get_attributes()` return in Patch 4 and log warning on failure
5. Consider adding a deprecation notice for Patch 6 or clarify that software offloads are not subject to deprecation policy