|WARNING| [v7, 16/16] dma/dpaa: add SG data validation and ERR050757 fix

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

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

### Summary
Renames `ccsr_map` to `memac_map` and `bmi_map` to `rx_bmi_map` for clarity, and makes `fman_ccsr_map_fd` static.

**No issues found.**

---

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

### Summary
Dynamically calculates maximum BPID from device tree instead of hardcoded value.

**No issues found.**

---

## Patch 03/16: drivers: add process-type guards for secondary process

### Summary
Adds `RTE_PROC_PRIMARY` checks in crypto/dpaa_sec and dma/dpaa drivers, and reads environment variables in dma/dpaa init.

### Errors

1. **Resource leak on error path in `dpaa_qdma_init()`**
   ```c
   char *penv = getenv("DPAA_QDMA_SG_ENABLE");
   if (penv)
       s_sg_enable = atoi(penv);
   
   penv = getenv("DPAA_QDMA_SG_MAX_ENTRY_SIZE");
   if (penv)
       s_sg_max_entry_sz = atoi(penv);
   
   #ifdef RTE_DMA_DPAA_ERRATA_ERR050757
   penv = getenv("DPAA_QDMA_PCI_READ");
   if (penv)
       s_pci_read = atoi(penv);
   #endif
   ```
   These environment variable reads occur after the early return for secondary process, but before any allocations. However, the function contains later error paths (e.g., `fsl_qdma->n_queues = QDMA_QUEUES * QDMA_BLOCKS;` and subsequent allocations) that could fail. If any of those fail after these globals are set, the globals remain modified even though init failed. This is not a correctness bug per se since these are static globals that persist across init attempts, but it could lead to unexpected behavior if init is retried. More critically, later code allocates resources (`fsl_qdma->status_queue`, `fsl_qdma->queue`, etc.) but if any allocation fails, prior allocations are not freed before returning error. Verify all error paths after allocations perform cleanup.

   **Recommendation**: Audit all error paths in `dpaa_qdma_init()` after resource allocations to ensure cleanup (free of `status_queue`, `queue`, etc.). If an allocation fails, all prior allocations in the function should be freed before returning error.

2. **Stale static globals if init fails partway**
   If `dpaa_qdma_init()` sets `s_data_validation`, `s_hw_err_check`, `s_sg_enable`, `s_sg_max_entry_sz`, or `s_pci_read` from environment variables and then fails later, these globals remain set. On a subsequent init attempt (different device or retry), the environment may have changed but the stale values persist. This is a minor consistency issue.

   **Recommendation**: Consider reading these environment variables once in a global init function or at library load time, not per-device init. Alternatively, reset them at the start of `dpaa_qdma_init()` if device init can be retried.

---

## Patch 04/16: bus/dpaa: define helpers for qman channel and wq

### Summary
Adds `qm_fqd_get_chan()` and `qm_fqd_get_wq()` inline helpers.

**No issues found.**

---

## Patch 05/16: bus/dpaa: shutdown DPAA FQ by fq descriptor

### Summary
Changes `qman_shutdown_fq()` to take `struct qman_fq *fq` instead of just `fqid`.

**No issues found.**

---

## Patch 06/16: bus/dpaa: improve FQ shutdown with channel validation

### Summary
Fixes channel range check to use DTS-derived values, validates portal affine channel matches FQ channel for pool channels, and restores SDQCR only when changed.

### Warnings

1. **Potential unchecked error path**
   ```c
   ret = qman_find_fq_by_cgrid(dpaa_intf->cgr_rx[loop].cgrid, &fqid);
   if (!ret) {
       /** Should be FQ not cleaned in previous program.*/
       DPAA_BUS_DEBUG("FQ(fqid=0x%x) with rx cgid=%d is still alive?",
           fqid, dpaa_intf->cgr_rx[loop].cgrid);
       ret = qman_shutdown_fq_by_fqid(fqid);
       if (ret) {
           DPAA_PMD_WARN("Failed(%d) to shutdown fq(fqid=0x%x)",
               ret, fqid);
       }
   }
   ```
   If `qman_shutdown_fq_by_fqid()` fails, the warning is logged but execution continues to `qman_delete_cgr()`. If the FQ is still alive and the shutdown failed, deleting the CGR could leave the FQ referencing a freed CGR (use-after-free of CGR structure). However, this may be acceptable behavior in a cleanup/close path where best-effort is applied. Verify that `qman_delete_cgr()` is safe to call even if the FQ shutdown failed.

   **Recommendation**: If `qman_delete_cgr()` requires all FQs to be shut down first, return error instead of continuing. If best-effort is acceptable, document that this is intentional.

