|WARNING| [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/172116

_AI Code Review_

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

# DPDK Patch Review

## SUMMARY

This patch series refactors resource management in the DPAA and FSLMC bus drivers, fixing multiple resource leaks and improving error handling. The changes move interrupt handle allocation from scan time to probe time, fix several FD and memory leaks, and refactor device filtering logic.

---

## ERRORS

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

**Error path interrupt handle cleanup incomplete:**
```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);  /* Sets dev_fd to -1 */
	return error;
}
```
The error path sets `vfio_dev_fd` to -1 in the interrupt handle but does not close `vfio_dev_fd` itself. The caller's `vfio_dev_fd` remains open and is leaked. Should be:
```c
close(fd);
close(vfio_dev_fd);  /* Close the device FD passed by caller */
rte_intr_fd_set(intr_handle, -1);
rte_intr_dev_fd_set(intr_handle, -1);
```
Or document that the caller must close `vfio_dev_fd` on error (but that contradicts the pattern where success leaves both FDs owned by the intr_handle).

---

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

**Resource leak on dpaa_setup_intr failure:**
```c
ret = dpaa_setup_intr(dpaa_dev->intr_handle);
if (ret != 0) {
	DPAA_BUS_ERR("error setting up interrupt: %s", dpaa_dev->name);
	goto release_intr;
}

ret = dpaa_drv->probe(dpaa_drv, dpaa_dev);
if (ret != 0) {
	DPAA_BUS_ERR("unable to probe: %s", dpaa_dev->name);
	rte_intr_fd_close(dpaa_dev->intr_handle);  /* Closes FD */
release_intr:
	rte_intr_instance_free(dpaa_dev->intr_handle);  /* Frees handle */
	dpaa_dev->intr_handle = NULL;
}
```
If `dpaa_drv->probe()` succeeds, the function returns 0. If it then fails *later* (after the device is in use), the interrupt FD remains open with no path to close it. The interrupt handle is freed in `dpaa_bus_cleanup()`, but `rte_intr_fd_close()` is called there too--so the FD *is* closed. However, this is only on full bus cleanup. If the device is unplugged via `dpaa_bus_unplug_device()` (added in patch 04/16), the interrupt resources are freed but the setup is never reversed. Should add interrupt teardown in the unplug path in patch 04/16.

Wait--checking patch 04/16:
```c
next:
	rte_intr_fd_close(dev->intr_handle);
	rte_intr_instance_free(dev->intr_handle);
	dev->intr_handle = NULL;
```
This is in `dpaa_bus_cleanup()`, which is called on bus-wide teardown, not per-device unplug. The new `dpaa_bus_unplug_device()` in patch 04/16 does not close the interrupt FD. So: **interrupt FD leaked on device unplug** (before full bus cleanup).

---

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

**Double-close risk in fslmc_vfio_setup_device error path:**
```c
ret = fslmc_vfio_group_add_dev(vfio_group_fd, *vfio_dev_fd, dev_addr);
if (ret) {
	DPAA2_BUS_ERR("%s cannot add device in group err(%d)(%s)",
		dev_addr, ret, strerror(-ret));
	close(*vfio_dev_fd);  /* Close the FD */
	*vfio_dev_fd = -1;
	return ret;
}
```
If `fslmc_vfio_group_add_dev()` succeeds in opening/storing the FD but then fails *internally* (e.g., list insertion fails), the FD may already be tracked in the group's device list. Closing it here would leave a stale FD in the list, which `fslmc_vfio_clear_group()` would close again--double-close. Check `fslmc_vfio_group_add_dev()` implementation:
```c
LIST_FOREACH(group, &s_vfio_container.groups, next) {
	if (group->fd == vfio_group_fd) {
		dev = rte_zmalloc(NULL, sizeof(struct fslmc_vfio_device), 0);
		if (dev == NULL)
			return -ENOMEM;  /* FD not added to list yet */
		dev->fd = dev_fd;
		rte_strscpy(dev->dev_name, name, sizeof(dev->dev_name));
		LIST_INSERT_HEAD(&group->vfio_devices, dev, next);  /* FD now in list */
		return 0;
	}
}
```
Actually safe: if `rte_zmalloc()` fails, the FD is never added to the list, so the caller closing it is correct. No double-close. Not an error--clarification only.

---

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

**Use-after-free in dpaa2_close_dpio_device:**
```c
TAILQ_REMOVE(&dpio_dev_list, dpio_dev, next);
rte_intr_fd_close(dpio_dev->intr_handle);
/* dev_fd is closed by caller, clear reference here. */
rte_intr_dev_fd_set(dpio_dev->intr_handle, -1);
rte_intr_instance_free(dpio_dev->intr_handle);
rte_free(dpio_dev);
```
The comment says "dev_fd is closed by caller," implying the caller closes the VFIO device FD. But the caller is `fslmc_vfio_close_group()`, which does:
```c
RTE_TAILQ_FOREACH_SAFE(rte_dev, &fslmc_control_devices, next, tmp_dev) {
	dev = RTE_BUS_DEVICE(rte_dev, *dev);
	fslmc_close_iodevices(dev, vfio_group_fd);  /* Calls dpaa2_close_dpio_device */
}
```
Then:
```c
fslmc_vfio_clear_group(vfio_group_fd);
```
`fslmc_vfio_clear_group()` closes all device FDs in the group's device list:
```c
LIST_FOREACH(dev, &group->vfio_devices, next) {
	close(dev->fd);  /* Closes the dev_fd */
```
So the VFIO device FD (`dev_fd`) is closed by `fslmc_vfio_clear_group()`, not by the immediate caller. The `rte_intr_dev_fd_set(..., -1)` clears the stale reference in the interrupt handle so `rte_intr_fd_close()` doesn't try to close it again. This is correct--no error. The comment is slightly misleading (it's not the *direct* caller but a later step in the same function) but the code is safe.

