|SUCCESS| event/cnxk: fix TIM HWWQE handling and chunk alignment

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/171478

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

### Resource leak on mempool creation error
In `cnxk_tim_chnk_pool_create()`, when `rte_mempool_set_ops_byname()` fails, the function jumps to the `free:` label which calls `rte_mempool_free()`. However, if the subsequent `cnxk_tim_chnk_pool_populate_aligned()` or `rte_mempool_populate_default()` fails, the same cleanup path is used. The `cnxk_tim_chnk_pool_populate_aligned()` function may have already allocated memory via `rte_zmalloc_socket()` that gets added to the mempool. If `rte_mempool_populate_iova()` fails after the memory is allocated but before objects are added, the memory is freed in that function. But if it returns `0` (no objects added but no error from the mempool API perspective), the code treats this as an error (`rc < 0 ? rc : -ENOMEM`) but the mempool free callback won't be called since the memory was never registered.

**Line 107-112:**
```c
if (tim_ring->disable_npa) {
    rc = cnxk_tim_chnk_pool_populate_aligned(tim_ring->chunk_pool, align);
} else {
    rc = rte_mempool_populate_default(tim_ring->chunk_pool);
}
if (rc < 0) {
```

Actually, looking more carefully at the code in `cnxk_tim_chnk_pool_populate_aligned()` lines 42-47, the function already handles this correctly - it only calls `rte_free(mem)` when `rte_mempool_populate_iova()` returns <= 0, and the comment states "The pool only takes ownership of the memory when objects were added." This is correct. Deleting this finding.

### Missing error propagation check
In `cnxk_tim_chnk_pool_create()`, the return value of `rte_mempool_obj_iter()` at line 123 is not checked. While `rte_mempool_obj_iter()` returns the number of objects iterated (which could be useful for validation), ignoring errors from the callback function could hide initialization failures.

**Line 123:**
```c
rte_mempool_obj_iter(tim_ring->chunk_pool, cnxk_tim_chunk_init,
             tim_ring);
```

The callback `cnxk_tim_chunk_init()` is not shown in the patch, but if it can fail, those failures would be silently ignored. However, without seeing the callback implementation or evidence that it can fail, this is speculation about code not in the patch. Deleting this finding.

## WARNINGS

### Variable alignment calculation duplicated
The `cnxk_tim_chunk_buf_align()` helper is introduced to centralize the alignment logic, but the mempool header alignment in `cnxk_tim_chnk_pool_create()` duplicates the same `roc_model_is_cn20k()` check pattern. Consider using the helper consistently.

**Lines 92-94:**
```c
mp = tim_ring->chunk_pool;
align = cnxk_tim_chunk_buf_align();
mp->header_size = RTE_ALIGN_CEIL(mp->header_size, align);
```

This is actually correct - it gets `align` from the helper and uses it. The helper centralizes the model check. Not an issue.

### Inconsistent chunk size validation
In `cnxk_tim_ring_create()` at lines 304-306, the patch validates `chunk_slots` against min/max bounds before using it, but then at lines 317-320 it clamps the chunk size again after alignment. If the initial validation passes but alignment pushes it over the limit, the final clamp silently corrects it rather than returning an error.

**Lines 304-306:**
```c
if (ring_ctl->chunk_slots >= CNXK_TIM_MIN_CHUNK_SLOTS &&
    ring_ctl->chunk_slots <= CNXK_TIM_MAX_CHUNK_SLOTS)
    tim_ring->chunk_sz = ((uint32_t)ring_ctl->chunk_slots + 1) *
                         CNXK_TIM_CHUNK_ALIGNMENT;
```

**Lines 317-320:**
```c
if (CNXK_TIM_NB_CHUNK_SLOTS(tim_ring->chunk_sz) > CNXK_TIM_MAX_CHUNK_SLOTS)
    tim_ring->chunk_sz = RTE_ALIGN_FLOOR(
        (CNXK_TIM_MAX_CHUNK_SLOTS + 1) * CNXK_TIM_CHUNK_ALIGNMENT,
        CNXK_TIM_CN20K_CHUNK_BUF_ALIGN);
```

The clamping logic ensures the final size is valid after CN20K alignment requirements. The initial validation is on the user-provided `chunk_slots`, while the clamp is on the final aligned `chunk_sz`. These serve different purposes and the pattern is acceptable.

### Magic numbers in response code checks
The response code checks in `cnxk_tim_add_entry_tmo_hwwqe()` use magic numbers `0x1`, `0x3`, `0x4`, `0x5`, `0x2`, `0x10`, `0x50` without named constants or comments explaining their meaning.

**Lines 695, 698, 704, 707, 722, 752, 755, 761:**
```c
if (rsp != 0x1 && rsp != 0x5) {
    switch (rsp) {
    case 0x3:
        ...
    case 0x4:
        ...
    case 0x2:
```

While these are hardware-defined values, using symbolic constants would improve readability. However, this is a style suggestion and the code is functional. The meaning is implicit from the context (response codes from the hardware).

Review-Result: CLEAN