---

## Patch 07/16: bus/dpaa: enhance DPAA FQ shutdown

### Summary
Improves FQ shutdown sequence and adds `qman_find_fq_by_cgrid()`.

### Errors

1. **Infinite loop in `qman_find_fq_by_cgrid()`**
   ```c
   do {
       err = qman_query_fq_np(&fq, &np);
       if (err == -ERANGE) {
           DPAA_BUS_INFO("No FQ found with cgrid(0x%x)", cgrid);
           return err;
       } else if (err) {
           DPAA_BUS_WARN("Failed(%d) to Query np FQ(fqid=0x%x)", err, fq.fqid);
           return err;
       }
       if ((np.state & QM_MCR_NP_STATE_MASK) != QM_MCR_NP_STATE_OOS) {
           err = qman_query_fq(&fq, &fqd);
           if (err) {
               DPAA_PMD_WARN("Failed(%d) to Query FQ(fqid=0x%x)", err, fq.fqid);
           } else if ((fqd.fq_ctrl & QM_FQCTRL_CGE) && fqd.cgid == cgrid) {
               if (fqid)
                   *fqid = fq.fqid;
               return 0;
           }
       }
       /* Move to the next FQID */
       fq.fqid++;
   } while (1);
   ```
   The loop increments `fq.fqid` indefinitely. If no FQ with the given `cgrid` exists and `qman_query_fq_np()` never returns `-ERANGE`, this is an unbounded loop. The loop should have an upper bound (maximum FQID value).

   **Fix**: Add a loop counter or FQID range check:
   ```c
   #define MAX_FQID 0xFFFFFF  /* 24-bit FQID */
   do {
       if (fq.fqid > MAX_FQID) {
           DPAA_BUS_INFO("No FQ found with cgrid(0x%x) after scanning all FQIDs", cgrid);
           return -ENOENT;
       }
       err = qman_query_fq_np(&fq, &np);
       // ... rest of loop
       fq.fqid++;
   } while (1);
   ```

2. **Missing error propagation after `qman_query_fq()` failure**
   If `qman_query_fq()` fails, the code logs a warning but continues to the next FQID. If this is a transient HW error, the function may skip over the FQ that actually uses the CGID. The intent is unclear.

   **Recommendation**: If `qman_query_fq()` failure indicates a serious error, return the error instead of continuing. If the error is expected (e.g., FQ state prevents query), document this and consider returning error only for certain error codes.

---

## Patch 08/16: bus/dpaa: add DPAA cgrid cleanup support

### Summary
Exports `qman_find_fq_by_cgrid()`.

**No issues found** (inherits issues from Patch 07).

---

## Patch 09/16: net/dpaa: add ONIC port checks

### Summary
Adds `fman_onic` handling and removes `fif` parameter from `dpaa_port_vsp_cleanup()`.

### Warnings

1. **Potential double-free of VSP handle**
   In `dpaa_eth_dev_close()`:
   ```c
   if (fif->num_profiles) {
       ret = dpaa_port_vsp_cleanup(dpaa_intf);
       if (ret) {
           DPAA_PMD_WARN("%s: cleanup VSP failed(%d)",
               dev->data->name, ret);
       }
   }
   // ... later in the same function:
   if (fif->num_profiles) {
       ret = dpaa_port_vsp_cleanup(dpaa_intf);
       if (ret) {
           DPAA_PMD_WARN("%s: cleanup VSP failed(%d)",
               dev->data->name, ret);
       }
   }
   ```
   `dpaa_port_vsp_cleanup()` is called twice in the same close function. If the first call succeeds, it calls `fm_vsp_free()` on each VSP handle and sets `dpaa_intf->vsp[idx].vsp_handle = NULL`. The second call will see NULL handles and do nothing, which is safe. However, this indicates redundant code.

   **Recommendation**: Remove the duplicate call. Likely the second call was intended to be in a different error path or the first should have been removed.

---

## Patch 10/16: drivers: add BMI Tx statistics

### Summary
Adds Tx BMI statistics structures.

**No issues found.**

---

## Patch 11/16: net/dpaa: optimize FM deconfig

