[PATCH] qedr: Fix missing unlock on error in qedr_post_send()

[PATCH] qedr: Fix missing unlock on error in qedr_post_send()

[Wei Yongjun]

From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Add the missing unlock before return from function qedr_post_send()
in the error handling case.

Signed-off-by: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 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;
 	}
 
 	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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] qedr: Fix missing unlock on error in qedr_post_send()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

On Fri, Oct 28, 2016 at 04:33:26PM +0000, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>
> Add the missing unlock before return from function qedr_post_send()
> in the error handling case.
>
> Signed-off-by: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] qedr: Fix missing unlock on error in qedr_post_send()

[Amrani, Ram]

> > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] qedr: remove pointless NULL check in qedr_post_send()

[Wei Yongjun]

From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Remove pointless NULL check for 'wr' in qedr_post_send().

Signed-off-by: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
v1 -> v2: remove pointless NULL check as Ram's suggestion
---
 drivers/infiniband/hw/qedr/verbs.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index a615142..ed7521a 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -2981,11 +2981,6 @@ int qedr_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		return -EINVAL;
 	}
 
-	if (!wr) {
-		DP_ERR(dev, "Got an empty post send.\n");
-		return -EINVAL;
-	}
-
 	while (wr) {
 		rc = __qedr_post_send(ibqp, wr, bad_wr);
 		if (rc)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] qedr: Fix missing unlock on error in qedr_post_send()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2] qedr: remove pointless NULL check in qedr_post_send()

[Amrani, Ram]

> diff --git a/drivers/infiniband/hw/qedr/verbs.c
> b/drivers/infiniband/hw/qedr/verbs.c
> index a615142..ed7521a 100644
> --- a/drivers/infiniband/hw/qedr/verbs.c
> +++ b/drivers/infiniband/hw/qedr/verbs.c
> @@ -2981,11 +2981,6 @@ int qedr_post_send(struct ib_qp *ibqp, struct
> ib_send_wr *wr,
>  		return -EINVAL;
>  	}
> 
> -	if (!wr) {
> -		DP_ERR(dev, "Got an empty post send.\n");
> -		return -EINVAL;
> -	}
> -
>  	while (wr) {
>  		rc = __qedr_post_send(ibqp, wr, bad_wr);
>  		if (rc)

Thanks Wei.

Acked-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] qedr: remove pointless NULL check in qedr_post_send()

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 390 bytes --]

On 11/2/2016 9:11 AM, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> Remove pointless NULL check for 'wr' in qedr_post_send().
> 
> Signed-off-by: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Thanks, applied.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]