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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch Series Overview
This series refactors the NXP DPAA and FSLMC bus drivers to:
1. Fix resource management during scan/probe lifecycle
2. Add unplug operation support
3. Enable generic bus cleanup helpers

---

## Patch 1/11: drivers/bus: cleanup device freeing in NXP bus scan

### Issues Found

**Error:**
Resource leak on error path in `dpaa_create_device_list()`:
- After the `cleanup:` label, the code removes devices from the bus and frees them, but does NOT free the `dev->device.name` string that was allocated with `strdup()` earlier in the function.
- The same pattern exists in the removed `dpaa_clean_device_list()` helper.

```c
/* Current code at 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);  /* removed in this patch */
    free(dev);  /* leaks dev->device.name */
}
```

**Suggested fix:**
```c
RTE_BUS_FOREACH_DEV(dev, &rte_dpaa_bus) {
    rte_bus_remove_device(&rte_dpaa_bus, &dev->device);
    free(dev->device.name);
    free(dev);
}
```

---

## Patch 2/11: bus/dpaa: allocate interrupt during probing

### Issues Found

**Error:**
Error path in `dpaa_bus_probe_device()` calls `rte_intr_instance_free()` without checking if `dpaa_dev->intr_handle` is NULL first:
- If `rte_intr_instance_alloc()` fails and returns NULL, the subsequent `dpaa_setup_intr()` check jumps to `release_intr:` where `rte_intr_instance_free(dpaa_dev->intr_handle)` is called on NULL.
- While `rte_intr_instance_free()` may handle NULL gracefully, the code sets `dpaa_dev->intr_handle = NULL` redundantly.

```c
/* Current code: */
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;  /* Should return here directly */
}
```

**Suggested fix:**
Return directly on allocation failure instead of jumping to cleanup:
```c
dpaa_dev->intr_handle = rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
if (dpaa_dev->intr_handle == NULL) {
    DPAA_BUS_LOG(ERR, "Failed to allocate intr handle");
    return -ENOMEM;
}
```

---

**Warning:**
Missing error code propagation in `dpaa_bus_cleanup()`:
- After calling `dpaa_bus_unplug_device()` which returns `int`, the return value `ret` is checked but not propagated. The function continues and returns a potentially different `ret` value from the final steps.
- If any device unplug fails with an error, subsequent operations may proceed incorrectly.

```c
/* Current code: */
ret = drv->remove(dev);
if (ret < 0) {
    rte_errno = errno;
    goto next;  /* continues loop, ignoring error */
}
```

**Suggested fix:**
Accumulate or propagate the first error:
```c
int first_error = 0;
...
ret = drv->remove(dev);
if (ret < 0) {
    if (first_error == 0)
        first_error = ret;
}
...
return first_error;
```

---

## Patch 3/11: bus/dpaa: support unplug and use generic cleanup

### Issues Found

None. The refactoring correctly separates unplug operations from cleanup, and the new `dpaa_bus_free_device()` properly frees the device structure. Error handling in `dpaa_bus_cleanup()` now relies on `rte_bus_generic_cleanup()` which should handle device iteration safely.

---

## Patch 4/11: bus/fslmc: fix memory leaks in scan

### Issues Found

**Error:**
The helper `fslmc_bus_remove_device()` does NOT free `dev->device.name`:
- The `scan_one_fslmc_device()` function allocates `dev->device.name` with `strdup()`.
- When a device is removed via `fslmc_bus_remove_device()`, only the interrupt handle and the device structure are freed, but `dev->device.name` is leaked.

```c
/* Current code in fslmc_bus_remove_device(): */
void
fslmc_bus_remove_device(struct rte_dpaa2_device *dev)
{
    rte_bus_remove_device(&rte_fslmc_bus, &dev->device);
    rte_intr_instance_free(dev->intr_handle);
    free(dev);  /* leaks dev->device.name */
}
```

