Re: [PATCH rdma-next v9 2/9] RDMA/mana: Create and destroy kernel RC QPs

[Konstantin Taranov]

> On Thu, Nov 05, 2026 at 09:59:21AM -0800, Konstantin Taranov wrote:
> > From: Konstantin Taranov <kotaranov@xxxxxxxxxxxxx>
> >
> > Implement kernel RC QP creation and destruction. This feature requires
> > extended-WQE and power-of-two-SQ support from the HW.
> >
> > Signed-off-by: Konstantin Taranov <kotaranov@xxxxxxxxxxxxx>
> > ---
> > v5:
> > - Validate kernel QP queue sizes before allocating queues.
> > - Bound SGE counts and enforce the TX/RX WQE size limits.
> > v4:
> > - Reject RNIC firmware send-SGE counts below 2.
> > - Subtract the reserved SGE for extra-large OOB only for RNIC mana_ib.
> >
> >  drivers/infiniband/hw/mana/main.c    |  10 +-
> >  drivers/infiniband/hw/mana/mana_ib.h |   6 +
> >  drivers/infiniband/hw/mana/qp.c      | 206 +++++++++++++++++++++++++--
> >  3 files changed, 211 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mana/main.c
> b/drivers/infiniband/hw/mana/main.c
> > index ceb4ec58023a..f54905764f59 100644
> > --- a/drivers/infiniband/hw/mana/main.c
> > +++ b/drivers/infiniband/hw/mana/main.c
> > @@ -733,6 +733,13 @@ int mana_ib_gd_query_adapter_caps(struct
> mana_ib_dev *dev)
> >  	if (err)
> >  		return err;
> >
> > +	/* Hardware supports at least one data SGE plus one reserved for extra-
> large OOB. */
> > +	if (resp.max_send_sge_count < 2) {
> > +		ibdev_err(&dev->ib_dev, "Invalid max_send_sge_count %u,
> expected at least 2\n",
> > +			  resp.max_send_sge_count);
> > +		return -EINVAL;
> > +	}
> 
> This check does not make sense here. If this firmware is present, continue to
> support it; do not fail just because the kernel has been upgraded. If it is
> not present, why check for it?

It was Sashiko comment at some point that I should add this protection as
a faulty HW could respond with sge cap to be 0 or 1. I can remove this check as 
it is useless, but it is true that a faulty HW could return 0 or 1 and it may result
in a bug. I am happy to remove, but I also think that AI had a point.

Leon, should I remote it?

