|WARNING| [v4, 6/6] net/dpaa2: pin Rx queue interrupt to the polling core

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

Reviewing 6 patches from the series against AGENTS.md guidelines.

---

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

### Errors

**NULL function pointer dereference (correctness bug)**
The commit message describes the bug: `eth_dev->rx_queue_count` is left NULL on the primary process path, causing a crash when `rte_eth_rx_queue_count()` is invoked. The patch assigns the callback on the primary path.

The fix itself is correct. However, verify that `dpaa2_dev_rx_queue_count` is implemented and visible in the compilation unit. The patch does not show its definition, but the commit message implies it already exists (used by secondary process).

---

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

### Warnings

**Release notes missing**
The patch moves `dpaa2_hw_dpcon.c` from `drivers/event/dpaa2/` to `drivers/bus/fslmc/portal/` and exports `rte_dpaa2_alloc_dpcon_dev()` / `rte_dpaa2_free_dpcon_dev()` as internal symbols. This is an API change (new internal symbols) but no release notes entry is present. Internal API changes should be documented when they affect multiple subsystems.

**Symbol export macro placement**
The `RTE_EXPORT_INTERNAL_SYMBOL` macros are correctly placed in the `.c` file before the function definitions. No issue here.

---

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

### Warnings

**Error path resource leak (potential)**
In `dpaa2_dpio_intr_init()`, if `epoll_create1()` succeeds but `epoll_ctl()` fails, the code calls `rte_dpaa2_intr_disable()` and closes the epoll fd. However, if `epoll_ctl()` fails, the interrupt was already enabled by `rte_dpaa2_intr_enable()` earlier. The cleanup on the `epoll_ctl()` failure path is correct.

Wait--re-reading the error path:
```c
if (ret < 0) {
    DPAA2_BUS_ERR("epoll_ctl failed");
    rte_dpaa2_intr_disable(dpio_dev->intr_handle, 0);
    close(dpio_epoll_fd);
    return -1;
}
```
The interrupt is disabled, the epoll fd is closed. This is correct. The earlier `rte_dpaa2_intr_enable()` is outside the `if (build_epoll)` block, so the interrupt is enabled regardless of whether the epoll is built. The cleanup is appropriate.

No issue here after full trace.

**Indentation and comment clarity**
The comment block starting "The event PMD dequeues by sleeping..." is clear and explains the conditional epoll creation. No issue.

---

## Patch 4/6: bus/fslmc/mc: implement DPCON set notification

### Info

**New internal API**
The patch adds `dpcon_set_notification()` and exports it with `RTE_EXPORT_INTERNAL_SYMBOL`. The function signature and Doxygen comment are present and clear. The implementation calls `mc_send_command()` after encoding parameters into the MC command structure. No correctness issues identified.

The `struct dpcon_notification_cfg` is added to the public header `fsl_dpcon.h` with Doxygen. This is acceptable for an internal API structure.

---

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

### Errors

**Missing error check on `dpcon_enable()`**
In `dpaa2_dev_rx_queue_setup()`, after allocating the DPCON channel:
```c
ret = dpcon_enable(&dpaa2_q->napi_dpcon->dpcon, CMD_PRI_LOW,
           dpaa2_q->napi_dpcon->token);
if (ret) {
    DPAA2_PMD_ERR("rxq %d: dpcon_enable: %d", rx_queue_id, ret);
    goto err_free_dpcon;
}
```
The error check is present and correctly jumps to the cleanup label. This is correct.

**Potential race on `napi_sub_dpio` atomic store ordering**
In `dpaa2_napi_subscribe()`, the code writes `napi_sub_dpio` with `rte_memory_order_release`:
```c
rte_atomic_store_explicit(&dpaa2_q->napi_sub_dpio, dpio,
        rte_memory_order_release);
```
And reads it in `dpaa2_dev_rx_queue_intr_enable()` with `acquire`:
```c
old = rte_atomic_load_explicit(&dpaa2_q->napi_sub_dpio, rte_memory_order_acquire);
```
This establishes proper synchronization between the subscription and the enable path. The pairing is correct.

However, `dpaa2_dev_rx_queue_intr_unbind()` stores NULL with `release`, and `dpaa2_dev_rx_queue_intr_disable()` loads with `acquire`. The pairing is consistent.

The use of `napi_armed` (plain `uint8_t`) is documented as protected by per-lcore portal isolation, not atomics. The comment states: "napi_armed and ethrx_intr_refcnt are plain (not atomic): these ops run on the queue's owner lcore against its own portal (one portal per lcore), so per-portal isolation keeps them from racing, not control-plane serialization."

