|WARNING| [v6,19/19] net/dpaa: add ONIC port checks

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review - series.patch

## Summary

Reviewed 19 patches from the DPAA driver series. Found **several critical correctness issues** (resource leaks, missing error checks, potential use-after-free) and some style/process items. The majority of patches are refactoring and hardware configuration improvements.

---

## Patch-by-Patch Findings

### Patch 01/19: bus/dpaa: refine fman naming and fix global scope

**Errors:**
- **Missing `fman_ccsr_map_fd` declaration**: The patch removes `extern int fman_ccsr_map_fd;` from `include/fman.h` (line 476 in diff) but adds `extern int fman_ccsr_map_fd;` to `fman_hw.c` (line 19 in diff). If `fman_ccsr_map_fd` is now static (as the commit message claims), this `extern` declaration in `fman_hw.c` is wrong. If it's not static, removing it from the header breaks encapsulation. The patch does not show `fman_ccsr_map_fd` being made `static` anywhere.

**Warnings:**
- None

**Info:**
- Renaming `ccsr_map` to `memac_map` and `bmi_map` to `rx_bmi_map` improves clarity (these are indeed MEMAC and RX BMI register spaces, not generic CCSR). However, the `fman_ccsr_map_fd` handling is inconsistent.

---

### Patch 02/19: bus/dpaa: scan max BPID from DTS

**Errors:**
- **Missing error check propagation** (line 235-240 in `qman_global_init`): `bman_init_ccsr()` can fail and returns an error, but the code only logs the error and continues with `done = 1;` at line 249. This leaves `bman_pool_max` and other globals in an inconsistent state if CCSR init fails. Should `return ret;` instead of falling through.

**Warnings:**
- None

**Info:**
- Dynamic BPID range calculation from DTS is a good improvement over hardcoded values.

---

### Patch 03/19: drivers: add BMI Tx statistics

**Errors:**
- None

**Warnings:**
- None

**Info:**
- Straightforward addition of Tx BMI stats fields to match existing Rx pattern. Clean.

---

### Patch 04/19: drivers: add process-type guards for secondary process

**Errors:**
- **`getenv()` calls in portable library code** (lines 1349-1374 in `dpaa_qdma.c`):  
  `getenv()` is called for `DPAA_QDMA_DATA_VALIDATION`, `DPAA_QDMA_HW_ERR_CHECK`, `DPAA_QDMA_SG_ENABLE`, `DPAA_QDMA_SG_MAX_ENTRY_SIZE`, and `DPAA_QDMA_PCI_READ`.  
  These are in `drivers/dma/dpaa/`, which is a library. Per guidelines, `getenv()` is forbidden in `lib/` and `drivers/`.  
  Use devargs or driver parameters instead.

- **Missing secondary process handling in crypto**: The patch removes the `RTE_PROC_PRIMARY` check from `dpaa_sec_probe` (line 3785 deleted). This means secondary processes will now attempt full device initialization, which is wrong. The check should remain (secondary processes should return early, not initialize hardware).

**Warnings:**
- None

---

### Patch 05/19: bus/dpaa: define helpers for qman channel and wq

**Errors:**
- None

**Warnings:**
- None

**Info:**
- `GENMASK` and `qm_fqd_get_{chan,wq}` helper functions are good cleanup, replacing open-coded bit shifts.

---

### Patch 06/19: drivers: shutdown DPAA FQ by fq descriptor

**Errors:**
- None

**Warnings:**
- Redundant shutdown in `dpaa_dev_init` (line 2368 in `dpaa_ethdev.c`): The comment says "Shutdown FQ before configure to clean the queue", but `qman_shutdown_fq()` is already called in `dpaa_eth_dev_close()` (which is called during device stop). This may be intentional for reconfiguration, but should be verified that it doesn't double-shutdown FQs.

**Info:**
- Passing the full `struct qman_fq` (with its `qp` portal pointer) to `qman_shutdown_fq()` is correct for affine portal access.

---

### Patch 07/19: bus/dpaa: improve FQ shutdown with channel validation

