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

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

Patch Review

**kfree leak fix (st.c)**

```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 ... */
}
```

Confirmed — the current tree at `st.c:176-179` has `xa_erase` without `kfree`, leaking `idx_data`. This is a real pre-existing bug fix. Should arguably be a separate patch with its own Fixes: tag since it affects existing users of `mlx5_st_dealloc_index`, not just the new dma-buf path.

**Refactoring mlx5_st_alloc_index (st.c)**

The split into `mlx5_st_alloc_index_by_tag` + a thin wrapper `mlx5_st_alloc_index` is clean. The `int ret = 0` initialization silences the clang warning for the `direct_mode` early-return path where `ret` was previously uninitialized. The export is `_GPL`, matching existing conventions.

**get_tph_mr_dmabuf (mr.c)**

```c
req_type = pcie_tph_enabled_req_type(dev->mdev->pdev);
switch (req_type) {
case PCI_TPH_REQ_TPH_ONLY:
	st_width = 8;
	break;
case PCI_TPH_REQ_EXT_TPH:
	st_width = 16;
	break;
default:
	return;
}
```

This correctly maps the enabled requester type to the ST width for the callback.

**Concern**: After `get_tph` fails, the code does:

```c
if (ret) {
	mlx5_ib_dbg(dev, "get_tph failed (%d)\n", ret);
	*ph = MLX5_IB_NO_PH;
	return;
}
```

This resets `*ph` to `MLX5_IB_NO_PH` but `*st_index` remains `MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX` from the caller's initialization, so the caller won't attempt deallocation. Correct.

**ST index lifecycle (mr.c)**

The `dmabuf_st_owned` flag and `mlx5_ib_mr_put_dmabuf_st` cleanup function correctly handle ownership:

1. On `alloc_cacheable_mr` failure, immediate dealloc before returning error.
2. On success, ownership transferred to MR via `dmabuf_st_owned = 1`.
3. In `mlx5r_handle_mkey_cleanup`, the ST is released after the mkey is revoked but before the MR re-enters the FRMR pool — this ordering is critical and correct.
4. In the non-FRMR cleanup path, `mlx5_ib_mr_put_dmabuf_st` is called after successful mkey cleanup.

```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;
}
```

This ensures the ST index is freed before an MR gets recycled via the FRMR pool, preventing a reused MR from referencing a stale firmware ST entry. Good fix.

**MODULE_IMPORT_NS("DMA_BUF")**

```c
MODULE_IMPORT_NS("DMA_BUF");
```

Required for using dma_buf symbols. Correct.

**Bitfield placement (mlx5_ib.h)**

```c
u8 revoked :1;
u8 dmabuf_faulted:1;
+u8 dmabuf_st_owned:1;
+u16 dmabuf_st_index;
```

Adding these fields inside the anonymous struct within the union is fine, but the `u16 dmabuf_st_index` after three 1-bit `u8` bitfields may introduce padding depending on compiler/ABI. This should be checked — the anonymous struct contains other fields too (`null_mmkey`), so it likely has room, but it's worth verifying that the union size doesn't unexpectedly grow. Not a blocking issue.

**Summary of concerns:**

1. **(Minor)** The `kfree(idx_data)` leak fix in `st.c` is a pre-existing bug — consider splitting it into its own patch with a `Fixes:` tag so it can be backported independently.
2. **(Minor)** The `revoked` type change from bitfield to `bool` in patch 1 is unrelated to TPH and should be mentioned in the commit message.
3. **(Observation)** No `PROBE`/`GET` support for `VFIO_DEVICE_FEATURE_DMA_BUF_TPH` — userspace can't discover whether the feature is available. The `vfio_check_feature` call only allows `SET`. This may be intentional (probe via attempting SET, or probe via the existing DMA_BUF feature), but is worth documenting in the uAPI comment.

---
Generated by Claude Code Patch Reviewer