> From: Tian, Kevin <kevin.tian@xxxxxxxxx> > Sent: Thursday, January 19, 2023 5:45 PM > > > From: Liu, Yi L <yi.l.liu@xxxxxxxxx> > > Sent: Tuesday, January 17, 2023 9:50 PM > > static int vfio_device_group_open(struct vfio_device_file *df) > > { > > struct vfio_device *device = df->device; > > + u32 ioas_id; > > + u32 *pt_id = NULL; > > int ret; > > > > mutex_lock(&device->group->group_lock); > > @@ -165,6 +167,14 @@ static int vfio_device_group_open(struct > > vfio_device_file *df) > > goto err_unlock_group; > > } > > > > + if (device->group->iommufd) { > > + ret = iommufd_vfio_compat_ioas_id(device->group- > > >iommufd, > > + &ioas_id); > > + if (ret) > > + goto err_unlock_group; > > + pt_id = &ioas_id; > > + } > > + > > mutex_lock(&device->dev_set->lock); > > /* > > * Here we pass the KVM pointer with the group under the lock. If > > the > > @@ -174,7 +184,7 @@ static int vfio_device_group_open(struct > > vfio_device_file *df) > > df->kvm = device->group->kvm; > > df->iommufd = device->group->iommufd; > > > > - ret = vfio_device_open(df); > > + ret = vfio_device_open(df, NULL, pt_id); > > having both ioas_id and pt_id in one function is a bit confusing. > > Does it read better with below? > > if (device->group->iommufd) > ret = vfio_device_open(df, NULL, &ioas_id); > else > ret = vfio_device_open(df, NULL, NULL); Yes. 😊 > > +/* @pt_id == NULL implies detach */ > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id) > > +{ > > + lockdep_assert_held(&vdev->dev_set->lock); > > + > > + return vdev->ops->attach_ioas(vdev, pt_id); > > +} > > what benefit does this one-line wrapper give actually? > > especially pt_id==NULL is checked in the callback instead of in this > wrapper. Yep, will just open code in the caller. Regards, Yi Liu