| Message ID | 1477672406-31487-1-git-send-email-weiyj.lk@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, Oct 28, 2016 at 04:33:26PM +0000, Wei Yongjun wrote: > From: Wei Yongjun <weiyongjun1@huawei.com> > > Add the missing unlock before return from function qedr_post_send() > in the error handling case. > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > --- > drivers/infiniband/hw/qedr/verbs.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c > index a615142..e7c7417 100644 > --- a/drivers/infiniband/hw/qedr/verbs.c > +++ b/drivers/infiniband/hw/qedr/verbs.c > @@ -2983,7 +2983,8 @@ int qedr_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, > > if (!wr) { > DP_ERR(dev, "Got an empty post send.\n"); > - return -EINVAL; > + rc = -EINVAL; > + goto out_unlock; > } IMHO, this if needs to be moved to be before acquiring spinlock and avoid introducing new labels for this one case only. > > while (wr) { > @@ -3012,6 +3013,7 @@ int qedr_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, > /* Make sure write sticks */ > mmiowb(); > > +out_unlock: > spin_unlock_irqrestore(&qp->q_lock, flags); > > return rc; > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > index a615142..e7c7417 100644 > > --- a/drivers/infiniband/hw/qedr/verbs.c > > +++ b/drivers/infiniband/hw/qedr/verbs.c > > @@ -2983,7 +2983,8 @@ int qedr_post_send(struct ib_qp *ibqp, struct > > ib_send_wr *wr, > > > > if (!wr) { > > DP_ERR(dev, "Got an empty post send.\n"); > > - return -EINVAL; > > + rc = -EINVAL; > > + goto out_unlock; > > } > > IMHO, this if needs to be moved to be before acquiring spinlock and avoid > introducing new labels for this one case only. > Thanks Wei and Leon. Actually, perhaps we can totally remove the check itself - Since this is kernel space, is it safe to presume that all ULPs are trusted to be well coded? (if not, and as a side note, I see that in MLX4 there's a for(..;wr;..) loop, but that wr is dereferenced earlier so perhaps this is a potential bug) Ram -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 01, 2016 at 02:09:42PM +0000, Amrani, Ram wrote: > > > index a615142..e7c7417 100644 > > > --- a/drivers/infiniband/hw/qedr/verbs.c > > > +++ b/drivers/infiniband/hw/qedr/verbs.c > > > @@ -2983,7 +2983,8 @@ int qedr_post_send(struct ib_qp *ibqp, struct > > > ib_send_wr *wr, > > > > > > if (!wr) { > > > DP_ERR(dev, "Got an empty post send.\n"); > > > - return -EINVAL; > > > + rc = -EINVAL; > > > + goto out_unlock; > > > } > > > > IMHO, this if needs to be moved to be before acquiring spinlock and avoid > > introducing new labels for this one case only. > > > > Thanks Wei and Leon. > > Actually, perhaps we can totally remove the check itself - > Since this is kernel space, is it safe to presume that all ULPs are trusted to be well coded? I think so.
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c index a615142..e7c7417 100644 --- a/drivers/infiniband/hw/qedr/verbs.c +++ b/drivers/infiniband/hw/qedr/verbs.c @@ -2983,7 +2983,8 @@ int qedr_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, if (!wr) { DP_ERR(dev, "Got an empty post send.\n"); - return -EINVAL; + rc = -EINVAL; + goto out_unlock; } while (wr) { @@ -3012,6 +3013,7 @@ int qedr_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, /* Make sure write sticks */ mmiowb(); +out_unlock: spin_unlock_irqrestore(&qp->q_lock, flags); return rc;