On Mon, Feb 24, 2020 at 7:13 PM Jason Gunthorpe <jgg@xxxxxxxxxxxx> wrote:
>
> On Mon, Feb 24, 2020 at 02:49:55AM -0800, Selvin Xavier wrote:
> >
> > @@ -1785,32 +1777,23 @@ static int __init bnxt_re_mod_init(void)
> >
> > static void __exit bnxt_re_mod_exit(void)
> > {
> > - struct bnxt_re_dev *rdev, *next;
> > - LIST_HEAD(to_be_deleted);
> > + struct bnxt_re_dev *rdev;
> >
> > + flush_workqueue(bnxt_re_wq);
>
> What is this for? Usually flushing a work queue before preventing new
> work from queueing (ie via unregister) is racy.
To flush out any netdev events scheduled to be handled by
bnxt_re_task. Mainly to wait for
case where we are in the middle of NETDEV_REGISTER event handled from
bnxt_re_task.
>
> > mutex_lock(&bnxt_re_dev_lock);
> > - /* Free all adapter allocated resources */
> > - if (!list_empty(&bnxt_re_dev_list))
> > - list_splice_init(&bnxt_re_dev_list, &to_be_deleted);
> > - mutex_unlock(&bnxt_re_dev_lock);
> > - /*
> > - * Cleanup the devices in reverse order so that the VF device
> > - * cleanup is done before PF cleanup
> > - */
> > - list_for_each_entry_safe_reverse(rdev, next, &to_be_deleted, list) {
> > - ibdev_info(&rdev->ibdev, "Unregistering Device");
> > - /*
> > - * Flush out any scheduled tasks before destroying the
> > - * resources
> > + list_for_each_entry(rdev, &bnxt_re_dev_list, list) {
> > + /* VF device removal should be called before the removal
> > + * of PF device. Queue VFs unregister first, so that VFs
> > + * shall be removed before the PF during the call of
> > + * ib_unregister_driver. Commands to any VFs after the
> > + * PF removal will timeout and driver will proceed with
> > + * unregisteration and free up the host resources.
> > */
> > - flush_workqueue(bnxt_re_wq);
> > - bnxt_re_dev_stop(rdev);
> > - bnxt_re_ib_uninit(rdev);
> > - /* Acquire the rtnl_lock as the L2 resources are freed here */
> > - rtnl_lock();
> > - bnxt_re_remove_device(rdev);
> > - rtnl_unlock();
> > + if (rdev->is_virtfn)
> > + ib_unregister_device_queued(&rdev->ibdev);
>
> Why do it queued? Just call ib_unregister_device(). Otherwise it won't
> order reliably.
Sure. Will change it.
>
> But be very careful about lifetime. All the other drivers had problems
> managing the lifetime of the pointers in their device lists.
>
> Jason
