On Tue, Feb 7, 2017 at 1:56 AM, Doug Ledford <dledford@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
