Re: [PATCH] RDMA/cm: fix cond_no_effect.cocci warnings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux