Re: [PATCH rdma-next v3 1/4] RDMA/mana_ib: Introduce helpers to create and destroy mana queues

[Leon Romanovsky]

On Tue, Mar 26, 2024 at 01:08:05PM -0700, Konstantin Taranov wrote:
> From: Konstantin Taranov <kotaranov@xxxxxxxxxxxxx>
> 
> Intoduce helpers to work with mana ib queues (struct mana_ib_queue).
> A queue always consists of umem, gdma_region, and id.
> A queue can become a WQ or a CQ.
> 
> Signed-off-by: Konstantin Taranov <kotaranov@xxxxxxxxxxxxx>
> Reviewed-by: Long Li <longli@xxxxxxxxxxxxx>
> ---
>  drivers/infiniband/hw/mana/main.c    | 43 ++++++++++++++++++++++++++++
>  drivers/infiniband/hw/mana/mana_ib.h | 10 +++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/mana/main.c b/drivers/infiniband/hw/mana/main.c
> index 71e33feee..4524c6b80 100644
> --- a/drivers/infiniband/hw/mana/main.c
> +++ b/drivers/infiniband/hw/mana/main.c
> @@ -237,6 +237,49 @@ void mana_ib_dealloc_ucontext(struct ib_ucontext *ibcontext)
>  		ibdev_dbg(ibdev, "Failed to destroy doorbell page %d\n", ret);
>  }
>  
> +int mana_ib_create_queue(struct mana_ib_dev *mdev, u64 addr, u32 size,
> +			 struct mana_ib_queue *queue)
> +{
> +	struct ib_umem *umem;
> +	int err;
> +
> +	queue->umem = NULL;
> +	queue->id = INVALID_QUEUE_ID;
> +	queue->gdma_region = GDMA_INVALID_DMA_REGION;
> +
> +	umem = ib_umem_get(&mdev->ib_dev, addr, size, IB_ACCESS_LOCAL_WRITE);
> +	if (IS_ERR(umem)) {
> +		err = PTR_ERR(umem);
> +		ibdev_dbg(&mdev->ib_dev, "Failed to get umem, %d\n", err);
> +		return err;
> +	}
> +
> +	err = mana_ib_create_zero_offset_dma_region(mdev, umem, &queue->gdma_region);
> +	if (err) {
> +		ibdev_dbg(&mdev->ib_dev, "Failed to create dma region, %d\n", err);
> +		goto free_umem;
> +	}
> +	queue->umem = umem;
> +
> +	ibdev_dbg(&mdev->ib_dev,
> +		  "create_dma_region ret %d gdma_region 0x%llx\n",
> +		  err, queue->gdma_region);

I see such prints in all code and patches, please remove them. We assume
that the code works and you don't need to print about success.

Also you are printing "err" too which is always 0 in this case.

Thanks

> +
> +	return 0;
> +free_umem:
> +	ib_umem_release(umem);
> +	return err;
> +}
> +
> +void mana_ib_destroy_queue(struct mana_ib_dev *mdev, struct mana_ib_queue *queue)
> +{
> +	/* Ignore return code as there is not much we can do about it.
> +	 * The error message is printed inside.
> +	 */
> +	mana_ib_gd_destroy_dma_region(mdev, queue->gdma_region);
> +	ib_umem_release(queue->umem);
> +}
> +
>  static int
>  mana_ib_gd_first_dma_region(struct mana_ib_dev *dev,
>  			    struct gdma_context *gc,
> diff --git a/drivers/infiniband/hw/mana/mana_ib.h b/drivers/infiniband/hw/mana/mana_ib.h
> index f83390eeb..859fd3bfc 100644
> --- a/drivers/infiniband/hw/mana/mana_ib.h
> +++ b/drivers/infiniband/hw/mana/mana_ib.h
> @@ -45,6 +45,12 @@ struct mana_ib_adapter_caps {
>  	u32 max_inline_data_size;
>  };
>  
> +struct mana_ib_queue {
> +	struct ib_umem *umem;
> +	u64 gdma_region;
> +	u64 id;
> +};
> +
>  struct mana_ib_dev {
>  	struct ib_device ib_dev;
>  	struct gdma_dev *gdma_dev;
> @@ -169,6 +175,10 @@ int mana_ib_create_dma_region(struct mana_ib_dev *dev, struct ib_umem *umem,
>  int mana_ib_gd_destroy_dma_region(struct mana_ib_dev *dev,
>  				  mana_handle_t gdma_region);
>  
> +int mana_ib_create_queue(struct mana_ib_dev *mdev, u64 addr, u32 size,
> +			 struct mana_ib_queue *queue);
> +void mana_ib_destroy_queue(struct mana_ib_dev *mdev, struct mana_ib_queue *queue);
> +
>  struct ib_wq *mana_ib_create_wq(struct ib_pd *pd,
>  				struct ib_wq_init_attr *init_attr,
>  				struct ib_udata *udata);
> -- 
> 2.43.0
> 
>