Date:   Thu, 23 Dec 2021 11:59:50 +0100
From:   Stefano Garzarella <sgarzare@redhat.com>
To:     Yi Wang <wang.yi59@zte.com.cn>
Cc:     mst@redhat.com, Zhang Min <zhang.min9@zte.com.cn>,
        wang.liang82@zte.com.cn, kvm@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org, xue.zhihong@zte.com.cn
Subject: Re: [PATCH] vdpa: regist vhost-vdpa dev class
Message-ID: <20211223105950.ovywoj6v3aooh2v5@steredhat>
References: <20211223073145.35363-1-wang.yi59@zte.com.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20211223073145.35363-1-wang.yi59@zte.com.cn>
Precedence: bulk

On Thu, Dec 23, 2021 at 03:31:45PM +0800, Yi Wang wrote:
>From: Zhang Min <zhang.min9@zte.com.cn>
>
>Some applications like kata-containers need to acquire MAJOR/MINOR/DEVNAME
>for devInfo [1], so regist vhost-vdpa dev class to expose uevent.
>
>1. https://github.com/kata-containers/kata-containers/blob/main/src/runtime/virtcontainers/device/config/config.go
>
>Signed-off-by: Zhang Min <zhang.min9@zte.com.cn>
>Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
>---
> drivers/vhost/vdpa.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>index fb41db3da611..90fbad93e7a2 100644
>--- a/drivers/vhost/vdpa.c
>+++ b/drivers/vhost/vdpa.c
>@@ -1012,6 +1012,7 @@ static void vhost_vdpa_release_dev(struct device *device)
> 	kfree(v);
> }
>
>+static struct class *vhost_vdpa_class;
   ^
Maybe is better to move this declaration on top, near `static dev_t 
vhost_vdpa_major;`

> static int vhost_vdpa_probe(struct vdpa_device *vdpa)
> {
> 	const struct vdpa_config_ops *ops = vdpa->config;
>@@ -1040,6 +1041,7 @@ static int vhost_vdpa_probe(struct vdpa_device *vdpa)
> 	v->dev.release = vhost_vdpa_release_dev;
> 	v->dev.parent = &vdpa->dev;
> 	v->dev.devt = MKDEV(MAJOR(vhost_vdpa_major), minor);
>+	v->dev.class = vhost_vdpa_class;
> 	v->vqs = kmalloc_array(v->nvqs, sizeof(struct vhost_virtqueue),
> 			       GFP_KERNEL);
> 	if (!v->vqs) {
>@@ -1097,6 +1099,14 @@ static int __init vhost_vdpa_init(void)
> {
> 	int r;
>
>+	vhost_vdpa_class = class_create(THIS_MODULE, "vhost-vdpa");
>+	if (IS_ERR(vhost_vdpa_class)) {
>+		r = PTR_ERR(vhost_vdpa_class);
>+		pr_warn("vhost vdpa class create error %d,  maybe mod reinserted\n", r);
                                                           ^
double space.

I'm not a native speaker, but I would rephrase the second part to "maybe 
the module is already loaded"

>+		vhost_vdpa_class = NULL;
>+		return r;
>+	}
>+
> 	r = alloc_chrdev_region(&vhost_vdpa_major, 0, VHOST_VDPA_DEV_MAX,
> 				"vhost-vdpa");
> 	if (r)
>@@ -1111,6 +1121,7 @@ static int __init vhost_vdpa_init(void)
> err_vdpa_register_driver:
> 	unregister_chrdev_region(vhost_vdpa_major, VHOST_VDPA_DEV_MAX);
> err_alloc_chrdev:
>+	class_destroy(vhost_vdpa_class);

Should we set `vhost_vdpa_class` to NULL here?

If yes, maybe better to add a new label, and a goto in the 
`class_create` error handling.

> 	return r;
> }
> module_init(vhost_vdpa_init);
>@@ -1118,6 +1129,7 @@ module_init(vhost_vdpa_init);
> static void __exit vhost_vdpa_exit(void)
> {
> 	vdpa_unregister_driver(&vhost_vdpa_driver);
>+	class_destroy(vhost_vdpa_class);

I would move it after unregister_chrdev_region() to have the reverse 
order of initialization (as we already rightly do in the error path of 
vhost_vdpa_init()).

Thanks,
Stefano

> 	unregister_chrdev_region(vhost_vdpa_major, VHOST_VDPA_DEV_MAX);
> }
> module_exit(vhost_vdpa_exit);
>-- 
>2.27.0
>_______________________________________________
>Virtualization mailing list
>Virtualization@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>