|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

## Summary

This patch addresses a hardware interrupt spuriously firing between NPA pool creation and range installation on CN10K. The fix masks the RANGE interrupt at creation time and re-enables it after the valid range is set.

---

## Errors

None found.

---

## Warnings

### 1. Error handling incomplete in sqb_pool_populate()

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

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

The `npa_fail` label performs cleanup but the error code `rc` is then overwritten on line 1674 by the `roc_npa_aura_limit_modify()` call. If `roc_npa_pool_range_int_enable()` fails, the function will return success (0) or the wrong error code instead of propagating `rc`.

**Suggested fix**: Move the `roc_npa_aura_limit_modify()` call into a conditional that only executes on success:

```c
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;
```

Alternatively, if `roc_npa_aura_limit_modify()` can fail, check its return value separately and preserve the original failure code.

---

### 2. Conditional range interrupt enablement logic unclear

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

```c
if (rte_eal_iova_mode() == RTE_IOVA_VA &&
    roc_npa_pool_range_int_enable(mp->pool_id) < 0)
    return -EBUSY;
```

This enables the range interrupt only in IOVA_VA mode, but the sqb_pool_populate() path (which handles NIX SQ buffer pools) calls `roc_npa_pool_range_int_enable()` unconditionally. The asymmetry suggests either:
- Both paths should enable unconditionally (if the fix applies to all modes), or
- Both paths should have the same conditional logic

**Questions**:
- Does the spurious interrupt issue only affect IOVA_VA mode?
- Why does sqb_pool_populate() enable unconditionally while cnxk_mempool_populate() is conditional?

Document the rationale in code comments or make the behavior consistent across both call sites.

---

### 3. Include directive ordering

**Location**: `drivers/mempool/cnxk/cnxk_mempool_ops.c:5`

```c
#include <rte_eal.h>
#include <rte_mbuf_pool_ops.h>
#include <rte_mempool.h>
```

The new `#include <rte_eal.h>` should come before `<rte_mbuf_pool_ops.h>` alphabetically within the DPDK library includes group.

**Suggested fix**:
```c
#include <rte_eal.h>
#include <rte_mbuf_pool_ops.h>
#include <rte_mempool.h>
```

(Note: This is already correct alphabetically. No change needed unless group separation is required.)

---

## Info

### 1. Comment formatting style

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

The multi-line comment explaining the RANGE interrupt omission does not follow DPDK's standard multi-line comment style (asterisks aligned on the left edge).

**Current**:
```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.
 */
```

**Preferred DPDK style**:
```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. Release notes update missing

This patch fixes a critical hardware issue (spurious interrupts causing pool stack drain) and introduces a new public API function (`roc_npa_pool_range_int_enable`). The current release notes file should document:
- The bug fix (under "Fixed Issues" or "Bug Fixes")
- The new internal API symbol if it's part of the driver ABI

---

### 3. Function documentation

**Location**: `drivers/common/cnxk/roc_npa.h:855`

The new function `roc_npa_pool_range_int_enable()` lacks Doxygen documentation. Since it's exported with `RTE_EXPORT_INTERNAL_SYMBOL`, it should have API documentation explaining:
- Purpose: Enable RANGE/OVFLS/PERR interrupts after pool range is set
- Parameters: `aura_handle` - the aura handle
- Return: 0 on success, negative error code on failure

**Suggested addition before the function declaration**:
```c
/**
 * @internal
 * Enable pool range and error interrupts after valid range is set.
 *
 * @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);
```

---

## Overall Assessment

The patch correctly addresses the root cause (spurious RANGE interrupt before range is set) by deferring interrupt enablement until after the range is installed. The implementation is sound from a correctness perspective. The primary concerns are:

1. **Error propagation** in sqb_pool_populate() (Warning #1) - verify this doesn't silently drop errors
2. **Conditional logic asymmetry** between the two call sites (Warning #2) - document or unify
3. Missing release notes and API documentation (Info #2, #3)

After addressing these, the patch will be ready for merge.