* Re: [PATCH] IB/cma: Fix reversed test
@ 2017-01-28 0:05 ` Doug Ledford
0 siblings, 0 replies; 17+ messages in thread
From: Doug Ledford @ 2017-01-28 0:05 UTC (permalink / raw)
To: Bart Van Assche, christophe.jaillet-39ZsbGIQGT5GWvitb5QawA,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
sean.hefty-ral2JQCrhuEAvxtiuMwx3w
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] IB/cma: Fix reversed test
@ 2017-01-28 0:05 ` Doug Ledford
0 siblings, 0 replies; 17+ messages in thread
From: Doug Ledford @ 2017-01-28 0:05 UTC (permalink / raw)
To: Bart Van Assche, christophe.jaillet, hal.rosenstock, sean.hefty
Cc: linux-kernel, linux-rdma, kernel-janitors
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] IB/cma: Fix reversed test
2017-01-28 0:05 ` Doug Ledford
@ 2017-01-28 6:59 ` Dan Carpenter
-1 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2017-01-28 6:59 UTC (permalink / raw)
To: Doug Ledford
Cc: Bart Van Assche, christophe.jaillet, hal.rosenstock, sean.hefty,
linux-kernel, linux-rdma, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] IB/cma: Fix reversed test
@ 2017-01-28 6:59 ` Dan Carpenter
0 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2017-01-28 6:59 UTC (permalink / raw)
To: Doug Ledford
Cc: Bart Van Assche, christophe.jaillet, hal.rosenstock, sean.hefty,
linux-kernel, linux-rdma, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] IB/cma: Fix reversed test
2017-01-28 6:59 ` Dan Carpenter
(?)
@ 2017-01-28 12:47 ` Majd Dibbiny
2017-01-28 13:01 ` Majd Dibbiny
-1 siblings, 1 reply; 17+ messages in thread
From: Majd Dibbiny @ 2017-01-28 12:47 UTC (permalink / raw)
To: Dan Carpenter
Cc: Doug Ledford, Bart Van Assche, christophe.jaillet,
hal.rosenstock, sean.hefty, linux-kernel, linux-rdma,
kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] IB/cma: Fix reversed test
2017-01-28 12:47 ` Majd Dibbiny
@ 2017-01-28 13:01 ` Majd Dibbiny
0 siblings, 0 replies; 17+ messages in thread
From: Majd Dibbiny @ 2017-01-28 13:01 UTC (permalink / raw)
To: Dan Carpenter
Cc: Doug Ledford, Bart Van Assche, christophe.jaillet,
hal.rosenstock, sean.hefty, linux-kernel, linux-rdma,
kernel-janitors
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] IB/cma: Fix reversed test
2017-01-28 6:59 ` Dan Carpenter
@ 2017-02-03 18:34 ` Doug Ledford
-1 siblings, 0 replies; 17+ messages in thread
From: Doug Ledford @ 2017-02-03 18:34 UTC (permalink / raw)
To: Dan Carpenter
Cc: Bart Van Assche, christophe.jaillet, hal.rosenstock, sean.hefty,
linux-kernel, linux-rdma, kernel-janitors
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] IB/cma: Fix reversed test
@ 2017-02-03 18:34 ` Doug Ledford
0 siblings, 0 replies; 17+ messages in thread
From: Doug Ledford @ 2017-02-03 18:34 UTC (permalink / raw)
To: Dan Carpenter
Cc: Bart Van Assche, christophe.jaillet, hal.rosenstock, sean.hefty,
linux-kernel, linux-rdma, kernel-janitors
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread[parent not found: <2fde1e87-a345-4697-e205-051a3973e624-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] IB/cma: Fix reversed test
2017-02-03 18:34 ` Doug Ledford
(?)
@ 2017-02-06 9:32 ` Dan Carpenter
-1 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2017-02-06 9:32 UTC (permalink / raw)
To: Doug Ledford
Cc: Bart Van Assche, christophe.jaillet-39ZsbGIQGT5GWvitb5QawA,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] IB/cma: Fix reversed test
@ 2017-02-06 9:32 ` Dan Carpenter
0 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2017-02-06 9:32 UTC (permalink / raw)
To: Doug Ledford
Cc: Bart Van Assche, christophe.jaillet-39ZsbGIQGT5GWvitb5QawA,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] IB/cma: Fix reversed test
@ 2017-02-06 9:32 ` Dan Carpenter
0 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2017-02-06 9:32 UTC (permalink / raw)
To: Doug Ledford
Cc: Bart Van Assche, christophe.jaillet, hal.rosenstock, sean.hefty,
linux-kernel, linux-rdma, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread