Re: [PATCH for bnxt_re V4 03/21] RDMA/bnxt_re: register with the NIC driver

On Tue, Feb 7, 2017 at 1:56 AM, Doug Ledford [off-list ref] wrote:

quoted

+static void bnxt_re_dev_remove(struct bnxt_re_dev *rdev)
+{
+       int i = BNXT_RE_REF_WAIT_COUNT;
+
+       /* Wait for rdev refcount to come down */
+       while ((atomic_read(&rdev->ref_count) > 1) && i--)
+               msleep(100);
+
+       if (atomic_read(&rdev->ref_count) > 1)
+               dev_err(rdev_to_dev(rdev),
+                       "Failed waiting for ref count to deplete %d",
+                       atomic_read(&rdev->ref_count));
+
+       atomic_set(&rdev->ref_count, 0);
+       dev_put(rdev->netdev);
+       rdev->netdev = NULL;
+
+       mutex_lock(&bnxt_re_dev_lock);
+       list_del_rcu(&rdev->list);
+       mutex_unlock(&bnxt_re_dev_lock);
+
+       synchronize_rcu();
+       flush_workqueue(bnxt_re_wq);
+
+       ib_dealloc_device(&rdev->ibdev);
+       /* rdev is gone */
+}

This looks bad.  Either your ref counting is right, and your ref count
should go to 1, or you have an issue that won't be helped by forcibly
removing the device.  In its current form, this looks like an oopser
waiting to happen.

If you know your ref counting is right, then simply wait until it goes
to 1, don't have this bailout logic.  If you truly need to bailout for
some reason, then you need to not free things.  Better to shutdown then
leak and live than release and have a use after free data corrupter.

Thanks for your commet.
I reviewed the usage of ref_count again and it is not really required
in this patch series.
It is being incremented and decremented from the netdev notifier and
will be always 1
in bnxt_re_dev_remove. bnxt_re_dev_remove is also invoked from netdev
notifier, so
no need to wait in this function.  So, I have removed ref_count
variable and cleaned up
this function in the v5 patch set.

Thanks,
Selvin