|WARNING| [2/2] event/cnxk: fix out of bounds access in eventdev

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/2: drivers: align cn20k TIM chunk buffers

### Errors

None identified.

### Warnings

**1. Missing release notes**

This patch modifies device-specific alignment requirements and hardware response handling, which affects driver behavior. These changes should be documented in the release notes.

**2. Chunk size alignment may silently increase allocation beyond user request**

In `cnxk_tim_ring_create()`:
```c
if (roc_model_is_cn20k())
    tim_ring->chunk_sz =
        RTE_ALIGN_CEIL(tim_ring->chunk_sz, CNXK_TIM_CN20K_CHUNK_BUF_ALIGN);
```

The user-requested chunk size is modified after validation. If the original size was close to a limit, aligning it upward could exceed memory budgets. Consider adding a comment explaining this behavior or validating the aligned size against any constraints.

**3. Response code interpretation changed without documentation**

In `cnxk_tim_add_entry_hwwqe()`:
```c
case 0x3:
-   tim->state = RTE_EVENT_TIMER_ERROR_TOOEARLY;
+   tim->state = RTE_EVENT_TIMER_ERROR_TOOLATE;
    rc = !rc;
    break;
case 0x4:
-   tim->state = RTE_EVENT_TIMER_ERROR_TOOLATE;
+   tim->state = RTE_EVENT_TIMER_ERROR_TOOEARLY;
    rc = !rc;
    break;
```

The error states for response codes 0x3 and 0x4 are swapped. This changes the user-visible error reporting. The commit message mentions "Update HWWQE response types" but does not explain why the meanings were previously reversed. If this is correcting a prior bug, document that. If it reflects a hardware change, reference the specification version.

A new case `0x5` is added without explanation -- document what this response code means and why it indicates success.

**4. Mempool ops selection logic change**

The original code used `rte_mbuf_platform_mempool_ops()` only when `!disable_npa`, and fell through to inline `rte_mempool_create()` with implicit default ops otherwise. The refactored version explicitly selects `"ring_sp_sc"` or `"ring_mp_mc"` based on `mp_flags & RTE_MEMPOOL_F_SP_PUT`.

However, `mp_flags` is set to 0 at the top of the function, and `RTE_MEMPOOL_F_SP_PUT` is never set. So the condition `mp_flags & RTE_MEMPOOL_F_SP_PUT` is always false, and `"ring_mp_mc"` will always be chosen. If single-producer behavior is needed in some configurations, this logic is broken. If `mp_flags` should be set based on some driver property, that should be done before the ops selection.

### Info

**1. Timeout value change**

```c
-#define TIM_NPA_TMO  0xFFFF
+#define TIM_NPA_TMO  0
```

Changing the NPA timeout from a large value to zero changes allocation retry behavior. If this timeout is passed to a hardware or mempool API, zero may mean "no wait" instead of "wait indefinitely". Confirm this is intentional and does not introduce allocation failures under load.

**2. Alignment applied to mempool header size**

```c
if (roc_model_is_cn20k())
    tim_ring->chunk_pool->header_size = RTE_ALIGN_CEIL(
        tim_ring->chunk_pool->header_size, CNXK_TIM_CN20K_CHUNK_BUF_ALIGN);
```

Modifying `chunk_pool->header_size` after `rte_mempool_create_empty()` but before `rte_mempool_populate_default()` is unusual. Typically alignment is handled by passing the alignment parameter to the create call, or by configuring the mempool ops. Verify this approach is safe -- the mempool code may compute internal layout based on the header size at create time.

**3. Hardware version check uses inequality instead of equality**

```c
-if (dev->tim.feat.hwwqe) {
+if (dev->tim.feat.hwwqe && dev->tim.feat.hwwqe_ver != TIM_HWWQE_VER_0) {
```

The hwwqe feature is now only enabled if `hwwqe_ver != TIM_HWWQE_VER_0`. This implies version 0 hardware does not support the hwwqe feature the same way, but no fallback or error handling is added for version 0. If version 0 exists in the field, this silently disables hwwqe without informing the user.

---

## Patch 2/2: event/cnxk: fix out of bounds access in eventdev

### Errors

**1. Loop bound check incomplete -- still allows out-of-bounds write**

In `parse_queue_param()` and `parse_stash_param()`:
```c
while (tok != NULL && val < (&queue_qos.iaq_prcnt + 1)) {
    *val = atoi(tok);
    tok = strtok(NULL, "-");
    val++;
}
```

The loop condition `val < (&queue_qos.iaq_prcnt + 1)` checks the pointer **before** the write, but `val` is incremented **after** the write. If the loop enters with `val == &queue_qos.iaq_prcnt`, the condition is true, the write happens to the last valid field, then `val++` moves the pointer one past the end. On the next iteration, `tok != NULL` could still be true if there are more tokens, the condition `val < (&queue_qos.iaq_prcnt + 1)` is now false, so the loop exits. This is correct.

