Re: [PATCH for-next 5/6] RDMA/bnxt_re: Use driver_unregister and unregistration API

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Nov 25, 2019 at 12:39:33AM -0800, Selvin Xavier wrote:

>  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);
>  	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) {
> -		dev_info(rdev_to_dev(rdev), "Unregistering Device");
> +	list_for_each_entry(rdev, &bnxt_re_dev_list, list) {
>  		/*
> -		 * Flush out any scheduled tasks before destroying the
> -		 * resources
> +		 * Set unreg flag to avoid VF resource cleanup
> +		 * in module unload path. This is required because
> +		 * dealloc_driver for VF can come after PF cleaning
> +		 * the VF 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)
> +			rdev->rcfw.res_deinit = true;
>  	}
> +	mutex_unlock(&bnxt_re_dev_lock);

This is super ugly. This driver already has bugs if it has a
dependency on driver unbinding order as drivers can become unbound
from userspace using sysfs or hot un-plug in any ordering.

If the VF driver somehow depends on the PF driver then destruction of
the PF must synchronize and fence the VFs during it's own shutdown.

But this seems very very strange, how can it work if the VF is in a VM
or something and the PF driver is unplugged?

Jason