**Errors:**
- None (the original code had a bug where `sdqcr` was not restored if it wasn't changed; this patch fixes that by checking `if (sdqcr != p->sdqcr)` before restoring, which is correct).

**Warnings:**
- None

---

### Patch 08/19: bus/dpaa: enhance DPAA FQ shutdown

**Errors:**
- **`qman_find_fq_by_cgrid` unbounded loop** (lines 2972-3004 in `qman.c`):  
  The function loops with `do { ... fq.fqid++; } while (1);` and only breaks when `err == -ERANGE` (no more FQs) or a matching FQID is found. If `qman_query_fq_np()` or `qman_query_fq()` always succeeds but never finds a match, this is an infinite loop. The loop should have a bound (e.g., `fq.fqid < max_fqid`).

**Warnings:**
- None

---

### Patch 09/19: drivers: add DPAA cgrid cleanup support

**Errors:**
- **Potential double-free of CGR** (lines 582-629 in `dpaa_ethdev.c`):  
  In `dpaa_eth_dev_close`, the code does:
  ```c
  ret = qman_find_fq_by_cgrid(...);
  if (!ret) {  // found an FQ still using this CGRID
      ret = qman_shutdown_fq_by_fqid(fqid);
      ...
  }
  ret = qman_delete_cgr(&dpaa_intf->cgr_rx[loop]);
  ```
  If `qman_find_fq_by_cgrid` returns 0 (FQ found), it then calls `qman_delete_cgr()` regardless of whether `qman_shutdown_fq_by_fqid` succeeded. If the FQ shutdown failed, the CGRID is still in use, and deleting the CGR causes a use-after-free. Should check shutdown result before calling `qman_delete_cgr()`.

- **CGRID range release without checking all FQs** (line 604):  
  `qman_release_cgrid_range()` is called immediately after the per-queue loop. If a later CGRID in the range is still in use by an FQ not in this device's queue list, releasing it causes a use-after-free. The code should verify *all* CGRIDs in the range are unused before calling `qman_release_cgrid_range()`.

**Warnings:**
- None

---

### Patch 10/19: net/dpaa: clean Tx confirmation FQ on device stop

**Errors:**
- None

**Warnings:**
- None

**Info:**
- Adds missing cleanup of `tx_conf_queues` in `dpaa_eth_dev_close` and `dpaa_dev_init` error path. Good fix.

---

### Patch 11/19: net/dpaa: remove redundant FQ shutdown from Rx queue setup

**Errors:**
- None (the patch removes `qman_shutdown_fq()` from `dpaa_eth_rx_queue_setup` because FQ shutdown is already handled in device stop/close. The comment in Patch 06 about "shutdown before configure" suggests this was intentional redundancy, but if shutdown is reliably done in close, this removal is correct).

**Warnings:**
- None

---

### Patch 12/19: net/dpaa: optimize FM deconfig

**Errors:**
- **Double VSP cleanup call** (lines 592-597 and 679-685 in `dpaa_ethdev.c`):  
  `dpaa_port_vsp_cleanup()` is called twice in `dpaa_eth_dev_close()`: once at line 592 (before FQ/CGR cleanup) and again at line 679 (after). The second call will attempt to free already-freed VSP handles. Should only be called once.

**Warnings:**
- None

---

### Patch 13/19: bus/dpaa: improve log macro and fix bus detection

**Errors:**
- **Bus detection logic in `dpaa_bus_dev_compare` is wrong** (lines 563-591 in `dpaa_bus.c`):  
  `dpaa_bus_dev_compare()` is a name comparison function (returns 0 if names match, 1 if not). The patch adds bus detection (sysfs path check, `dpaa_bus.detected` flag, `pthread_key_create`) into this function. This is called for *every device name comparison*, not just once at bus initialization. The detection code will run multiple times, and `pthread_key_create()` will fail on subsequent calls (key already exists). Bus detection should be in `rte_dpaa_bus_probe()` or a separate init function, not in a comparison callback.

**Warnings:**
- None

---

### Patch 14/19: net/dpaa: optimize FMC MAC type parsing

**Errors:**
- **`dpaa_port_fmc_get_idx_from_name` can return negative error code, but callers treat it as index** (lines 207-242 in `dpaa_fmc.c`):  
  `dpaa_port_fmc_get_idx_from_name()` returns `int`. It can return `-EINVAL` (line 228). The caller at line 244 assigns this to `int num` and then uses `num` directly as an index (line 260: `if (fif->mac_idx == num)`). If `num` is negative, this comparison is wrong. Should check `if (num < 0) return num;` immediately after the call.

**Warnings:**
- None

---

### Patch 15/19: net/dpaa: report error on using deferred start

**Errors:**
- None

**Warnings:**
- None

**Info:**
- Correctly rejects deferred start in Rx and Tx queue setup (DPAA HW does not support it).

---

### Patch 16/19: drivers: optimize DPAA multi-entry buffer pool operations

**Errors:**
- None

**Warnings:**
- None

**Info:**
- Replaces hardcoded `8` with `FSL_BM_BURST_MAX` and uses a single HW descriptor initialization. Clean refactoring.

---

### Patch 17/19: drivers: release DPAA bpid on driver destructor

**Errors:**
- **`RTE_FINI_PRIO` value 104 is arbitrary** (line 528 in `dpaa_mempool.c`):  
  The guidelines mention `RTE_PRIORITY_104` but this is not a standard priority. Should use a named priority from `rte_common.h` or document why 104 is chosen. This is not a correctness bug but is poor practice.

- **Potential use-after-free in `dpaa_mpool_finish`** (lines 530-542):  
  The destructor iterates `s_dpaa_bpid_allocated_flag[]` and calls `bman_free_bpid()` for each used BPID. However, `bman_free_bpid()` may call `bm_pool_set()` (line 241 in `bman.c`), which accesses `bman_pool` structures. The comment at line 540-542 says "The rte_dpaa_bpid_info and bman_pool from EAL mem have been released with EAL mem pool being destroyed." If those are already freed when this destructor runs, `bman_free_bpid()` accesses freed memory. The destructor priority may need adjustment to run *before* EAL mempool cleanup.

**Warnings:**
- Mempool cache threshold adjustment (lines 149-156 in `dpaa_mempool.c`): The code directly modifies `mp->local_cache[lcore_id].flushthresh`. This is internal to the mempool library and may break if the mempool implementation changes. Consider if there's a public API for this (there isn't, so this is a necessary hack, but should be commented as such).