However, if `tok` still has more values, the post-loop check `tok != NULL || val != (&queue_qos.iaq_prcnt + 1)` will catch it. So this is actually safe. Disregard this item.

**2. xstats reset logic inverted -- negative queue_port_id resets wrong entity**

In `cnxk_sso_xstats_reset()`:
```c
case RTE_EVENT_DEV_XSTATS_PORT:
    if (queue_port_id >= (int16_t)dev->nb_event_ports)
        goto invalid_value;
    /* ... */
    first_id = (queue_port_id < 0) ? 0 : queue_port_id;
    last_id = (queue_port_id < 0) ? (int16_t)dev->nb_event_ports - 1 : queue_port_id;
    break;
case RTE_EVENT_DEV_XSTATS_QUEUE:
    if (queue_port_id >= (int16_t)dev->nb_event_queues)
        goto invalid_value;
    /* ... */
    first_id = (queue_port_id < 0) ? 0 : queue_port_id;
    last_id = (queue_port_id < 0) ? (int16_t)dev->nb_event_queues - 1 : queue_port_id;
    break;
```

The early bounds check `queue_port_id >= (int16_t)dev->nb_event_ports` rejects negative values (since `queue_port_id` is `int16_t` and `nb_event_ports` is unsigned, negative `queue_port_id` is a large positive number after the implicit cast, so the check fails). This means the "reset all" logic that checks `queue_port_id < 0` is unreachable -- any negative `queue_port_id` was already rejected.

If the intent is to allow negative `queue_port_id` to mean "reset all", the bounds check must be:
```c
if (queue_port_id >= 0 && queue_port_id >= (int16_t)dev->nb_event_ports)
    goto invalid_value;
```

If negative `queue_port_id` is not a valid input, remove the `< 0` branches as dead code.

**3. xstats return value changed from count to zero**

The original `cnxk_sso_xstats_reset()` returned `i` (the number of stats reset). The new version returns `0` on success. This changes the function's contract. If callers expect the return value to indicate how many stats were reset, they will break. Verify this matches the eventdev API expectation for xstats reset (most xstats_reset callbacks return 0 on success, so this is likely correct, but the commit message does not mention the behavior change).

### Warnings

**1. cnt multiplication moved after range check -- potential dead code**

In `cnxk_tim_add_entry_tmo_hwwqe()`:
```c
+   cnt *= CNXK_TIM_ENT_PER_LMT;
    /* SIZEM1 is 0 */
    pa = (tim_ring->tbase & ~0xFF) + TIM_LF_SCHED_TIMER0;
    pa |= (uint64_t)((nb_timers - cnt) << 4);
```

The line `cnt *= CNXK_TIM_ENT_PER_LMT;` is added to convert `cnt` (LMT line count) to a timer count. However, the patch does not show where `cnt` is set or how it is used before this multiplication. If `cnt` was already validated against `nb_timers` before this multiplication, the multiplication could overflow or produce a value larger than `nb_timers`, making the subtraction `nb_timers - cnt` wrap. Verify that `cnt` is bounded such that `cnt * CNXK_TIM_ENT_PER_LMT <= nb_timers`.

**2. xstats reset loop does not short-circuit on out-of-range id**

In the new `cnxk_sso_xstats_reset()`:
```c
for (i = 0; i < n; i++) {
    if (ids[i] < start_offset || (ids[i] - start_offset) >= xstats_mode_count)
        goto invalid_value;
    /* ... */
}
```

If one `ids[i]` is out of range, the function jumps to `invalid_value` and returns `-EINVAL`, failing the entire call. But prior iterations may have already reset some stats. This leaves stats in an inconsistent state (some reset, some not). Consider either validating all IDs in a pre-pass before resetting any, or skipping invalid IDs and continuing (depending on the API contract).

### Info

**1. Explicit casts to int16_t added**

```c
-if (queue_port_id >= (signed int)dev->nb_event_ports)
+if (queue_port_id >= (int16_t)dev->nb_event_ports)
```

The cast changed from `(signed int)` to `(int16_t)`. This is a narrowing change if `nb_event_ports` could be larger than `INT16_MAX`. Verify `nb_event_ports` is bounded to 16 bits.

---

## Summary

**Patch 1/2**: No correctness bugs. Warnings on missing release notes, undocumented behavior changes in error code handling, and mempool ops selection logic that appears broken (always selects `"ring_mp_mc"` because `mp_flags` is never set).

**Patch 2/2**: One significant error -- negative `queue_port_id` (intended to mean "reset all") is rejected by the bounds check, making the "reset all" branches unreachable. One warning about inconsistent partial reset on invalid ID.