> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@xxxxxxxxx>
> Sent: Thursday, October 26, 2023 8:55 AM
> To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>; jgg@xxxxxxxx; leon@xxxxxxxxxx
> Cc: linux-rdma@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH V2 18/20] RDMA/siw: Only check attrs-
> >cap.max_send_wr in siw_create_qp
>
>
>
> On 10/25/23 21:27, Bernard Metzler wrote:
> >> -----Original Message-----
> >> From: Guoqing Jiang<guoqing.jiang@xxxxxxxxx>
> >> Sent: Friday, October 13, 2023 4:01 AM
> >> To: Bernard Metzler<BMT@xxxxxxxxxxxxxx>;jgg@xxxxxxxx;leon@xxxxxxxxxx
> >> Cc:linux-rdma@xxxxxxxxxxxxxxx
> >> Subject: [EXTERNAL] [PATCH V2 18/20] RDMA/siw: Only check attrs-
> >>> cap.max_send_wr in siw_create_qp
> >> We can just check max_send_wr here given both max_send_wr and
> >> max_recv_wr are defined as u32 type, and we also need to ensure
> >> num_sqe (derived from max_send_wr) shouldn't be zero.
> >>
> >> Signed-off-by: Guoqing Jiang<guoqing.jiang@xxxxxxxxx>
> >> ---
> >> drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
> >> 1 file changed, 5 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> >> b/drivers/infiniband/sw/siw/siw_verbs.c
> >> index dcd69fc01176..ef149ed98946 100644
> >> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> >> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> >> @@ -333,11 +333,10 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> >> ib_qp_init_attr *attrs,
> >> goto err_atomic;
> >> }
> >> /*
> >> - * NOTE: we allow for zero element SQ and RQ WQE's SGL's
> >> - * but not for a QP unable to hold any WQE (SQ + RQ)
> >> + * NOTE: we don't allow for a QP unable to hold any SQ WQE
> >> */
> >> - if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
> >> - siw_dbg(base_dev, "QP must have send or receive queue\n");
> >> + if (attrs->cap.max_send_wr == 0) {
> >> + siw_dbg(base_dev, "QP must have send queue\n");
> >> rv = -EINVAL;
> >> goto err_atomic;
> >> }
> >> @@ -357,21 +356,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> >> ib_qp_init_attr *attrs,
> >> if (rv)
> >> goto err_atomic;
> >>
> >> - num_sqe = attrs->cap.max_send_wr;
> >> - num_rqe = attrs->cap.max_recv_wr;
> >>
> >> /* 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
> >> * avoids handling counter wrap around.
> >> */
> >> - if (num_sqe)
> >> - num_sqe = roundup_pow_of_two(num_sqe);
> >> - else {
> >> - /* Zero sized SQ is not supported */
> >> - rv = -EINVAL;
> >> - goto err_out_xa;
> >> - }
> >> + num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
> >> + num_rqe = attrs->cap.max_recv_wr;
> >> if (num_rqe)
> >> num_rqe = roundup_pow_of_two(num_rqe);
> >>
> >> --
> >> 2.35.3
> > No the original code allows for a QP which cannot send but
> > just receive or vice versa. A QP which cannot send should be allowed.
> > Only a QP which cannot do anything should be refused to be created.
> > Keep it as is.
>
> Seems it is not consistent with the original code, because Zero sized SQ
> (num_seq = 0) is not supported, also num_seq is got from max_send_wr,
> which means a QP without sq is not permitted here.
>
You are right. The RQ can be zero especially if a shared receive queue
is provided. So your patch looks good.
> But I probably misunderstood something.
>
> Thanks,
> Guoqing
