> From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> Sent: Friday, January 20, 2023 3:27 PM
>
> > 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);
Yes.
> > @@ -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:
Yes. Btw. Will adding another device_cdev.c better than reusing iommufd.c?
>
> 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"
Yes.
> >
> > +#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.
Got it.
>
> > +#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()
All above comments got.
Regards,
Yi Liu