This is acceptable given the single-owner model. However, if a queue is re-homed to another lcore without proper synchronization, `napi_armed` could be accessed from multiple threads concurrently. The code does document that re-homing requires the old lcore to be quiesced and the queue disabled there first, so the race is avoided by protocol, not by atomics.

No error here, but the reliance on protocol-level synchronization is noted.

**`ethrx_intr_refcnt` is plain, not atomic**
`ethrx_intr_refcnt` is a `uint16_t`, not atomic, and is incremented/decremented in `dpaa2_dev_rx_queue_intr_enable()` and `dpaa2_dev_rx_queue_intr_disable()`. The comment states these operations run on the portal's owner lcore, so no concurrent access. This relies on the application correctly calling these functions only from the polling lcore.

If the application violates this (calls `intr_enable` from a different lcore), the plain increment/decrement on `ethrx_intr_refcnt` would race. However, the API contract (documented in `dpaa2.rst` and the code comments) requires the caller to invoke these from the polling lcore. Given the documented contract, this is acceptable, though fragile.

Recommendation: add a runtime check in `intr_enable`/`intr_disable` that `rte_lcore_id()` matches the expected lcore for the queue. The code has access to `dpaa2_q->q_storage[rte_lcore_id()]`, which suggests per-lcore storage, but there's no explicit affinity check. Consider adding:
```c
if (rte_lcore_id() != expected_lcore) {
    DPAA2_PMD_ERR("intr_enable must be called from the polling lcore");
    return -EINVAL;
}
```
This is a **Warning**-level suggestion, not an error, because the current code is correct under the documented API contract.

### Warnings

**Release notes entry is present**
The patch adds a line to `doc/guides/rel_notes/release_26_07.rst`:
```
* Added Rx queue interrupt support (rte_eth_dev_rx_intr_*).
```
This is correct.

**PMD features matrix updated**
The patch adds `Rx interrupt = Y` to `doc/guides/nics/features/dpaa2.ini`. This is correct.

**Documentation added**
The patch adds a section "Rx queue interrupts" to `doc/guides/nics/dpaa2.rst` with detailed notes on usage and limitations. This is thorough and helpful.

**Atomic vs `volatile` for `napi_sub_dpio`**
The `napi_sub_dpio` field is declared as `RTE_ATOMIC(struct dpaa2_dpio_dev *)`, which is correct for a shared pointer updated by one thread and read by the interrupt path. The loads and stores use `rte_memory_order_acquire`/`release`, which is appropriate for a producer/consumer pattern (the subscription publishes the portal pointer, the disable path consumes it).

No issue here.

**Error path cleanup in `dpaa2_dev_rx_queue_setup()`**
The `err_free_dpcon:` label frees the DPCON only if `dpcon_allocated` is true (the DPCON was allocated in this call, not a prior one). This avoids double-free on a reconfigure. The logic is correct.

