|WARNING| [v4,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/165895

_AI Code Review_

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

# DPDK Patch Review: bus/dpaa and drivers series

## Review Summary

This 19-patch series refactors DPAA bus and driver code, adds new features (Tx BMI statistics, cgrid cleanup, FQ shutdown improvements), and fixes several correctness issues. Below are findings organized by patch and severity.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

- Clean rename from `ccsr_map` to `memac_map` and `bmi_map` to `rx_bmi_map` improves clarity.
- Making `fman_ccsr_map_fd` static reduces global symbol pollution (good practice).

---

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

**Errors:**

1. **Resource leak on error path** (fman_init_ccsr failure):
   ```c
   ret = bman_init_ccsr(dt_node);
   if (ret) {
       pr_err("Failed(%d) to init bman ccsr", ret);
       return ret;  // ERROR: dt_node from for_each_compatible_node not freed
   }
   ```
   When `bman_init_ccsr()` fails, `dt_node` obtained from `for_each_compatible_node()` is not released.
   **Fix:** Add `of_node_put(dt_node);` before returning on error.

**Warnings:**

None.

**Info:**

- Dynamic BPID max calculation from DTS is correct.
- The loop correctly breaks after finding the first `fsl,bpid-range`.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

- Adds `fmbm_tfrc`, `fmbm_tfdc`, etc. to Tx BMI registers.
- Extends xstats with `tx_bad_frames_count`, `tx_frame_length_discard`, etc.
- Looks correct; no mismatches between register definitions and xstats offsets.

---

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

**Errors:**

1. **Secondary process crash risk** (dpaa_qdma_init):
   ```c
   if (rte_eal_process_type() != RTE_PROC_PRIMARY)
       return -ENOTSUP;
   ```
   Returning `-ENOTSUP` from device init for secondary process will cause probe to fail, which is correct if the driver doesn't support secondary processes. However, later in the same function:
   ```c
   if (getenv("DPAA_QDMA_DATA_VALIDATION"))
       s_data_validation = 1;
   ```
   These `getenv()` calls execute **after** the secondary-process guard, so they would be skipped in secondary. But these are static flags initialized in primary -- secondary should inherit them from shared memory or not access them at all. If secondary is not supported, the early return is correct, but the `getenv()` calls are dead code after the return. If secondary **is** supported later, these would need to be moved before the guard or handled differently.

   **Recommendation:** If secondary support is planned, move static flag initialization to a one-time global init (e.g., driver constructor) so both primary and secondary see the same config. If not, document that DMA driver is primary-only.

**Warnings:**

None.

**Info:**

- Added guards in crypto and net drivers prevent secondary from re-initializing hardware.
- `rte_dpaa_remove()` guard is correct.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

- Inline helpers `qm_fqd_get_chan()` and `qm_fqd_get_wq()` replace open-coded bit extraction.
- Clean refactor.

---

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

**Errors:**

1. **Potential NULL dereference** (qman_shutdown_fq):
   ```c
   struct qman_portal *p = fq->qp;
   ...
   if (!p)
       p = get_affine_portal();
   ```
   If `fq->qp` is NULL (which is likely for queues not yet bound to a portal), `p` is set to the affine portal. This is correct. However, the code does not check if `get_affine_portal()` returns NULL. If no portal is affine to the current thread, `p` would be NULL and dereferenced at `qm_mc_start(&p->p)`.

   **Fix:** After `p = get_affine_portal();`, add:
   ```c
   if (!p) {
       DPAA_BUS_ERR("No affine portal for FQ 0x%x", fq->fqid);
       return -EINVAL;
   }
   ```

**Warnings:**

None.

**Info:**

- Passing `struct qman_fq *` instead of just `fqid` allows correct portal selection for pool channels.
- `qman_shutdown_fq_by_fqid()` wrapper preserves old API.

---

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

**Errors:**

None.

**Warnings:**

1. **Hardcoded assumption about pool channel layout**:
   ```c
   const u16 pool_ch_start = dpaa_get_qm_channel_pool();
   const u16 pool_ch_end = pool_ch_start + dpaa_get_qm_channel_pool_num();
   if (channel >= pool_ch_start && channel < pool_ch_end) {
       if (p->config->channel != channel) { ... }
   ```
   The code assumes pool channels are contiguous from `pool_ch_start` to `pool_ch_end - 1`. While this is typical for DPAA1, it is not enforced by the DTS schema (pool-channel-range gives start and count, which **are** contiguous by definition). This is acceptable.

**Info:**

- Channel validation prevents shutting down an FQ on the wrong portal.
- SDQCR is only modified for dedicated channels; pool channels use the affine portal's existing SDQCR.
- SDQCR restoration now checks `if (sdqcr != p->sdqcr)` before writing (micro-optimization).

---

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

**Errors:**

1. **`qman_find_fq_by_cgrid()` unbounded loop**:
   ```c
   do {
       err = qman_query_fq_np(&fq, &np);
       if (err == -ERANGE) {
           ...
           return err;
       } else if (err) {
           ...
           return err;
       }
       if ((np.state & QM_MCR_NP_STATE_MASK) != QM_MCR_NP_STATE_OOS) {
           err = qman_query_fq(&fq, &fqd);
           if (err) {
               ...
           } else if ((fqd.fq_ctrl & QM_FQCTRL_CGE) && fqd.cgid == cgrid) {
               if (fqid)
                   *fqid = fq.fqid;
               return 0;
           }
       }
       fq.fqid++;
   } while (1);

   return -ENODEV;  // unreachable
   ```
   The loop increments `fq.fqid` indefinitely until `qman_query_fq_np()` returns `-ERANGE` (no more FQs). This is unbounded: if `cgrid` is not found, the loop scans all possible FQIDs. Given that FQIDs are 24-bit (up to 16M), this could take a very long time. While `-ERANGE` will eventually terminate the loop, a malformed CGID or a missing FQ could cause a massive delay.

   **Recommendation:** Add a max iteration count (e.g., `DPAA_MAX_FQID` or a reasonable search limit) and return `-ENODEV` if exceeded. Alternatively, maintain a reverse mapping (CGID - FQID list) if this lookup is performance-critical.

**Warnings:**

None.

**Info:**

- `qman_find_fq_by_cgrid()` is used in patch 09 to verify all FQs using a CGR are shut down before releasing it.

---

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

**Errors:**

None.

**Warnings:**

1. **Unbounded FQ scan on every cgrid** (dpaa_eth_dev_close):
   ```c
   ret = qman_find_fq_by_cgrid(dpaa_intf->cgr_rx[loop].cgrid, &fqid);
   if (!ret) {
       DPAA_PMD_DEBUG("FQ(fqid=0x%x) with rx cgid=%d is still alive?", ...);
       ret = qman_shutdown_fq_by_fqid(fqid);
   }
   ```
   This calls `qman_find_fq_by_cgrid()` for every Rx and Tx cgrid on device close. Given the unbounded loop in patch 08, this could be slow if there are stale cgrids. However, the comment "Should be FQ not cleaned in previous program" suggests this is a recovery path for application crashes, so the performance impact may be acceptable in that scenario.

**Info:**

- The `qman_release_cgrid_range()` calls correctly release the cgrid range after deleting the CGRs.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

- Adds cleanup of `dpaa_intf->tx_conf_queues` in both close and error paths.
- Also nulls `fc_conf` to prevent double-free.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

- The commit message states FQ shutdown happens during device stop, so the call in `dpaa_eth_rx_queue_setup()` was redundant. However, patch 06 **adds** a shutdown in `dpaa_dev_init()` before queue setup. The two patches should be reviewed together: patch 06 adds a shutdown before `rx_queue_setup`, and patch 11 removes the one **inside** `rx_queue_setup`. This is consistent as long as patch 06's early shutdown covers the case.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

- Consolidates `dpaa_fm_deconfig()` and `dpaa_port_vsp_cleanup()` calls to single locations.
- Adds VSP cleanup before FQ shutdown (comment: "deconfig FM to direct ingress traffic to kernel before fq shutdown").

---

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

**Errors:**

1. **Bus detection moved into dev_compare breaks probe**:
   ```c
   static int
   dpaa_bus_dev_compare(const char *name1, const char *name2)
   {
       ...
       if ((access(DPAA_DEV_PATH1, F_OK) != 0) &&
           (access(DPAA_DEV_PATH2, F_OK) != 0)) {
           DPAA_BUS_DEBUG("DPAA Bus not present. Skipping.");
           return 0;  // BUG: should return non-zero to indicate no match
       }

       if (dpaa_bus.detected)
           return 0;  // BUG: should compare names

       dpaa_bus.detected = 1;
       ...
       return strncmp(devname1, devname2, sizeof(devname1));
   }
   ```
   **Problems:**
   - If the sysfs path does not exist, `dpaa_bus_dev_compare()` returns `0`, which means the names **match**. This is backwards: it should return non-zero to indicate the bus is not present and the devices don't match.
   - Once `dpaa_bus.detected` is set, all subsequent calls to `dev_compare()` return `0` (match) without comparing names. This would cause **all** devices to appear identical.
   - The `pthread_key_create()` and one-time init logic is in the wrong place. `dev_compare()` is called for every device pair; init should happen once at bus probe, not during device comparison.

   **Fix:** Move bus detection, `pthread_key_create()`, and `dpaa_bus.detected` guard to `rte_dpaa_bus_scan()` or a dedicated init function called early in scan. `dev_compare()` should only compare device names. The current logic will cause probe failures or incorrect device matching.

**Warnings:**

None.

**Info:**

- Log macro replacements are correct.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

- `dpaa_port_fmc_get_idx_from_name()` parses MAC index from FMC port name string instead of combining MAC type and port number.
- This handles LS104xA where MAC9/MAC10 can be 10G/2.5G/1G depending on serdes config.

---

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

**Errors:**

None.

**Warnings:**

None.

**Info:**

- Correctly rejects `rx_deferred_start` and `tx_deferred_start` with `-EINVAL`.

---

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

**Errors:**

1. **Use-after-free risk in bman_release_fast**:
   ```c
   bm_bufs[0].be_desc.bpid = bpid;
   for (i = 0; i < num; i++)
       bm_buffer_set64_to_be(&bm_bufs[i], bufs[i]);
   ...
   r->bufs[0].opaque = bm_bufs[0].opaque;
   if (num > 1)
       rte_memcpy(&r->bufs[1], &bm_bufs[1],
           sizeof(struct bm_buffer) * (num - 1));
   ```
   The code copies `bm_bufs[0].opaque` to `r->bufs[0].opaque` before committing the descriptor. If another thread or the hardware reads `r->bufs[0]` after this copy but before the commit (race), it could see a partially-constructed descriptor. However, the original code did the same sequence (copy first entry, then memcpy rest), so this is not a new bug introduced by the patch.

   **Recommendation:** If `r` is in shared memory accessible by hardware before commit, add a comment explaining that the copy is safe because the descriptor is not yet visible (or use a barrier if needed). The current code appears safe as-is, assuming the commit is the visibility fence.

**Warnings:**

None.

**Info:**

- Replacing magic `8` with `FSL_BM_BURST_MAX` improves clarity.
- Using a single `bm_buffer` for HW init and copying it is more maintainable than duplicating the init logic.

---

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

**Errors:**

None.

**Warnings:**

1. **Global array increases BSS size**:
   ```c
   static struct dpaa_bpid_flag s_dpaa_bpid_allocated_flag[DPAA_MAX_BPOOLS];
   ```
   `DPAA_MAX_BPOOLS = 256`, and each entry is `sizeof(uint32_t) + sizeof(int) = 8 bytes` (assuming no padding), totaling 2 KB. This is small and acceptable for the benefit of tracking BPID allocations.

**Info:**

- The destructor `dpaa