|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: NXP Bus Cleanup Series

## Summary

This 11-patch series refactors device lifecycle management for the DPAA and FSLMC buses, implementing unplug operations and moving toward generic bus cleanup patterns. The series addresses resource management, device filtering, and VFIO setup/teardown.

---

## Patch-by-Patch Findings

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

**Errors:**
- **Resource leak on error path** (dpaa_bus.c:338): The cleanup label frees device structures but does NOT free `dev->device.name` (allocated via `strdup()` in dpaa_create_device_list). Each device has a `dev->device.name = strdup(dev_name)` but the cleanup only does `free(dev)`, leaking the name string.
  ```c
  /* In dpaa_create_device_list, name is allocated: */
  dev->device.name = strdup(dev_name);
  /* But cleanup does not free it: */
  cleanup:
      RTE_BUS_FOREACH_DEV(dev, &rte_dpaa_bus) {
          rte_bus_remove_device(&rte_dpaa_bus, &dev->device);
          free(dev);  /* dev->device.name is leaked */
      }
  ```
  Fix: Add `free((void *)dev->device.name);` before `free(dev)`.

- **Resource leak on error path** (fslmc_bus.c:407-410): Same issue - devices have `dev->device.name = strdup(dev_name)` but the cleanup loop does not free the name before freeing the device structure.
  ```c
  RTE_BUS_FOREACH_DEV(dev, &rte_fslmc_bus) {
      rte_bus_remove_device(&rte_fslmc_bus, &dev->device);
      rte_intr_instance_free(dev->intr_handle);
      free(dev);  /* dev->device.name is leaked */
  }
  ```

**Warnings:**
- (dpaa_bus.c:337): The variable `dev` is set to NULL in the loop body but then reused as the loop iterator in the next iteration. This works because `RTE_BUS_FOREACH_DEV` is a macro that likely saves the next pointer, but the `dev = NULL` assignment is misleading and serves no purpose here.

---

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

**Errors:**
- **Error propagation bug** (dpaa_bus.c:778): `ret = -ret;` converts the error code returned by `dpaa_setup_intr()` to positive. But `dpaa_setup_intr()` already returns negative error codes (it returns 0 on success, -1 on failure from `open()` or `eventfd()`). The negation produces a positive error code, which will be misinterpreted by the caller. Remove the negation.
  ```c
  ret = dpaa_setup_intr(dpaa_dev->intr_handle);
  if (ret != 0) {
      DPAA_BUS_ERR("error setting up interrupt: %s", dpaa_dev->name);
      ret = -ret;  /* WRONG: dpaa_setup_intr already returns negative */
      goto release_intr;
  }
  ```

- **Error path does not execute cleanup on all errors** (dpaa_bus.c:814-822): The `dpaa_bus_cleanup()` function's loop over devices has `goto next` on several error conditions but only the last block (after `dev->device.driver = NULL;`) executes the cleanup (close intr, free intr_handle). If `drv->remove()` returns an error, the function does `goto next` without cleaning up the device's interrupt resources. This leaves the eventfd and intr_handle leaked on that device.
  ```c
  ret = drv->remove(dev);
  if (ret < 0) {
      rte_errno = errno;
      goto next;  /* BUG: skips dpaa_close_intr/rte_intr_instance_free */
  }
  dev->device.driver = NULL;
  next:
      dpaa_close_intr(dev->intr_handle);
      rte_intr_instance_free(dev->intr_handle);
  ```
  The `goto next` should either be removed (so cleanup happens even if remove fails) or the label should be moved above the cleanup code. The current code only cleans up devices where `drv->remove()` succeeded.

**Warnings:**
- (dpaa_bus.c:649): `dpaa_close_intr()` does not set the fd to -1 after closing it. If the same intr_handle is passed to this function twice, it will close the fd twice (double-close), which could close an unrelated fd if the first close's fd number was reused. Add `rte_intr_fd_set(intr_handle, -1);` after the close.

---

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

**Errors:**
- None identified. The refactoring correctly splits unplug from cleanup and the release notes update is appropriate.

