> From: Liu, Yi L <yi.l.liu@xxxxxxxxx> > Sent: Wednesday, March 8, 2023 9:29 PM > > @@ -177,7 +177,7 @@ static int vfio_device_group_open(struct > vfio_device_file *df) > mutex_lock(&device->group->group_lock); > if (!vfio_group_has_iommu(device->group)) { > ret = -EINVAL; > - goto out_unlock; > + goto err_unlock; > } My impression - out_xxx means go to do xxx while err_xxx means go to do something for error xxx, though in many places the two are mixed to both meaning 'do xxx'. either way I don't see a need of changing it. > -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) > +static int vfio_iommufd_device_probe_comapt_noiommu(struct vfio_device > *vdev, > + struct iommufd_ctx *ictx) s/comapt/compat/ btw it's clearer to move this check into vfio_device_group_open(). if noiommu then pass NULL to vfio_device_open(), same as the cdev path. > + > +int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) > +{ > u32 device_id; > int ret; > > lockdep_assert_held(&vdev->dev_set->lock); > > if (vfio_device_is_noiommu(vdev)) { > - if (!capable(CAP_SYS_RAWIO)) > - return -EPERM; > - > - /* > - * Require no compat ioas to be assigned to proceed. The > basic > - * statement is that the user cannot have done something > that > - * implies they expected translation to exist > - */ > - if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id)) > - return -EPERM; > - return 0; > + ret = vfio_iommufd_device_probe_comapt_noiommu(vdev, > ictx); > + if (ret) > + return ret; > } > > if (WARN_ON(!vdev->ops->bind_iommufd)) > return -ENODEV; > > - ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id); > - if (ret) > - return ret; > + /* The legacy path has no way to return the device id */ > + return vdev->ops->bind_iommufd(vdev, ictx, &device_id); > +} > > - ret = iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id); > - if (ret) > - goto err_unbind; > - ret = vdev->ops->attach_ioas(vdev, &ioas_id); > - if (ret) > - goto err_unbind; after noiommu check and attach_ioas are moved out then this entire function can be removed now. Just call the ops in vfio_device_first_open(). > +int vfio_iommufd_attach_compat_ioas(struct vfio_device *vdev, > + struct iommufd_ctx *ictx) > +{ > + u32 ioas_id; > + int ret; > + > + lockdep_assert_held(&vdev->dev_set->lock); > > /* > - * The legacy path has no way to return the device id or the selected > - * pt_id > + * If the driver doesn't provide this op then it means the device does > + * not do DMA at all. So nothing to do. > */ > - return 0; > + if (WARN_ON(!vdev->ops->bind_iommufd)) > + return -ENODEV; > > -err_unbind: > - if (vdev->ops->unbind_iommufd) > - vdev->ops->unbind_iommufd(vdev); > - return ret; > + if (vfio_device_is_noiommu(vdev)) { > + if > (WARN_ON(vfio_iommufd_device_probe_comapt_noiommu(vdev, ictx))) > + return -EINVAL; > + return 0; > + } no need. let's directly call following from vfio_device_group_open(). In that case no need to do noiommu check twice in one function. > + > + ret = iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id); > + if (ret) > + return ret; > + > + /* The legacy path has no way to return the selected pt_id */ > + return vdev->ops->attach_ioas(vdev, &ioas_id); > } >