**Suggested fix:**
```c
void
fslmc_bus_remove_device(struct rte_dpaa2_device *dev)
{
    if (dev->dev_type < DPAA2_DEVTYPE_MAX)
        fslmc_bus_device_count[dev->dev_type]--;
    rte_bus_remove_device(&rte_fslmc_bus, &dev->device);
    rte_intr_instance_free(dev->intr_handle);
    free(dev->device.name);
    free(dev);
}
```

---

## Patch 5/11: bus/fslmc: fix per type device count

### Issues Found

None. The refactoring correctly moves the device count update into `insert_in_device_list()` and `fslmc_bus_remove_device()`, ensuring accurate tracking when devices are blocklisted during scan.

---

## Patch 6/11: bus/fslmc: simplify device parsing in scan

### Issues Found

**Warning:**
In the device type lookup, using `strncmp()` without verifying the string lengths could theoretically match a prefix of a longer device name:
- For example, `strncmp("dpni.", "dpni.foo.bar", 5)` succeeds even if the input is longer than expected.
- This is acceptable because the code immediately uses the remainder of the string (`dev_id`) for parsing, but it's worth noting.

No functional issue here; the logic is sound.

---

## Patch 7/11: bus/fslmc: refactor device filtering for multiprocess

### Issues Found

**Error:**
In `fslmc_filter_control_devices()`, the logic for selecting MPORTAL/DPIO devices has a potential off-by-one or loop termination issue:
- The `RTE_BUS_FOREACH_DEV()` macro iterates over the bus device list, but the code modifies the list during iteration by calling `fslmc_bus_remove_device(dev)`.
- If `dev` is removed from the list, the iterator may become invalid or skip devices.

**Suggested fix:**
Use a safe iteration pattern or collect devices to remove in a first pass, then remove them in a second pass:
```c
struct rte_dpaa2_device *devices_to_remove[MAX_DEVICES];
int count = 0;
RTE_BUS_FOREACH_DEV(dev, &rte_fslmc_bus) {
    if (/* condition to remove */)
        devices_to_remove[count++] = dev;
}
for (int i = 0; i < count; i++)
    fslmc_bus_remove_device(devices_to_remove[i]);
```

---

**Warning:**
The new `fslmc_filter_control_devices()` function performs complex multiprocess and blocklist filtering logic. The code is readable, but the nesting and multiple `RTE_BUS_FOREACH_DEV()` loops make it harder to verify correctness. Consider adding comments explaining the MPORTAL/DPIO split policy (primary keeps first, secondary keeps last).

---

## Patch 8/11: bus/fslmc: move unplug for some device out of VFIO

### Issues Found

None. The refactoring moves device unplug out of VFIO-specific code and into the bus layer, which is a cleaner separation. The `RTE_VERIFY()` assertions in `fslmc_close_iodevices()` ensure that ETH/CRYPTO/QDMA devices are not processed there.

---

## Patch 9/11: bus/fslmc: call VFIO setup for some device from bus layer

### Issues Found

**Error:**
In `fslmc_vfio_dev_close()`, the function returns an error if `vfio_group_fd == 0`:
```c
if (vfio_group_fd <= 0) {
    DPAA2_BUS_ERR("Get fd by name(%s) failed(%d)", group_name, vfio_group_fd);
    if (vfio_group_fd < 0)
        return vfio_group_fd;
    return -EIO;
}
```
- If `vfio_group_fd == 0`, the function returns `-EIO`.
- However, `0` is a valid file descriptor (stdin). The check should be `< 0` only, or document why `0` is invalid here.

**Suggested fix:**
If `0` is invalid for VFIO group fds, document it. Otherwise:
```c
if (vfio_group_fd < 0) {
    DPAA2_BUS_ERR("Get fd by name(%s) failed(%d)", group_name, vfio_group_fd);
    return vfio_group_fd;
}
```

---

## Patch 10/11: bus/fslmc: allocate interrupt during probing

