On Tue, Feb 7, 2017 at 1:56 AM, Doug Ledford <[email protected]> wrote: >> +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
