|WARNING| drivers: improve DPAA crypto fq resource handling

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

### 1. Missing error checks on portal initialization
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`, line 3627

```c
if (unlikely(!DPAA_PER_LCORE_PORTAL)) {
    ret = rte_dpaa_portal_init((void *)0);
    if (ret) {
        DPAA_SEC_ERR("Failure in affining portal");
        return ret;
    }
}
```

**Issue:** After the early return on portal init failure, the function continues to access resources that were never initialized. If `rte_dpaa_portal_init()` fails, `dpaa_sec_uninit()` should clean up whatever it can before returning, not just bail out. More importantly, later code in this function calls `qman_shutdown_fq()` which requires a portal - if the init failed, those calls will fail too, potentially leaking FQIDs.

**Fix:** Either proceed with cleanup that doesn't require a portal (releasing already-allocated FQIDs if any), or ensure the early return path is safe. The current code assumes that if we reach this point, all queues were created and need shutdown, which requires a working portal.

### 2. Integer overflow in FQID range release logic
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`, lines 3640-3651, 3654-3665

```c
if (run && (ret || fqid != base + run)) {
    qman_release_fqid_range(base, run);
    run = 0;
}
```

**Issue:** The expression `base + run` can overflow if `base` is near `UINT32_MAX` and `run` is large. While this is unlikely in practice (FQIDs are allocated in ranges), the check `fqid != base + run` should use subtraction to avoid overflow: `fqid - base != run`.

**Fix:**
```c
if (run && (ret || (fqid < base) || (fqid - base != run))) {
    qman_release_fqid_range(base, run);
    run = 0;
}
```

### 3. Resource leak on error path - security context
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`, lines 3796-3807

When `qman_alloc_fqid_range()` fails after the security context is allocated (line 3795), the code jumps to `init_error`, which calls `rte_free(dev->security_ctx)` only if `cmd_map` mapping succeeded. But the first `qman_alloc_fqid_range()` failure happens before `cmd_map` is mapped, so `map_fd` is still -1, and the security context leaks.

Looking at the `init_error` label (line 3904):
```c
init_error:
    DPAA_SEC_ERR("driver %s: create failed", cryptodev->data->name);

    rte_free(cryptodev->security_ctx);
```

Actually, the `init_error` label at line 3904 *does* free the security context unconditionally. But the new error paths `init_error3`, `init_error2`, `init_error1` all jump to `init_error` at the end, so they should be covered. Let me re-check...

Upon closer inspection, the error paths do eventually reach `init_error` which frees the security context. However, the code at line 3904 only frees `cryptodev->security_ctx`, not `dev->security_ctx`. Given that `cryptodev` is the parameter name, this should be correct. 

Actually, reviewing the full flow: all new error labels (`init_error1`, `init_error2`, `init_error3`) fall through to `init_error`, so the security context should be freed. I was wrong about this leak - the existing cleanup at `init_error` handles it. Removing this item.

### 4. No check for `qman_alloc_fqid_range()` partial success before release
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`, line 3805

```c
if (num < (int)internals->max_nb_queue_pairs) {
    if (num < 0) {
        DPAA_SEC_ERR("%s: Failed(%d) to alloc %d TX fqids",
            cryptodev->data->name, num, internals->max_nb_queue_pairs);
    } else {
        DPAA_SEC_ERR("%s: Alloc %d fqids < %d",
            cryptodev->data->name, num, internals->max_nb_queue_pairs);
        qman_release_fqid_range(fqids[0], num);
    }
    goto init_error;
}
```

**Issue:** When `num < 0`, the code does NOT call `qman_release_fqid_range()` but jumps directly to `init_error`. However, `qman_alloc_fqid_range()` may have allocated some FQIDs before failing (partial success is not documented as impossible). The `fqids` array could contain garbage or uninitialized values in the negative return case.

**Fix:** Only call `qman_release_fqid_range()` when `num > 0`:
```c
if (num < (int)internals->max_nb_queue_pairs) {
    if (num > 0) {
        DPAA_SEC_ERR("%s: Alloc %d fqids < %d",
            cryptodev->data->name, num, internals->max_nb_queue_pairs);
        qman_release_fqid_range(fqids[0], num);
    } else {
        DPAA_SEC_ERR("%s: Failed(%d) to alloc %d TX fqids",
            cryptodev->data->name, num, internals->max_nb_queue_pairs);
    }
    goto init_error;
}
```

### 5. Use-after-destroy in qman_shutdown_fq()
**File:** `drivers/bus/dpaa/base/qbman/qman.c`, lines 2991-2993

```c
if (!ret) {
    fq->state = qman_fq_state_oos;
    qman_destroy_fq(fq, 0);
}
```

**Issue:** After `qman_destroy_fq()` is called, `fq` is in an undefined state and should not be accessed. However, the current code is internal to qman and the FQ is owned by the caller, so writing to `fq->state` before destroy is safe. But the semantic question is whether the caller expects the FQ to be destroyed automatically.

Looking at the old vs new signature:
- Old: `qman_shutdown_fq(u32 fqid)` - took an FQID, caller retained FQ struct
- New: `qman_shutdown_fq(struct qman_fq *fq)` - takes FQ pointer, now destroys it

The problem is that callers in `dpaa_sec.c` iterate over `internals->qps[i].outq` and `internals->inq[i]`, calling `qman_shutdown_fq(&internals->qps[i].outq)`. After this call returns successfully, the embedded FQ struct inside `qps[i]` has been destroyed. This is use-after-destroy if any code later accesses those FQ fields.

Actually, looking at the shutdown+destroy pattern more carefully: `qman_destroy_fq()` is documented as requiring the FQ to be in 'out-of-service' or 'parked' state. The shutdown sequence brings it to OOS, then destroy is called. This is correct IF the caller no longer needs the FQ struct. But since the FQ is embedded in `internals->qps[i]` or `internals->inq[i]`, destroying it is wrong - the memory is still part of the larger structure.