### Issues Found

**Error:**
In `fslmc_bus_probe_device()`, on failure after `fslmc_vfio_dev_setup()`, the error path jumps to `release_intr:` which calls `fslmc_vfio_dev_close()`:
- If `fslmc_vfio_dev_setup()` fails partway through (e.g., interrupt setup fails), calling `fslmc_vfio_dev_close()` may attempt to close resources that were never fully initialized.
- The code should only call `fslmc_vfio_dev_close()` if `fslmc_vfio_dev_setup()` succeeded.

**Suggested fix:**
```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);  /* only call if setup succeeded */
release_intr:
    rte_intr_instance_free(dev->intr_handle);
    dev->intr_handle = NULL;
}
```

---

**Error:**
In `scan_one_fslmc_device()`, the removal of interrupt handle allocation means `dev->intr_handle` is now uninitialized until `fslmc_bus_probe_device()` is called:
- If any code path accesses `dev->intr_handle` before probing, it will dereference uninitialized memory.
- Verify that no code between scan and probe accesses `dev->intr_handle`.

**Suggested fix:**
Initialize `dev->intr_handle = NULL` in `scan_one_fslmc_device()`:
```c
dev->device.numa_node = SOCKET_ID_ANY;
dev->dev_type = dev_type;
dev->intr_handle = NULL;  /* initialize to NULL */
```

---

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

### Issues Found

**Error:**
In `fslmc_cleanup()`, the return value of `rte_bus_generic_cleanup()` is stored in `ret`, but then immediately overwritten by the return value of `fslmc_vfio_close_group()`:
```c
ret = rte_bus_generic_cleanup(bus);

ret = fslmc_vfio_close_group();  /* overwrites ret */
if (ret)
    DPAA2_BUS_INFO("Unable to close devices %d", ret);

return ret;  /* loses error from generic_cleanup */
```

**Suggested fix:**
Preserve the first error or combine them:
```c
int ret1 = rte_bus_generic_cleanup(bus);
int ret2 = fslmc_vfio_close_group();
if (ret2)
    DPAA2_BUS_INFO("Unable to close devices %d", ret2);
return ret1 ? ret1 : ret2;
```

---

**Warning:**
The new `fslmc_free_device()` does NOT free `dev->device.name`:
- The same issue as in Patch 4: `dev->device.name` was allocated with `strdup()` in `scan_one_fslmc_device()` but is never freed.
- This should be added to either `fslmc_free_device()` or `fslmc_bus_remove_device()`.

---

## Summary

### Errors (must fix):
1. **Patch 1**: Resource leak: `dev->device.name` not freed in cleanup path
2. **Patch 2**: Redundant NULL check and cleanup jump when allocation fails
3. **Patch 4**: Memory leak: `dev->device.name` not freed in `fslmc_bus_remove_device()`
4. **Patch 7**: Unsafe list modification during iteration in `fslmc_filter_control_devices()`
5. **Patch 9**: File descriptor `0` incorrectly treated as error in `fslmc_vfio_dev_close()`
6. **Patch 10**: Error path calls `fslmc_vfio_dev_close()` even if setup never completed
7. **Patch 10**: `dev->intr_handle` uninitialized between scan and probe
8. **Patch 11**: Error from `rte_bus_generic_cleanup()` overwritten and lost
9. **Patch 11**: `dev->device.name` still not freed in `fslmc_free_device()`

### Warnings:
1. **Patch 2**: Error propagation in `dpaa_bus_cleanup()` could be improved
2. **Patch 7**: Complex filtering logic in `fslmc_filter_control_devices()` needs comments

### Info:
- The overall refactoring direction is sound: moving resource allocation to probe time and adding unplug support are good improvements.
- The use of `rte_bus_generic_cleanup()` simplifies the bus cleanup logic.
- Consider adding release notes entries for the new unplug support as mentioned in Patch 3.