|WARNING| [v2,18/18] net/dpaa: fix mbuf leak in SG fd creation

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Series Review

## Patch 01/18: bus/dpaa: fix error handling of qman_create_fq

**Errors:**

None.

**Warnings:**

None.

**Info:**

The added cleanup code under `#ifdef CONFIG_FSL_QMAN_FQ_LOOKUP` correctly mirrors the allocation path. The fix addresses a resource leak on error.

---

## Patch 02/18: bus/dpaa: fix fqid endianness

**Errors:**

None.

**Warnings:**

None.

**Info:**

Correct endianness fix. Using `fq->fqid_be` (pre-converted big-endian) instead of `fq->fqid` (host-endian) is consistent with other enqueue paths that communicate with hardware.

---

## Patch 03/18: bus/dpaa: fix error handling in qman_query

**Errors:**

None.

**Warnings:**

None.

**Info:**

The reordering is correct: error check first, then assignment and conversion. The commit message says "optimize error handling to avoid redundant checks" -- the old code performed the assignment and conversion unconditionally, then checked `res` twice (once for the assignment decision, once for the return). The new code skips the assignment/conversion when `res != QM_MCR_RESULT_OK`, which is a minor optimization and clearer logic.

---

## Patch 04/18: net/dpaa: fix modify cgr to use index

**Errors:**

None.

**Warnings:**

None.

**Info:**

Correct indexing fix. The code was always passing `dpaa_intf->cgr_rx` (pointer to the first element) instead of `&dpaa_intf->cgr_rx[queue_idx]` (pointer to the queue-specific element). This would cause all queues to share the configuration of the first CGR entry.

---

## Patch 05/18: net/dpaa/fmlib: add null check in scheme delete

**Errors:**

**1. Null check on wrong variable**

The added null check is `if (p_dev == NULL)`, but `p_dev` is assigned from `DEV_TO_ID(h_scheme)` macro expansion. The commit message says "Add a null pointer check before dereferencing the scheme handle" but the code checks `p_dev`, not `h_scheme`. If the intent is to guard against a null `h_scheme`, the check should be:

```c
if (h_scheme == NULL)
	return E_INVALID_HANDLE;
```

before the `DEV_TO_ID(h_scheme)` expansion. Alternatively, if `p_dev` can legitimately be NULL after `DEV_TO_ID`, the check is correct but the commit message is misleading (it says "scheme handle" but checks the result of the macro). Review the `DEV_TO_ID` macro implementation to determine which variable should be checked. The pattern "matches the defensive pattern used in sibling functions in fm_lib.c" suggests other functions may check `h_scheme` directly or check `p_dev`; verify consistency.

**Warnings:**

None.

---

## Patch 06/18: bus/dpaa: fix BMI RX stats register offset

**Errors:**

None.

**Warnings:**

None.

**Info:**

Register offset correction. The array name change from `reserved0120[16]` to `reserved0140[8]` is consistent with the comment change and implies the reserved region starts at offset 0x140 instead of 0x120/0x130, which would shift all subsequent register offsets. Verify against hardware documentation.

---

## Patch 07/18: bus/dpaa: fix fd leak for ccsr mmap

**Errors:**

None.

**Warnings:**

None.

**Info:**

Correct fd leak fix. The file descriptor is only needed for `mmap()`; keeping it open afterward wastes a resource. Note that the variable is changed from `static int ccsr_map_fd` to `int ccsr_map_fd` in both files, which is also correct -- no need for static storage after the fd is closed.

---

## Patch 08/18: bus/dpaa: fix device probe issue

**Errors:**

None.

**Warnings:**

None.

**Info:**

The logic is correct: for LS1043A, push mode is disabled (due to errata) and the env-var override is skipped. For other SoCs, the env-var is read and clamped. The `else` block ensures the env-var override does not re-enable push on LS1043A. The commit message explanation is thorough.

---

## Patch 09/18: net/dpaa: fix device remove

**Errors:**

None.

**Warnings:**

None.

**Info:**

The added check `if (eth_dev->state != RTE_ETH_DEV_UNUSED)` prevents double-close. The commit message notes that `dpaa_finish()` is removed from the explicit call here but will still run at process exit via `RTE_FINI_PRIO`. This is acceptable for the run-then-exit use case. The message also notes that re-probe in a running process is not supported, which is a limitation but does not make the patch incorrect.

---

## Patch 10/18: net/dpaa: fix invalid check on interrupt unregister

**Errors:**

None.

**Warnings:**

None.

**Info:**

Correct fix. `rte_intr_callback_unregister()` returns the number of callbacks removed (>= 1) on success, or a negative value on error. The old check `if (ret)` logged a warning on every successful unregister. The new check `if (ret < 0)` is correct.

