Claude review: RDMA/mlx5: get tph for p2p access when registering dma-buf mr

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This patch wires up the mlx5 RDMA driver as a TPH consumer, refactors `mlx5_st_alloc_index` to expose a `by_tag` variant, and fixes a memory leak.

**Memory leak fix is correct and important:**

```c
if (refcount_dec_and_test(&idx_data->usecount)) {
	xa_erase(&st->idx_xa, st_index);
+	kfree(idx_data);
	/* We leave PCI config space as was before, no mkey will refer to it */
}
```

The existing code erases from the xarray but never frees the `idx_data`. This is a real leak on every ST deallocation that drops the last reference. This fix should arguably be a standalone patch or at least called out more prominently in the commit message since it fixes a pre-existing bug independent of the TPH feature.

**`ret = 0` initialization changes:**

```c
-	int ret;
+	int ret = 0;
```

In `mlx5_st_create()` and `mlx5_st_alloc_index_by_tag()`, `ret` is initialized to 0. In `mlx5_st_create()`, this appears unnecessary as `ret` is always assigned before use. In `mlx5_st_alloc_index_by_tag()`, it's needed because the `direct_mode` early-return path doesn't set `ret` (it returns 0 directly), but the main path always assigns `ret`. So the initialization is harmless but slightly misleading.

**ST ownership tracking:**

```c
if (!dmah && st_index != MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX) {
	mr->dmabuf_st_index = st_index;
	mr->dmabuf_st_owned = 1;
}
```

The `!dmah` guard is correct — when DMAH supplies the ST index, the DMAH path owns the lifecycle. The ownership flag avoids double-free. Good.

**Error path cleanup:**

```c
if (IS_ERR(mr)) {
+	if (!dmah && st_index != MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX)
+		mlx5_st_dealloc_index(dev->mdev, st_index);
	ib_umem_release(&umem_dmabuf->umem);
	return ERR_CAST(mr);
}
```

Correct — if MR allocation fails, the ST index must be released since no MR will own it.

**FRMR pool reuse path:**

```c
if (mr->ibmr.frmr.pool && !mlx5_umr_revoke_mr_with_lock(mr)) {
+	mlx5_ib_mr_put_dmabuf_st(mr);
+
	if (!ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr))
		return 0;
}
```

The ST index is released after the mkey is revoked (firmware no longer references it) but before the MR goes back to the FRMR cache. This is the correct ordering. If `ib_frmr_pool_push` fails, the MR falls through to `destroy_mkey` and the later `mlx5_ib_mr_put_dmabuf_st(mr)` call — but since `dmabuf_st_owned` was already cleared by the first call, the second is a no-op. Correct.

**`mlx5_ib_mr_put_dmabuf_st` checks `mr->umem`:**

```c
static void mlx5_ib_mr_put_dmabuf_st(struct mlx5_ib_mr *mr)
{
	if (mr->umem && mr->dmabuf_st_owned) {
```

The `mr->umem` check guards against non-dma-buf MRs. Since `dmabuf_st_owned` is only set for dma-buf MRs that got a valid ST, the `mr->umem` check is slightly redundant but provides defense-in-depth. Fine.

**Direct callback invocation:**

```c
if (!dmabuf->ops->get_tph)
	return;
ret = dmabuf->ops->get_tph(dmabuf, &steering_tag, ph, st_width);
```

As noted in patch 2 review, calling through the ops table directly rather than a wrapper is functional but differs from other dma-buf callback patterns. The NULL check is present so it's safe.

**MODULE_IMPORT_NS("DMA_BUF") addition:**

The `mr.c` file adds `MODULE_IMPORT_NS("DMA_BUF")`. The `vfio_pci_dmabuf.c` file already had this. The `mr.c` file needs this because it now calls `dmabuf->ops->get_tph` (accessing dma-buf internals). This is correct if `get_tph` is exported in the DMA_BUF namespace, though since it's a direct callback invocation rather than an exported symbol, the necessity depends on whether other dma-buf symbols used by mr.c are in that namespace.

---
Generated by Claude Code Patch Reviewer