Received-SPF: pass (google.com: domain of linux-kernel+bounces-200569-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 4 Jun 2024 15:08:46 +0300
From: Leon Romanovsky <leon@kernel.org>
To: Konstantin Taranov <kotaranov@linux.microsoft.com>
Cc: kotaranov@microsoft.com, weh@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 1/1] RDMA/mana_ib: process QP error events
Message-ID: <20240604120846.GQ3884@unreal>
References: <1717500963-1108-1-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: <1717500963-1108-1-git-send-email-kotaranov@linux.microsoft.com>

On Tue, Jun 04, 2024 at 04:36:03AM -0700, Konstantin Taranov wrote:
> From: Konstantin Taranov <kotaranov@microsoft.com>
> 
> Process QP fatal events from the error event queue.
> For that, find the QP, using QPN from the event, and then call its
> event_handler. To find the QPs, store created RC QPs in an xarray.
> 
> Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
> Reviewed-by: Wei Hu <weh@microsoft.com>
> ---
>  drivers/infiniband/hw/mana/device.c           |  3 ++
>  drivers/infiniband/hw/mana/main.c             | 37 ++++++++++++++++++-
>  drivers/infiniband/hw/mana/mana_ib.h          |  4 ++
>  drivers/infiniband/hw/mana/qp.c               | 11 ++++++
>  .../net/ethernet/microsoft/mana/gdma_main.c   |  1 +
>  include/net/mana/gdma.h                       |  1 +
>  6 files changed, 55 insertions(+), 2 deletions(-)

<...>

> +static void
> +mana_ib_event_handler(void *ctx, struct gdma_queue *q, struct gdma_event *event)
> +{
> +	struct mana_ib_dev *mdev = (struct mana_ib_dev *)ctx;
> +	struct mana_ib_qp *qp;
> +	struct ib_event ev;
> +	unsigned long flag;
> +	u32 qpn;
> +
> +	switch (event->type) {
> +	case GDMA_EQE_RNIC_QP_FATAL:
> +		qpn = event->details[0];
> +		xa_lock_irqsave(&mdev->qp_table_rq, flag);
> +		qp = xa_load(&mdev->qp_table_rq, qpn);
> +		if (qp)
> +			refcount_inc(&qp->refcount);
> +		xa_unlock_irqrestore(&mdev->qp_table_rq, flag);
> +		if (!qp)
> +			break;
> +		if (qp->ibqp.event_handler) {
> +			ev.device = qp->ibqp.device;
> +			ev.element.qp = &qp->ibqp;
> +			ev.event = IB_EVENT_QP_FATAL;
> +			qp->ibqp.event_handler(&ev, qp->ibqp.qp_context);
> +		}
> +		if (refcount_dec_and_test(&qp->refcount))
> +			complete(&qp->free);
> +		break;
> +	default:
> +		break;
> +	}
> +}

<...>

> @@ -620,6 +626,11 @@ static int mana_ib_destroy_rc_qp(struct mana_ib_qp *qp, struct ib_udata *udata)
>  		container_of(qp->ibqp.device, struct mana_ib_dev, ib_dev);
>  	int i;
>  
> +	xa_erase_irq(&mdev->qp_table_rq, qp->ibqp.qp_num);
> +	if (refcount_dec_and_test(&qp->refcount))
> +		complete(&qp->free);
> +	wait_for_completion(&qp->free);

This flow is unclear to me. You are destroying the QP and need to make
sure that mana_ib_event_handler is not running at that point and not
mess with refcount and complete.

Thanks