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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-17

# DPDK Patch Review

## Patch Series Overview
This 16-patch series refactors the NXP DPAA/FSLMC bus drivers to fix resource leaks, improve device lifecycle management, and enable proper cleanup operations.

---

## Patch 01/16: drivers/bus: cleanup device freeing in NXP bus scan

**Summary:** Removes `dpaa_clean_device_list()` and `cleanup_fslmc_device_list()` helpers after scan/probe refactoring.

### Errors

**Error 1: Resource leak on scan failure (DPAA)**
The cleanup path calls `rte_bus_remove_device()` but does not free the device memory itself:
```c
cleanup:
	RTE_BUS_FOREACH_DEV(dev, &rte_dpaa_bus) {
		rte_bus_remove_device(&rte_dpaa_bus, &dev->device);
		rte_intr_instance_free(dev->intr_handle);
		free(dev);  /* MISSING: dev pointer is leaked after removal */
	}
```
**Issue:** `RTE_BUS_FOREACH_DEV` iterates over `&rte_dpaa_bus.device_list`, but after `rte_bus_remove_device()` the device is no longer in the list. The loop continues with a stale iterator, causing undefined behavior. The `free(dev)` call operates on a removed list node.

**Fix:** Use `RTE_BUS_FOREACH_DEV_SAFE` or collect devices to free in a separate pass:
```c
cleanup:
	while (!TAILQ_EMPTY(&rte_dpaa_bus.device_list)) {
		struct rte_device *rte_dev = TAILQ_FIRST(&rte_dpaa_bus.device_list);
		dev = RTE_BUS_DEVICE(rte_dev, struct rte_dpaa_device);
		rte_bus_remove_device(&rte_dpaa_bus, &dev->device);
		rte_intr_instance_free(dev->intr_handle);
		free(dev);
	}
```

**Error 2: Resource leak on scan failure (FSLMC)**
Same issue in FSLMC cleanup loop -- iterator invalidation after `rte_bus_remove_device()`.

---

## Patch 02/16: drivers/bus: fix FD leak during scan in NXP drivers

**Summary:** Closes eventfd on interrupt handle setup failure.

### Review: Correct

The patch correctly closes the `fd` and resets the interrupt handle on error:
```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;
}
```
This prevents leaking the eventfd when `rte_intr_fd_set()` or `rte_intr_type_set()` fails.

---

## Patch 03/16: bus/dpaa: allocate interrupt during probing

**Summary:** Moves interrupt handle allocation from scan to probe, with corresponding unplug cleanup.

### Errors

**Error 1: `dpaa_bus_cleanup()` return value lost on error path**
```c
ret = drv->remove(dev);
if (ret < 0) {
	rte_errno = errno;
	error = -1;
	goto next;
}
```
Sets `rte_errno = errno`, but `errno` is not necessarily set by `drv->remove()`. Should be:
```c
if (ret < 0) {
	error = ret;  /* propagate actual error code */
	goto next;
}
```

**Error 2: Missing check for `dpaa_dev->intr_handle` already allocated**
In `dpaa_bus_probe_device()`:
```c
dpaa_dev->intr_handle = rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
if (dpaa_dev->intr_handle == NULL) {
	DPAA2_BUS_LOG(ERR, "Failed to allocate intr handle");
	return -ENOMEM;
}
```
If `probe_device` is called twice on the same device (e.g., after a failed probe retry), this leaks the previous `intr_handle`. Should check:
```c
if (dpaa_dev->intr_handle != NULL) {
	/* Already allocated, cleanup or return error */
	return -EALREADY;
}
```

---

## Patch 04/16: bus/dpaa: support unplug and use generic cleanup

**Summary:** Adds `.unplug` callback and uses `rte_bus_generic_cleanup()`.

### Warnings

**Warning 1: `dpaa_bus_cleanup()` ignores `rte_bus_generic_cleanup()` return value context**
```c
ret = rte_bus_generic_cleanup(bus);

dpaa_portal_finish((void *)DPAA_PER_LCORE_PORTAL);
dpaa_bus_global_init = 0;
```
If `rte_bus_generic_cleanup()` fails, the portal is still finalized and `global_init` reset. This may leave the bus in an inconsistent state. Consider:
```c
ret = rte_bus_generic_cleanup(bus);
if (ret == 0) {
	dpaa_portal_finish((void *)DPAA_PER_LCORE_PORTAL);
	dpaa_bus_global_init = 0;
}
```

