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

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

Patch Review

**The `mlx5_st_alloc_index` refactoring is clean.** The `pcie_tph_get_cpu_st()` call is properly lifted into the wrapper, and the new `mlx5_st_alloc_index_by_tag()` takes the raw tag directly:

```c
int mlx5_st_alloc_index_by_tag(struct mlx5_core_dev *dev, u16 tag,
			       u16 *st_index)
{
	...
	if (st->direct_mode) {
		*st_index = tag;
		return 0;
	}
	...
```

**The `ret = 0` initialization in `mlx5_st_alloc_index_by_tag` is necessary.** After the refactoring, the cached path (`xa_for_each` finding a match and jumping to `end:`) returns `ret` without having set it — previously `pcie_tph_get_cpu_st()` returning 0 served as the implicit initialization, but that call is now in the wrapper.

**The `ret = 0` initialization in `mlx5_st_create` is unnecessary but harmless.** In the current code, `ret` is assigned by `pcie_enable_tph()` (line 58) and only used after that point. The function never reaches a point where `ret` is used without being set. This is likely a compiler warning suppression for clang. Fine, but might be better as a separate patch since the commit message describes it as "Initialize ret in mlx5_st_create()" alongside unrelated functional changes.

**Issue: Bitfield sharing in `mlx5_ib_mr`.** The new `dmabuf_st_owned:1` is packed into the same byte as `revoked:1` and `dmabuf_faulted:1`:

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

Patch 1's commit message explicitly calls out converting `revoked` from a bitfield to a bool in `vfio_pci_dma_buf` because "`revoked` is written under `dma_resv_lock`; the new TPH fields are written under `memory_lock`." The same class of concern applies here: `revoked` is set under `dma_resv_lock`, `dmabuf_faulted` is set in the page fault path, and `dmabuf_st_owned` is set/cleared during registration/cleanup. If any pair is updated concurrently under different locks, the non-atomic RMW on the shared byte is a bug. This pre-exists (revoked + dmabuf_faulted already share a byte), but the patch should not make it worse without confirming the locking is safe.

**TOCTOU fix is good.** Using `umem_dmabuf->attach->dmabuf` (already resolved) instead of `dma_buf_get(fd)` eliminates a real race:

```c
} else {
	get_tph_mr_dmabuf(dev, umem_dmabuf->attach->dmabuf,
			  &st_index, &ph);
}
```

**ST index cleanup covers both paths.** The FRMR cache path and the destroy_mkey path both call `mlx5_ib_mr_put_dmabuf_st()`:

```c
// FRMR cache path (line ~1380):
if (mr->ibmr.frmr.pool && !mlx5_umr_revoke_mr_with_lock(mr) &&
    !ib_frmr_pool_push(mr->ibmr.device, &mr->ibmr)) {
	mlx5_ib_mr_put_dmabuf_st(mr);
	return 0;
}

// destroy_mkey path (line ~1468):
if (!ret)
	mlx5_ib_mr_put_dmabuf_st(mr);
```

This is correct — the ST index must be freed once the firmware mkey is revoked or destroyed.

**Error path cleanup in `reg_user_mr_dmabuf` is correct:**

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

**`pcie_tph_get_st_width` uses `EXPORT_SYMBOL`** (not GPL), consistent with the other exports in `drivers/pci/tph.c`. The stub for `!CONFIG_PCIE_TPH` correctly returns 0.

**Minor: `get_tph_mr_dmabuf` is a void function that silently falls through on failure.** This is intentional — the caller initializes `st_index` and `ph` to the no-TPH defaults, and the function leaves them unchanged on failure. The `mlx5_ib_dbg` calls provide observability. The pattern is fine, but the function's contract (modifies outputs only on success, leaves them at caller's defaults on failure) could be clearer in the comment, since the `*ph = MLX5_IB_NO_PH` reset in error paths suggests the function *does* touch the outputs on failure in some cases. The inconsistency between "leave as default" and "reset to default" is confusing — on `get_tph` callback failure, `*ph` is explicitly reset to `MLX5_IB_NO_PH`, but on `!get_tph` or `!st_width`, the values are left untouched. The effect is the same (the caller initialized `ph = MLX5_IB_NO_PH`), but it reads as if the function is inconsistent about its own contract.

---
Generated by Claude Code Patch Reviewer