### Summary
Consolidates FM deconfiguration to a single location.

**No issues found.**

---

## Patch 12/16: bus/dpaa: improve log macro and fix bus detection

### Summary
Replaces `DPAA_BUS_LOG(LEVEL, ...)` with shorthand macros.

**No issues found.**

---

## Patch 13/16: net/dpaa: optimize FMC MAC type parsing

### Summary
Uses port name to identify MAC index instead of parsing MAC type and port number.

**No issues found.**

---

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

### Summary
Replaces hardcoded `8` with `FSL_BM_BURST_MAX` and uses single HW descriptor for initialization.

**No issues found.**

---

## Patch 15/16: drivers: release DPAA bpid on driver destructor

### Summary
Adds a driver destructor to release BPIDs at exit and tunes mempool cache flush threshold.

### Errors

1. **Destructor priority may not guarantee cleanup order**
   ```c
   #define RTE_PRIORITY_104 104
   
   RTE_FINI_PRIO(dpaa_mpool_finish, RTE_PRIORITY_104)
   ```
   The destructor is registered at priority 104. If other DPDK components (e.g., EAL, devices) are torn down before this runs, accessing `rte_dpaa_bpid_info` or calling `bman_free_bpid()` may fail or crash. The comment states "The rte_dpaa_bpid_info and bman_pool from EAL mem have been released with EAL mem pool being destroyed" but the destructor still tries to use them.

   **Fix**: Verify that priority 104 runs before EAL mem cleanup. If not, either increase priority (lower number = earlier) or remove the destructor and rely on explicit cleanup in `dpaa_mbuf_free_pool()`.

2. **Race condition in `dpaa_mbuf_free_pool()`**
   ```c
   if (!rte_dpaa_bpid_info)
       return;
   
   for (i = 0; i < DPAA_MAX_BPOOLS; i++) {
       if (rte_dpaa_bpid_info[i].mp)
           break;
   }
   
   if (i == DPAA_MAX_BPOOLS) {
       rte_free(rte_dpaa_bpid_info);
       rte_dpaa_bpid_info = NULL;
   }
   ```
   If two threads call `dpaa_mbuf_free_pool()` concurrently (multi-thread mempool free), both may pass the `if (!rte_dpaa_bpid_info)` check, both loop, and both call `rte_free(rte_dpaa_bpid_info)`, causing double-free.

   **Fix**: Add a global lock around `rte_dpaa_bpid_info` access in both `dpaa_mbuf_free_pool()` and the destructor.

---

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

### Summary
Adds scatter-gather support and errata workaround.

### Errors

1. **Potential NULL dereference in `dma_pool_alloc()`**
   ```c
   if (phy_addr)
       *phy_addr = rte_mem_virt2iova(virt_addr);
   ```
   If `phy_addr` is NULL, the physical address is not written, but `virt_addr` is returned. Callers that expect `phy_addr` to be filled may use an uninitialized `dma_addr_t` variable. Verify all callers of `dma_pool_alloc()` either pass non-NULL `phy_addr` or do not use the physical address.

   **Recommendation**: If `phy_addr == NULL` is valid, document this in the function. If not, add an assertion or return NULL.

2. **Unbounded loop in `fsl_qdma_enqueue_desc()`**
   ```c
   } else if (!s_sg_enable) {
       while (fsl_queue->pending_num > 0) {
           ret = fsl_qdma_enqueue_desc_single(fsl_queue,
               fsl_queue->pending_desc[start].dst,
               fsl_queue->pending_desc[start].src,
               fsl_queue->pending_desc[start].len);
           if (!ret) {
               start = (start + 1) &
                   (fsl_queue->pending_max - 1);
               fsl_queue->pending_start = start;
               fsl_queue->pending_num--;
           } else {
               DPAA_QDMA_ERR("Eq pending desc failed(%d)",
                   ret);
               return -EIO;
           }
       }
   ```
   If `fsl_qdma_enqueue_desc_single()` always returns non-zero (hardware busy), this is an infinite loop because `pending_num` is never decremented. The error path returns `-EIO`, but if the caller retries, the same loop will execute again.

   **Recommendation**: Add a retry limit or backoff mechanism. If the descriptor cannot be enqueued after N attempts, fail permanently.

---

## Summary

### Errors
- **Patch 03**: Resource leak on error paths in `dpaa_qdma_init()` (allocations not freed on failure).
- **Patch 07**: