Received-SPF: pass (google.com: domain of linux-kernel+bounces-51634-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 4 Feb 2024 14:25:52 +0200
From: Leon Romanovsky <leon@kernel.org>
To: Konstantin Taranov <kotaranov@linux.microsoft.com>
Cc: kotaranov@microsoft.com, sharmaajay@microsoft.com, longli@microsoft.com,
	jgg@ziepe.ca, linux-rdma@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH rdma-next v2 1/5] RDMA/mana_ib: Add EQ creation for rnic
 adapter
Message-ID: <20240204122552.GD5400@unreal>
References: <1706886397-16600-1-git-send-email-kotaranov@linux.microsoft.com>
 <1706886397-16600-2-git-send-email-kotaranov@linux.microsoft.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1706886397-16600-2-git-send-email-kotaranov@linux.microsoft.com>

On Fri, Feb 02, 2024 at 07:06:33AM -0800, Konstantin Taranov wrote:
> This patch introduces functions for RNIC creation
> and creates one EQ for RNIC creation.

Please invest more time in commit messages, it is obvious that this
patch "introduces functions for RNIC creation"" by looking in the code.

> 
> Signed-off-by: Konstantin Taranov <kotaranov@linux.microsoft.com>
> ---
>  drivers/infiniband/hw/mana/device.c  |  9 ++++--
>  drivers/infiniband/hw/mana/main.c    | 53 ++++++++++++++++++++++++++++++++++++
>  drivers/infiniband/hw/mana/mana_ib.h |  5 ++++
>  3 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mana/device.c b/drivers/infiniband/hw/mana/device.c
> index 6fa902e..d8e8b10 100644
> --- a/drivers/infiniband/hw/mana/device.c
> +++ b/drivers/infiniband/hw/mana/device.c
> @@ -92,15 +92,19 @@ static int mana_ib_probe(struct auxiliary_device *adev,
>  		goto deregister_device;
>  	}
>  
> +	mana_ib_gd_create_rnic_adapter(dev);
> +
>  	ret = ib_register_device(&dev->ib_dev, "mana_%d",
>  				 mdev->gdma_context->dev);
>  	if (ret)
> -		goto deregister_device;
> +		goto destroy_rnic_adapter;
>  
>  	dev_set_drvdata(&adev->dev, dev);
>  
>  	return 0;
>  
> +destroy_rnic_adapter:
> +	mana_ib_gd_destroy_rnic_adapter(dev);
>  deregister_device:
>  	mana_gd_deregister_device(dev->gdma_dev);
>  free_ib_device:
> @@ -113,9 +117,8 @@ static void mana_ib_remove(struct auxiliary_device *adev)
>  	struct mana_ib_dev *dev = dev_get_drvdata(&adev->dev);
>  
>  	ib_unregister_device(&dev->ib_dev);
> -
> +	mana_ib_gd_destroy_rnic_adapter(dev);
>  	mana_gd_deregister_device(dev->gdma_dev);
> -
>  	ib_dealloc_device(&dev->ib_dev);
>  }
>  
> diff --git a/drivers/infiniband/hw/mana/main.c b/drivers/infiniband/hw/mana/main.c
> index 29dd243..c64d569 100644
> --- a/drivers/infiniband/hw/mana/main.c
> +++ b/drivers/infiniband/hw/mana/main.c
> @@ -548,3 +548,56 @@ int mana_ib_gd_query_adapter_caps(struct mana_ib_dev *dev)
>  
>  	return 0;
>  }
> +
> +static int mana_ib_create_eqs(struct mana_ib_dev *mdev)
> +{
> +	struct gdma_context *gc = mdev_to_gc(mdev);
> +	struct gdma_queue_spec spec = {};
> +	int err;
> +
> +	spec.type = GDMA_EQ;
> +	spec.monitor_avl_buf = false;
> +	spec.queue_size = EQ_SIZE;
> +	spec.eq.callback = NULL;
> +	spec.eq.context = mdev;
> +	spec.eq.log2_throttle_limit = LOG2_EQ_THROTTLE;
> +	spec.eq.msix_index = 0;
> +
> +	err = mana_gd_create_mana_eq(&gc->mana_ib, &spec, &mdev->fatal_err_eq);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void mana_ib_destroy_eqs(struct mana_ib_dev *mdev)
> +{
> +	if (!mdev->fatal_err_eq)
> +		return;
> +
> +	mana_gd_destroy_queue(mdev_to_gc(mdev), mdev->fatal_err_eq);
> +	mdev->fatal_err_eq = NULL;

Please don't set NULL to the pointers if they are not used anymore.

> +}
> +
> +void mana_ib_gd_create_rnic_adapter(struct mana_ib_dev *mdev)
> +{
> +	int err;
> +
> +	err = mana_ib_create_eqs(mdev);
> +	if (err) {
> +		ibdev_err(&mdev->ib_dev, "Failed to create EQs for RNIC err %d", err);
> +		goto cleanup;
> +	}
> +
> +	return;
> +
> +cleanup:
> +	ibdev_warn(&mdev->ib_dev,
> +		   "RNIC is not available. Only RAW QPs are supported");
> +	mana_ib_destroy_eqs(mdev);

If mana_ib_create_eqs() fails, you shouldn't call to mana_ib_destroy_eqs().
mana_ib_create_eqs() needs to clean everything when it fails.

Thanks

> +}
> +
> +void mana_ib_gd_destroy_rnic_adapter(struct mana_ib_dev *mdev)
> +{
> +	mana_ib_destroy_eqs(mdev);
> +}
> diff --git a/drivers/infiniband/hw/mana/mana_ib.h b/drivers/infiniband/hw/mana/mana_ib.h
> index 6a03ae6..a4b94ee 100644
> --- a/drivers/infiniband/hw/mana/mana_ib.h
> +++ b/drivers/infiniband/hw/mana/mana_ib.h
> @@ -48,6 +48,7 @@ struct mana_ib_adapter_caps {
>  struct mana_ib_dev {
>  	struct ib_device ib_dev;
>  	struct gdma_dev *gdma_dev;
> +	struct gdma_queue *fatal_err_eq;
>  	struct mana_ib_adapter_caps adapter_caps;
>  };
>  
> @@ -228,4 +229,8 @@ int mana_ib_query_gid(struct ib_device *ibdev, u32 port, int index,
>  void mana_ib_disassociate_ucontext(struct ib_ucontext *ibcontext);
>  
>  int mana_ib_gd_query_adapter_caps(struct mana_ib_dev *mdev);
> +
> +void mana_ib_gd_create_rnic_adapter(struct mana_ib_dev *mdev);
> +
> +void mana_ib_gd_destroy_rnic_adapter(struct mana_ib_dev *mdev);
>  #endif
> -- 
> 1.8.3.1
>