|WARNING| [v7-S2,13/13] net/dpaa2: rename Rx queue flags

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPAA2 PMD Patch Series Review

## Summary

Reviewed 13 patches in series v7-S2 for the DPAA2 network PMD. The series includes fixes, feature additions, and code cleanups. Several correctness issues and style violations were identified.

---

## Patch 01/13: net/dpaa2: fix integer overflow in CCSR region mapping

### Errors

1. **Use-after-free on error path** (line 203, `dpaa2_recycle.c`)
   ```c
   if (page_size <= 0) {
       close(fd);
       return NULL;  // fd already closed, but no cleanup of other resources
   }
   ```
   The `fd` is correctly closed, but if `mmap()` later fails at line 199, the function returns `NULL` without closing `fd` again. However, the actual bug is at line 203: after checking `page_size <= 0`, the code closes `fd` and returns, but **does not set `fd` to -1**. If the caller were to attempt cleanup (though none is shown here), a stale `fd` could be reused. This is a **minor use-after-close risk** in the local scope. More critically, at line 199, if `mmap()` fails (`tmp == MAP_FAILED`), the code now correctly closes `fd` before returning (line 201), which is good. The original code had a **resource leak** here (the old version closed `fd` only after the `if (tmp != MAP_FAILED)` branch, so a failed `mmap` would leak the file descriptor). The new code fixes that leak. **No error to report here--this is a correct fix.**

2. **RTE_ALIGN_CEIL correctness** (line 197, `dpaa2_recycle.c`)
   ```c
   map_len = RTE_ALIGN_CEIL(offset + len, (size_t)page_size);
   ```
   The commit message states: "The correct mapping size must cover the region from the page-aligned start through offset + len." The code does `RTE_ALIGN_CEIL(offset + len, page_size)`, which rounds up `(offset + len)` to the next `page_size` boundary. This is correct: the mapping starts at the page-aligned `start` address and must extend at least `offset + len` bytes from that start. The old code did `len & PAGE_MASK`, which masked off the lower bits of `len`, producing 0 for small `len` on large-page kernels (as the commit message explains). The new code is **correct**--no error here.

**Conclusion:** Patch 01 is **correct**. The commit message accurately describes the bug (the old `len & PAGE_MASK` was wrong) and the fix (explicit page size check + `RTE_ALIGN_CEIL`). No errors to report.

---

## Patch 02/13: net/dpaa2: set Tx confirmation on device init

### Info

This patch moves Tx confirmation mode setup from `dpaa2_dev_tx_queue_setup()` (which is only called for queue 0) to `dpaa2_dev_init()` so that all channels are configured regardless of which queues the user sets up.

- The loop at line 3599-3611 (`dpaa2_ethdev.c`) correctly iterates over all channels (`priv->num_channels`) and calls `dpni_set_tx_confirmation_mode()` for each.
- The `ceetm_ch_idx` variable in the original code (removed block) is replaced by the generic loop index `i`, which is fine.
- No resource leaks or logic errors introduced.

**No issues.**

---

## Patch 03/13: net/dpaa2: support larger burst size

### Errors

1. **Missing header inclusion for `dpaa2_svr_family` and `SVR_LX2160A`** (line 19, `dpaa2_tm.c`)
   The new inline function `dpaa2_get_burst_max()` references `dpaa2_svr_family` and `SVR_LX2160A`, which are defined in `<dpaa2_hw_pvt.h>` (included at line 8). This is **correct**--no error.

2. **Incorrect shift logic in MC command fields** (lines 1119-1127, `dpni.c`)
   ```c
   tx_cr_burst = tx_cr_shaper->max_burst_size;
   tx_er_burst = tx_er_shaper->max_burst_size;
   cmd_params->tx_cr_max_burst_size =
       cpu_to_le16(DPNI_BURST_LO(tx_cr_burst));
   cmd_params->tx_er_max_burst_size =
       cpu_to_le16(DPNI_BURST_LO(tx_er_burst));
   cmd_params->tx_cr_max_burst_size_hi =
       cpu_to_le16(DPNI_BURST_HI(tx_cr_burst));
   cmd_params->tx_er_max_burst_size_hi =
       cpu_to_le16(DPNI_BURST_HI(tx_er_burst));
   ```
   The macros are defined (line 833, `fsl_dpni.h`):
   ```c
   #define DPNI_BURST_LO(burst)	((burst) & GENMASK(15, 0))
   #define DPNI_BURST_HI(burst)	((burst) >> 16)
   ```
   Here `tx_cr_burst` and `tx_er_burst` are `uint32_t`. `DPNI_BURST_HI` shifts right by 16 to get the upper 16 bits, then the result is cast to `uint16_t` by the `cpu_to_le16()` wrapper. This is **correct**--no error.