---

## Patch 05/16: bus/fslmc: fix device name leak

**Summary:** Stores device name in `dev->name` array instead of allocated string, fixes leak.

### Review: Correct

Replacing `strdup()` + separate pointer with `rte_strscpy()` into the device's own `name` field eliminates the leak and simplifies cleanup. The change from `FSLMC_OBJECT_MAX_LEN` (32) to `RTE_DEV_NAME_MAX_LEN` (64) is safe.

---

## Patch 06/16: bus/fslmc: fix memory leaks in scan

**Summary:** Adds `fslmc_bus_remove_device()` helper to free device and interrupt handle together.

### Errors

**Error 1: `scan_one_fslmc_device()` cleanup path may double-free on error**
```c
cleanup:
	free(dup_dev_name);
	if (dev) {
		rte_intr_instance_free(dev->intr_handle);
		free(dev);
	}
```
If `dup_dev_name` allocation failed, `free(dup_dev_name)` is called on uninitialized pointer. Should be:
```c
	if (dup_dev_name)
		free(dup_dev_name);
```
Or initialize `dup_dev_name = NULL` at declaration.

---

## Patch 07/16: bus/fslmc: fix per type device count

**Summary:** Moves device count increment/decrement into `insert_in_device_list()` / `fslmc_free_device()`.

### Review: Correct

The count is now correctly decremented when a blocklisted device is removed, fixing the discrepancy.

---

## Patch 08/16: bus/fslmc: fix some VFIO device FD and memory leaks

**Summary:** Closes VFIO device FD on setup failure and in cleanup, frees device tracking structures.

### Errors

**Error 1: `fslmc_vfio_clear_group()` list removal inside loop**
```c
LIST_FOREACH(group, &s_vfio_container.groups, next) {
	if (group->fd == vfio_group_fd) {
		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);
		}
		close(vfio_group_fd);
		LIST_REMOVE(group, next);
		rte_free(group);
		clear = 1;
		break;  /* REQUIRED: must break after removing group */
	}
}
```
The code does `break`, so this is correct. No issue.

---

## Patch 09/16: bus/fslmc: fix interrupt leak in DPIO cleanup

**Summary:** Adds `rte_intr_instance_free(dpio_dev->intr_handle)` in `dpaa2_close_dpio_device()`.

### Review: Correct

The DPIO device has a separate `intr_handle` from the bus device object; this was being leaked on close. Patch correctly frees it.

---

## Patch 10/16: bus/fslmc: simplify device parsing in scan

**Summary:** Refactors string parsing to use `strncmp()` on a prefix table and avoids `strdup()`.

### Review: Correct

Eliminates allocation, uses stack parsing. The `sscanf(dev_id, "%hu", &dev->object_id)` correctly parses the numeric ID. The fallback for unknown devices (finding `.` delimiter) preserves existing behavior.

---

## Patch 11/16: bus/fslmc: release resources on scan failure

**Summary:** Unwinds VFIO setup, DMA mapping, and device list on scan failure.

### Errors

**Error 1: Missing `closedir(dir)` on early scan failure**
```c
ret = scan_one_fslmc_device(group_name);
if (ret == 0) {
	while ((entry = readdir(dir)) != NULL) {
		/* ... */
	}
}

closedir(dir);
if (ret != 0)
	goto scan_fail;
```
If `scan_one_fslmc_device(group_name)` fails (`ret != 0`), the code skips the `while` loop but still calls `closedir(dir)` before jumping to `scan_fail`. This is correct. No leak.

---

## Patch 12/16: bus/fslmc: refactor device filtering for multiprocess

**Summary:** Splits device filtering into separate control device list, moves MPORTAL/DPIO selection into `fslmc_filter_control_devices()`.

### Errors

**Error 1: Control device list not freed on scan failure before VFIO setup**
If `fslmc_filter_control_devices()` fails after populating the control list but before `fslmc_vfio_process_group()`, the cleanup at `scan_fail:` only frees devices in `&rte_fslmc_bus.device_list`, not `&fslmc_control_devices`. The patch adds:
```c
scan_fail:
	while (!TAILQ_EMPTY(&fslmc_control_devices)) {
		struct rte_device *rte_dev = TAILQ_FIRST(&fslmc_control_devices);
		dev = RTE_BUS_DEVICE(rte_dev, *dev);
		fslmc_remove_control_device(dev);
	}
```
This is correct.

