|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. Memory leak on probe failure - `security_instance` not freed

**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`  
**Line:** `goto init_error;` at line 3808

The `security_instance` allocated at line 3790 (`rte_zmalloc_socket(...)`) is not freed in any of the error paths (`init_error3`, `init_error2`, `init_error1`, `init_error`). After `goto init_error;` at line 3808, the code jumps to line 3905 which only frees `dev->security_ctx` (line 3663) in `dpaa_sec_uninit()`, but in the probe path `cryptodev->security_ctx` is assigned to `security_instance` at line 3795, so `init_error:` should free it explicitly.

**Suggested fix:**
```c
init_error:
	DPAA_SEC_ERR("driver %s: create failed", cryptodev->data->name);
	rte_free(security_instance);

	return -1;
```

---

### 2. Missing `close(map_fd)` on all error paths

**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`  
**Lines:** Error paths at 3808, 3830, 3847, 3868

The `map_fd` opened at line 3779 (`open()`) is not closed on any error path. The `close(map_fd)` at line 3812 is only executed if `cmd_map` initialization succeeds. If any `goto init_errorN;` is taken before the normal cleanup, `map_fd` leaks.

**Suggested fix:**  
Add `if (map_fd >= 0) close(map_fd);` to the `init_error:` label:
```c
init_error:
	DPAA_SEC_ERR("driver %s: create failed", cryptodev->data->name);
	rte_free(security_instance);
	if (map_fd >= 0)
		close(map_fd);

	return -1;
```

---

### 3. `security_instance` not freed on later error paths

**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`  
**Lines:** Labels `init_error1`, `init_error2`, `init_error3`

After `security_instance` is allocated (line 3790) and assigned to `cryptodev->security_ctx` (line 3795), all later error paths (`init_error1`, `init_error2`, `init_error3`) fall through to `init_error:` without freeing it. Only `dpaa_sec_uninit()` frees `dev->security_ctx`, but that is called from the normal teardown path, not from probe failure.

**Suggested fix:**  
All error labels should eventually reach a cleanup that frees `security_instance`. The fix for Error #1 handles this if all labels fall through to `init_error:`.

---

### 4. `munmap(cmd_map)` not called on error paths after successful `mmap()`

**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`  
**Lines:** After line 3802 (`mmap()` succeeds), error paths `init_error1/2/3`

If `mmap()` succeeds (line 3802) but a later error occurs (`goto init_error1/2/3;`), `cmd_map` is not unmapped. The normal cleanup path calls `munmap(dpaa_ioctl_portal)` at line 3814, but error paths bypass this.

**Suggested fix:**  
Track whether `mmap()` succeeded and unmap on error:
```c
init_error:
	DPAA_SEC_ERR("driver %s: create failed", cryptodev->data->name);
	rte_free(security_instance);
	if (cmd_map != MAP_FAILED && cmd_map != NULL)
		munmap(cmd_map, DPAA_SEC_IOCTL_PORTAL_SIZE);
	if (map_fd >= 0)
		close(map_fd);

	return -1;
```

---

## WARNINGS

### 1. Variable `num` type inconsistency

