[PATCH] IB/cma: Fix reversed test

[PATCH] IB/cma: Fix reversed test

[Christophe JAILLET]

This test looks reverted.
We should log an error message only if 'ib_attach_mcast()' fails.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/infiniband/core/cma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index cfefb941d729..175f62e5841e 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -3838,7 +3838,7 @@ static int cma_ib_mc_handler(int status, struct ib_sa_multicast *multicast)
 	if (!status && id_priv->id.qp) {
 		status = ib_attach_mcast(id_priv->id.qp, &multicast->rec.mgid,
 					 be16_to_cpu(multicast->rec.mlid));
-		if (!status)
+		if (status)
 			pr_debug_ratelimited("RDMA CM: MULTICAST_ERROR: failed to attach QP. status %d\n",
 					     status);
 	}
-- 
2.9.3

Re: [PATCH] IB/cma: Fix reversed test

[Doug Ledford]

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

On Fri, 2017-01-27 at 14:15 +0100, Christophe JAILLET wrote:
> This test looks reverted.
> We should log an error message only if 'ib_attach_mcast()' fails.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 

You are right, thanks, applied.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] IB/cma: Fix reversed test

[Bart Van Assche]

On Fri, 2017-01-27 at 14:15 +0100, Christophe JAILLET wrote:
> This test looks reverted.
> We should log an error message only if 'ib_attach_mcast()' fails.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/infiniband/core/cma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index cfefb941d729..175f62e5841e 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -3838,7 +3838,7 @@ static int cma_ib_mc_handler(int status, struct ib_sa_multicast *multicast)
>  	if (!status && id_priv->id.qp) {
>  		status = ib_attach_mcast(id_priv->id.qp, &multicast->rec.mgid,
>  					 be16_to_cpu(multicast->rec.mlid));
> -		if (!status)
> +		if (status)
>  			pr_debug_ratelimited("RDMA CM: MULTICAST_ERROR: failed to attach QP. status %d\n",
>  					     status);
>  	}

Hello Christophe,

Do you think this patch needs "Fixes:" and "Cc: stable" tags?

Thanks,

Bart.

Re: [PATCH] IB/cma: Fix reversed test

[Doug Ledford]

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

On Fri, 2017-01-27 at 21:31 +0000, Bart Van Assche wrote:
> On Fri, 2017-01-27 at 14:15 +0100, Christophe JAILLET wrote:
> > 
> > This test looks reverted.
> > We should log an error message only if 'ib_attach_mcast()' fails.
> > 
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> >  drivers/infiniband/core/cma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/core/cma.c
> > b/drivers/infiniband/core/cma.c
> > index cfefb941d729..175f62e5841e 100644
> > --- a/drivers/infiniband/core/cma.c
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -3838,7 +3838,7 @@ static int cma_ib_mc_handler(int status,
> > struct ib_sa_multicast *multicast)
> >  	if (!status && id_priv->id.qp) {
> >  		status = ib_attach_mcast(id_priv->id.qp,
> > &multicast->rec.mgid,
> >  					 be16_to_cpu(multicast-
> > >rec.mlid));
> > -		if (!status)
> > +		if (status)
> >  			pr_debug_ratelimited("RDMA CM:
> > MULTICAST_ERROR: failed to attach QP. status %d\n",
> >  					     status);
> >  	}
> 
> Hello Christophe,
> 
> Do you think this patch needs "Fixes:" and "Cc: stable" tags?

It does not.  I already tried to apply it to my 4.10-rc branch and it
doesn't apply there.  In the for-4.11 queue is a patch to improve debug
printouts in the core, and it is that patch that introduced this error.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] IB/cma: Fix reversed test

[Dan Carpenter]

On Fri, Jan 27, 2017 at 07:05:52PM -0500, Doug Ledford wrote:
> > Do you think this patch needs "Fixes:" and "Cc: stable" tags?
> 
> It does not.

We always should have fixes tags.

When I'm reviewing, I try to look up the patch which introduced the bug
so I can figure out what the intent was.  Having a Fixes tag speeds up
my work.

Looking at how the bug was introduced sometimes helps to prevent bugs
from recurring in the future.  For example, I've seen several bugs
introduced because the right people weren't on the CC to review it.  For
this particular bug it feels like probably this bug could have been
detected with more testing.  I doubt it would have made it into a
released kernel.

