RE: [PATCH 08/18] vfio/mdev: Reorganize mdev_device_create()

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 24, 2021 1:55 AM
> 
> Once the memory for the struct mdev_device is allocated it should
> immediately be device_initialize()'d and filled in so that put_device()
> can always be used to undo the allocation.
> 
> Place the mdev_get/put_parent() so that they are clearly protecting the
> mdev->parent pointer. Move the final put to the release function so that
> the lifetime rules are trivial to understand.
> 
> Remove mdev_device_free() as the release function via device_put() is now
> usable in all cases.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/mdev/mdev_core.c | 46 +++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c
> b/drivers/vfio/mdev/mdev_core.c
> index 7ec21c907397a5..517b6fd351b63a 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -71,7 +71,6 @@ static void mdev_device_remove_common(struct
> mdev_device *mdev)
> 
>  	/* Balances with device_initialize() */
>  	put_device(&mdev->dev);
> -	mdev_put_parent(parent);
>  }
> 
>  static int mdev_device_remove_cb(struct device *dev, void *data)
> @@ -208,8 +207,13 @@ void mdev_unregister_device(struct device *dev)
>  }
>  EXPORT_SYMBOL(mdev_unregister_device);
> 
> -static void mdev_device_free(struct mdev_device *mdev)
> +static void mdev_device_release(struct device *dev)
>  {
> +	struct mdev_device *mdev = to_mdev_device(dev);
> +
> +	/* Pairs with the get in mdev_device_create() */
> +	mdev_put_parent(mdev->parent);
> +
>  	mutex_lock(&mdev_list_lock);
>  	list_del(&mdev->next);
>  	mutex_unlock(&mdev_list_lock);
> @@ -218,59 +222,50 @@ static void mdev_device_free(struct mdev_device
> *mdev)
>  	kfree(mdev);
>  }
> 
> -static void mdev_device_release(struct device *dev)
> -{
> -	struct mdev_device *mdev = to_mdev_device(dev);
> -
> -	mdev_device_free(mdev);
> -}
> -
>  int mdev_device_create(struct mdev_type *type, const guid_t *uuid)
>  {
>  	int ret;
>  	struct mdev_device *mdev, *tmp;
>  	struct mdev_parent *parent = type->parent;
> 
> -	mdev_get_parent(parent);
>  	mutex_lock(&mdev_list_lock);
> 
>  	/* Check for duplicate */
>  	list_for_each_entry(tmp, &mdev_list, next) {
>  		if (guid_equal(&tmp->uuid, uuid)) {
>  			mutex_unlock(&mdev_list_lock);
> -			ret = -EEXIST;
> -			goto mdev_fail;
> +			return -EEXIST;
>  		}
>  	}
> 
>  	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>  	if (!mdev) {
>  		mutex_unlock(&mdev_list_lock);
> -		ret = -ENOMEM;
> -		goto mdev_fail;
> +		return -ENOMEM;
>  	}
> 
> +	device_initialize(&mdev->dev);
> +	mdev->dev.parent  = parent->dev;
> +	mdev->dev.bus = &mdev_bus_type;
> +	mdev->dev.release = mdev_device_release;
> +	mdev->dev.groups = parent->ops->mdev_attr_groups;
> +	mdev->type = type;
> +	mdev->parent = parent;
> +	/* Pairs with the put in mdev_device_release() */
> +	mdev_get_parent(parent);
> +
>  	guid_copy(&mdev->uuid, uuid);
>  	list_add(&mdev->next, &mdev_list);
>  	mutex_unlock(&mdev_list_lock);
> 
> -	mdev->parent = parent;
> +	dev_set_name(&mdev->dev, "%pUl", uuid);
> 
>  	/* Check if parent unregistration has started */
>  	if (!down_read_trylock(&parent->unreg_sem)) {
> -		mdev_device_free(mdev);
>  		ret = -ENODEV;
>  		goto mdev_fail;
>  	}
> 
> -	device_initialize(&mdev->dev);
> -	mdev->dev.parent = parent->dev;
> -	mdev->dev.bus     = &mdev_bus_type;
> -	mdev->dev.release = mdev_device_release;
> -	dev_set_name(&mdev->dev, "%pUl", uuid);
> -	mdev->dev.groups = parent->ops->mdev_attr_groups;
> -	mdev->type = type;
> -
>  	ret = parent->ops->create(&type->kobj, mdev);
>  	if (ret)
>  		goto ops_create_fail;
> @@ -295,9 +290,8 @@ int mdev_device_create(struct mdev_type *type,
> const guid_t *uuid)
>  	parent->ops->remove(mdev);
>  ops_create_fail:
>  	up_read(&parent->unreg_sem);
> -	put_device(&mdev->dev);
>  mdev_fail:
> -	mdev_put_parent(parent);
> +	put_device(&mdev->dev);
>  	return ret;
>  }
> 
> --
> 2.31.0