Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1568347 > unrolled thread

[PATCH] IB/cma: Fix reversed test

Back to article view | Back to linux.kernel

Contents

  [PATCH] IB/cma: Fix reversed test Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2017-01-27 14:20 +0100
    Re: [PATCH] IB/cma: Fix reversed test Doug Ledford <dledford@redhat.com> - 2017-01-27 20:40 +0100
    Re: [PATCH] IB/cma: Fix reversed test Doug Ledford <dledford@redhat.com> - 2017-01-28 01:10 +0100
      Re: [PATCH] IB/cma: Fix reversed test Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-28 08:10 +0100
        Re: [PATCH] IB/cma: Fix reversed test Majd Dibbiny <majd@mellanox.com> - 2017-01-28 14:10 +0100
      Re: [PATCH] IB/cma: Fix reversed test Majd Dibbiny <majd@mellanox.com> - 2017-01-28 14:10 +0100

#1568347 — [PATCH] IB/cma: Fix reversed test

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

[toc] | [next] | [standalone]

#1568641

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1568797

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1568840

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

[toc] | [prev] | [next] | [standalone]

#1568896

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

[toc] | [prev] | [next] | [standalone]

#1568895

> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web