---

## Patch 13/16: bus/fslmc: move unplug for some device out of VFIO

**Summary:** Moves device unplug from `fslmc_vfio.c` to `fslmc_bus.c` cleanup.

### Review: Correct

Separation of concerns: the VFIO layer should not be responsible for calling driver `remove()` callbacks. The bus cleanup now handles unplug before closing VFIO.

---

## Patch 14/16: bus/fslmc: call VFIO setup for some device from bus layer

**Summary:** Moves VFIO setup for ETH/CRYPTO/QDMA from `fslmc_vfio_process_group()` to scan, wrapped in `fslmc_vfio_dev_setup()`.

### Errors

**Error 1: Missing cleanup on partial VFIO setup failure**
```c
RTE_BUS_FOREACH_DEV(dev, &rte_fslmc_bus) {
	ret = fslmc_vfio_dev_setup(dev);
	if (ret) {
		DPAA2_BUS_ERR("Dev (%s) VFIO setup failed", dev->device.name);
		goto vfio_dev_close;
	}
}
```
At `vfio_dev_close`, the code calls:
```c
RTE_BUS_FOREACH_DEV(dev, &rte_fslmc_bus)
	fslmc_vfio_dev_close(dev);
```
This attempts to close all devices, including those that were never set up. If `fslmc_vfio_dev_setup()` fails on the first device, the subsequent devices have no VFIO state to close, but `fslmc_vfio_dev_close()` is called anyway. The implementation of `fslmc_vfio_dev_close()` calls `fslmc_vfio_group_remove_dev()`, which may return `-ENODEV` if the device was never added. This is benign but inefficient. Consider tracking which devices were successfully set up, or making `fslmc_vfio_dev_close()` idempotent.

---

## Patch 15/16: bus/fslmc: allocate interrupt during probing

**Summary:** Moves interrupt handle allocation from scan to `probe_device`, with cleanup in `unplug_device`.

### Review: Correct

Matches the pattern in Patch 03 for DPAA. Interrupt handle is allocated only when the device is probed, avoiding waste for blocklisted devices.

---

## Patch 16/16: bus/fslmc: use generic cleanup

**Summary:** Switches to `rte_bus_generic_cleanup()` and adds control device list cleanup.

### Review: Correct

The cleanup now properly calls the generic helper, then closes VFIO, then frees control devices. The order is correct: unplug drivers first, then release bus resources.

---

## Summary of Findings

### Errors (Must Fix)
1. **Patch 01:** Iterator invalidation in DPAA and FSLMC cleanup loops (use-after-remove).
2. **Patch 03:** `dpaa_bus_cleanup()` sets `rte_errno = errno` incorrectly; should propagate `ret`.
3. **Patch 03:** Missing check for already-allocated `intr_handle` in `probe_device` (potential leak on retry).
4. **Patch 06:** `dup_dev_name` may be uninitialized in cleanup path (use-after-free or double-free risk).

### Warnings (Should Fix)
1. **Patch 04:** `dpaa_bus_cleanup()` proceeds with portal finalization even if `rte_bus_generic_cleanup()` fails.
2. **Patch 14:** Partial VFIO setup failure attempts to close devices that were never set up (inefficiency, not a bug).

### Info (Consider)
- The series is a substantial improvement, converting ad-hoc cleanup into structured probe/unplug callbacks.
- Release notes are correctly updated in Patch 04 for the DPAA unplug feature.
- The separation of control devices from bus devices in Patch 12 is a good design change.

---

## Code Style and API Compliance

- **Naming:** All symbols follow `rte_` / `RTE_` or driver-specific prefixes (`dpaa_`, `fslmc_`).
- **Error handling:** Generally correct, except for the issues noted above.
- **Release notes:** Present where required (Patch 04 for new unplug operation).
- **Resource management:** Significantly improved, though the iterator invalidation bugs must be fixed.

---

## Final Recommendation

**Do not merge until the iterator invalidation bugs in Patch 01 are fixed.** The other errors and warnings should also be addressed. Once corrected, this series is a valuable cleanup that resolves multiple resource leaks and brings the NXP buses closer to the standard DPDK bus API patterns.