|WARNING| [16/16] bus/fslmc: use generic cleanup

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review - NXP Bus Drivers Cleanup Series

Reviewed patches 01-16/16 addressing NXP DPAA/FSLMC bus driver cleanup, resource leak fixes, and architectural improvements.

---

## ERRORS

### Patch 02/16 - FD leak fix

**Line**: `drivers/bus/dpaa/dpaa_bus.c:651-657`
```c
if (rte_intr_fd_set(intr_handle, fd) ||
		rte_intr_type_set(intr_handle, RTE_INTR_HANDLE_EXT)) {
	int err = rte_errno;

	close(fd);
	rte_intr_fd_set(intr_handle, -1);
	return err;
}
```
**Issue**: Eventfd leaked if first `rte_intr_fd_set()` succeeds but second `rte_intr_type_set()` fails -- the fd stored in `intr_handle` by the first call is not closed.

**Fix**: Retrieve fd from handle before closing:
```c
if (rte_intr_fd_set(intr_handle, fd)) {
	close(fd);
	return -rte_errno;
}
if (rte_intr_type_set(intr_handle, RTE_INTR_HANDLE_EXT)) {
	int err = -rte_errno;
	close(rte_intr_fd_get(intr_handle));
	rte_intr_fd_set(intr_handle, -1);
	return err;
}
```

---

### Patch 02/16 - FSLMC eventfd leak

**Line**: `drivers/bus/fslmc/fslmc_vfio.c:1385-1393`
```c
if (rte_intr_fd_set(intr_handle, fd) ||
		rte_intr_type_set(intr_handle, RTE_INTR_HANDLE_VFIO_MSI) ||
		rte_intr_dev_fd_set(intr_handle, vfio_dev_fd)) {
	int error = -rte_errno;

	close(fd);
	rte_intr_fd_set(intr_handle, -1);
	rte_intr_dev_fd_set(intr_handle, -1);
	return error;
}
```
**Issue**: Same pattern as DPAA -- if `rte_intr_fd_set()` succeeds but a later setter fails, the fd stored by the first call is leaked.

**Fix**: Close fd from handle if already stored:
```c
if (rte_intr_fd_set(intr_handle, fd)) {
	close(fd);
	return -rte_errno;
}
if (rte_intr_type_set(intr_handle, RTE_INTR_HANDLE_VFIO_MSI) ||
		rte_intr_dev_fd_set(intr_handle, vfio_dev_fd)) {
	int error = -rte_errno;
	close(rte_intr_fd_get(intr_handle));
	rte_intr_fd_set(intr_handle, -1);
	rte_intr_dev_fd_set(intr_handle, -1);
	return error;
}
```

---

### Patch 03/16 - Interrupt cleanup on probe failure

**Line**: `drivers/bus/dpaa/dpaa_bus.c:790-796`
```c
ret = dpaa_drv->probe(dpaa_drv, dpaa_dev);
if (ret != 0) {
	DPAA_BUS_ERR("unable to probe: %s", dpaa_dev->name);
	dpaa_close_intr(dpaa_dev->intr_handle);
release_intr:
	rte_intr_instance_free(dpaa_dev->intr_handle);
	dpaa_dev->intr_handle = NULL;
}
```
**Issue**: If `dpaa_setup_intr()` succeeds (sets fd in handle) but driver probe fails, the eventfd is closed twice: once in `dpaa_close_intr()` and again implicitly when `rte_intr_instance_free()` is called (if the interrupt instance cleanup closes the fd). The `dpaa_close_intr()` call sets fd to -1, so the double-close is avoided, but this relies on knowing the internals of `rte_intr_instance_free()`.

**Recommendation**: Document that `dpaa_close_intr()` must be called before `rte_intr_instance_free()` when the handle holds a valid fd, or verify `rte_intr_instance_free()` does not close fds set to -1. Code is likely correct but fragile.

---

### Patch 12/16 - Device filtering

**Line**: `drivers/bus/fslmc/fslmc_bus.c:442-446`
```c
if (rte_eal_process_type() == RTE_PROC_SECONDARY && dpmcp_count < 2) {
	DPAA2_BUS_ERR("No MC Portal device found for secondary");
	return -ENODEV;
}
```
**Issue**: Error path returns without cleaning up devices already allocated during scan. All `fslmc_control_devices` and bus device list entries are leaked.

**Fix**: On any error in `fslmc_filter_control_devices()`, caller (`rte_fslmc_probe()`) must unwind to cleanup labels that free both lists (as done for other errors in probe).

---

### Patch 15/16 - Interrupt allocation on probe

**Line**: `drivers/bus/fslmc/fslmc_bus.c:728-738`
```c
ret = fslmc_vfio_dev_setup(dev);
if (ret) {
	DPAA2_BUS_ERR("Dev (%s) VFIO setup failed", dev->device.name);
	goto release_intr;
}

ret = drv->probe(drv, dev);
if (ret != 0) {
	DPAA2_BUS_ERR("Unable to probe");
	fslmc_vfio_dev_close(dev);
release_intr:
	rte_intr_instance_free(dev->intr_handle);
	dev->intr_handle = NULL;
}
```
**Issue**: If `fslmc_vfio_dev_setup()` succeeds (opens device fd, sets interrupt) but driver probe fails, the error path calls `fslmc_vfio_dev_close()` which closes the interrupt fd, then `rte_intr_instance_free()` which may attempt to close it again. Same fragility as Patch 03/16.

**Recommendation**: Verify that `fslmc_vfio_dev_close()` sets fd to -1 in the handle after closing (review shows `dpaa2_close_intr()` does this, so likely safe). Document the ordering requirement.

---

## WARNINGS

### Patch 04/16 - Release notes

