diff mbox series

[01/10] vfio: Move vfio_device driver open/close code to a function

Message ID 1-v1-4991695894d8+211-vfio_iommufd_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Connect VFIO to IOMMUFD | expand

Commit Message

Jason Gunthorpe Oct. 25, 2022, 6:17 p.m. UTC
This error unwind is getting complicated. Move all the code into two
pair'd function. The functions should be called when the open_count == 1
after incrementing/before decrementing.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio_main.c | 95 ++++++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

Comments

Tian, Kevin Nov. 1, 2022, 7:33 a.m. UTC | #1
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, October 26, 2022 2:17 AM
> 
> This error unwind is getting complicated. Move all the code into two
> pair'd function. The functions should be called when the open_count == 1
> after incrementing/before decrementing.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with a nit

> +	/*
> +	 * Here we pass the KVM pointer with the group under the read lock.

Now the read lock is replaced by mutex. Let's correct it when moving this
piece of code.
Jason Gunthorpe Nov. 1, 2022, 12:12 p.m. UTC | #2
On Tue, Nov 01, 2022 at 07:33:30AM +0000, Tian, Kevin wrote:

> > +	/*
> > +	 * Here we pass the KVM pointer with the group under the read lock.
> 
> Now the read lock is replaced by mutex. Let's correct it when moving this
> piece of code.

Done, thanks

Jason
Yi Liu Nov. 1, 2022, 2:36 p.m. UTC | #3
On 2022/10/26 02:17, Jason Gunthorpe wrote:
> This error unwind is getting complicated. Move all the code into two
> pair'd function. The functions should be called when the open_count == 1
> after incrementing/before decrementing.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/vfio/vfio_main.c | 95 ++++++++++++++++++++++------------------
>   1 file changed, 53 insertions(+), 42 deletions(-)

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 2d168793d4e1ce..d043383fc3ba2b 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -734,6 +734,51 @@ bool vfio_assert_device_open(struct vfio_device *device)
>   	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
>   }
>   
> +static int vfio_device_first_open(struct vfio_device *device)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&device->dev_set->lock);
> +
> +	if (!try_module_get(device->dev->driver->owner))
> +		return -ENODEV;
> +
> +	/*
> +	 * Here we pass the KVM pointer with the group under the read lock.  If
> +	 * the device driver will use it, it must obtain a reference and release
> +	 * it during close_device.
> +	 */
> +	mutex_lock(&device->group->group_lock);
> +	device->kvm = device->group->kvm;
> +	if (device->ops->open_device) {
> +		ret = device->ops->open_device(device);
> +		if (ret)
> +			goto err_module_put;
> +	}
> +	vfio_device_container_register(device);
> +	mutex_unlock(&device->group->group_lock);
> +	return 0;
> +
> +err_module_put:
> +	device->kvm = NULL;
> +	mutex_unlock(&device->group->group_lock);
> +	module_put(device->dev->driver->owner);
> +	return ret;
> +}
> +
> +static void vfio_device_last_close(struct vfio_device *device)
> +{
> +	lockdep_assert_held(&device->dev_set->lock);
> +
> +	mutex_lock(&device->group->group_lock);
> +	vfio_device_container_unregister(device);
> +	if (device->ops->close_device)
> +		device->ops->close_device(device);
> +	device->kvm = NULL;
> +	mutex_unlock(&device->group->group_lock);
> +	module_put(device->dev->driver->owner);
> +}
> +
>   static struct file *vfio_device_open(struct vfio_device *device)
>   {
>   	struct file *filep;
> @@ -745,29 +790,12 @@ static struct file *vfio_device_open(struct vfio_device *device)
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> -	if (!try_module_get(device->dev->driver->owner)) {
> -		ret = -ENODEV;
> -		goto err_unassign_container;
> -	}
> -
>   	mutex_lock(&device->dev_set->lock);
>   	device->open_count++;
>   	if (device->open_count == 1) {
> -		/*
> -		 * Here we pass the KVM pointer with the group under the read
> -		 * lock.  If the device driver will use it, it must obtain a
> -		 * reference and release it during close_device.
> -		 */
> -		mutex_lock(&device->group->group_lock);
> -		device->kvm = device->group->kvm;
> -
> -		if (device->ops->open_device) {
> -			ret = device->ops->open_device(device);
> -			if (ret)
> -				goto err_undo_count;
> -		}
> -		vfio_device_container_register(device);
> -		mutex_unlock(&device->group->group_lock);
> +		ret = vfio_device_first_open(device);
> +		if (ret)
> +			goto err_unassign_container;
>   	}
>   	mutex_unlock(&device->dev_set->lock);
>   
> @@ -800,20 +828,11 @@ static struct file *vfio_device_open(struct vfio_device *device)
>   
>   err_close_device:
>   	mutex_lock(&device->dev_set->lock);
> -	mutex_lock(&device->group->group_lock);
> -	if (device->open_count == 1 && device->ops->close_device) {
> -		device->ops->close_device(device);
> -
> -		vfio_device_container_unregister(device);
> -	}
> -err_undo_count:
> -	mutex_unlock(&device->group->group_lock);
> +	if (device->open_count == 1)
> +		vfio_device_last_close(device);
> +err_unassign_container:
>   	device->open_count--;
> -	if (device->open_count == 0 && device->kvm)
> -		device->kvm = NULL;
>   	mutex_unlock(&device->dev_set->lock);
> -	module_put(device->dev->driver->owner);
> -err_unassign_container:
>   	vfio_device_unassign_container(device);
>   	return ERR_PTR(ret);
>   }
> @@ -1016,19 +1035,11 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>   
>   	mutex_lock(&device->dev_set->lock);
>   	vfio_assert_device_open(device);
> -	mutex_lock(&device->group->group_lock);
> -	if (device->open_count == 1 && device->ops->close_device)
> -		device->ops->close_device(device);
> -
> -	vfio_device_container_unregister(device);
> -	mutex_unlock(&device->group->group_lock);
> +	if (device->open_count == 1)
> +		vfio_device_last_close(device);
>   	device->open_count--;
> -	if (device->open_count == 0)
> -		device->kvm = NULL;
>   	mutex_unlock(&device->dev_set->lock);
>   
> -	module_put(device->dev->driver->owner);
> -
>   	vfio_device_unassign_container(device);
>   
>   	vfio_device_put_registration(device);
diff mbox series

