|WARNING| common/cnxk: add bulk Rx queue enable/disable

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds a bulk Rx queue enable/disable function `roc_nix_rq_multi_ena_dis()` to batch mailbox operations. The implementation has several correctness issues related to error handling and resource management that must be fixed.

---

## Errors (Must Fix)

### 1. Resource leak on mbox allocation failure in macro loop
**File:** `drivers/common/cnxk/roc_nix_queue.c`  
**Location:** `NIX_RQ_BULK_ENA_DIS_LOOP` macro

The macro calls `mbox_process()` on the first allocation failure, but if the second allocation also fails, the function jumps to `exit` which calls `mbox_put()`. However, if `mbox_process()` succeeded but left the mbox in a partially-processed state before the second allocation failure, queued messages may be lost or responses not consumed.

More critically: if the first `mbox_process()` returns an error (`rc != 0`), the code correctly jumps to exit, but if it succeeds and the second allocation fails, `rc` is set to `-ENOSPC` after a successful `mbox_process()`. The caller cannot distinguish "some queues processed, later ones failed" from "all failed".

**Suggested fix:**
Track how many queues were successfully queued before the failure, and document that partial success is possible. Consider either processing all-or-nothing, or returning the number of queues successfully modified.

---

### 2. Missing error check on `mbox_get()`
**File:** `drivers/common/cnxk/roc_nix_queue.c`  
**Location:** `nix_rq_bulk_ena_dis()` function

```c
struct mbox *mbox = mbox_get((&nix->dev)->mbox);
```

`mbox_get()` can return NULL if the mbox is unavailable. Dereferencing it without checking leads to NULL pointer dereference.

**Suggested fix:**
```c
struct mbox *mbox = mbox_get((&nix->dev)->mbox);
if (!mbox)
	return -EBUSY;
```

---

### 3. Inconsistent error state after partial queue processing
**File:** `drivers/common/cnxk/roc_nix_queue.c`  
**Location:** `roc_nix_rq_multi_ena_dis()` function

When `nix_rq_bulk_ena_dis()` fails after successfully enabling some queues, the function returns an error but some queues may already be in the enabled state. The subsequent loop that checks `need_meta_aura` starts from the beginning of the array, not from where the failure occurred. If an earlier queue's meta aura check fails, queues enabled by a partial `nix_rq_bulk_ena_dis()` remain enabled with no rollback.

**Suggested fix:**
Either ensure `nix_rq_bulk_ena_dis()` is atomic (all succeed or all fail), or track which queues were successfully modified and provide a way to roll back on error. At minimum, document that partial enable/disable is possible and the caller must track state.

---

## Warnings (Should Fix)

### 1. Missing documentation for new API function
**File:** `drivers/common/cnxk/roc_nix.h`  
**Location:** Function declaration

```c
int __roc_api roc_nix_rq_multi_ena_dis(struct roc_nix *roc_nix, struct roc_nix_rq *rqs,
				       int nb_rx_queues, bool enable);
```

This new API function lacks Doxygen documentation. Users need to know:
- Parameter semantics (especially the `rqs` array requirements)
- Whether `rqs[i].qid == UINT16_MAX` is a special sentinel value to skip queues
- Return value semantics (does negative return mean all failed, or partial failure?)
- Whether the operation is atomic or can partially succeed

**Suggested fix:**
Add Doxygen comment block explaining the function contract.

---

### 2. Unlocked mbox state check
**File:** `drivers/common/cnxk/roc_mbox_priv.h`  
**Location:** `mbox_nonempty_nolock()` function

```c
static inline bool
mbox_nonempty_nolock(struct mbox *mbox, int devid)
{
	struct mbox_dev *mdev = &mbox->dev[devid];
	bool ret;

	ret = mdev->num_msgs != 0 && mdev->msg_size != 0;

	return ret;
}
```

The new function reads `mdev->num_msgs` and `mdev->msg_size` without holding `mdev->mbox_lock`. This creates a TOCTOU (time-of-check-time-of-use) race: another thread could modify the mbox state between this check and the subsequent `mbox_process()` call. While the existing code path in `nix_rq_bulk_ena_dis()` holds the mbox via `mbox_get()`/`mbox_put()`, there's no documentation that the caller MUST hold the mbox, and the function name `_nolock` suggests it's safe to call without holding any lock.

**Suggested fix:**
Either document that the caller must hold the mbox (via `mbox_get()`), or add an assertion/check that the mbox is held. Consider renaming to `mbox_nonempty_caller_holds_mbox()` to make the requirement explicit.

---

### 3. Macro hygiene - potential name collision
**File:** `drivers/common/cnxk/roc_nix_queue.c`  
**Location:** `NIX_RQ_BULK_ENA_DIS_LOOP` macro

The macro uses loop variables `i`, `rc`, and local variable `aq` without ensuring they don't collide with surrounding scope. While `i` and `rc` are declared in the calling function's scope (which is intended), `aq` is declared inside the macro's `do-while` block. This is safe, but the macro name doesn't indicate it's a statement block (not just a loop expression).

**Suggested fix (optional):**
Consider naming the macro `NIX_RQ_BULK_ENA_DIS_BATCH` to indicate it's a full statement block, not just a loop helper. Alternatively, add a comment documenting the macro's requirements (caller must declare `i` and `rc`).

---

## Info (Consider)

### 1. Simplification opportunity in error path
**File:** `drivers/common/cnxk/roc_nix_queue.c`  
**Location:** `nix_rq_bulk_ena_dis()` exit label

The `exit:` label appears before `mbox_put(mbox)`. Every error path jumps here. The label name and placement are fine, but consider whether the pattern of "double allocation attempt" in the macro could be simplified by pre-checking mbox capacity.

---

### 2. Sentinel value (UINT16_MAX) not documented in header
**File:** `drivers/common/cnxk/roc_nix.h`

The code skips queues where `rqs[i].qid == UINT16_MAX`, but this sentinel value convention is not documented in the API. Callers might not know they can use this to skip queues.

**Suggested fix:**
Document in the Doxygen comment that `qid == UINT16_MAX` is a sentinel to skip a queue in the array.

---

## Additional Observations

### 1. New `msg_size` check in `mbox_nonempty_nolock()`
The original `mbox_nonempty()` checked only `mdev->num_msgs != 0`. The new `mbox_nonempty_nolock()` adds `&& mdev->msg_size != 0`. This change is not explained in the commit message. If `msg_size == 0` is a valid state when `num_msgs > 0`, this would be a logic bug. If it's a safety check, it should be documented.

---

## Style Notes (No Action Required)

- Code formatting and indentation are consistent with DPDK style.
- The macro `NIX_RQ_BULK_ENA_DIS_LOOP` is appropriately uppercase for a macro.
- Function naming follows `roc_nix_rq_*` convention.

---

## Conclusion

**This patch cannot be merged as-is.** The missing NULL check on `mbox_get()` and the resource leak/error propagation issues in the allocation loop must be fixed. The unlocked mbox state check is a potential race condition that should be addressed. Documentation for the new API is also required.