| Message ID | 20170127131535.19918-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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.
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
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.
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
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
> 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
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.
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 --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); }
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(-)