Patch

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 2d168793d4e1ce..d043383fc3ba2b 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -734,6 +734,51 @@  bool vfio_assert_device_open(struct vfio_device *device)
 	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
 }
 
+static int vfio_device_first_open(struct vfio_device *device)
+{
+	int ret;
+
+	lockdep_assert_held(&device->dev_set->lock);
+
+	if (!try_module_get(device->dev->driver->owner))
+		return -ENODEV;
+
+	/*
+	 * Here we pass the KVM pointer with the group under the read lock.  If
+	 * the device driver will use it, it must obtain a reference and release
+	 * it during close_device.
+	 */
+	mutex_lock(&device->group->group_lock);
+	device->kvm = device->group->kvm;
+	if (device->ops->open_device) {
+		ret = device->ops->open_device(device);
+		if (ret)
+			goto err_module_put;
+	}
+	vfio_device_container_register(device);
+	mutex_unlock(&device->group->group_lock);
+	return 0;
+
+err_module_put:
+	device->kvm = NULL;
+	mutex_unlock(&device->group->group_lock);
+	module_put(device->dev->driver->owner);
+	return ret;
+}
+
+static void vfio_device_last_close(struct vfio_device *device)
+{
+	lockdep_assert_held(&device->dev_set->lock);
+
+	mutex_lock(&device->group->group_lock);
+	vfio_device_container_unregister(device);
+	if (device->ops->close_device)
+		device->ops->close_device(device);
+	device->kvm = NULL;
+	mutex_unlock(&device->group->group_lock);
+	module_put(device->dev->driver->owner);
+}
+
 static struct file *vfio_device_open(struct vfio_device *device)
 {
 	struct file *filep;
@@ -745,29 +790,12 @@  static struct file *vfio_device_open(struct vfio_device *device)
 	if (ret)
 		return ERR_PTR(ret);
 
-	if (!try_module_get(device->dev->driver->owner)) {
-		ret = -ENODEV;
-		goto err_unassign_container;
-	}
-
 	mutex_lock(&device->dev_set->lock);
 	device->open_count++;
 	if (device->open_count == 1) {
-		/*
-		 * Here we pass the KVM pointer with the group under the read
-		 * lock.  If the device driver will use it, it must obtain a
-		 * reference and release it during close_device.
-		 */
-		mutex_lock(&device->group->group_lock);
-		device->kvm = device->group->kvm;
-
-		if (device->ops->open_device) {
-			ret = device->ops->open_device(device);
-			if (ret)
-				goto err_undo_count;
-		}
-		vfio_device_container_register(device);
-		mutex_unlock(&device->group->group_lock);
+		ret = vfio_device_first_open(device);
+		if (ret)
+			goto err_unassign_container;
 	}
 	mutex_unlock(&device->dev_set->lock);
 
@@ -800,20 +828,11 @@  static struct file *vfio_device_open(struct vfio_device *device)
 
 err_close_device:
 	mutex_lock(&device->dev_set->lock);
-	mutex_lock(&device->group->group_lock);
-	if (device->open_count == 1 && device->ops->close_device) {
-		device->ops->close_device(device);
-
-		vfio_device_container_unregister(device);
-	}
-err_undo_count:
-	mutex_unlock(&device->group->group_lock);
+	if (device->open_count == 1)
+		vfio_device_last_close(device);
+err_unassign_container:
 	device->open_count--;
-	if (device->open_count == 0 && device->kvm)
-		device->kvm = NULL;
 	mutex_unlock(&device->dev_set->lock);
-	module_put(device->dev->driver->owner);
-err_unassign_container:
 	vfio_device_unassign_container(device);
 	return ERR_PTR(ret);
 }
@@ -1016,19 +1035,11 @@  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 
 	mutex_lock(&device->dev_set->lock);
 	vfio_assert_device_open(device);
-	mutex_lock(&device->group->group_lock);
-	if (device->open_count == 1 && device->ops->close_device)
-		device->ops->close_device(device);
-
-	vfio_device_container_unregister(device);
-	mutex_unlock(&device->group->group_lock);
+	if (device->open_count == 1)
+		vfio_device_last_close(device);
 	device->open_count--;
-	if (device->open_count == 0)
-		device->kvm = NULL;
 	mutex_unlock(&device->dev_set->lock);
 
-	module_put(device->dev->driver->owner);
-
 	vfio_device_unassign_container(device);
 
 	vfio_device_put_registration(device);