**Info:**
- The patch moves cleanup of intr_handle into the `dpaa_bus_cleanup()` loop's `next:` label. Combined with Patch 02's error path bug, this means devices where `drv->remove()` fails will now clean up their interrupt resources on bus cleanup. However, the bug from Patch 02 (error path in probe not cleaning up on all failures) should still be fixed.

---

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

**Errors:**
- **Resource leak** (fslmc_bus.c:91): `fslmc_bus_remove_device()` frees the device structure but does NOT free `dev->device.name` (allocated via `strdup()` in `scan_one_fslmc_device()`). Every device has a name allocated with `strdup(dev_name)` but the remove function does not free it.
  ```c
  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);  /* dev->device.name is leaked */
  }
  ```
  Fix: Add `free((void *)dev->device.name);` before `free(dev)`.

---

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

**Errors:**
- None. The patch correctly moves the device count increment/decrement into `insert_in_device_list()` / `fslmc_bus_remove_device()` so that blocklisted devices do not inflate the count.

---

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

**Errors:**
- **Uninitialized variable use** (fslmc_bus.c:203): If the device name does not match any of the known prefixes in the `dev_types[]` array, the loop completes without setting `dev_id` or `dev_type`. Then `dev_id` is checked at line 203, but `dev_type` was never set to anything other than the local initializer `DPAA2_UNKNOWN`. The code then creates a device with `dev_type == DPAA2_UNKNOWN` and allocates the device structure. This leaks the device if the name was genuinely invalid (should have returned early). However, the subsequent check `if (dev_id == NULL)` catches this case and jumps to cleanup. So the code is correct but convoluted. Not an error, but the `dev_type` initialization to `DPAA2_UNKNOWN` is dead code since the loop always either sets `dev_type` or leaves `dev_id == NULL`.

**Info:**
- The refactoring removes the `strdup(dev_name)` / `strtok()` usage and uses pointer arithmetic instead. This is cleaner and avoids an allocation/free.

---

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

**Errors:**
- None identified. The logic for MPORTAL/DPIO filtering is moved from `fslmc_vfio_process_group()` into a new `fslmc_filter_control_devices()` helper called during scan. The split is clean and the device removal uses the correct helper.

**Warnings:**
- (fslmc_bus.c:311): `fslmc_filter_control_devices()` calls `rte_bus_device_is_ignored()` inside a loop over devices already on the bus. The bus device list is being modified (via `fslmc_bus_remove_device()`) while iterating over it. This is safe only if `RTE_BUS_FOREACH_DEV` saves the next pointer before entering the loop body. Verify that `RTE_BUS_FOREACH_DEV` is defined as `TAILQ_FOREACH_SAFE` or equivalent. If not, this is a use-after-free.

---

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

**Errors:**
- None identified. The patch moves device unplug logic from `fslmc_vfio_close_group()` into the bus layer's `rte_fslmc_close()` function. The call to `fslmc_vfio_dev_close()` is added to clean up VFIO resources.

**Warnings:**
- (fslmc_bus.c:535): The loop in `rte_fslmc_close()` checks `rte_dev_is_probed()` before calling `fslmc_bus_unplug_device()`, but then calls `fslmc_vfio_dev_close()` unconditionally on all matching device types. If a device was not probed (and thus did not have VFIO setup called), `fslmc_vfio_dev_close()` may attempt to clean up resources that were never allocated. Review `fslmc_vfio_dev_close()` in Patch 09 to ensure it handles this case.

---

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

**Errors:**
- **Missing error check propagation** (fslmc_vfio.c:1471): `fslmc_process_iodevices()` is called but its return value is not checked. If the device setup fails, `fslmc_vfio_dev_setup()` returns 0 anyway, silently ignoring the failure.
  ```c
  switch (dev->dev_type) {
  case DPAA2_ETH:
      /* No call to fslmc_process_iodevices here anymore */
      ret = rte_dpaa2_vfio_setup_intr(dev->intr_handle, dev_fd,
                      device_info.num_irqs);
      if (ret)
          return ret;
      break;  /* Missing: no device-specific init for CRYPTO/QDMA? */
  }
  ```
  Wait, re-reading the patch: the `switch` statement in `fslmc_vfio_dev_setup()` only has the `DPAA2_ETH` case for interrupt setup. There is no call to `fslmc_process_iodevices()` in this function. But the comment at line 1672 in `fslmc_vfio.c` says "ethdev, cryptodev, dmadev are handled at the bus level". This implies that `fslmc_process_iodevices()` was supposed to be called from `fslmc_vfio_dev_setup()` for those device types, but the code does not do so. Check if `fslmc_process_iodevices()` is needed for CRYPTO/QDMA. If so, this is a functional bug - VFIO setup incomplete.

