| 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 |
> 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.
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
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 --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);
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(-)