Also it let's you CC the original authors and hopefully they can Ack it.

regards,
dan carpenter

Re: [PATCH] IB/cma: Fix reversed test

[Majd Dibbiny]

We have message sniffer that checks for unwanted prints after each test..

Sent from my iPhone

> On Jan 28, 2017, at 8:59 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> On Fri, Jan 27, 2017 at 07:05:52PM -0500, Doug Ledford wrote:
>>> Do you think this patch needs "Fixes:" and "Cc: stable" tags?
>> 
>> It does not.
> 
> We always should have fixes tags.
> 
> When I'm reviewing, I try to look up the patch which introduced the bug
> so I can figure out what the intent was.  Having a Fixes tag speeds up
> my work.
> 
> Looking at how the bug was introduced sometimes helps to prevent bugs
> from recurring in the future.  For example, I've seen several bugs
> introduced because the right people weren't on the CC to review it.  For
> this particular bug it feels like probably this bug could have been
> detected with more testing.  I doubt it would have made it into a
> released kernel.
> 
> Also it let's you CC the original authors and hopefully they can Ack it.
> 
> regards,
> dan carpenter
> --
> 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

Re: [PATCH] IB/cma: Fix reversed test

[Majd Dibbiny]

> On Jan 28, 2017, at 2:47 PM, Majd Dibbiny <majd@mellanox.com> wrote:
> 
Please ignore the previous email.
It was part of an internal discussion..
> We have message sniffer that checks for unwanted prints after each test..
> 
> Sent from my iPhone
> 
>> On Jan 28, 2017, at 8:59 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> 
>> On Fri, Jan 27, 2017 at 07:05:52PM -0500, Doug Ledford wrote:
>>>> Do you think this patch needs "Fixes:" and "Cc: stable" tags?
>>> 
>>> It does not.
>> 
>> We always should have fixes tags.
>> 
>> When I'm reviewing, I try to look up the patch which introduced the bug
>> so I can figure out what the intent was.  Having a Fixes tag speeds up
>> my work.
>> 
>> Looking at how the bug was introduced sometimes helps to prevent bugs
>> from recurring in the future.  For example, I've seen several bugs
>> introduced because the right people weren't on the CC to review it.  For
>> this particular bug it feels like probably this bug could have been
>> detected with more testing.  I doubt it would have made it into a
>> released kernel.
>> 
>> Also it let's you CC the original authors and hopefully they can Ack it.
>> 
>> regards,
>> dan carpenter
>> --
>> 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

Re: [PATCH] IB/cma: Fix reversed test

[Doug Ledford]

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

On 1/28/2017 1:59 AM, Dan Carpenter wrote:
> On Fri, Jan 27, 2017 at 07:05:52PM -0500, Doug Ledford wrote:
>>> Do you think this patch needs "Fixes:" and "Cc: stable" tags?
>>
>> It does not.
> 
> We always should have fixes tags.
> 
> When I'm reviewing, I try to look up the patch which introduced the bug
> so I can figure out what the intent was.  Having a Fixes tag speeds up
> my work.
> 
> Looking at how the bug was introduced sometimes helps to prevent bugs
> from recurring in the future.  For example, I've seen several bugs
> introduced because the right people weren't on the CC to review it.  For
> this particular bug it feels like probably this bug could have been
> detected with more testing.  I doubt it would have made it into a
> released kernel.
> 
> Also it let's you CC the original authors and hopefully they can Ack it.

OK, in my mind, there is a specific reason for Fixes: tags, and it
relates to the automated means by which other maintainers pull patches
for long term stable trees.  Because both the buggy patch and this fix
are being queued in the same general kernel release, there is no need
for this patch to get automatically pulled for any of the long term
stable kernels.  Hence my statement that it doesn't need a fixes tag.  I
don't disagree with your reasons for wanting one, but even if you added
the fixes tag, the Cc: stable is definitely not needed.


-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


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

Re: [PATCH] IB/cma: Fix reversed test

[Dan Carpenter]

Right.  The stable tag isn't needed.  But the fixes tag is a different
thing.  Greg doesn't automatically pull patches which have a fixes tag.

Fixes is also useful for collecting metrics about how long it takes to
fix bugs.  It's just really useful on it's own.

regards,
dan carpenter