RE: [PATCH v9 rdma-next 3/7] RDMA/efa: Use the common mmap_xa helpers

[Michal Kalderon <mkalderon@marvell.com>]

> From: Gal Pressman <galpress@amazon.com>
> Sent: Tuesday, September 3, 2019 6:07 PM
> 
> On 03/09/2019 12:31, Michal Kalderon wrote:
> >> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> >> owner@vger.kernel.org> On Behalf Of Gal Pressman
> >>
> >> On 02/09/2019 19:23, Michal Kalderon wrote:
> >>>  int efa_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)  {
> >>> +	struct efa_ucontext *ucontext =
> >> rdma_udata_to_drv_context(udata,
> >>> +		struct efa_ucontext, ibucontext);
> >>>  	struct efa_dev *dev = to_edev(ibqp->pd->device);
> >>>  	struct efa_qp *qp = to_eqp(ibqp);
> >>>  	int err;
> >>>
> >>>  	ibdev_dbg(&dev->ibdev, "Destroy qp[%u]\n", ibqp->qp_num);
> >>> +
> >>> +	efa_qp_user_mmap_entries_remove(ucontext, qp);
> >>> +
> >>>  	err = efa_destroy_qp_handle(dev, qp->qp_handle);
> >>>  	if (err)
> >>>  		return err;
> >>> @@ -509,57 +412,114 @@ int efa_destroy_qp(struct ib_qp *ibqp, struct
> >> ib_udata *udata)
> >>>  	return 0;
> >>>  }
> >>>
> >>>  void efa_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)  {
> >>> +	struct efa_ucontext *ucontext =
> >> rdma_udata_to_drv_context(udata,
> >>> +			struct efa_ucontext, ibucontext);
> >>> +
> >>>  	struct efa_dev *dev = to_edev(ibcq->device);
> >>>  	struct efa_cq *cq = to_ecq(ibcq);
> >>>
> >>> @@ -897,17 +862,28 @@ void efa_destroy_cq(struct ib_cq *ibcq, struct
> >> ib_udata *udata)
> >>>  	efa_destroy_cq_idx(dev, cq->cq_idx);
> >>>  	dma_unmap_single(&dev->pdev->dev, cq->dma_addr, cq->size,
> >>>  			 DMA_FROM_DEVICE);
> >>> +	rdma_user_mmap_entry_remove(&ucontext->ibucontext,
> >>> +				    cq->mmap_key);
> >>
> >> How come in destroy_qp we do entry removal first, but in destroy_cq
> >> it's last?
> >> Shouldn't it be the same?
> > Yes, you're right, it should be done after memory is unmapped, I'll
> > move it down In the destroy qp flow. Is this the only comment on this
> series ?
> 
> Other than that the patch looks good to me,
> Acked-by: Gal Pressman <galpress@amazon.com>
> 
> A few nits (feel free to ignore):
> * The rdma_user_mmap_entry is always referred to as rdma_entry except in
> efa_mmap_free declaration and to_emmap.
> * efa_qp_user_mmap_entries_remove isn't really in reverse insertion order
> but OK :).
> * In case of length mismatch in __efa_mmap two error messages are
> printed, consider keeping the "Couldn't mmap address ..." print in the if (not
> part of the goto label).
> 
Thanks, as I'm already sending another series I'll fix the nits 😊 

> Thanks for doing this!