RE: [PATCH v2 01/14] vfio: Remove extra put/gets around vfio_device->group

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

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, March 13, 2021 8:56 AM
> 
> The vfio_device->group value has a get obtained during
> vfio_add_group_dev() which gets moved from the stack to vfio_device-
> >group
> in vfio_group_create_device().
> 
> The reference remains until we reach the end of vfio_del_group_dev() when
> it is put back.
> 
> Thus anything that already has a kref on the vfio_device is guaranteed a
> valid group pointer. Remove all the extra reference traffic.
> 
> It is tricky to see, but the get at the start of vfio_del_group_dev() is
> actually pairing with the put hidden inside vfio_device_put() a few lines
> below.

I feel that the put inside vfio_device_put was meant to pair with the get in 
vfio_group_create_device before this patch is applied. Because vfio_device_
put may drop the last reference to the group, vfio_del_group_dev then 
issues its own get to hold the reference until the put at the end of the func. 

Nevertheless this patch does make the flow much cleaner:

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

> 
> A later patch merges vfio_group_create_device() into vfio_add_group_dev()
> which makes the ownership and error flow on the create side easier to
> follow.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 38779e6fd80cb4..15d8e678e5563a 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -546,14 +546,12 @@ struct vfio_device
> *vfio_group_create_device(struct vfio_group *group,
> 
>  	kref_init(&device->kref);
>  	device->dev = dev;
> +	/* Our reference on group is moved to the device */
>  	device->group = group;
>  	device->ops = ops;
>  	device->device_data = device_data;
>  	dev_set_drvdata(dev, device);
> 
> -	/* No need to get group_lock, caller has group reference */
> -	vfio_group_get(group);
> -
>  	mutex_lock(&group->device_lock);
>  	list_add(&device->group_next, &group->device_list);
>  	group->dev_counter++;
> @@ -585,13 +583,11 @@ void vfio_device_put(struct vfio_device *device)
>  {
>  	struct vfio_group *group = device->group;
>  	kref_put_mutex(&device->kref, vfio_device_release, &group-
> >device_lock);
> -	vfio_group_put(group);
>  }
>  EXPORT_SYMBOL_GPL(vfio_device_put);
> 
>  static void vfio_device_get(struct vfio_device *device)
>  {
> -	vfio_group_get(device->group);
>  	kref_get(&device->kref);
>  }
> 
> @@ -841,14 +837,6 @@ int vfio_add_group_dev(struct device *dev,
>  		vfio_group_put(group);
>  		return PTR_ERR(device);
>  	}
> -
> -	/*
> -	 * Drop all but the vfio_device reference.  The vfio_device holds
> -	 * a reference to the vfio_group, which holds a reference to the
> -	 * iommu_group.
> -	 */
> -	vfio_group_put(group);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(vfio_add_group_dev);
> @@ -928,12 +916,6 @@ void *vfio_del_group_dev(struct device *dev)
>  	unsigned int i = 0;
>  	bool interrupted = false;
> 
> -	/*
> -	 * The group exists so long as we have a device reference.  Get
> -	 * a group reference and use it to scan for the device going away.
> -	 */
> -	vfio_group_get(group);
> -
>  	/*
>  	 * When the device is removed from the group, the group suddenly
>  	 * becomes non-viable; the device has a driver (until the unbind
> @@ -1008,6 +990,7 @@ void *vfio_del_group_dev(struct device *dev)
>  	if (list_empty(&group->device_list))
>  		wait_event(group->container_q, !group->container);
> 
> +	/* Matches the get in vfio_group_create_device() */

There is no get there now.

>  	vfio_group_put(group);
> 
>  	return device_data;
> --
> 2.30.2