|WARNING| [v3,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/165870

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Process compliance:**
- `fman_ccsr_map_fd` is declared `extern` in `fman.h` but should be made `static` as stated in the commit message. The patch does not actually change the variable's scope in `fman.c` -- it remains a global symbol, just removed from the header. To fix this, `fman.c` should declare it `static`.

### Warnings

None.

### Info

- Renaming `ccsr_map` to `memac_map` is good for clarity (MEMAC register space vs generic CCSR).
- Renaming `bmi_map` to `rx_bmi_map` distinguishes Rx from Tx BMI.
- Copyright year updated to 2026.

---

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

### Errors

None.

### Warnings

None.

### Info

- Replaces hardcoded `bman_pool_max` values with DTS-derived `fsl,bpid-range` property.
- Improves error handling in `bman_global_init()` (checks for NULL before `bman_init_ccsr()`).
- Log messages updated to use `pr_info()` consistently.

---

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

### Errors

None.

### Warnings

None.

### Info

- Adds Tx BMI statistics counters to `dpaa_if_tx_bmi_stats` structure.
- Extends `dpaa_xstats_strings` array with Tx BMI xstats.
- Register definitions in `fman.h` updated for Tx BMI statistics.

---

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

### Errors

**Variable declared but not used:**
```c
char *penv;
```
Declared in `dpaa_qdma_init()` but only used inside `#if 0` blocks or removed code. Should be removed or moved inside the conditional blocks where it is used.

### Warnings

**Inconsistent error handling:**
- `dpaa_qdma_init()` returns `-ENOTSUP` for secondary processes.
- `cryptodev_dpaa_sec_probe()` originally returned `0` for secondary processes (that return was removed in this patch).
- `rte_dpaa_remove()` now returns `0` for secondary processes.

These should be consistent: either all return an error, or all return success. Best practice is to return success (0) for secondary processes to avoid spurious error logs, since skipping hardware init in secondary is expected behavior.

### Info

- Adds `RTE_PROC_PRIMARY` checks to prevent secondary processes from attempting hardware initialization.
- Prevents segfaults from unmapped hardware registers in secondary processes.

---

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

### Errors

None.

### Warnings

None.

### Info

- Adds `qm_fqd_get_chan()` and `qm_fqd_get_wq()` inline helpers.
- Replaces open-coded bit manipulation for channel/WQ extraction.
- `GENMASK()` macro defined (duplicate definition if already in `rte_common.h` -- check for conflicts, but not an error).

---

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

### Errors

**Potential NULL pointer dereference:**
```c
struct qman_portal *p = fq->qp;
...
if (!p)
    p = get_affine_portal();
```
If `fq->qp` is NULL and `get_affine_portal()` also returns NULL, `p` will be NULL. Subsequent access to `p->p` (e.g., `qm_mc_start(&p->p)`) will segfault. Add:
```c
if (!p)
    p = get_affine_portal();
if (!p) {
    DPAA_BUS_ERR("No portal available for FQ 0x%x", fqid);
    return -ENODEV;
}
```

### Warnings

None.

### Info

- Changes `qman_shutdown_fq()` signature to take `struct qman_fq *` instead of `u32 fqid`.
- Adds `qman_shutdown_fq_by_fqid()` wrapper for backward compatibility.
- `dpaa_dev_init()` now shuts down FQ before configure to clean the queue.

---

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

### Errors

**Missing bounds check on DTS values:**
```c
start = rte_be_to_cpu_32(range[BPID_RANGE_START_INDEX]);
count = rte_be_to_cpu_32(range[BPID_RANGE_COUNT_INDEX]);
bman_pool_max = start + count;
```
If `start + count` overflows `uint32_t`, `bman_pool_max` will be wrong. Add:
```c
if (start > UINT32_MAX - count) {
    pr_err("BPID range overflow: start %u + count %u", start, count);
    return -EINVAL;
}
```

### Warnings

None.

### Info

- Replaces hardcoded channel range check with DTS-derived `qm_channel_pool1` and `qm_channel_pool_num`.
- Adds validation that portal affine channel matches FQ channel for pool-channel FQs.
- Only restores SDQCR when it was actually changed (check removed in final version, but logic is clearer).
- Adds `dpaa_get_qm_channel_pool_num()` export.

---

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

### Errors

**Infinite loop risk:**
```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;
    }
    ...
    fq.fqid++;
} while (1);
```
The loop increments `fq.fqid` and relies on `qman_query_fq_np()` to return `-ERANGE` when the FQID exceeds the valid range. If the query function returns an error other than `-ERANGE` for a valid FQID, the loop may iterate through all `2^24` possible FQIDs (millions of iterations). Add a loop counter or FQID upper bound.

### Warnings

**Inconsistent log level:**
- `qman_find_fq_by_cgrid()` uses `DPAA_BUS_INFO()` when no FQ is found, but `DPAA_BUS_WARN()` for query failures. The former should be `DPAA_BUS_DEBUG()` (not finding a FQ for a CGID is not necessarily an error in a cleanup context).

### Info

- Improves FQ shutdown edge-case handling.
- Adds `qman_find_fq_by_cgrid()` helper to locate FQs by CGID.
- Better error messages in shutdown path.

---

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

### Errors

None.

### Warnings

None.

### Info

- Uses `qman_find_fq_by_cgrid()` to verify all FQs using a CGR are shut down before releasing the CGRID.
- Adds `qman_release_cgrid_range()` calls in `dpaa_eth_dev_close()`.
- Prevents use-after-free of CGR resources.

---

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

### Errors

None.

### Warnings

None.

### Info

- Frees `tx_conf_queues` in `dpaa_eth_dev_close()` and on error path in `dpaa_dev_init()`.
- Prevents stale FQ state on device restart.

---

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

### Errors

None.

### Warnings

None.

### Info

- Removes `qman_shutdown_fq()` call from `dpaa_eth_rx_queue_setup()`.
- FQ is already shut down during device stop.

---

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

### Errors

**Logic error in FM deconfig placement:**
```c
/** For FMCLESS mode of share MAC, deconfig FM to direct
 * ingress traffic to kernel before fq shutdown.
 */
if (!(default_q || fmc_q)) {
    ret = dpaa_fm_deconfig(dpaa_intf, dev->process_private);
    ...
}
if (fif->num_profiles) {
    ret = dpaa_port_vsp_cleanup(dpaa_intf, fif);
    ...
}
/** Release congestion Groups after releasing FQIDs*/
```
The comment says "deconfig FM to direct ingress traffic to kernel **before** fq shutdown", but this code is **after** the comment says "Release congestion Groups **after** releasing FQIDs". The FQ shutdown is later in the function. This placement may cause ingress traffic to be misdirected. Verify that `dpaa_fm_deconfig()` is called at the correct point relative to FQ shutdown and VSP cleanup.

### Warnings

None.

### Info

- Consolidates FM deconfig calls to avoid duplication.
- Moves `dpaa_port_vsp_cleanup()` to run before FQ/CGR cleanup.

---

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

### Errors

**Use-after-check logic error:**
```c
static int
dpaa_bus_dev_compare(const char *name1, const char *name2)
{
    ...
    if (dpaa_bus.detected)
        return 0;

    dpaa_bus.detected = 1;

    ret = pthread_key_create(&dpaa_portal_key, dpaa_portal_finish);
    if (ret) {
        DPAA_BUS_DEBUG("Unable to create pthread key. (%d)", ret);
        dpaa_clean_device_list();
        return ret;
    }

    return 0;
}
```
The function `dpaa_bus_dev_compare()` is supposed to compare two device names and return 0 if they match, non-zero otherwise. However, this code always returns 0 after the first successful bus detection, regardless of whether `name1` and `name2` match. The original `strncmp()` call was removed. This will break device enumeration. The bus detection logic should be in `dpaa_bus_scan()`, not in `dev_compare()`.

### Warnings

None.

### Info

- Log macros changed from `DPAA_BUS_LOG(LEVEL, ...)` to `DPAA_BUS_LEVEL(...)` for consistency.
- Bus detection moved into `dpaa_bus_dev_compare()` (but see error above).

---

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

### Errors

None.

### Warnings

None.

### Info

- Replaces MAC type and port number parsing with port name parsing.
- Adds `dpaa_port_fmc_get_idx_from_name()` helper.
- Handles ls104xa MAC9/MAC10 which 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

- Adds checks for `rx_deferred_start` and `tx_deferred_start` in queue setup functions.
- Returns `-EINVAL` if deferred start is requested (not supported).

---

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

### Errors

**Buffer overrun risk:**
```c
if (num > 1)
    rte_memcpy(&r->bufs[1], &bm_bufs[1],
        sizeof(struct bm_buffer) * (num - 1));
```
If `num` exceeds the size of `r->bufs[]` array, this will overrun the buffer. The caller should ensure `num <= FSL_BM_BURST_MAX`, but add a defensive check:
```c
if (num > FSL_BM_BURST_MAX) {
    DPAA_PMD_ERR("num %u exceeds burst max %u", num, FSL_BM_BURST_MAX);
    return -EINVAL;
}
```

### Warnings

**Cache flush threshold change:**
```c
if (cache->flushthresh)
    cache->flushthresh = cache->size + DPAA_MBUF_MAX_ACQ_REL;
```
This unconditionally increases `flushthresh` without checking if it exceeds the cache size. If `cache->size` is already large, this could set `flushthresh` to a value larger than the cache can hold, breaking assumptions in the mempool code. Verify that `cache->size + DPAA_MBUF_MAX_ACQ_REL` is a valid threshold.

### Info

- Replaces hardcoded `8` with `FSL_BM_BURST_MAX`.
- Uses a single `bm_hw_buf_desc` for HW init, then copies to remaining entries.
- Optimizes buffer pool operations for bulk release.

---

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

### Errors

**Non-portable priority value:**
```c
#define RTE_PRIORITY_104 104

RTE_FINI_PRIO(dpaa_mpool_finish, 104)
```
The `RTE_FINI_PRIO()` macro expects a priority defined in `rte_common.h` (e.g., `RTE_PRIO_LAST`). Using a literal `104` is non-portable and may conflict with other destructors. Use an existing `RTE_PRIO_*` constant or justify why 104 is needed.

### Warnings

**Static array size:**
```c
static struct dpaa_bpid_flag s_dpaa_bpid_allocated_flag[DPAA_MAX_BPOOLS];
```
`DPAA_MAX_BPOOLS` is 256, so this is a 2KB static array (256 * 8 bytes). Not a problem, but note it's allocated at program start.

### Info

- Tracks allocated BPIDs in `s_dpaa_bpid_allocated_flag[]` array.
- Registers destructor `dpaa_mpool_finish()` to release BPIDs at process exit.
- Tunes mempool cache flush threshold to match HW bulk release size.

---

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

### Errors

**Dead code:**
```c
if (getenv("DPAA_QDMA_DATA_VALIDATION"))
    s_data_validation = 1;

if (getenv("DPAA_QDMA_HW_ERR_CHECK"))
    s_hw_err_check = 1;
```
These environment variables are not documented. If they are for debugging only, add a comment. If they are production features, document them in the driver docs.

**Uninitialized variable on error path:**
```c
char *penv;
...
penv = getenv("DPAA_QDMA_SG_ENABLE");
if (penv)
    s_sg_enable = atoi(penv);
```
If `getenv()` returns NULL, `penv` is NULL, and the code correctly skips `atoi()`. However, `penv` is declared but not initialized at the top of the function. If any code path dereferences `penv` before it is assigned, this is UB. Not an issue in this patch, but be