---

### Patch 18/19: dma/dpaa: add SG data validation and ERR050757 fix

**Errors:**
- **`getenv()` calls in driver init** (lines 12-15, 1349-1374 in `dpaa_qdma.c`):  
  Same issue as Patch 04 (which *added* these `getenv()` calls). The static variables `s_data_validation`, `s_hw_err_check`, `s_sg_enable`, `s_sg_max_entry_sz`, and `s_pci_read` are initialized from environment variables. This is forbidden in drivers; use devargs instead.

**Warnings:**
- None

**Info:**
- ERR050757 workaround (PCI read length limit) is guarded by `#ifdef RTE_DMA_DPAA_ERRATA_ERR050757` and only applied when `s_pci_read` is set. The logic looks correct.

---

### Patch 19/19: net/dpaa: add ONIC port checks

**Errors:**
- **Missing `default_q` flag check in VSP cleanup** (line 593 in `dpaa_ethdev.c`):  
  The first `dpaa_port_vsp_cleanup()` call is guarded by `if (!(default_q || fmc_q))`. The second call (line 683) is not. If `default_q` or `fmc_q` is set, the second call will attempt to clean up VSPs that were never configured, potentially accessing NULL pointers. Should add the same guard.

**Warnings:**
- None

**Info:**
- Adding `fman_onic` handling to `get_rx_port_type()` is correct.

---

## Cross-Cutting Issues

### Forbidden `getenv()` usage

**Patches 04 and 18** introduce multiple `getenv()` calls in `drivers/dma/dpaa/dpaa_qdma.c`. This violates the guideline that `getenv()` is forbidden in `lib/` and `drivers/` (except for specific EAL use cases). All configuration should use devargs.

**Recommendation:** Replace the environment variable checks with devargs parsing, as done for `DPAA_DMA_ERROR_CHECK` (which correctly uses `dpaa_get_devargs()`).

---

## Summary of Critical Issues

1. **Patch 02**: Missing error check propagation in `bman_init_ccsr()` (minor, unlikely to cause issues in practice but incorrect).
2. **Patch 04**: `getenv()` in driver code (violates guidelines); removed secondary process check in crypto driver (wrong).
3. **Patch 08**: Unbounded loop in `qman_find_fq_by_cgrid` (potential infinite loop).
4. **Patch 09**: Potential double-free of CGR if FQ shutdown fails; CGRID range release without verifying all CGRIDs are unused.
5. **Patch 12**: Double VSP cleanup call in `dpaa_eth_dev_close`.
6. **Patch 13**: Bus detection logic in wrong function (`dpaa_bus_dev_compare`), will run on every name comparison and fail after first call.
7. **Patch 14**: Negative error code from `dpaa_port_fmc_get_idx_from_name` not checked before use as array index.
8. **Patch 17**: Destructor may access freed memory if EAL mempool cleanup runs before destructor.
9. **Patch 18**: `getenv()` in driver code (same as Patch 04).
10. **Patch 19**: Missing `default_q`/`fmc_q` check in second VSP cleanup call.

---

## Recommendations

- **Patch 02**: Add `return ret;` after `