3. **Structure field type change propagation** (line 843, `fsl_dpni.h`)
   ```c
   struct dpni_tx_shaping_cfg {
       uint32_t rate_limit;
       uint32_t max_burst_size;  // was uint16_t
   };
   ```
   All users of this structure in the patch series are in the same file (`dpni.c` and `dpaa2_tm.c`). The caller in `dpaa2_tm.c` passes `params->committed.size` and `params->peak.size` (both `uint64_t` from `rte_mtr`), but the new structure field is `uint32_t`. The bounds checks at lines 292 and 302 (`dpaa2_tm.c`) ensure the values fit within the new per-SoC limits (up to 229375 on LX2160A, 0xF7FF otherwise), which fit in `uint32_t`. **Correct**.

**No errors.**

---

## Patch 04/13: net/dpaa2: support MPLS and PPPoE flow distribution

### Errors

1. **MPLS label extraction size mismatch (fixed by this patch)** (line 4065, `dpaa2_flow.c`)
   The commit message states: "The original code passed `sizeof(label_tc_s) = 3`, so the driver key profile recorded the field as 3 bytes while MC placed 4 bytes in the key; any extract field added after MPLS was off by one byte, corrupting the key layout. Fix by passing the full `struct rte_flow_item_mpls` pointer (spec/mask) with `sizeof(*mask) = 4`."

   At line 4065-4067:
   ```c
   ret = dpaa2_flow_add_hdr_extract_rule(flow, NET_PROT_MPLS,
           NH_FLD_MPLS_MPLSL_1, spec,
           mask, sizeof(*mask),
   ```
   Here `mask` is `const struct rte_flow_item_mpls *`, so `sizeof(*mask)` is `sizeof(struct rte_flow_item_mpls)` = 4 bytes (the struct contains a 3-byte `label_tc_s` array plus 1 byte `ttl`). This matches the 4 bytes that MC extracts for `NH_FLD_MPLS_MPLSL_1`. The old code (not shown in the patch but described in the commit message) passed `sizeof(label_tc_s)` = 3, which was wrong. **This patch fixes a correctness bug--no error here.**

2. **PPPoE session ID extraction** (line 4205, `dpaa2_flow.c`)
   ```c
   ret = dpaa2_flow_add_hdr_extract_rule(flow, NET_PROT_PPPOE,
           NH_FLD_PPPOE_SID, &spec->session_id,
           &mask->session_id, sizeof(rte_be16_t),
   ```
   `session_id` is a `rte_be16_t` (2 bytes). Passing `sizeof(rte_be16_t)` is correct. **No error.**

**No errors.** This patch correctly fixes the MPLS key width bug.

---

## Patch 05/13: net/dpaa2: support meter and policing

### Errors

1. **Byte-mode rate conversion overflow** (line 73, `dpaa2_meter.c`)
   ```c
   if (profile->policer_unit == DPNI_POLICER_UNIT_BYTES_L3) {
       /* rte_mtr rates are bytes/s; DPNI expects Kbps (1 Kbps = 125 B/s). */
       cfg.cir = (uint32_t)(profile->cir / 125);
       cfg.eir = (uint32_t)(profile->pir / 125);
   ```
   `profile->cir` and `profile->pir` are `uint64_t` (from `struct dpaa2_dev_meter_profile`). Dividing by 125 then casting to `uint32_t` is safe **only if** the result fits in 32 bits. The comment says "DPNI expects Kbps," and typical link speeds (e.g., 100 Gbps = 12.5 GB/s  100,000,000 Kbps) fit in `uint32_t` (max ~4.29 billion). However, if a user sets an absurdly high rate (e.g., `UINT64_MAX` bytes/s), the division by 125 still produces a value > `UINT32_MAX`, and the cast truncates the upper 32 bits. **This is a potential integer overflow on the cast, but the risk is low because real rates won't exceed 32-bit Kbps.** The code should either document the limit or add a check. **Flag as Warning**: "Byte-mode rate conversion may truncate if profile->cir / 125 exceeds UINT32_MAX. Consider adding a bounds check or documenting the maximum supported rate."