**Interrupt handle allocation sizing**
The interrupt handle is allocated with `priv->nb_rx_queues` as the size. If the queue count changes across a reconfigure, the handle is not reallocated (it's kept across reconfigure). The code documents this: "sized to the queue ceiling, and kept for the life of the port so the application's rx-intr epoll registration stays valid across a reconfigure."

If the queue count shrinks, the excess queues are left with their DPCON channels (the MC refuses to un-schedule an FQ at runtime), and the interrupt vector is oversized but harmless. If the queue count grows beyond the initial ceiling, the code would need to handle it. However, the typical pattern is to configure with the maximum queue count once, then adjust the active count within that ceiling.

The current implementation assumes the queue count does not grow beyond the initial `nb_rx_queues`. If it does, the interrupt handle's vector is too small. The code should either:
1. Document that the queue count cannot grow beyond the initial configure, or
2. Reallocate the interrupt handle if the queue count grows.

Given the comment "sized to the queue ceiling," the intent is (1). The code already refuses to change the interrupt mode after the first configure (`intr_mode_set`), so the pattern is to fix the configuration early.

This is acceptable, but a runtime check in `dpaa2_dev_rx_queue_setup()` would catch violations:
```c
if (rx_queue_id >= rte_intr_nb_efd_get(dev->intr_handle)) {
    DPAA2_PMD_ERR("Queue count exceeds interrupt handle size");
    return -EINVAL;
}
```
This is a **Warning**-level suggestion.

**DPCON pool exhaustion**
The patch documents in `dpaa2.rst` that queue setup fails with `-ENODEV` if the DPCON pool is exhausted. The code in `dpaa2_dev_rx_queue_setup()` calls `rte_dpaa2_alloc_dpcon_dev()`, which returns NULL if no DPCON is available, and the patch handles this:
```c
if (!dpaa2_q->napi_dpcon) {
    DPAA2_PMD_ERR("rxq %d: no DPCON for rx-queue interrupts",
              rx_queue_id);
    return -ENODEV;
}
```
This is correct.

**`dpaa2_napi_drain_portal()` drains DQRR before clearing ISR**
The comment explains the order: drain the DQRR (consuming pending CDANs), then clear the interrupt status register. If the order were reversed, the interrupt would re-assert immediately. The implementation is correct.

**`dpaa2_napi_quiesce_vdq()` checks for stranded frames**
The function waits for a pending VDQ to complete, then checks if it captured a frame. If so, it returns `-EAGAIN` to abort the sleep. This prevents stranding frames in the prefetch pipeline during an interrupt sleep. The logic is sound.

**`dpaa2_napi_set_cdan_dest()` routing**
The function calls `dpcon_set_notification()` to route the DPCON's CDAN to the worker's DPIO. The error is checked and propagated. Correct.

**`dpaa2_napi_subscribe()` re-homing logic**
On re-home, the code drops the subscription on the old portal (`rte_atomic_store_explicit(&dpaa2_q->napi_sub_dpio, NULL, ...)`), then subscribes to the new portal. The comment documents that the old portal must have the queue disabled first (to avoid the `-EBUSY` error). This is a protocol-level requirement enforced by the caller. The code is correct given the documented contract.

**`dpaa2_dev_rx_queue_intr_enable()` concurrency check**
The function checks if the queue is armed on a different portal:
```c
if (old && old != dpio && dpaa2_q->napi_armed) {
    DPAA2_PMD_ERR("rxq %d still armed on another portal; disable it first",
              queue_id);
    return -EBUSY;
}
```
This prevents concurrent arming from multiple lcores and detects protocol violations. Good.

**`dpaa2_dev_rx_queue_intr_disable()` reads `napi_sub_dpio` atomically**
The function loads the portal pointer with `acquire` ordering and only disables the CDAN if the queue is armed. The refcount decrement and portal mask are protected by the single-owner invariant. The eventfd drain is correct (matches dpaa/mlx4/mlx5 patterns).

**Burst mode selection in `dpaa2_dev_start()`**
The code checks that every rx queue has a DPCON if interrupt mode is enabled:
```c
if (!dpaa2_q->napi_dpcon) {
    DPAA2_PMD_ERR("napi: rxq %d has no DPCON channel", i);
    return -ENODEV;
}
```
This prevents dereferencing a NULL `napi_dpcon` in the channel burst functions. Correct.

**`dpaa2_dev_prefetch_rx_common()` and `dpaa2_dev_rx_common()` portal selection**
The code uses `DPAA2_PER_LCORE_ETHRX_PORTAL` for the channel path and `DPAA2_PER_LCORE_PORTAL` for the regular path. The portal affinity functions are called if the portal is not yet set. This is consistent with the model.

**Error queue drain skipped on channel path**
The code skips the error queue drain when `by_channel` is true, with the comment:
```c
/* the error-queue drain runs on the regular portal (DPAA2_PER_LCORE_PORTAL);
 * the channel/interrupt path owns the ethrx portal, so skip it there
 */
```
This is documented in `dpaa2.rst` as a limitation: "The Rx error queue (`drv_err_queue` devarg) is not drained while a queue runs in interrupt mode." Acceptable trade-off.

---

## Patch 6/6: net/dpaa2: pin Rx queue interrupt to the polling core

### Info

**IRQ affinity pinning is best-effort**
The patch pins the DPIO's MSI to the polling core to reduce wake latency. The commit message and code comments document that this is best-effort (requires privilege, irqbalance may move it back). The function `dpaa2_affine_dpio_intr_to_respective_core()` writes `/proc/irq/<n>/smp_affinity`, which is a common pattern in DPDK drivers.

The function returns void and does not propagate errors (the `fopen()` and `fprintf()` failures are silently ignored). This is acceptable for a best-effort optimization--the wake is correct regardless of the affinity.

**Symbol export**
The patch exports `dpaa2_dpio_affine_intr_to_core()` with `RTE_EXPORT_INTERNAL_SYMBOL` and adds the `__rte_internal` declaration to the header. This is correct.

---

## Summary

### Patch 1/6: net/dpaa2: fix Rx queue count for primary process
**Status: Acceptable** (assuming `dpaa2_dev_rx_queue_count` is implemented)

### Patch 2/6: bus/fslmc: move DPCON management from event driver to bus
**Warnings:**
- Release notes entry missing for internal API change (new internal symbols affecting multiple subsystems).