* [PATCH] RDMA/cm: fix cond_no_effect.cocci warnings @ 2022-06-10 9:45 Jiapeng Chong 2022-06-14 1:19 ` Mark Zhang 0 siblings, 1 reply; 5+ messages in thread From: Jiapeng Chong @ 2022-06-10 9:45 UTC (permalink / raw) To: jgg; +Cc: leon, linux-rdma, linux-kernel, Jiapeng Chong, Abaci Robot This was found by coccicheck: ./drivers/infiniband/core/cm.c:685:7-9: WARNING: possible condition with no effect (if == else). Reported-by: Abaci Robot <abaci@linux.alibaba.com> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> --- drivers/infiniband/core/cm.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index 1c107d6d03b9..bb6a2b6b9657 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -676,14 +676,9 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device, refcount_inc(&cm_id_priv->refcount); return cm_id_priv; } - if (device < cm_id_priv->id.device) + if (device < cm_id_priv->id.device || + be64_lt(service_id, cm_id_priv->id.service_id)) node = node->rb_left; - else if (device > cm_id_priv->id.device) - node = node->rb_right; - else if (be64_lt(service_id, cm_id_priv->id.service_id)) - node = node->rb_left; - else if (be64_gt(service_id, cm_id_priv->id.service_id)) - node = node->rb_right; else node = node->rb_right; } -- 2.20.1.7.g153144c ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] RDMA/cm: fix cond_no_effect.cocci warnings 2022-06-10 9:45 [PATCH] RDMA/cm: fix cond_no_effect.cocci warnings Jiapeng Chong @ 2022-06-14 1:19 ` Mark Zhang 2022-06-24 20:17 ` Jason Gunthorpe 0 siblings, 1 reply; 5+ messages in thread From: Mark Zhang @ 2022-06-14 1:19 UTC (permalink / raw) To: Jiapeng Chong, jgg; +Cc: leon, linux-rdma, linux-kernel, Abaci Robot On 6/10/2022 5:45 PM, Jiapeng Chong wrote: > This was found by coccicheck: > > ./drivers/infiniband/core/cm.c:685:7-9: WARNING: possible condition with no effect (if == else). > > Reported-by: Abaci Robot <abaci@linux.alibaba.com> > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> > --- > drivers/infiniband/core/cm.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index 1c107d6d03b9..bb6a2b6b9657 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -676,14 +676,9 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device, > refcount_inc(&cm_id_priv->refcount); > return cm_id_priv; > } > - if (device < cm_id_priv->id.device) > + if (device < cm_id_priv->id.device || > + be64_lt(service_id, cm_id_priv->id.service_id)) > node = node->rb_left; > - else if (device > cm_id_priv->id.device) > - node = node->rb_right; > - else if (be64_lt(service_id, cm_id_priv->id.service_id)) > - node = node->rb_left; > - else if (be64_gt(service_id, cm_id_priv->id.service_id)) > - node = node->rb_right; > else > node = node->rb_right; > } Not sure if the fix is correct, e.g. with this condition: device > cm_id_priv->id.device && be64_lt(service_id, cm_id_priv->id.service_id) The original code gets rb_right but this fix gets rb_left. Maybe the warning is complain about this: ... else if (be64_gt(service_id, cm_id_priv->id.service_id)) node = node->rb_right; else node = node->rb_right; Besides cm_insert_listen() has same logic. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RDMA/cm: fix cond_no_effect.cocci warnings 2022-06-14 1:19 ` Mark Zhang @ 2022-06-24 20:17 ` Jason Gunthorpe 2022-08-02 2:15 ` Mark Zhang 0 siblings, 1 reply; 5+ messages in thread From: Jason Gunthorpe @ 2022-06-24 20:17 UTC (permalink / raw) To: Mark Zhang; +Cc: Jiapeng Chong, leon, linux-rdma, linux-kernel, Abaci Robot On Tue, Jun 14, 2022 at 09:19:14AM +0800, Mark Zhang wrote: > On 6/10/2022 5:45 PM, Jiapeng Chong wrote: > > This was found by coccicheck: > > > > ./drivers/infiniband/core/cm.c:685:7-9: WARNING: possible condition with no effect (if == else). > > > > Reported-by: Abaci Robot <abaci@linux.alibaba.com> > > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> > > --- > > drivers/infiniband/core/cm.c | 9 ++------- > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > > index 1c107d6d03b9..bb6a2b6b9657 100644 > > --- a/drivers/infiniband/core/cm.c > > +++ b/drivers/infiniband/core/cm.c > > @@ -676,14 +676,9 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device, > > refcount_inc(&cm_id_priv->refcount); > > return cm_id_priv; > > } > > - if (device < cm_id_priv->id.device) > > + if (device < cm_id_priv->id.device || > > + be64_lt(service_id, cm_id_priv->id.service_id)) > > node = node->rb_left; > > - else if (device > cm_id_priv->id.device) > > - node = node->rb_right; > > - else if (be64_lt(service_id, cm_id_priv->id.service_id)) > > - node = node->rb_left; > > - else if (be64_gt(service_id, cm_id_priv->id.service_id)) > > - node = node->rb_right; > > else > > node = node->rb_right; > > } > > Not sure if the fix is correct, e.g. with this condition: > device > cm_id_priv->id.device && > be64_lt(service_id, cm_id_priv->id.service_id) > > The original code gets rb_right but this fix gets rb_left. Maybe the warning > is complain about this: > ... > else if (be64_gt(service_id, cm_id_priv->id.service_id)) > node = node->rb_right; > else > node = node->rb_right; > > Besides cm_insert_listen() has same logic. Yes, this is a standard pattern for walking tree with priority, we should not obfuscate it. The final else means 'equal' and the first if should ideally be placed there However this function is complicated by the use of the service_mask for equality checking, and it doesn't even work right if the service_mask is not -1. If someone wants to clean this then please go through and eliminate service_mask completely. From what I can see its value is always -1. Three patches: - Remove the service_mask parameter from ib_cm_listen(), all callers use 0 - Remove the service_mask parameter from cm_init_listen(), all callers use 0. Inspect and remove cm_id_priv->id.service_mask, it is the constant value ~cpu_to_be64(0) which is a NOP when &'d - Move the test at the top of cm_find_listen() into the final else Jason ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RDMA/cm: fix cond_no_effect.cocci warnings 2022-06-24 20:17 ` Jason Gunthorpe @ 2022-08-02 2:15 ` Mark Zhang 2022-08-02 13:52 ` Jason Gunthorpe 0 siblings, 1 reply; 5+ messages in thread From: Mark Zhang @ 2022-08-02 2:15 UTC (permalink / raw) To: Jason Gunthorpe Cc: Jiapeng Chong, leon, linux-rdma, linux-kernel, Abaci Robot > Yes, this is a standard pattern for walking tree with priority, we > should not obfuscate it. > > The final else means 'equal' and the first if should ideally be placed > there > > However this function is complicated by the use of the service_mask > for equality checking, and it doesn't even work right if the > service_mask is not -1. > > If someone wants to clean this then please go through and eliminate > service_mask completely. From what I can see its value is always -1. > Three patches: > - Remove the service_mask parameter from ib_cm_listen(), all callers > use 0 > - Remove the service_mask parameter from cm_init_listen(), all > callers use 0. Inspect and remove cm_id_priv->id.service_mask, > it is the constant value ~cpu_to_be64(0) which is a NOP when &'d > - Move the test at the top of cm_find_listen() into the final else > I'll do it. For the 3rd one, do you mean a patch like (similar change in cm_insert_listen): diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index a2973436b16f..8749165bbe3d 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -626,9 +626,15 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv, parent = *link; cur_cm_id_priv = rb_entry(parent, struct cm_id_private, service_node); - if ((cur_cm_id_priv->id.service_mask & service_id) == - (service_mask & cur_cm_id_priv->id.service_id) && - (cm_id_priv->id.device == cur_cm_id_priv->id.device)) { + if (cm_id_priv->id.device < cur_cm_id_priv->id.device) + link = &(*link)->rb_left; + else if (cm_id_priv->id.device > cur_cm_id_priv->id.device) + link = &(*link)->rb_right; + else if (be64_lt(service_id, cur_cm_id_priv->id.service_id)) + link = &(*link)->rb_left; + else if (be64_gt(service_id, cur_cm_id_priv->id.service_id)) + link = &(*link)->rb_right; + else { /* * Sharing an ib_cm_id with different handlers is not * supported @@ -644,17 +650,6 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv, spin_unlock_irqrestore(&cm.lock, flags); return cur_cm_id_priv; } - - if (cm_id_priv->id.device < cur_cm_id_priv->id.device) - link = &(*link)->rb_left; - else if (cm_id_priv->id.device > cur_cm_id_priv->id.device) - link = &(*link)->rb_right; - else if (be64_lt(service_id, cur_cm_id_priv->id.service_id)) - link = &(*link)->rb_left; - else if (be64_gt(service_id, cur_cm_id_priv->id.service_id)) - link = &(*link)->rb_right; - else - link = &(*link)->rb_right; } cm_id_priv->listen_sharecount++; rb_link_node(&cm_id_priv->service_node, parent, link); @@ -671,12 +666,6 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device, while (node) { cm_id_priv = rb_entry(node, struct cm_id_private, service_node); - if ((cm_id_priv->id.service_mask & service_id) == - cm_id_priv->id.service_id && - (cm_id_priv->id.device == device)) { - refcount_inc(&cm_id_priv->refcount); - return cm_id_priv; - } if (device < cm_id_priv->id.device) node = node->rb_left; else if (device > cm_id_priv->id.device) @@ -685,8 +674,10 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device, node = node->rb_left; else if (be64_gt(service_id, cm_id_priv->id.service_id)) node = node->rb_right; - else - node = node->rb_right; + else { + refcount_inc(&cm_id_priv->refcount); + return cm_id_priv; + } } return NULL; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] RDMA/cm: fix cond_no_effect.cocci warnings 2022-08-02 2:15 ` Mark Zhang @ 2022-08-02 13:52 ` Jason Gunthorpe 0 siblings, 0 replies; 5+ messages in thread From: Jason Gunthorpe @ 2022-08-02 13:52 UTC (permalink / raw) To: Mark Zhang; +Cc: Jiapeng Chong, leon, linux-rdma, linux-kernel, Abaci Robot On Tue, Aug 02, 2022 at 10:15:24AM +0800, Mark Zhang wrote: > > > Yes, this is a standard pattern for walking tree with priority, we > > should not obfuscate it. > > > > The final else means 'equal' and the first if should ideally be placed > > there > > > > However this function is complicated by the use of the service_mask > > for equality checking, and it doesn't even work right if the > > service_mask is not -1. > > > > If someone wants to clean this then please go through and eliminate > > service_mask completely. From what I can see its value is always -1. > > Three patches: > > - Remove the service_mask parameter from ib_cm_listen(), all callers > > use 0 > > - Remove the service_mask parameter from cm_init_listen(), all > > callers use 0. Inspect and remove cm_id_priv->id.service_mask, > > it is the constant value ~cpu_to_be64(0) which is a NOP when &'d > > - Move the test at the top of cm_find_listen() into the final else > > > > I'll do it. For the 3rd one, do you mean a patch like (similar change in > cm_insert_listen): Yes Jason ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-02 13:52 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-10 9:45 [PATCH] RDMA/cm: fix cond_no_effect.cocci warnings Jiapeng Chong 2022-06-14 1:19 ` Mark Zhang 2022-06-24 20:17 ` Jason Gunthorpe 2022-08-02 2:15 ` Mark Zhang 2022-08-02 13:52 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).