From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: RDMA/mlx5: get tph for p2p access when registering dma-buf mr Date: Wed, 27 May 2026 14:35:50 +1000 Message-ID: In-Reply-To: <20260526144401.1485788-5-zhipingz@meta.com> References: <20260526144401.1485788-1-zhipingz@meta.com> <20260526144401.1485788-5-zhipingz@meta.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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`. Th= is is a real leak on every ST deallocation that drops the last reference. T= his fix should arguably be a standalone patch or at least called out more p= rominently in the commit message since it fixes a pre-existing bug independ= ent of the TPH feature. **`ret =3D 0` initialization changes:** ```c - int ret; + int ret =3D 0; ``` In `mlx5_st_create()` and `mlx5_st_alloc_index_by_tag()`, `ret` is initiali= zed to 0. In `mlx5_st_create()`, this appears unnecessary as `ret` is alway= s assigned before use. In `mlx5_st_alloc_index_by_tag()`, it's needed becau= se the `direct_mode` early-return path doesn't set `ret` (it returns 0 dire= ctly), but the main path always assigns `ret`. So the initialization is har= mless but slightly misleading. **ST ownership tracking:** ```c if (!dmah && st_index !=3D MLX5_MKC_PCIE_TPH_NO_STEERING_TAG_INDEX) { mr->dmabuf_st_index =3D st_index; mr->dmabuf_st_owned =3D 1; } ``` The `!dmah` guard is correct =E2=80=94 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 !=3D 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 =E2=80=94 if MR allocation fails, the ST index must be released sin= ce 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 refe= rences it) but before the MR goes back to the FRMR cache. This is the corre= ct 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 =E2=80=94 but sinc= e `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 =3D dmabuf->ops->get_tph(dmabuf, &steering_tag, ph, st_width); ``` As noted in patch 2 review, calling through the ops table directly rather t= han a wrapper is functional but differs from other dma-buf callback pattern= s. 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 `dm= abuf->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 callb= ack invocation rather than an exported symbol, the necessity depends on whe= ther other dma-buf symbols used by mr.c are in that namespace. --- Generated by Claude Code Patch Reviewer