Re: [PATCH V1 rdma-next 04/15] RDMA/umem: Reuse ib_umem_get_cq_buf_or_va() for VA-only CQ pinning

[Leon Romanovsky <leon@kernel.org>]

On Tue, Sep 15, 2026 at 05:09:22PM +0300, Yishai Hadas wrote:
> Several drivers pin their CQ ring buffer via ib_umem_get_va() rather
> than the CQ-specific helper. Switch them to ib_umem_get_cq_buf_or_va()
> so the subsequent DMA_FROM_DEVICE change covers all CQ paths at once.
> 
> For shared helpers used by both CQ and non-CQ callers (qedr, mana,
> erdma, hns) a new is_cq parameter selects the appropriate pinning
> function; non-CQ callers pass false.
> 
> vmw_pvrdma's CQ is excluded: it embeds a ring-state header that the
> driver CPU-writes after polling, making it genuinely bidirectional.
> 
> This is a pure refactor with no functional change.
> 
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/infiniband/hw/bnxt_re/ib_verbs.c      |  9 ++++----
>  drivers/infiniband/hw/erdma/erdma_verbs.c     | 17 +++++++++-----
>  drivers/infiniband/hw/hns/hns_roce_cq.c       |  2 +-
>  drivers/infiniband/hw/hns/hns_roce_device.h   |  2 +-
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c    |  2 +-
>  drivers/infiniband/hw/hns/hns_roce_mr.c       | 22 +++++++++++++-----
>  drivers/infiniband/hw/hns/hns_roce_qp.c       |  2 +-
>  drivers/infiniband/hw/hns/hns_roce_srq.c      |  4 ++--
>  .../infiniband/hw/ionic/ionic_controlpath.c   |  5 ++--
>  drivers/infiniband/hw/mana/cq.c               |  2 +-
>  drivers/infiniband/hw/mana/main.c             |  9 ++++++--
>  drivers/infiniband/hw/mana/mana_ib.h          |  2 +-
>  drivers/infiniband/hw/mana/qp.c               |  9 ++++----
>  drivers/infiniband/hw/mana/wq.c               |  3 ++-
>  drivers/infiniband/hw/mlx4/cq.c               | 14 ++++++-----
>  drivers/infiniband/hw/mlx5/cq.c               |  6 ++---
>  drivers/infiniband/hw/qedr/verbs.c            | 23 +++++++++++++------
>  17 files changed, 84 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index ef08d42f377e..7f9aa3620abb 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -3749,12 +3749,13 @@ int bnxt_re_resize_cq(struct ib_cq *ibcq, unsigned int cqe,
>  	if (rc)
>  		goto fail;
>  
> -	cq->resize_umem = ib_umem_get_va(&rdev->ibdev, req.cq_va,
> -					 entries * sizeof(struct cq_base),
> -					 IB_ACCESS_LOCAL_WRITE);
> +	cq->resize_umem = ib_umem_get_cq_buf_or_va(&rdev->ibdev, NULL,
> +						   req.cq_va,
> +						   entries * sizeof(struct cq_base),
> +						   IB_ACCESS_LOCAL_WRITE);


<...>

> index fca2553e47ad..3519b5044e8f 100644
> --- a/drivers/infiniband/hw/erdma/erdma_verbs.c
> +++ b/drivers/infiniband/hw/erdma/erdma_verbs.c
> @@ -828,11 +828,16 @@ static void erdma_destroy_mtt(struct erdma_dev *dev, struct erdma_mtt *mtt)
>  
>  static int get_mtt_entries(struct erdma_dev *dev, struct erdma_mem *mem,
>  			   u64 start, u64 len, int access, u64 virt,
> -			   unsigned long req_page_size, bool force_continuous)
> +			   unsigned long req_page_size, bool force_continuous,
> +			   bool is_cq)
>  {
>  	int ret = 0;
>  
> -	mem->umem = ib_umem_get_va(&dev->ibdev, start, len, access);
> +	if (is_cq)
> +		mem->umem = ib_umem_get_cq_buf_or_va(&dev->ibdev, NULL, start,
> +						     len, access);
> +	else
> +		mem->umem = ib_umem_get_va(&dev->ibdev, start, len, access);

Sorry, but this patch doesn't look right to me.

In bnxt_re_resize_cq(), you chose to move away from the ib_umem_get_va()
API, but here you still keep that API because most of its callers are not
CQs. Why is it so important to specify the direction for CQs while QPs
and MRs are left without it?

Thanks

>  	if (IS_ERR(mem->umem)) {
>  		ret = PTR_ERR(mem->umem);
>  		mem->umem = NULL;
> @@ -951,7 +956,7 @@ static int init_user_qp(struct erdma_qp *qp, struct erdma_ucontext *uctx,
>  
>  	ret = get_mtt_entries(qp->dev, &qp->user_qp.sq_mem, va,
>  			      qp->attrs.sq_size << SQEBB_SHIFT, 0, va,
> -			      (SZ_1M - SZ_4K), true);
> +			      (SZ_1M - SZ_4K), true, false);
>  	if (ret)
>  		return ret;
>  
> @@ -960,7 +965,7 @@ static int init_user_qp(struct erdma_qp *qp, struct erdma_ucontext *uctx,
>  
>  	ret = get_mtt_entries(qp->dev, &qp->user_qp.rq_mem, va + rq_offset,
>  			      qp->attrs.rq_size << RQE_SHIFT, 0, va + rq_offset,
> -			      (SZ_1M - SZ_4K), true);
> +			      (SZ_1M - SZ_4K), true, false);
>  	if (ret)
>  		goto put_sq_mtt;
>  
> @@ -1250,7 +1255,7 @@ struct ib_mr *erdma_reg_user_mr(struct ib_pd *ibpd, u64 start, u64 len,
>  		return ERR_PTR(-ENOMEM);
>  
>  	ret = get_mtt_entries(dev, &mr->mem, start, len, access, virt,
> -			      SZ_2G - SZ_4K, false);
> +			      SZ_2G - SZ_4K, false, false);
>  	if (ret)
>  		goto err_out_free;
>  
> @@ -1931,7 +1936,7 @@ static int erdma_init_user_cq(struct erdma_ucontext *ctx, struct erdma_cq *cq,
>  
>  	ret = get_mtt_entries(dev, &cq->user_cq.qbuf_mem, ureq->qbuf_va,
>  			      ureq->qbuf_len, IB_ACCESS_LOCAL_WRITE,
> -			      ureq->qbuf_va, SZ_64M - SZ_4K, true);
> +			      ureq->qbuf_va, SZ_64M - SZ_4K, true, true);
>  	if (ret)
>  		return ret;

Thanks