**Missing entry**: Patch adds `.unplug` callback to DPAA bus and notes it in `doc/guides/rel_notes/release_26_11.rst`, but does not document what this enables for users. The note "Implemented device unplug operation to allow runtime removal of DPAA devices" is accurate but does not explain the user-visible benefit (e.g., does this enable hotplug? dynamic reconfiguration?).

**Suggestion**: Expand release note to clarify use case, e.g., "Added device unplug support to DPAA bus, enabling runtime device removal via `rte_dev_remove()` and cleanup on `rte_eal_cleanup()`."

---

### Patch 06/16 - Memory leak fix

**Missing Fixes tag and Cc stable**: Patch fixes memory leak introduced in commit `828d51d8fc3e`. Should have:
```
Fixes: 828d51d8fc3e ("bus/fslmc: refactor scan and probe functions")
Cc: stable at dpdk.org
```
(Note: Patch already has these, this is informational only.)

---

### Patch 10/16 - String parsing refactor

**Line**: `drivers/bus/fslmc/fslmc_bus.c:142-164`
```c
static const struct {
	const char *prefix;
	enum rte_dpaa2_dev_type type;
} dev_types[] = {
	{ "dpni.", DPAA2_ETH },
	...
};
```
**Style preference**: Array of structures with string prefixes is clear, but the loop uses `strlen(dev_types[i].prefix)` repeatedly. Consider extracting prefix length into the struct or using a sentinel (empty prefix) to avoid repeated `strlen()` calls in hot path (scan is not hot, but good practice).

**Not an error**, just an optimization opportunity.

---

### Patch 12/16 - Control device list ordering

**Line**: `drivers/bus/fslmc/fslmc_bus.c:118-133`
```c
static void
insert_in_control_device_list(struct rte_dpaa2_device *dev)
{
	struct rte_device *rte_dev;
	struct rte_device *prev;

	prev = NULL;
	TAILQ_FOREACH(rte_dev, &fslmc_control_devices, next) {
		if (compare_dpaa2_devname(dev, RTE_BUS_DEVICE(rte_dev, *dev)) < 0)
			break;
		prev = rte_dev;
	}
	...
}
```
**Issue**: Insertion maintains sorted order, but `compare_dpaa2_devname()` compares device names lexicographically. If device discovery order is stable, sorting is unnecessary overhead. If discovery order is unstable, sorting ensures deterministic iteration, which is good for testing but not explained in comments.

**Suggestion**: Document why control device list must be sorted (if it must), or remove sorting if discovery order is already deterministic.

---

## INFO (Pre-existing or minor observations)

### Patch 01/16 - Cleanup refactor

**Observation**: Removes `dpaa_clean_device_list()` and `cleanup_fslmc_device_list()` helpers. Good -- fewer one-shot functions. The `dev = NULL` assignment after `free(dev)` in the original code was unnecessary (dead store), correctly removed.

---

### Patch 05/16 - Device name allocation fix

**Improvement**: Storing device name in the `rte_dpaa2_device.name` array instead of `strdup()` eliminates a malloc and simplifies cleanup. Good change.

---

### Patch 07/16 - Per-type device count

**Observation**: Moving counter increment/decrement into `insert_in_device_list()` and `fslmc_free_device()` centralizes accounting. Clean design.

---

### Patch 08/16 - VFIO device FD tracking

**Line**: `drivers/bus/fslmc/fslmc_vfio.c:229`
```c
while (!LIST_EMPTY(&group->vfio_devices)) {
	struct fslmc_vfio_device *dev = LIST_FIRST(&group->vfio_devices);

	close(dev->fd);
	LIST_REMOVE(dev, next);
	rte_free(dev);
}
```
**Good pattern**: Correct cleanup of list with embedded allocation. No leak.

---

### Patch 11/16 - DMA unmap on probe failure

**Line**: `drivers/bus/fslmc/fslmc_vfio.c:1233-1236`
```c
if (ret != 0) {
	i = 0;
	rte_memseg_walk(fslmc_dmaunmap_seg, &i);
}
```
**Observation**: On failure, all segments mapped so far are unmapped. Correct unwind, but note that `fslmc_dmaunmap_seg()` logs errors but does not stop on first failure -- all segments are attempted, which is the right behavior for cleanup.

---

### Patch 13/16 - Device unplug separation

**Observation**: Moving device removal loop out of `fslmc_vfio_close_group()` into `rte_fslmc_close()` separates concerns cleanly. VFIO code no longer knows about driver `->remove()` callbacks, which is the correct boundary.

---

### Patch 16/16 - Generic cleanup

**Line**: `drivers/bus/fslmc/fslmc_bus.c:642-648`
```c
while (!TAILQ_EMPTY(&fslmc_control_devices)) {
	struct rte_device *rte_dev = TAILQ_FIRST(&fslmc_control_devices);
	struct rte_dpaa2_device *dev = RTE_BUS_DEVICE(rte_dev, *dev);

	fslmc_remove_control_device(dev);
}
```
**Good**: Cleanup of control devices is explicit and separate from bus device cleanup. No leak.

---

## SUMMARY

Series refactors NXP DPAA/FSLMC bus drivers to fix resource leaks, improve error handling, and adopt generic bus operations. Most patches are correct and improve code quality.

**Critical fixes needed**:
- Patch 02: Eventfd leak on partial interrupt setup failure (both DPAA and FSLMC).
- Patch 12: Missing cleanup on `fslmc_filter_control_devices()` failure.

**Review needed**:
- Patches 03 and 15: Interrupt fd double-close risk (likely safe due to fd reset to -1, but verify).

**Minor issues**:
- Patch 04: Release note could be more descriptive.
- Patch 12: Control device list sorting rationale undocumented.

---

Review-Result: ERROR