|WARNING| [v10,1/1] net/mana: add device reset support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-16

# DPDK Patch Review

## Summary

This patch adds device reset support to the MANA PMD. The implementation includes a two-phase reset flow (enter/exit), atomic burst state management, multi-process IPC coordination, and recovery event notifications. Overall, the patch is well-structured, but there are several correctness issues and style concerns that must be addressed.

---

## Errors (Must Fix)

### 1. **Use-after-free in `mana_reset_exit_delay`**

**File:** `drivers/net/mana/mana.c`  
**Function:** `mana_reset_exit_delay`  
**Line:** ~1601

```c
ret = ibv_close_device(priv->ib_ctx);
if (ret) {
    DRV_LOG(ERR, "Failed to close ibv device %d", ret);
    rte_atomic_store_explicit(&priv->dev_state, MANA_DEV_RESET_FAILED, ...);
    goto out;
}
priv->ib_ctx = NULL;

ret = mana_pci_probe(NULL, pci_dev);  // re-probe opens a new ib_ctx
```

**Problem:** After `ibv_close_device(priv->ib_ctx)` succeeds, `priv->ib_ctx` is set to NULL. However, `mana_pci_probe` is called immediately after, which expects `priv->ib_ctx` to be valid (or at least re-assigns it). If `mana_pci_probe` fails and the error path attempts to use `priv->ib_ctx` (or if any code between close and probe touches it), this is undefined behavior. The existing code sets `priv->ib_ctx = NULL` after close, which is correct, but the comment and flow suggest the intention is to re-probe, which will overwrite it. The issue is that if `mana_pci_probe` fails partway through, `priv->ib_ctx` may be left in an inconsistent state.

**Why it matters:** The `ib_ctx` pointer is used throughout the driver. A stale or NULL pointer after a failed re-probe can cause crashes or silent corruption.

**Suggested fix:**  
Verify that `mana_pci_probe` always either fully succeeds (setting `priv->ib_ctx` to a valid new context) or fully fails (leaving `priv->ib_ctx` as NULL and cleaning up). Document this invariant. Add a NULL check before any later use of `priv->ib_ctx` in error paths after `mana_pci_probe` fails.

---

### 2. **Resource leak: `priv` allocated but not freed on probe failure paths**

**File:** `drivers/net/mana/mana.c`  
**Function:** `mana_probe_port`  
**Lines:** ~1300-1350

The patch changes `mana_probe_port` to support reset (re-probe). When `is_reset == false` (new probe), `priv` is allocated with `rte_zmalloc_socket`. On the `failed:` label:

```c
if (!is_reset) {
    if (eth_dev)
        rte_eth_dev_release_port(eth_dev);
    rte_free(priv);
}
```

However, several error paths between `priv` allocation and the `failed:` label may jump to `failed:` with `is_reset` still false but without having allocated `eth_dev`. For example:

- If `ibv_alloc_pd(ctx)` fails, `goto failed` is called with `priv` allocated but `eth_dev` still NULL.
- The code frees `priv` only if `!is_reset`, but `eth_dev` may or may not have been allocated.

**Problem:** If `eth_dev` allocation or any step between `priv` allocation and `eth_dev` allocation fails, `priv` is leaked when `is_reset == true` (because the `if (!is_reset)` block is skipped).

Wait, re-reading: when `is_reset == true`, `priv` is **not** allocated -- it's retrieved from `eth_dev->data->dev_private`. So the leak concern is only when `is_reset == false`. The code does `rte_free(priv)` in that case. But let me check the order:

```c
if (!is_reset) {
    eth_dev = rte_eth_dev_allocate(name);
    if (!eth_dev) {
        ret = -ENOMEM;
        goto failed;
    }
    // ...
}
```

If `rte_eth_dev_allocate` fails, we goto `failed:` with `eth_dev == NULL`. The `failed:` label does:

```c
if (!is_reset) {
    if (eth_dev)
        rte_eth_dev_release_port(eth_dev);
    rte_free(priv);
}
```

So `priv` **is** freed. But there's a problem earlier: before the `if (!is_reset)` block that allocates `eth_dev`, there are these lines:

```c
priv->ib_ctx = ctx;
priv->max_rx_queues = dev_attr->orig_attr.max_qp;
// ...
```

These assignments happen **before** the `if (!is_reset)` block. So when `is_reset == false`, `priv` is allocated, then these fields are set, then `eth_dev` is allocated. If `eth_dev` allocation fails, we goto `failed:` and `priv` is freed -- OK so far.