> 
> > +
> >  	caps->max_sq_id = resp.max_sq_id;
> >  	caps->max_rq_id = resp.max_rq_id;
> >  	caps->max_cq_id = resp.max_cq_id;
> > @@ -748,7 +755,8 @@ int mana_ib_gd_query_adapter_caps(struct
> mana_ib_dev *dev)
> >  				resp.max_requester_sq_size /
> GDMA_MAX_SQE_SIZE,
> >  				resp.max_requester_rq_size /
> GDMA_MAX_RQE_SIZE);
> >  	caps->max_inline_data_size = resp.max_inline_data_size;
> > -	caps->max_send_sge_count = resp.max_send_sge_count;
> > +	/* Account for the SGE reserved for extra-large OOB. */
> > +	caps->max_send_sge_count = resp.max_send_sge_count - 1;
> >  	caps->max_recv_sge_count = resp.max_recv_sge_count;
> >  	caps->feature_flags = resp.feature_flags;
> >
> > diff --git a/drivers/infiniband/hw/mana/mana_ib.h
> b/drivers/infiniband/hw/mana/mana_ib.h
> > index 5ca6e2acf974..6f5df64e838a 100644
> > --- a/drivers/infiniband/hw/mana/mana_ib.h
> > +++ b/drivers/infiniband/hw/mana/mana_ib.h
> > @@ -267,6 +267,10 @@ struct mana_ib_qp {
> >  	u32 sq_psn;
> >  	bool sq_sig_all;
> >
> > +	/* Serializes receive WR posting. */
> > +	spinlock_t rq_lock;
> > +	/* Serializes send and memory-management WR posting. */
> > +	spinlock_t sq_lock;
> >  	/* Serializes QP modification and error-list transitions. */
> >  	struct mutex modify_lock;
> >
> > @@ -274,6 +278,7 @@ struct mana_ib_qp {
> >  	struct list_head recv_err_node;
> >  	struct shadow_queue shadow_rq;
> >  	struct shadow_queue shadow_sq;
> > +	struct shadow_queue shadow_mmq;
> >
> >  	refcount_t		refcount;
> >  	struct completion	free;
> > @@ -315,6 +320,7 @@ enum mana_ib_adapter_features {
> >  	MANA_IB_FEATURE_DEV_COUNTERS_SUPPORT = BIT(5),
> >  	MANA_IB_FEATURE_MULTI_PORTS_SUPPORT = BIT(6),
> >  	MANA_IB_FEATURE_MSN_IN_WQE_SUPPORT = BIT(7),
> > +	MANA_IB_FEATURE_EXTENDED_WQE_FORMAT = BIT(9),
> >  	MANA_IB_FEATURE_RC_QP_SQ_POW2_SUPPORT = BIT(14),
> >  	MANA_IB_FEATURE_MULTI_PORT_GSI_SUPPORT = BIT(15),
> >  };
> > diff --git a/drivers/infiniband/hw/mana/qp.c
> b/drivers/infiniband/hw/mana/qp.c
> > index ae2ebbcc759a..387586c45aa0 100644
> > --- a/drivers/infiniband/hw/mana/qp.c
> > +++ b/drivers/infiniband/hw/mana/qp.c
> > @@ -427,27 +427,88 @@ static u32 mana_ib_fixed_wqe_size(u32 sge, u32
> oob_size)
> >  	return roundup_pow_of_two(mana_ib_wqe_size(sge, oob_size));
> >  }
> >
> > -static u32 mana_ib_queue_size(struct ib_qp_init_attr *attr, u32 queue_type)
> > +static u32 mana_ib_queue_size(struct mana_ib_dev *mdev, struct
> ib_qp_init_attr *attr,
> > +			      u32 queue_type)
> >  {
> > -	u32 queue_size;
> > +	u32 send_wr = max(attr->cap.max_send_wr, 1U);
> > +	u32 recv_wr = max(attr->cap.max_recv_wr, 1U);
> > +	u32 max_wqe_size = MAX_TX_WQE_SIZE;
> > +	u32 wr, wqe_size, read_sge;
> > +	u64 queue_size;
> > +
> > +	if (attr->cap.max_send_sge > MAX_TX_WQE_SIZE / sizeof(struct
> gdma_sge) ||
> > +	    attr->cap.max_recv_sge > MAX_RX_WQE_SIZE / sizeof(struct
> gdma_sge))
> > +		return 0;
> >
> >  	switch (attr->qp_type) {
> >  	case IB_QPT_UD:
> >  	case IB_QPT_GSI:
> > -		if (queue_type == MANA_UD_SEND_QUEUE)
> > -			queue_size = attr->cap.max_send_wr *
> > -				mana_ib_wqe_size(attr->cap.max_send_sge,
> INLINE_OOB_LARGE_SIZE);
> > -		else
> > -			queue_size = attr->cap.max_recv_wr *
> > -				mana_ib_wqe_size(attr->cap.max_recv_sge,
> INLINE_OOB_SMALL_SIZE);
> > +		if (queue_type == MANA_UD_SEND_QUEUE) {
> > +			wr = send_wr;
> > +			wqe_size = mana_ib_wqe_size(attr-
> >cap.max_send_sge, INLINE_OOB_LARGE_SIZE);
> > +		} else {
> > +			wr = recv_wr;
> > +			max_wqe_size = MAX_RX_WQE_SIZE;
> > +			wqe_size = mana_ib_wqe_size(attr->cap.max_recv_sge,
> INLINE_OOB_SMALL_SIZE);
> > +		}
> > +		break;
> > +	case IB_QPT_RC:
> > +		switch (queue_type) {
> > +		case MANA_RC_SEND_QUEUE_REQUESTER:
> > +			wr = send_wr;
> > +			wqe_size = mana_ib_fixed_wqe_size(attr-
> >cap.max_send_sge,
> > +
> INLINE_OOB_EXTRA_LARGE_SIZE);
> > +			break;
> > +		case MANA_RC_SEND_QUEUE_MMQ:
> > +			wr = send_wr;
> > +			wqe_size = mana_ib_wqe_size(1U,
> INLINE_OOB_EXTRA_LARGE_SIZE);
> > +			break;
> > +		case MANA_RC_SEND_QUEUE_RESPONDER:
> > +			return MANA_PAGE_SIZE;
> > +		case MANA_RC_RECV_QUEUE_REQUESTER:
> > +			wr = send_wr;
> > +			max_wqe_size = MAX_RX_WQE_SIZE;
> > +			read_sge = min(attr->cap.max_send_sge, mdev-
> >ib_dev.attrs.max_sge_rd);
> > +			wqe_size = mana_ib_wqe_size(read_sge,
> INLINE_OOB_SMALL_SIZE);
> > +			break;
> > +		case MANA_RC_RECV_QUEUE_RESPONDER:
> > +			wr = recv_wr;
> > +			max_wqe_size = MAX_RX_WQE_SIZE;
> > +			wqe_size = mana_ib_wqe_size(attr->cap.max_recv_sge,
> INLINE_OOB_SMALL_SIZE);
> > +			break;
> > +		default:
> > +			return 0;
> > +		}
> >  		break;
> >  	default:
> >  		return 0;
> >  	}
> >
> > +	if (!wqe_size || wqe_size > max_wqe_size)
> > +		return 0;
> > +
> > +	queue_size = (u64)wr * wqe_size;
> > +	if (queue_size > U32_MAX / 2 + 1)
> > +		return 0;
> > +
> >  	return MANA_PAGE_ALIGN(roundup_pow_of_two(queue_size));
> >  }
> >
> > +static int mana_ib_check_kernel_qp_sizes(struct mana_ib_dev *mdev,
> > +					 struct ib_qp_init_attr *attr, u32
> num_queues)
> > +{
> > +	u32 i;
> > +
> > +	for (i = 0; i < num_queues; i++) {
> > +		if (!mana_ib_queue_size(mdev, attr, i)) {
> > +			ibdev_dbg(&mdev->ib_dev, "Invalid kernel QP queue
> %u size\n", i);
> 
> If your code needs to validate in-kernel users, something is fundamentally
> wrong
> with your design. We trust them.

It was Sashiko comment at some point. I also agree that it is partially redundant, but it also
helps to detect u32 overflow. I think we should keep as we had a similar comment
for CQ u32 overflow, and Sashiko rated it as High. I think better safe than sorry

> 
> Thanks