|WARNING| [v2] common/cnxk: fix NPA pool RANGE interrupt handling

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: common/cnxk NPA Pool RANGE Interrupt Handling

## Summary

This patch addresses a spurious interrupt issue with NPA pool RANGE interrupts that occur between pool creation and range installation, which can drain the pool stack on CN10K. The fix involves masking the RANGE interrupt at creation and re-enabling it after the valid range is set.

---

## Errors

None found.

---

## Warnings

### 1. Missing error propagation in sqb_pool_populate()

**Location:** `drivers/common/cnxk/roc_nix_queue.c:1670`

The new call to `roc_npa_pool_range_int_enable()` checks the return code but the error path (`goto npa_fail`) occurs AFTER `roc_npa_aura_limit_modify()` has already executed. If `roc_npa_pool_range_int_enable()` fails, the pool will have a modified limit but the range interrupt will not be enabled, leaving the pool in an inconsistent state.

**Suggested fix:**
Move the range interrupt enable call before `roc_npa_aura_limit_modify()`:

```c
roc_npa_pool_op_range_set(sq->aura_handle, (uint64_t)sq->sqe_mem, iova);

rc = roc_npa_pool_range_int_enable(sq->aura_handle);
if (rc)
	goto npa_fail;

roc_npa_aura_limit_modify(sq->aura_handle, nb_sqb_bufs);
sq->aura_sqb_bufs = nb_sqb_bufs;
```

This ensures that if the interrupt enable fails, the limit has not been modified yet.

### 2. Conditional enablement in cnxk_mempool_populate() may be incorrect

**Location:** `drivers/mempool/cnxk/cnxk_mempool_ops.c:184-186`

The range interrupt is only enabled when `rte_eal_iova_mode() == RTE_IOVA_VA`. However, the patch description and the npa_aura_pool_pair_alloc() comment state that the spurious interrupt issue occurs on CN10K regardless of IOVA mode. If the issue is architecture-specific (CN10K vs CN20K) or universal, the IOVA mode check seems arbitrary.

**Questions to verify:**
- Does the spurious RANGE interrupt issue only occur in IOVA_VA mode?
- Should this be gated on `roc_model_is_cn10k()` instead of IOVA mode?
- Or should it be unconditional (matching sqb_pool_populate(), which has no such condition)?

**Suggested approach:**
If the issue is universal on CN10K, use a platform check:
```c
if (roc_model_is_cn10k() &&
    roc_npa_pool_range_int_enable(mp->pool_id) < 0)
	return -EBUSY;
```
Or if it's truly universal, remove the condition entirely to match sqb_pool_populate().

### 3. Inconsistent error handling between sqb_pool_populate() and cnxk_mempool_populate()

sqb_pool_populate() treats a failure from `roc_npa_pool_range_int_enable()` as fatal (`goto npa_fail`), while cnxk_mempool_populate() returns `-EBUSY` directly. Both should use consistent error codes if the failure modes are the same. If `-EBUSY` is semantically wrong (the pool isn't busy, the interrupt enable operation failed), consider returning the actual error code from the function:

```c
rc = roc_npa_pool_range_int_enable(mp->pool_id);
if (rc < 0)
	return rc;
```

---

## Info

### 1. Comment style preference

**Location:** `drivers/common/cnxk/roc_npa.c:741-745`

The multi-line comment explaining why RANGE is omitted follows Linux kernel style (`/* ... */` block). DPDK prefers the following format for multi-line comments:

```c
/*
 * NPA_POOL_ERR_INT_RANGE omitted: ptr_start/ptr_end are initialized to
 * 0/~0 and the actual IOVA range is set later via roc_npa_pool_op_range_set().
 * On CN10K the RANGE interrupt fires spuriously in the window between pool
 * creation and the range update, causing pool stack drain. Mask it here
 * and rely on the range being set correctly before any alloc/free.
 */
```

### 2. Function documentation

The new public function `roc_npa_pool_range_int_enable()` lacks Doxygen comments. Consider adding:

```c
/**
 * Enable pool range interrupt after valid IOVA range is installed.
 *
 * @param aura_handle
 *   Aura handle.
 * @return
 *   0 on success, negative error code on failure.
 */
int __roc_api roc_npa_pool_range_int_enable(uint64_t aura_handle);
```

### 3. Symbol ordering in exports file

**Location:** `drivers/common/cnxk/roc_platform_base_symbols.c:450`

The new export `RTE_EXPORT_INTERNAL_SYMBOL(roc_npa_pool_range_int_enable)` is placed after `roc_npa_aura_destroy` but before `roc_npa_pool_range_update_check`. For readability, group related functions together. Since this is range-related, placing it adjacent to `roc_npa_pool_range_update_check` would be more logical:

```c
RTE_EXPORT_INTERNAL_SYMBOL(roc_npa_pool_destroy)
RTE_EXPORT_INTERNAL_SYMBOL(roc_npa_aura_destroy)
RTE_EXPORT_INTERNAL_SYMBOL(roc_npa_pool_range_update_check)
RTE_EXPORT_INTERNAL_SYMBOL(roc_npa_pool_range_int_enable)
```

---

## Correctness Review Summary

**Error paths checked:** The new function `roc_npa_pool_range_int_enable()` allocates a mailbox message via `mbox_alloc_msg_*()` and must call `mbox_put()` on all paths. The code correctly does this via the `exit` label on both success and failure paths.

**Resource leaks:** None detected. The `mbox_get()`/`mbox_put()` pairing is correct.

**Return value propagation:** The return code from `mbox_process()` is correctly propagated to the caller. However, see Warning #3 about inconsistent error code handling at the call sites.

---

## Overall Assessment

The patch addresses a real hardware issue with a sensible approach (mask spurious interrupt, enable after range is valid). The code is structurally sound with correct resource management. The main concerns are around consistency of error handling and the correctness of the IOVA mode conditional in cnxk_mempool_populate().