On Fri, Nov 25, 2022 at 04:57:27PM +0800, Yi Liu wrote: > > +static int vfio_device_group_open(struct vfio_device *device) > > +{ > > + int ret; > > + > > + mutex_lock(&device->group->group_lock); > > now the group path holds group_lock first, and then device_set->lock. > this is different with existing code. is it acceptable? I had a quick > check with this change, basic test is good. no a-b-b-a locking issue. I looked for a while and couldn't find a reason why it wouldn't be OK > > + if (!vfio_group_has_iommu(device->group)) { > > + ret = -EINVAL; > > + goto out_unlock; > > + } > > + > > + /* > > + * Here we pass the KVM pointer with the group under the lock. If the > > + * device driver will use it, it must obtain a reference and release it > > + * during close_device. > > + */ > > + ret = vfio_device_open(device, device->group->iommufd, > > + device->group->kvm); > > + > > +out_unlock: > > + mutex_unlock(&device->group->group_lock); > > + return ret; > > +} > > + > > +void vfio_device_close_group(struct vfio_device *device) > > +{ > > + mutex_lock(&device->group->group_lock); > > + vfio_device_close(device, device->group->iommufd); > > + mutex_unlock(&device->group->group_lock); > > +} > > + > > above two functions should be put in group.c. Yes Jason