On 2024/12/18 17:24, Ferry Meng wrote:
> Introduce character interfaces for block device (per-device), facilitating
> access to block devices through io_uring I/O passsthrough.
>
> Besides, vblk initialize only use kmalloc with GFP_KERNEL flag, but for
> char device support, we should ensure cdev kobj must be zero before
> initialize. So better initial this struct with __GFP_ZERO flag.
>
> Now the character devices only named as
>
> - /dev/vdXc0
>
> Currently, only one character interface is created for one actual
> virtblk device, although it has been partitioned.
>
> Signed-off-by: Ferry Meng <mengferry@xxxxxxxxxxxxxxxxx>
> ---
> drivers/block/virtio_blk.c | 84 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 194417abc105..3487aaa67514 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -17,6 +17,7 @@
> #include <linux/numa.h>
> #include <linux/vmalloc.h>
> #include <uapi/linux/virtio_ring.h>
> +#include <linux/cdev.h>
>
> #define PART_BITS 4
> #define VQ_NAME_LEN 16
> @@ -25,6 +26,8 @@
> /* The maximum number of sg elements that fit into a virtqueue */
> #define VIRTIO_BLK_MAX_SG_ELEMS 32768
>
> +#define VIRTBLK_MINORS (1U << MINORBITS)
> +
> #ifdef CONFIG_ARCH_NO_SG_CHAIN
> #define VIRTIO_BLK_INLINE_SG_CNT 0
> #else
> @@ -45,6 +48,10 @@ MODULE_PARM_DESC(poll_queues, "The number of dedicated virtqueues for polling I/
> static int major;
> static DEFINE_IDA(vd_index_ida);
>
> +static DEFINE_IDA(vd_chr_minor_ida);
> +static dev_t vd_chr_devt;
> +static struct class *vd_chr_class;
> +
> static struct workqueue_struct *virtblk_wq;
>
> struct virtio_blk_vq {
> @@ -84,6 +91,10 @@ struct virtio_blk {
>
> /* For zoned device */
> unsigned int zone_sectors;
> +
> + /* For passthrough cmd */
> + struct cdev cdev;
> + struct device cdev_device;
> };
>
> struct virtblk_req {
> @@ -1239,6 +1250,55 @@ static const struct blk_mq_ops virtio_mq_ops = {
> .poll = virtblk_poll,
> };
>
> +static void virtblk_cdev_rel(struct device *dev)
> +{
> + ida_free(&vd_chr_minor_ida, MINOR(dev->devt));
> +}
> +
> +static void virtblk_cdev_del(struct cdev *cdev, struct device *cdev_device)
> +{
> + cdev_device_del(cdev, cdev_device);
> + put_device(cdev_device);
> +}
> +
> +static int virtblk_cdev_add(struct virtio_blk *vblk,
> + const struct file_operations *fops)
> +{
> + struct cdev *cdev = &vblk->cdev;
> + struct device *cdev_device = &vblk->cdev_device;
> + int minor, ret;
> +
> + minor = ida_alloc(&vd_chr_minor_ida, GFP_KERNEL);
> + if (minor < 0)
> + return minor;
> +
> + cdev_device->parent = &vblk->vdev->dev;
> + cdev_device->devt = MKDEV(MAJOR(vd_chr_devt), minor);
> + cdev_device->class = vd_chr_class;
> + cdev_device->release = virtblk_cdev_rel;
> + device_initialize(cdev_device);
> +
> + ret = dev_set_name(cdev_device, "%sc0", vblk->disk->disk_name);
> + if (ret)
> + goto err;
> +
> + cdev_init(cdev, fops);
> + ret = cdev_device_add(cdev, cdev_device);
> + if (ret) {
> + put_device(cdev_device);
> + goto err;
put_device() will call cdev_device->release() to free vd_chr_minor_ida.
> + }
> + return ret;
> +
> +err:
> + ida_free(&vd_chr_minor_ida, minor);
> + return ret;
> +}
> +
> +static const struct file_operations virtblk_chr_fops = {
> + .owner = THIS_MODULE,
> +};
> +
> static unsigned int virtblk_queue_depth;
> module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
>
> @@ -1456,7 +1516,7 @@ static int virtblk_probe(struct virtio_device *vdev)
> goto out;
> index = err;
>
> - vdev->priv = vblk = kmalloc(sizeof(*vblk), GFP_KERNEL);
> + vdev->priv = vblk = kzalloc(sizeof(*vblk), GFP_KERNEL);
> if (!vblk) {
> err = -ENOMEM;
> goto out_free_index;
> @@ -1544,6 +1604,10 @@ static int virtblk_probe(struct virtio_device *vdev)
> if (err)
> goto out_cleanup_disk;
>
> + err = virtblk_cdev_add(vblk, &virtblk_chr_fops);
> + if (err)
> + goto out_cleanup_disk;
Missing remove the added disk before.
> +
> return 0;
>
> out_cleanup_disk:
> @@ -1568,6 +1632,8 @@ static void virtblk_remove(struct virtio_device *vdev)
> /* Make sure no work handler is accessing the device. */
> flush_work(&vblk->config_work);
>
> + virtblk_cdev_del(&vblk->cdev, &vblk->cdev_device);
> +
> del_gendisk(vblk->disk);
> blk_mq_free_tag_set(&vblk->tag_set);
>
> @@ -1674,13 +1740,27 @@ static int __init virtio_blk_init(void)
> goto out_destroy_workqueue;
> }
>
> + error = alloc_chrdev_region(&vd_chr_devt, 0, VIRTBLK_MINORS,
> + "vblk-generic");
> + if (error < 0)
> + goto unregister_chrdev;
Should unregister blkdev.
> +
> + vd_chr_class = class_create("vblk-generic");
> + if (IS_ERR(vd_chr_class)) {
> + error = PTR_ERR(vd_chr_class);
> + goto unregister_chrdev;
> + }
> +
> error = register_virtio_driver(&virtio_blk);
> if (error)
> goto out_unregister_blkdev;
You've missed destroying vd_chr_class.
> +
> return 0;
>
> out_unregister_blkdev:
> unregister_blkdev(major, "virtblk");
> +unregister_chrdev:
> + unregister_chrdev_region(vd_chr_devt, VIRTBLK_MINORS);
The out labels should be re-ordered, e.g. move this up.
> out_destroy_workqueue:
> destroy_workqueue(virtblk_wq);
> return error;
> @@ -1690,7 +1770,9 @@ static void __exit virtio_blk_fini(void)
> {
> unregister_virtio_driver(&virtio_blk);
> unregister_blkdev(major, "virtblk");
Also missed destroying vd_chr_class.
Thanks,
Joseph
> + unregister_chrdev_region(vd_chr_devt, VIRTBLK_MINORS);
> destroy_workqueue(virtblk_wq);
> + ida_destroy(&vd_chr_minor_ida);
> }
> module_init(virtio_blk_init);
> module_exit(virtio_blk_fini);