Actually, looking more carefully: in the original code, `fslmc_vfio_process_group()` called `fslmc_process_iodevices()` for ETH/CRYPTO/QDMA. The new `fslmc_vfio_dev_setup()` only sets up interrupts for ETH but does not call `fslmc_process_iodevices()` at all. This is a **correctness bug** - the VFIO setup is incomplete for all three device types.

Fix: `fslmc_vfio_dev_setup()` should call `fslmc_process_iodevices(dev)` for ETH, CRYPTO, and QDMA before setting up interrupts (for ETH).

**Warnings:**
- (fslmc_vfio.c:1508): `fslmc_vfio_dev_close()` does not check if `dev->intr_handle` is NULL before calling `dpaa2_close_intr()`. If the device was never probed (and thus never had intr_handle allocated), this will segfault. Add a NULL check or ensure the caller (from Patch 08 warning) only calls this on probed devices.

---

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

**Errors:**
- **Resource leak on error path** (fslmc_bus.c:607): If `fslmc_vfio_dev_setup()` fails, the code does `goto release_intr` which frees the intr_handle. However, if `drv->probe()` fails, `fslmc_vfio_dev_close()` is called before freeing the intr_handle. If `fslmc_vfio_dev_close()` internally frees or closes the intr_handle (unclear from this patch alone), then `rte_intr_instance_free()` at line 617 would be a double-free. Verify that `fslmc_vfio_dev_close()` does NOT free the intr_handle, or remove the duplicate free.

Looking at Patch 09's `fslmc_vfio_dev_close()`, it calls `dpaa2_close_intr(dev->intr_handle)` which only closes the fd inside the handle but does not free the handle itself. So the free at line 617 is correct. However, the code should set `dev->intr_handle = NULL` after freeing it to make this explicit (it already does at line 618, which is good).

- **Uninitialized intr_handle in dpaa2_close_dpio_device** (dpaa2_hw_dpio.c:473): The function frees `dpio_dev->intr_handle`, but nowhere in the visible code is this field initialized for dpio_dev structures. If dpio devices were never probed (or their intr_handle was never allocated), this is a double-free or free of NULL. Add a NULL check before `rte_intr_instance_free()`.
  ```c
  TAILQ_REMOVE(&dpio_dev_list, dpio_dev, next);
  rte_intr_instance_free(dpio_dev->intr_handle);  /* May be NULL or uninitialized */
  rte_free(dpio_dev);
  ```

---

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

**Errors:**
- None identified. The patch correctly delegates to `rte_bus_generic_cleanup()` and adds a `free_device` callback.

**Info:**
- The `fslmc_free_device()` function added here should free `dev->device.name` before freeing the device structure (related to the leak identified in Patch 04).

---

## Cross-Patch Issues

- **Resource leak throughout series**: Multiple patches (01, 04, 11) introduce or perpetuate the leak of `dev->device.name`. Every device has its name allocated with `strdup()` but none of the cleanup/free_device functions free it. This is a systematic bug.

- **Incomplete VFIO setup in Patch 09**: The refactoring in Patch 09 removes the call to `fslmc_process_iodevices()` for ETH/CRYPTO/QDMA devices, which is required for correct device initialization. This will cause those devices to not work.

---

## Summary of Errors

1. **Patch 01**: Resource leak - `dev->device.name` not freed in cleanup (dpaa_bus.c, fslmc_bus.c)
2. **Patch 02**: Error code incorrectly negated (dpaa_bus.c:778); error path in cleanup does not free intr resources when `dr