Re: [PATCH v3] RDMA/siw: publish QP after initialization

[Bernard Metzler <bernard.metzler@linux.dev>]

On 27.06.2026 16:40, Ruoyu Wang wrote:
> siw_create_qp() currently calls siw_qp_add() before the queues, CQ
> pointers, state, completion, and device list entry are ready. A QPN
> lookup can therefore reach a QP that is still being constructed.
> 
> Move siw_qp_add() to the end of siw_create_qp(), after QP
> initialization and before adding the QP to the siw device list.
> 
> Fixes: f29dd55b0236 ("rdma/siw: queue pair methods")
> Suggested-by: Bernard Metzler <bernard.metzler@linux.dev>
> Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
> ---
> Changes in v3:
> - Move siw_qp_add()/xa_alloc() to the end of siw_create_qp().
> - Drop the QPN reservation helper from v2.
> 
>   drivers/infiniband/sw/siw/siw_verbs.c | 45 +++++++++++++++------------
>   1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> index 1e1d262a4ae2..ee3e5529d6f4 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -316,6 +316,7 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
>   	struct siw_ucontext *uctx =
>   		rdma_udata_to_drv_context(udata, struct siw_ucontext,
>   					  base_ucontext);
> +	struct siw_uresp_create_qp uresp = {};
>   	unsigned long flags;
>   	int num_sqe, num_rqe, rv = 0;
>   	size_t length;
> @@ -369,11 +370,6 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
>   	spin_lock_init(&qp->rq_lock);
>   	spin_lock_init(&qp->orq_lock);
>   
> -	rv = siw_qp_add(sdev, qp);
> -	if (rv)
> -		goto err_atomic;
> -
> -
>   	/* All queue indices are derived from modulo operations
>   	 * on a free running 'get' (consumer) and 'put' (producer)
>   	 * unsigned counter. Having queue sizes at power of two
> @@ -391,14 +387,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
>   
>   	if (qp->sendq == NULL) {
>   		rv = -ENOMEM;
> -		goto err_out_xa;
> +		goto err_out;
>   	}
>   	if (attrs->sq_sig_type != IB_SIGNAL_REQ_WR) {
>   		if (attrs->sq_sig_type == IB_SIGNAL_ALL_WR)
>   			qp->attrs.flags |= SIW_SIGNAL_ALL_WR;
>   		else {
>   			rv = -EINVAL;
> -			goto err_out_xa;
> +			goto err_out;
>   		}
>   	}
>   	qp->pd = pd;
> @@ -424,7 +420,7 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
>   
>   		if (qp->recvq == NULL) {
>   			rv = -ENOMEM;
> -			goto err_out_xa;
> +			goto err_out;
>   		}
>   		qp->attrs.rq_size = num_rqe;
>   	}
> @@ -439,11 +435,8 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
>   	qp->attrs.state = SIW_QP_STATE_IDLE;
>   
>   	if (udata) {
> -		struct siw_uresp_create_qp uresp = {};
> -
>   		uresp.num_sqe = num_sqe;
>   		uresp.num_rqe = num_rqe;
> -		uresp.qp_id = qp_id(qp);
>   
>   		if (qp->sendq) {
>   			length = num_sqe * sizeof(struct siw_sqe);
> @@ -452,7 +445,7 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
>   						      length, &uresp.sq_key);
>   			if (!qp->sq_entry) {
>   				rv = -ENOMEM;
> -				goto err_out_xa;
> +				goto err_out;
>   			}
>   		}
>   
> @@ -464,34 +457,46 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
>   			if (!qp->rq_entry) {
>   				uresp.sq_key = SIW_INVAL_UOBJ_KEY;
>   				rv = -ENOMEM;
> -				goto err_out_xa;
> +				goto err_out;
>   			}
>   		}
>  

move below check as well into your new 'if (udata) {'
clause below, right before doing the
ib_copy_to_udata() thing. It logically belongs there.
>   		if (udata->outlen < sizeof(uresp)) {
>   			rv = -EINVAL;
> -			goto err_out_xa;
> +			goto err_out;
>   		}
> -		rv = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> -		if (rv)
> -			goto err_out_xa;
>   	}
>   	qp->tx_cpu = siw_get_tx_cpu(sdev);
>   	if (qp->tx_cpu < 0) {
>   		rv = -EINVAL;
> -		goto err_out_xa;
> +		goto err_out;
>   	}
>   	INIT_LIST_HEAD(&qp->devq);
Put above line close to the list_add_tail() below,
that's where it logically belongs to.


Looks good otherwise!

Thanks,
Bernard.
> +	init_completion(&qp->qp_free);
> +
> +	rv = siw_qp_add(sdev, qp);
> +	if (rv)
> +		goto err_out_tx;
> +
> +	if (udata) {
> +		uresp.qp_id = qp_id(qp);
> +
> +		rv = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> +		if (rv)
> +			goto err_out_xa;
> +	}
> +
>   	spin_lock_irqsave(&sdev->lock, flags);
>   	list_add_tail(&qp->devq, &sdev->qp_list);
>   	spin_unlock_irqrestore(&sdev->lock, flags);
>   
> -	init_completion(&qp->qp_free);
> -
>   	return 0;
>   
>   err_out_xa:
>   	xa_erase(&sdev->qp_xa, qp_id(qp));
> +err_out_tx:
> +	siw_put_tx_cpu(qp->tx_cpu);
> +err_out:
>   	if (uctx) {
>   		rdma_user_mmap_entry_remove(qp->sq_entry);
>   		rdma_user_mmap_entry_remove(qp->rq_entry);