---

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

**MPORTAL device leak when no suitable device found:**
```c
RTE_TAILQ_FOREACH_SAFE(rte_dev, &fslmc_control_devices, next, tmp_dev) {
	dev = RTE_BUS_DEVICE(rte_dev, *dev);
	if (dev->dev_type != DPAA2_MPORTAL)
		continue;
	ret = fslmc_process_mcp(dev);
	if (ret) {
		DPAA2_BUS_ERR("Unable to map MC Portal");
		return ret;  /* ERROR: dev not freed, still in control list */
	}
	fslmc_remove_control_device(dev);
	break;
}
```
If `fslmc_process_mcp()` fails, the function returns early. The `dev` pointer is still in `fslmc_control_devices` but is never removed. On cleanup, `fslmc_cleanup()` (patch 16/16) frees all devices in that list:
```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);
}
```
So the device *is* freed on cleanup. However, the MPORTAL device that failed to process is left in the list during the active run, which is inconsistent (other devices are removed after processing). Should remove it on error:
```c
ret = fslmc_process_mcp(dev);
if (ret) {
	DPAA2_BUS_ERR("Unable to map MC Portal");
	fslmc_remove_control_device(dev);  /* Free failed device */
	return ret;
}
```
**Error: MPORTAL device not freed on fslmc_process_mcp failure.**

---

## WARNINGS

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

**Release notes update does not specify user-visible impact:**
```diff
+* **Added unplug operation support to DPAA bus.**
+
+  Implemented device unplug operation to allow runtime removal of DPAA devices.
```
The note says "runtime removal" but does not clarify whether this enables `rte_dev_remove()` or only affects cleanup paths. If this is the first time the DPAA bus supports dynamic unplug (i.e., it was previously impossible to remove a device without restarting the app), that is significant and should be clearer. If it only fixes internal cleanup, "runtime removal" overstates it. Suggest: "The DPAA bus now supports device unplug via `rte_dev_remove()`, enabling runtime device removal."

---

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

**Missing Fixes tag for pre-existing leak:**
The patch fixes an interrupt handle leak introduced in commit 274fd921ff7f. The Fixes tag is present:
```
Fixes: 274fd921ff7f ("bus/fslmc: support close operation")
```
But the leak exists in three places (the two error paths in `dpaa2_create_dpio_device()` and the cleanup path in `dpaa2_close_dpio_device()`). The commit message says "cleanup code was leaking this handle" but the error paths also leak it. Suggest clarifying: "Fixes interrupt handle leak in DPIO cleanup and error paths."

---

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

**Control device list ordering not documented:**
The patch adds `insert_in_control_device_list()` that maintains sorted order via `compare_dpaa2_devname()`. The commit message says "keep a special case for MPORTAL/DPIO objects" but does not explain *why* ordering is needed. Is it for deterministic iteration during filtering? For debugging? Or is there a semantic requirement (e.g., MPORTALs must be processed before other control devices)? If the latter, the code is correct but the reasoning should be in the commit message.

---

## INFO

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

**Pre-existing: no device list cleanup on pthread_key_create failure in DPAA:**
In `rte_dpaa_bus_scan()`, if `pthread_key_create()` fails after `dpaa_create_device_list()` has populated the device list, the devices are not freed:
```c
ret = pthread_key_create(&dpaa_portal_key, dpaa_portal_finish);
if (ret) {
	DPAA_BUS_LOG(DEBUG, "Unable to create pthread key. (%d)", ret);
	return ret;  /* Device list not cleaned up */
}
```
This patch removes the cleanup helper but does not fix this path. The leak pre-dates this series and is not introduced here, so it is Info-level. Suggest fixing in a follow-up or noting in the commit message that this path remains unhandled.

---

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

**sscanf without error check on dev_id:**
```c
if (sscanf(dev_id, "%hu", &dev->object_id) != 1) {
	DPAA2_BUS_ERR("Failed to parse object ID");
	ret = -EINVAL;
	goto cleanup;
}
```
This is correct--`sscanf()` is checked. However, note that `dev_id` points into `dev_name` after prefix stripping, so if `dev_name` is malformed (e.g., "dpni." with no digits), `sscanf()` would fail as intended. Not an error, just documenting that the existing validation is sufficient.

---

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

**Unnecessary VFIO device FD for CRYPTO/QDMA:**
The commit message notes:
> Note: before this change, CRYPTO and QDMA devices were getting a VFIO device FD even though nothing seems to be done with it. This is kept as is, the device FD is closed on call to fslmc_vfio_dev_close().

If the FD is unused, acquiring it is wasteful (one FD per device). Consider skipping VFIO setup for these device types in a follow-up. Not an error--current behavior preserved.

---

Review-Result: ERROR