**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`  
**Line:** 3731

`num` is declared `int` but `qman_alloc_fqid_range()` returns `int` and is checked `< 0` for error. However, it is also used to store positive counts up to `RTE_DPAA_MAX_RX_QUEUE` (64). While `int` suffices for the values involved, using `int` for a count that is compared against unsigned queue limits creates mixed signedness comparisons. Consider `unsigned int` for clarity, or keep `int` with explicit casts in comparisons.

**Suggested fix (optional):**  
Declare `num` as `unsigned int` and check `(int)num < 0` for the error case, or leave as-is if all comparisons are intentionally signed.

---

### 2. Release notes required for exported symbols and API changes

**Files:** `drivers/bus/dpaa/dpaa_bus_base_symbols.c`, `drivers/bus/dpaa/include/fsl_qman.h`  
**Lines:** Exports `qman_release_fqid_range`, `qman_destroy_fq`, changes `qman_shutdown_fq()` signature

This patch exports two previously internal symbols (`qman_release_fqid_range`, `qman_destroy_fq`) and changes the signature of `qman_shutdown_fq()` from `int qman_shutdown_fq(u32 fqid)` to `int qman_shutdown_fq(struct qman_fq *fq)`. These are ABI and API changes that require release notes. A compatibility wrapper `qman_shutdown_fq_by_fqid()` is added, but the original function's signature change is still an ABI break for any external code calling the old signature.

**Suggested fix:**  
Add a section to `doc/guides/rel_notes/release_26_11.rst` (or current release notes) documenting:
- New exported symbols: `qman_release_fqid_range`, `qman_destroy_fq`
- Changed function signature: `qman_shutdown_fq()` now takes `struct qman_fq *` instead of `u32 fqid`
- Availability of compatibility wrapper `qman_shutdown_fq_by_fqid()`

---

### 3. Exported symbols should use `RTE_EXPORT_INTERNAL_SYMBOL` in declaration order

**File:** `drivers/bus/dpaa/base/qbman/qman.c`  
**Line:** 1591

`RTE_EXPORT_INTERNAL_SYMBOL(qman_destroy_fq)` is placed immediately before the function definition. While this is acceptable, the DPDK convention is to place export macros in the corresponding `*_base_symbols.c` file (as done for most symbols). This one is added both in `qman.c` (line 1591) and in `dpaa_bus_base_symbols.c` (line 81). Having it in both places is redundant -- the build system only needs it in one.

**Suggested fix:**  
Remove the `RTE_EXPORT_INTERNAL_SYMBOL(qman_destroy_fq)` from line 1591 in `qman.c`, since it is already listed in `dpaa_bus_base_symbols.c` at line 81. Keep only the entry in `dpaa_bus_base_symbols.c`.

---

## INFO

### 1. Error message formatting inconsistency (pre-existing pattern, not introduced)

**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`  
**Lines:** Multiple new error messages (3646, 3649, 3731, etc.)

The new error messages use mixed formats: some prefix with `%s:` (device name), others do not. For example:
- Line 3646: `"%s: Failed(%d) to shutdown TXQ(0x%08x), fqid not released"`
- Line 3801: `"Failed(%d) to alloc %d fqids"` (no `%s:`)

This inconsistency is minor but reduces log clarity. Consider prefixing all error messages with `cryptodev->data->name` for uniform context.

---

### 2. Variable shadowing: `ret` reused in nested error-path cleanup loops

**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`  
**Lines:** 3852, 3878, etc.

In `init_error3:` and `init_error1:` cleanup loops, the loop variable `ret` is reused from the outer function scope (`int ret` declared at line 3731). While this is not a bug (the outer `ret` is already in an error state and will not be used again), reusing the same name for a loop-local error check reduces clarity. Consider using a distinct name like `ret2` or `shutdown_ret` for the cleanup loop.

**Example:**
```c
int shutdown_ret;
for (j = 0; j < i; j++) {
	shutdown_ret = qman_shutdown_fq(&internals->inq[j]);
	if (shutdown_ret) {
		DPAA_SEC_ERR("...", shutdown_ret, ...);
	}
}
```

---

### 3. Pre-existing: `dpaa_sec_uninit()` could return early on `dev->data == NULL`

**File:** `drivers/crypto/dpaa_sec/dpaa_sec.c`  
**Line:** 3621

The check `if (!dev) return -ENODEV;` does not verify `dev->data`. While the caller (rte_cryptodev layer) should ensure `dev->data` is valid, defensive programming would add:
```c
if (!dev || !dev->data)
	return -ENODEV;
```
This is a hardening suggestion, not a bug in this patch.

---

Review-Result: ERROR