-----"Dan Carpenter" <dan.carpenter@xxxxxxxxxx> wrote: -----
>To: bmt@xxxxxxxxxxxxxx
>From: "Dan Carpenter" <dan.carpenter@xxxxxxxxxx>
>Date: 10/24/2019 10:39PM
>Cc: linux-rdma@xxxxxxxxxxxxxxx
>Subject: [EXTERNAL] [bug report] RDMA/siw: Fix SQ/RQ drain logic
>
>Hello Bernard Metzler,
>
>The patch cf049bb31f71: "RDMA/siw: Fix SQ/RQ drain logic" from Oct 4,
>2019, leads to the following static checker warning:
>
> drivers/infiniband/sw/siw/siw_verbs.c:1079 siw_post_receive()
> error: locking inconsistency. We assume 'read_sem:&qp->state_lock'
>is both locked and unlocked at the start.
>
>drivers/infiniband/sw/siw/siw_verbs.c
> 978 int siw_post_receive(struct ib_qp *base_qp, const struct
>ib_recv_wr *wr,
> 979 const struct ib_recv_wr **bad_wr)
> 980 {
> 981 struct siw_qp *qp = to_siw_qp(base_qp);
> 982 unsigned long flags;
> 983 int rv = 0;
> 984
> 985 if (qp->srq) {
> 986 *bad_wr = wr;
> 987 return -EOPNOTSUPP; /* what else from
>errno.h? */
> 988 }
> 989 if (!qp->kernel_verbs) {
> 990 siw_dbg_qp(qp, "no kernel post_recv for user
>mapped sq\n");
> 991 up_read(&qp->state_lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^
>The patch changes the locking so this isn't held here and should be
>released. Should it be held, though?
Yes, this is a BUG. Thanks very much! There is no qp spinlock to
be held here. I moved that down to state checking. No need to
hold the qp lock before.
Shall I re-send, or can we just amend the patch,
removing that single 'up_read()' line?
Thanks very much,
Bernard
>
> 992 *bad_wr = wr;
> 993 return -EINVAL;
> 994 }
> 995
> 996 /*
> 997 * Try to acquire QP state lock. Must be non-blocking
> 998 * to accommodate kernel clients needs.
> 999 */
> 1000 if (!down_read_trylock(&qp->state_lock)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 1001 if (qp->attrs.state == SIW_QP_STATE_ERROR) {
> 1002 /*
> 1003 * ERROR state is final, so we can be
>sure
> 1004 * this state will not change as long
>as the QP
> 1005 * exists.
> 1006 *
> 1007 * This handles an ib_drain_rq() call
>with
> 1008 * a concurrent request to set the QP
>state
> 1009 * to ERROR.
> 1010 */
> 1011 rv = siw_rq_flush_wr(qp, wr, bad_wr);
> 1012 } else {
>
>regards,
>dan carpenter
>
>