2. **TOCTOU race on profile/policy lists** (lines 175-190, `dpaa2_mtr_profile_add`)
   The code locks `meter_lock`, walks the profile list to check for a duplicate ID, then inserts the new profile, all under the same lock. This is **correct**--no TOCTOU race. The same pattern is used in `dpaa2_mtr_policy_add()`. **No error.**

3. **Missing meter ID range validation in `dpaa2_mtr_meter_create()`** (lines 423-429, `dpaa2_meter.c`)
   ```c
   /* mtr_id maps 1:1 to an Rx TC; validate the range. */
   if (mtr_id >= priv->num_rx_tc) {
       snprintf(s_err_msg, sizeof(s_err_msg),
           "Meter ID(%u) >= num_rx_tc(%u)!", mtr_id,
           priv->num_rx_tc);
       return rte_mtr_error_set(error, EINVAL,
           RTE_MTR_ERROR_TYPE_MTR_ID, NULL, s_err_msg);
   }
   ```
   This is **correct**--the meter ID is validated before use. **No error.**

4. **Policing disabled at destroy** (lines 566-577, `dpaa2_mtr_meter_destroy`)
   ```c
   memset(&cfg, 0, sizeof(cfg));
   cfg.mode = DPNI_POLICER_MODE_NONE;
   ret = dpni_set_rx_tc_policing(dpni, CMD_PRI_LOW,
                                  priv->token,
                                  (uint8_t)mtr_id, &cfg);
   ```
   The cast `(uint8_t)mtr_id` is safe because `mtr_id` was validated to be `< num_rx_tc` at creation time (see above). **No error.**

**Warnings:**
- Byte-mode rate conversion should document or validate the maximum rate to prevent silent truncation (see #1 above).

---

## Patch 06/13: net/dpaa2: support flow drop action

### No issues

The drop action is implemented by setting `DPNI_FS_OPT_DISCARD` in the FS action config (line 4473, `dpaa2_flow.c`). The action is added to the supported list (line 100, `dpaa2_flow.c`) and handled in the switch at line 4952. No resource leaks or logic errors.

---

## Patch 07/13: net/dpaa2: set default flow miss action per device

### No issues

The patch replaces a file-scope global `dpaa2_flow_miss_flow_id` with a per-device `priv->default_flow` field, initialized at probe time (line 3543, `dpaa2_ethdev.c`). The environment variable `DPAA2_FLOW_CONTROL_MISS_FLOW` is removed. No correctness issues.

---

## Patch 08/13: net/dpaa2: identify Rx mbuf hash information by FLC

### Errors

1. **Incorrect `ol_flags` in `dpaa2_dev_rx_mbuf_sched_set()`** (line 104, `dpaa2_rxtx.c`)
   The original comment says: "Do NOT set RTE_MBUF_F_RX_FDIR: hash.sched overlaps hash.fdir and rte_mbuf_sched_set() would corrupt any fdir.hi value." This is **correct**--the code does not set `RTE_MBUF_F_RX_FDIR`, which is the right behavior. **No error.**

2. **FLC low-word bitmask extraction** (lines 104-108, `dpaa2_rxtx.c`)
   ```c
   tc = (flc_lo >> DPAA2_FS_FLC_TC_OFFSET) & DPAA2_FS_FLC_TC_MASK;
   flow = flc_lo >> DPAA2_FS_FLC_FLOW_OFFSET;
   ```
   The macros are defined in `dpaa2_ethdev.h` (lines 245-251). `DPAA2_FS_FLC_TC_MASK` is `((1 << DPAA2_FS_FLC_TC_BIT_SIZE) - 1)` where `TC_BIT_SIZE` is `sizeof(uint8_t) * 8` = 8. The literal `1` here is `int` (32 bits), so `1 << 8` is safe (no UB). The mask is 0xFF. **No error.**

3. **RSS vs FS detection** (lines 120-127, `dpaa2_rxtx.c`)
   The code checks bit `DPAA2_FS_FLC_FS_MARK_OFFSET` in `flc_lo`. If set, the frame is FS-marked and `rte_mbuf_sched_set()` is called; otherwise, `hash.rss` is set. This is **correct** based on the hardware behavior described in the commit message.

**No errors.**

---

## Patch 09/13: net/dpaa2: add minimum key size support

### No issues

The patch changes the fallback key size from `DPAA2_FLOW_ENTRY_MAX_SIZE` (56) to `DPAA2_FLOW_ENTRY_MIN_SIZE` (24) when the key fits. This is a performance optimization with no correctness impact.

---

## Patch 10/13: net/dpaa2: restructure dpaa2 parser processing