But wait: when `is_reset == true`, `priv` is taken from `eth_dev->data->dev_private` (which must already exist). The code does:

```c
eth_dev = rte_eth_dev_allocated(name);
if (eth_dev) {
    is_reset = true;
    priv = eth_dev->data->dev_private;
}
```

Then later:

```c
priv->ib_ctx = ctx;
// ...
```

If one of these operations fails (e.g., `ibv_alloc_pd` fails), we goto `failed:`. The `failed:` label has:

```c
if (!is_reset) {
    // free priv
}
```

So when `is_reset == true`, `priv` is **not** freed. This is correct because `priv` was not allocated by this function -- it already existed in `eth_dev->data->dev_private`. But we do need to clean up `ib_pd` and `ib_parent_pd` if they were allocated. The code does this:

```c
failed:
    if (priv) {
        if (priv->ib_parent_pd) {
            ibv_dealloc_pd(priv->ib_parent_pd);
            priv->ib_parent_pd = NULL;
        }
        if (priv->ib_pd) {
            ibv_dealloc_pd(priv->ib_pd);
            priv->ib_pd = NULL;
        }
    }
    if (!is_reset) {
        if (eth_dev)
            rte_eth_dev_release_port(eth_dev);
        rte_free(priv);
    }
    if (ctx) {
        ibv_close_device(ctx);
        if (is_reset && priv)
            priv->ib_ctx = NULL;
    }
```

So PDs are freed in both cases. Then:

- If `!is_reset`: free `eth_dev` and `priv`.
- If `is_reset`: do not free `priv` (it's reused), but close `ctx` and NULL out `priv->ib_ctx`.

This looks **mostly** correct, but there's a subtle issue: when `is_reset == true` and we fail, we close `ctx` and set `priv->ib_ctx = NULL`. But `priv` itself remains in `eth_dev->data->dev_private`, and `eth_dev` is not released. This means the device is left in a half-initialized state. The reset thread (which called `mana_pci_probe` via `mana_reset_exit_delay`) will later check the return value of `mana_pci_probe` and set `dev_state` to `MANA_DEV_RESET_FAILED`. But the application may still have a handle to `eth_dev` with `priv->ib_ctx == NULL`. If any code path tries to use `priv->ib_ctx` after this, it will crash.

**Why it matters:** A failed re-probe leaves the device in a broken state where `priv->ib_ctx` is NULL but the device is not fully torn down. Any subsequent operation that assumes `ib_ctx` is valid will crash.

**Suggested fix:**  
In `mana_reset_exit_delay`, after `mana_pci_probe` fails, ensure `dev_state` is set to `MANA_DEV_RESET_FAILED` (already done) and document that the device is no longer usable. Alternatively, consider fully tearing down `eth_dev` on re-probe failure (release port, free `priv`, etc.) so the device is removed from the ethdev layer entirely.

**Actually, re-checking the code flow:**

In `mana_reset_exit_delay`, after `mana_pci_probe(NULL, pci_dev)` fails, the code does:

```c
if (ret) {
    DRV_LOG(ERR, "Failed to probe mana pci dev ret %d", ret);
    rte_atomic_store_explicit(&priv->dev_state, MANA_DEV_RESET_FAILED, ...);
    goto out;
}
```

Then at `out:`, it sends either `RECOVERY_SUCCESS` or `RECOVERY_FAILED` based on `ret`. So the device is marked as `RESET_FAILED` and the application is notified. The problem is that `priv` and `eth_dev` are **not** freed or released. The device remains in the ethdev list with `dev_state == RESET_FAILED` and `ib_ctx == NULL`. Any subsequent ethdev operation on this port will fail (the lock wrappers check `dev_state` and return -EBUSY if not ACTIVE). But the device is still allocated.

Is this a **leak**? Not in the strictest sense -- `priv` and `eth_dev` are still reachable via `rte_eth_devices[port_id]`. But the device is unusable. The application is expected to close the device after receiving `RECOVERY_FAILED`, which will call `mana_dev_close_lock`, which will call `mana_dev_close`, which will free MR caches and deallocate PDs (if they exist). Then `mana_pci_remove` (or `mana_dev_uninit`) will free `priv` and release `eth_dev`.

So the lifecycle is: re-probe fails - `RESET_FAILED` - app closes device - `mana_dev_close` - `mana_pci_remove` - cleanup. This is acceptable **if** the app is cooperative. But if the app does not close the device, it's a leak.

**Conclusion:** This is **not** a correctness bug per se, but it's a resource ownership ambiguity. The patch assumes the application will close the device after `RECOVERY_FAILED`. This should be documented. However, there's a more serious issue: **what happens if the app calls `dev_stop` or `dev_close` while the reset thread is still running?** Let's check:

`mana_dev_stop_lock` and `mana_dev_close_lock` both call `mana_join_reset_thread(priv)` **before** acquiring `reset_ops_lock`. This joins the reset thread and transitions `dev_state` to `ACTIVE`. So if the app calls `dev_close` after `RECOVERY_FAILED`, the reset thread will have already exited (it sets `reset_thread_active = true` at the start and the caller joins it). The join succeeds, state transitions to ACTIVE, and then `mana_dev_close` runs normally. So the lifecycle is safe.

**Revised assessment:** This is **not** a leak or use-after-free. The design is correct: failed re-probe marks the device as `RESET_FAILED`, the app receives the event, and it's expected to close the device. The `mana_dev_close_lock` will join the reset thread and clean up. However, the code in `mana_probe_port` should ensure that when `is_reset == true` and re-probe fails, `priv->ib_ctx` is NULLed (already done) and the device is in a safe state (already done via `RESET_FAILED`). No error here after all.

Actually, wait. Let me re-read the `failed:` label in `mana_probe_port`:

```c
failed:
    if (priv) {
        if (priv->ib_parent_pd) {
            ibv_dealloc_pd(priv->ib_parent_pd);
            priv->ib_parent_pd = NULL;
        }
        if (priv->ib_pd) {
            ibv_dealloc_pd(priv->ib_pd);
            priv->ib_pd = NULL;
        }
    }
    if (!is_reset) {
        if (eth_dev)
            rte_eth_dev_release_port(eth_dev);
        rte_free(priv);
    }
    if (ctx) {
        ibv_close_device(ctx);
        if (is_reset && priv)
            priv->ib_ctx = NULL;
    }
    return ret;
```

When `is_reset == true`:

1. Deallocate PDs (if allocated).
2. Close `ctx` and NULL out `priv->ib_ctx`.
3. Do **not** free `priv` or release `eth_dev`.

This is correct. The function returns an error, and the caller (`mana_reset_exit_delay`) will set `dev_state = RESET_FAILED` and notify the app. No leak.

**Final verdict on item 2:** **Not an error.** The resource management is correct.

---

### 3. **Double-free risk in `mana_dev_close` on reset failure**

**File:** `drivers/net/mana/mana.c`  
**Function:** `mana_dev_close`  
**Lines:** ~287-320

```c
state = rte_atomic_load_explicit(&priv->dev_state, rte_memory_order_acquire);
if (state == MANA_DEV_ACTIVE || state == MANA_DEV_RESET_FAILED) {
    ret = mana_intr_uninstall(priv);
    // ...
}
if (priv->ib_parent_pd) {
    ret = ibv_dealloc_pd(priv->ib_parent_pd);
    // ...
    priv->ib_parent_pd = NULL;
}
if (priv->ib_pd) {
    ret = ibv_dealloc_pd(priv->ib_pd);
    // ...
    priv->ib_pd = NULL;
}
state = rte_atomic_load_explicit(&priv->dev_state, rte_memory_order_acquire);
if (state == MANA_DEV_ACTIVE || state == MANA_DEV_RESET_FAILED) {
    if (priv->ib_ctx) {
        ret = ibv_close_device(priv->ib_ctx);
        // ...
        priv->ib_ctx = NULL;
    }
}
```

**Problem:** If the reset thread fails partway through teardown in `mana_reset_thread` (e.g., `mana_dev_stop` fails), it sets `dev_state = RESET_FAILED` and releases `reset_ops_lock`. Later, the app calls `mana_dev_close_lock`, which joins the reset thread, acquires the lock, checks `dev_state` (now `RESET_FAILED`), and calls `mana_dev_close`. At this point:

- The reset thread may have already called `ibv_dealloc_pd(priv->ib_pd)` in its own `mana_dev_close` call (inside `mana_reset_thread`).
- Now `mana_dev_close` is called again from `mana_dev_close_lock`.
- If `priv->ib_pd` is not NULL, `ibv_dealloc_pd` is called again - **double-free**.

Wait, let me trace