**Fix:** Do NOT call `qman_destroy_fq()` inside `qman_shutdown_fq()`. Let the caller decide when to destroy. Revert the automatic destroy:

```c
if (!ret) {
    fq->state = qman_fq_state_oos;
}
```

The caller should explicitly call `qman_destroy_fq()` if needed, but in the case of embedded FQs, they should NOT be destroyed - only shut down.

### 6. Uninitialized struct qman_fq in qman_shutdown_fq_by_fqid()
**File:** `drivers/bus/dpaa/include/fsl_qman.h`, line 1904

```c
static inline int qman_shutdown_fq_by_fqid(u32 fqid)
{
    struct qman_fq fq;

    memset(&fq, 0, sizeof(struct qman_fq));
    fq.fqid = fqid;
    return qman_shutdown_fq(&fq);
}
```

**Issue:** After `memset(&fq, 0, ...)`, `fq.qp` is NULL. The new `qman_shutdown_fq()` checks `if (!p)` and falls back to `get_affine_portal()`, which is correct. However, the FQ state is not properly initialized. The original code assumed an FQ created with `qman_create_fq()` which sets up internal state. A zero'd FQ may not be safe to pass to `qman_shutdown_fq()`.

More critically, after `qman_shutdown_fq(&fq)` returns successfully, the code calls `qman_destroy_fq(fq, 0)` (in the modified qman.c). But `fq` is a local stack variable in `qman_shutdown_fq_by_fqid()`, so the destroy operates on a temporary. This works because `qman_destroy_fq()` doesn't actually free memory (it just clears state), but it's semantically wrong.

The real issue is the automatic destroy in `qman_shutdown_fq()` - see Error #5 above.

## WARNINGS

### 1. Missing release notes entry
**File:** Not present

**Issue:** This patch changes the signature of `qman_shutdown_fq()` from `(u32 fqid)` to `(struct qman_fq *fq)`, adds new exports (`qman_destroy_fq`, `qman_release_fqid_range`), and significantly changes resource cleanup behavior. These are API changes that should be documented in release notes.

**Fix:** Add an entry to `doc/guides/rel_notes/release_25_03.rst` (or current release) documenting the API changes and the improved resource handling.

### 2. Function parameter comparison style
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`, line 3620

```c
if (!dev)
    return -ENODEV;
```

**Good** - explicit comparison against NULL preferred for pointers, but `!dev` is acceptable for boolean-like checks. No change needed, just noting for consistency with DPDK style which prefers `if (dev == NULL)`.

Actually, reviewing AGENTS.md guidelines: "DPDK style requires explicit comparison against `NULL`, `0`, or `'\0'` rather than truthiness on pointers". So this should be:

```c
if (dev == NULL)
    return -ENODEV;
```

This is a **Warning** level style issue, not an error, since the code is functionally correct.

### 3. Inconsistent error message formatting
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`

The patch introduces error messages like:
- Line 3646: `"Failed(%d) to shutdown TXQ(0x%08x)"`  
- Line 3657: `"Failed(%d) to shutdown RXQ[%d](0x%08x)"`

But some error messages omit the device name (e.g., line 3826), while others include it (line 3815). For consistency, all error messages during init/uninit should include the device name for easier debugging.

**Fix:** Ensure all error messages consistently include the device name as in line 3815.

### 4. Potential infinite loop risk in error cleanup
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`, lines 3886-3902

The nested cleanup loops in the error paths iterate over `i` or `j` queues, calling `qman_shutdown_fq()` on each. If `qman_shutdown_fq()` itself fails internally and enters an infinite loop or hangs, the cleanup cannot proceed. While this is unlikely, consider adding a timeout or retry limit to the shutdown calls in error paths.

This is more of a robustness suggestion than a clear bug, so marking as **Warning**.

## INFO

### 1. Consider consolidating duplicate cleanup code
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`

The FQID range release logic appears three times with near-identical code:
- Lines 3640-3665 (uninit RX/TX queues)  
- Lines 3847-3873 (init_error3 RX queues)  
- Lines 3880-3902 (init_error1 TX queues)

**Suggestion:** Extract this into a helper function to reduce duplication and potential bugs from inconsistent updates.

### 2. Unsigned comparison always true
**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`, line 3798

```c
if (num < (int)internals->max_nb_queue_pairs) {
```

`max_nb_queue_pairs` is unsigned (likely `uint32_t` or `uint8_t`). The cast to `int` is necessary because `num` is `int` and can be negative. However, this cast could overflow if `max_nb_queue_pairs` exceeds `INT_MAX`. Given that `RTE_CRYPTODEV_PMD_MAX_NB_QUEUE_PAIRS` is typically small (64 or 128), this is unlikely, but worth noting.

**Suggestion:** Add a compile-time assert or comment that `max_nb_queue_pairs` is always less than `INT_MAX`.

---

## FINAL VERIFICATION

Re-reading the Errors section to ensure evidence is present:

1.  Portal init error path - can quote line 3629, path continues to use portal-dependent functions
2.  Integer overflow - can quote line 3640, expression `base + run` overflows when base near UINT32_MAX
3.  Security context leak - REMOVED (cleanup at init_error handles it)
4.  qman_alloc_fqid_range partial success - can quote line 3805, num < 0 case doesn't check fqids validity
5.  Use-after-destroy - can quote line 2993, qman_destroy_fq called on FQ still referenced by caller
6.  Uninitialized FQ - can quote line 1906, zero'd FQ passed to shutdown which expects initialized state

All remaining errors have direct line quotes and clear defects.

Review-Result: ERROR