> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Tuesday, January 17, 2023 9:50 PM
>
> @@ -156,7 +159,11 @@ static void vfio_device_release(struct device *dev)
> container_of(dev, struct vfio_device, device);
>
> vfio_release_device_set(device);
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> + ida_free(&vfio.device_ida, MINOR(device->device.devt));
> +#else
> ida_free(&vfio.device_ida, device->index);
> +#endif
There are many #if in this patch, leading to bad readability.
for this what about letting device->index always storing the minor
value? then here it could just be:
ida_free(&vfio.device_ida, device->index);
> @@ -232,17 +240,25 @@ static int vfio_init_device(struct vfio_device
> *device, struct device *dev,
> device->device.release = vfio_device_release;
> device->device.class = vfio.device_class;
> device->device.parent = device->dev;
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> + device->device.devt = MKDEV(MAJOR(vfio.device_devt), minor);
> + cdev_init(&device->cdev, &vfio_device_fops);
> + device->cdev.owner = THIS_MODULE;
> +#else
> + device->index = minor;
> +#endif
Probably we can have a vfio_init_device_cdev() in iommufd.c and let
it be empty if !CONFIG_IOMMUFD. Then here could be:
device->index = minor;
vfio_init_device_cdev(device, vfio.device_devt, minor);
> @@ -257,7 +273,12 @@ static int __vfio_register_dev(struct vfio_device
> *device,
> if (!device->dev_set)
> vfio_assign_device_set(device, device);
>
> - ret = dev_set_name(&device->device, "vfio%d", device->index);
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> + minor = MINOR(device->device.devt);
> +#else
> + minor = device->index;
> +#endif
then just "minor = device->index"
>
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> + ret = cdev_device_add(&device->cdev, &device->device);
> +#else
> ret = device_add(&device->device);
> +#endif
also add a wrapper vfio_register_device_cdev() which does
cdev_device_add() if CONFIG_IOMMUFD and device_add() otherwise.
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> + /*
> + * Balances device_add in register path. Putting it as the first
> + * operation in unregister to prevent registration refcount from
> + * incrementing per cdev open.
> + */
> + cdev_device_del(&device->cdev, &device->device);
> +#else
> + device_del(&device->device);
> +#endif
ditto
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> +static int vfio_device_fops_open(struct inode *inode, struct file *filep)
> +{
> + struct vfio_device *device = container_of(inode->i_cdev,
> + struct vfio_device, cdev);
> + struct vfio_device_file *df;
> + int ret;
> +
> + if (!vfio_device_try_get_registration(device))
> + return -ENODEV;
> +
> + /*
> + * device access is blocked until .open_device() is called
> + * in BIND_IOMMUFD.
> + */
> + df = vfio_allocate_device_file(device, true);
> + if (IS_ERR(df)) {
> + ret = PTR_ERR(df);
> + goto err_put_registration;
> + }
> +
> + filep->private_data = df;
> +
> + return 0;
> +
> +err_put_registration:
> + vfio_device_put_registration(device);
> + return ret;
> +}
> +#endif
move to iommufd.c
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> +static char *vfio_device_devnode(const struct device *dev, umode_t *mode)
> +{
> + return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> +}
> +#endif
ditto
> @@ -1543,9 +1617,21 @@ static int __init vfio_init(void)
> goto err_dev_class;
> }
>
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> + vfio.device_class->devnode = vfio_device_devnode;
> + ret = alloc_chrdev_region(&vfio.device_devt, 0,
> + MINORMASK + 1, "vfio-dev");
> + if (ret)
> + goto err_alloc_dev_chrdev;
> +#endif
vfio_cdev_init()
> static void __exit vfio_cleanup(void)
> {
> ida_destroy(&vfio.device_ida);
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> + unregister_chrdev_region(vfio.device_devt, MINORMASK + 1);
> +#endif
vfio_cdev_cleanup()
