diff mbox

IB/cma: Fix reversed test

Message ID 20170127131535.19918-1-christophe.jaillet@wanadoo.fr (mailing list archive)
State Accepted
Headers show

Commit Message

Christophe JAILLET Jan. 27, 2017, 1:15 p.m. UTC
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(-)

Comments

Doug Ledford Jan. 27, 2017, 7:24 p.m. UTC | #1
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.
Bart Van Assche Jan. 27, 2017, 9:31 p.m. UTC | #2
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.--
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
Doug Ledford Jan. 28, 2017, 12:05 a.m. UTC | #3
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.
Dan Carpenter Jan. 28, 2017, 6:59 a.m. UTC | #4
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
Majd Dibbiny Jan. 28, 2017, 12:47 p.m. UTC | #5
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
--
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
Majd Dibbiny Jan. 28, 2017, 1:01 p.m. UTC | #6
> 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
--
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
Doug Ledford Feb. 3, 2017, 6:34 p.m. UTC | #7
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.
Dan Carpenter Feb. 6, 2017, 9:32 a.m. UTC | #8
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

--
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
diff mbox

Patch

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);
 	}