|WARNING| [v6-S1, 6/6] mempool/dpaa2: look up ops index locally in secondary

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/6: bus/fslmc: defer bus initialization to probe

### Errors

**Resource leak on error path in rte_fslmc_probe()**

The new probe function registers an mbuf dynfield and sets up VFIO resources, but on error it returns without cleaning up the dynfield registration. If `rte_mbuf_dynfield_register()` succeeds but a later step fails (VFIO setup, DMA map, or process group), the registered dynfield remains claimed even though the bus init did not complete. On a retry the same offset may be returned (if still available) or registration may fail if the slot is now used, but nothing unregisters it on the error path.

**Suggested fix:**
Dynfield registration is global and cannot be rolled back via a standard API. Document in a comment that dynfield registration persists on error (callers expect it to be idempotent anyway since the same descriptor is re-registered on every call). Or, add a static flag `dynfield_registered` to guard the registration block so it only runs once across all retries, making the semantics explicit.

```c
static bool dynfield_registered = false;

if (!dynfield_registered) {
	dpaa2_seqn_dynfield_offset =
		rte_mbuf_dynfield_register(&dpaa2_seqn_dynfield_desc);
	if (dpaa2_seqn_dynfield_offset < 0) {
		DPAA2_BUS_ERR("Failed to register mbuf field for dpaa sequence number");
		return dpaa2_seqn_dynfield_offset;
	}
	dynfield_registered = true;
}
```

### Warnings

None.

---

## Patch 2/6: bus/fslmc: reduce probe logging and skip ignored devices

### Errors

**rte_bus_device_is_ignored() called with wrong name format**

In `rte_dpaa2_create_dprc_device()`, the new check calls:

```c
if (rte_bus_device_is_ignored(&rte_fslmc_bus, dev->device.name))
	continue;
```

However, `dev->device.name` is the full device name (e.g. `"dpni.0"`), but `rte_bus_device_is_ignored()` expects the name format that the user passed to `-a`/`-b`, which for fslmc is typically a short name or object ID. The function may not match correctly if the internal name does not match the user-specified format. Verify that `dev->device.name` is the correct field to pass, or use the original device string from the allowlist/denylist.

**Suggested fix:**
Confirm that `dev->device.name` matches the format users specify in allowlist/denylist. If not, use the appropriate name field or pre-formatted string that matches the bus's `parse()` function expectations.

### Warnings

None.

---

## Patch 3/6: dma/dpaa2: use memcpy to fill completion index ring

### Errors

None.

### Warnings

None.

### Info

Clean refactoring that replaces per-element loop with bulk memcpy for ring updates. No functional change, improves efficiency.

---

## Patch 4/6: dma/dpaa2: release SG FLE on completion ring overflow

### Errors

None.

### Warnings

None.

### Info

Clarifies ownership by having the error path release the FLE directly rather than relying on bulk cleanup. No functional change, improves code clarity.

---

## Patch 5/6: dma/dpaa2: validate FLE pool IOVA mapping at vchan setup

### Errors

**Missing cleanup of fle_check on successful path**

The `fle_check` struct is stack-allocated and zero-initialized, but the iterator callback `dpaa2_qdma_fle_pool_iova_check()` sets `bad_map` or `bad_offset` flags during `rte_mempool_mem_iter()`. If the iteration succeeds (no bad chunks), the code continues without resetting or clearing the struct. This is not a bug since the struct is local and discarded, but for clarity the flags should be checked immediately after iteration and the struct not referenced again.

**No issue** - the code is correct as written. The struct is only read once after the iteration completes. No fix needed.

---

**Error propagation inconsistency**

On failure, the function returns `-ENOMEM` regardless of whether the failure was due to unmapped memory (`bad_map`) or inconsistent offset (`bad_offset`). `-ENOMEM` suggests allocation failure, but the actual problem is configuration error (unmapped pool or fragmented IOVA).

**Suggested fix:**
Return `-EINVAL` instead of `-ENOMEM` to indicate that the mempool configuration is invalid for DMA use, not that allocation failed.

```c
if (fle_check.bad_map || fle_check.bad_offset) {
	if (fle_check.bad_map)
		DPAA2_QDMA_ERR("No IOMMU map for %s", pool_name);
	else
		DPAA2_QDMA_ERR("%s spans inconsistent IOVA offsets", pool_name);
	rte_mempool_free(qdma_dev->vqs[vchan].fle_pool);
	qdma_dev->vqs[vchan].fle_pool = NULL;
	return -EINVAL;  /* configuration error, not allocation failure */
}
```

### Warnings

None.

---

## Patch 6/6: mempool/dpaa2: look up ops index locally in secondary

### Errors

**Non-atomic initialization of s_dpaa2_pool_ops_idx in secondary**

`rte_dpaa2_mpool_get_ops_idx()` checks `s_dpaa2_pool_ops_idx != RTE_MEMPOOL_MAX_OPS_IDX` without synchronization, then performs a local ops table scan and assigns the result. If two threads in a secondary process call this function concurrently before the index is cached, both will scan the table and both will write to `s_dpaa2_pool_ops_idx`. The final value is the same (the ops are registered in the same order), so the race is benign, but the lack of synchronization allows double-scanning and non-atomic writes.

**Suggested fix:**
Use `rte_atomic_load_explicit()` and `rte_atomic_store_explicit()` with `rte_memory_order_relaxed` (since the value is process-invariant and no other data depends on it), or use `rte_atomic_compare_exchange_strong_explicit()` to ensure only one thread performs the scan. Alternatively, add a comment explaining why the race is benign (idempotent result, process-invariant value).

```c
int rte_dpaa2_mpool_get_ops_idx(void)
{
	int16_t cached;
	uint32_t i;

	cached = rte_atomic_load_explicit(&s_dpaa2_pool_ops_idx,
		rte_memory_order_relaxed);
	if (cached != RTE_MEMPOOL_MAX_OPS_IDX)
		return cached;

	/* Race is benign: concurrent scans produce the same result */
	for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
		if (strcmp(rte_mempool_ops_table.ops[i].name,
				DPAA2_MEMPOOL_OPS_NAME) == 0) {
			rte_atomic_store_explicit(&s_dpaa2_pool_ops_idx,
				(int16_t)i, rte_memory_order_relaxed);
			return (int16_t)i;
		}
	}

	DPAA2_MEMPOOL_ERR("dpaa2 mempool ops not found");
	return -ENOENT;
}
```

### Warnings

**Inconsistent return value on ops-not-found**

On success the function returns a non-negative ops index. On failure (ops not found) it returns `-ENOENT`. Callers that check `== RTE_MEMPOOL_MAX_OPS_IDX` for error (as the old primary-only code did) will not catch the new error case. Verify that all callers handle negative return values correctly, or return `RTE_MEMPOOL_MAX_OPS_IDX` on error to match the old behavior and avoid breaking callers that expect that sentinel.

---

## Summary

- **Patch 1**: Resource leak on error path (dynfield registration not rolled back); consider idempotent registration guard
- **Patch 2**: Possible incorrect device name format passed to `rte_bus_device_is_ignored()`
- **Patch 3**: Clean, no issues
- **Patch 4**: Clean, no issues
- **Patch 5**: Error code should be `-EINVAL` not `-ENOMEM` for configuration failure
- **Patch 6**: Non-atomic initialization of shared `s_dpaa2_pool_ops_idx` in secondary; return value inconsistency on error