---

## Patch 11/18: net/dpaa: fix port_handle leak in fm_prev_cleanup

**Errors:**

None.

**Warnings:**

None.

**Info:**

The added null check and `fm_port_close()` call prevent a resource leak. Each iteration of the loop was overwriting `dpaa_intf.port_handle` without closing the previous handle.

---

## Patch 12/18: dma/dpaa: fix out-of-bounds access in SG descriptor enqueue

**Errors:**

None.

**Warnings:**

None.

**Info:**

Correct bounds check. If `pending_num` is 0, `num` will be 0 and the access `desc_ssge[num - 1]` underflows. The added check prevents this. The upper bound check (`num > FSL_QDMA_SG_MAX_ENTRY`) guards against other corruption scenarios.

---

## Patch 13/18: net/dpaa: fix xstat name for tx undersized counter

**Errors:**

None.

**Warnings:**

None.

**Info:**

Typo fix. The xstat entry mapping to the 'tund' (TX undersized) field was mislabeled as 'rx_undersized'. The corrected name 'tx_undersized' matches the actual counter.

---

## Patch 14/18: net/dpaa: fix xstat string typos in BMI stats table

**Errors:**

None.

**Warnings:**

None.

**Info:**

Spelling and whitespace fixes in xstat names. All three corrections are valid.

---

## Patch 15/18: net/dpaa: remove duplicate ptype entries

**Errors:**

None.

**Warnings:**

None.

**Info:**

Correct removal of duplicate entries. `RTE_PTYPE_L4_TCP` and `RTE_PTYPE_L4_UDP` were listed twice.

---

## Patch 16/18: net/dpaa: fix wrong buffer in xstats get by id

**Errors:**

**1. Incorrect buffer passed to BMI stats function**

The fix is correct: `fman_if_bmi_stats_get_all()` should be called with `values_copy` (the scratch buffer) not `values` (the output array). However, the loop immediately after this call has a logic error:

```c
for (j = 0; i < stat_cnt; i++, j++)
	values[i] = values_copy[j];
```

This loop continues from wherever `i` left off in the previous loop. If the first loop iterated `DPAA_XSTATS_MAX` times, `i` is now `DPAA_XSTATS_MAX`, and the second loop condition `i < stat_cnt` is checking whether to append BMI stats starting at index `DPAA_XSTATS_MAX`. This is only correct if `stat_cnt == DPAA_XSTATS_MAX + BMI_STATS_CNT`. If `stat_cnt` is smaller, the loop does nothing. If it's exactly that sum, the loop writes BMI stats into `values[DPAA_XSTATS_MAX..]`. But if the first loop did not iterate all the way to `DPAA_XSTATS_MAX` (e.g., if `ids` requested fewer stats), `i` will be less than `DPAA_XSTATS_MAX` and the BMI stats will be written starting at the wrong index, overwriting earlier entries.

The correct logic should reset `i` or use a separate index for the BMI stats portion. For example:

```c
fman_if_bmi_stats_get_all(dev->process_private, values_copy);
for (j = 0; j < num_bmi_stats && (DPAA_XSTATS_MAX + j) < stat_cnt; j++)
	values[DPAA_XSTATS_MAX + j] = values_copy[j];
```

or similar, depending on the intended layout of `values`. The current code appears to assume `i` increments exactly to `DPAA_XSTATS_MAX`, which may not be true.

**Warnings:**

None beyond the error above.

---

## Patch 17/18: net/dpaa: fix null l3_len check in checksum offload

**Errors:**

**1. Early return without error indication**

The added check `if (mbuf->l3_len == 0) return;` silently skips checksum computation when `l3_len` is zero. A zero `l3_len` could indicate an uninitialized or corrupt mbuf. Silently returning means the packet will be transmitted without the requested checksum offload, which may cause protocol errors or silent data corruption on the wire.

Options:
- Log a warning or error before returning (to aid debugging).
- Increment a statistic counter for dropped/corrupted packets.
- Return an error code to the caller (if the function signature supports it; currently it's `void`).

The commit message says "on a corrupt or uninitialized packet" but the code does not signal the problem to the application or logs. This could hide bugs.

**Warnings:**

None beyond the error above.

---

## Patch 18/18: net/dpaa: fix mbuf leak in SG fd creation

**Errors:**

None.

**Warnings:**

None.

**Info:**

Correct mbuf leak fix. The allocated `temp` mbuf must be freed before returning the error.

---

## Summary

**Total Errors:** 3 (Patches 05, 16, 17)

**Total Warnings:** 0

**Patches with no issues:** 01, 02, 03, 04, 06, 07, 08, 09, 10, 11, 12, 13, 14, 15, 18