Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Dec 2022 01:28:58 -0800
From:   Christoph Hellwig <hch@infradead.org>
To:     Sergei Shtepa <sergei.shtepa@veeam.com>
Cc:     axboe@kernel.dk, corbet@lwn.net, linux-block@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 06/21] block, blksnap: module management interface
 functions
Message-ID: <Y5ro2qv8pxGQ5bTN@infradead.org>
References: <20221209142331.26395-1-sergei.shtepa@veeam.com>
 <20221209142331.26395-7-sergei.shtepa@veeam.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221209142331.26395-7-sergei.shtepa@veeam.com>
Precedence: bulk

> +	ret = register_chrdev(0, THIS_MODULE->name, &ctrl_fops);

This crashed when blksnap is built into the kernel because THIS_MODULE
is NULL for that case.

But I think most of this boilerplate can be removed - just use a misc
device and use /proc/misc to find the major from userspace if anything
cares, but nothing should as the misc devices already create the
node through udev.  I.e.g:

diff --git a/drivers/block/blksnap/Makefile b/drivers/block/blksnap/Makefile
index b196b17f9d9d97..cbcac12cc61d59 100644
--- a/drivers/block/blksnap/Makefile
+++ b/drivers/block/blksnap/Makefile
@@ -12,7 +12,6 @@ blksnap-y := 		\
 	main.o		\
 	snapimage.o	\
 	snapshot.o	\
-	sysfs.o		\
 	tracker.o
 
 obj-$(CONFIG_BLK_SNAP)	 += blksnap.o
diff --git a/drivers/block/blksnap/ctrl.c b/drivers/block/blksnap/ctrl.c
index 990ffb004db3f9..d1a5e3a13a19be 100644
--- a/drivers/block/blksnap/ctrl.c
+++ b/drivers/block/blksnap/ctrl.c
@@ -5,6 +5,7 @@
 #include <linux/poll.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
+#include <linux/miscdevice.h>
 #include <uapi/linux/blksnap.h>
 #include "ctrl.h"
 #include "params.h"
@@ -16,16 +17,6 @@
 static_assert(sizeof(uuid_t) == sizeof(struct blk_snap_uuid),
 	"Invalid size of struct blk_snap_uuid or uuid_t.");
 
-static int blk_snap_major;
-
-static long ctrl_unlocked_ioctl(struct file *filp, unsigned int cmd,
-				unsigned long arg);
-
-static const struct file_operations ctrl_fops = {
-	.owner = THIS_MODULE,
-	.unlocked_ioctl = ctrl_unlocked_ioctl,
-};
-
 static const struct blk_snap_version version = {
 	.major = VERSION_MAJOR,
 	.minor = VERSION_MINOR,
@@ -33,34 +24,6 @@ static const struct blk_snap_version version = {
 	.build = VERSION_BUILD,
 };
 
-int get_blk_snap_major(void)
-{
-	return blk_snap_major;
-}
-
-int ctrl_init(void)
-{
-	int ret;
-
-	ret = register_chrdev(0, THIS_MODULE->name, &ctrl_fops);
-	if (ret < 0) {
-		pr_err("Failed to register a character device. errno=%d\n",
-		       abs(blk_snap_major));
-		return ret;
-	}
-
-	blk_snap_major = ret;
-	pr_info("Register control device [%d:0].\n", blk_snap_major);
-	return 0;
-}
-
-void ctrl_done(void)
-{
-	pr_info("Unregister control device\n");
-
-	unregister_chrdev(blk_snap_major, THIS_MODULE->name);
-}
-
 static int ioctl_version(unsigned long arg)
 {
 	if (copy_to_user((void *)arg, &version, sizeof(version))) {
@@ -408,3 +371,24 @@ static long ctrl_unlocked_ioctl(struct file *filp, unsigned int cmd,
 
 	return blk_snap_ioctl_table[nr](arg);
 }
+
+static const struct file_operations blksnap_ctrl_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= ctrl_unlocked_ioctl,
+};
+
+static struct miscdevice blksnap_ctrl_misc = {
+	.minor		= MISC_DYNAMIC_MINOR,
+	.name		= "blksnap-control",
+	.fops		= &blksnap_ctrl_fops,
+};
+
+int ctrl_init(void)
+{
+	return misc_register(&blksnap_ctrl_misc);
+}
+
+void ctrl_done(void)
+{
+	misc_deregister(&blksnap_ctrl_misc);
+}
diff --git a/drivers/block/blksnap/ctrl.h b/drivers/block/blksnap/ctrl.h
index ade3f1cf57e90c..88f4b05296e575 100644
--- a/drivers/block/blksnap/ctrl.h
+++ b/drivers/block/blksnap/ctrl.h
@@ -2,8 +2,6 @@
 #ifndef __BLK_SNAP_CTRL_H
 #define __BLK_SNAP_CTRL_H
 
-int get_blk_snap_major(void);
-
 int ctrl_init(void);
 void ctrl_done(void);
 #endif /* __BLK_SNAP_CTRL_H */
diff --git a/drivers/block/blksnap/main.c b/drivers/block/blksnap/main.c
index a7939efc6497b9..f8b6f5c406ed7c 100644
--- a/drivers/block/blksnap/main.c
+++ b/drivers/block/blksnap/main.c
@@ -6,7 +6,6 @@
 #include "version.h"
 #include "params.h"
 #include "ctrl.h"
-#include "sysfs.h"
 #include "snapimage.h"
 #include "snapshot.h"
 #include "tracker.h"
@@ -44,8 +43,6 @@ static int __init blk_snap_init(void)
 	result = ctrl_init();
 	if (result)
 		return result;
-
-	result = sysfs_initialize();
 	return result;
 }
 
@@ -53,7 +50,6 @@ static void __exit blk_snap_exit(void)
 {
 	pr_info("Unloading module\n");
 
-	sysfs_finalize();
 	ctrl_done();
 
 	diff_io_done();
diff --git a/drivers/block/blksnap/sysfs.c b/drivers/block/blksnap/sysfs.c
deleted file mode 100644
index 6f53c4217d6c7b..00000000000000
--- a/drivers/block/blksnap/sysfs.c
+++ /dev/null
@@ -1,80 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#define pr_fmt(fmt) KBUILD_MODNAME "-sysfs: " fmt
-
-#include <linux/module.h>
-#include <linux/blkdev.h>
-#include <linux/sysfs.h>
-#include <linux/device.h>
-#include <uapi/linux/blksnap.h>
-#include "sysfs.h"
-#include "ctrl.h"
-
-static ssize_t major_show(struct class *class, struct class_attribute *attr,
-			  char *buf)
-{
-	sprintf(buf, "%d", get_blk_snap_major());
-	return strlen(buf);
-}
-
-/* Declare class_attr_major */
-CLASS_ATTR_RO(major);
-
-static struct class *blk_snap_class;
-
-static struct device *blk_snap_device;
-
-int sysfs_initialize(void)
-{
-	struct device *dev;
-	int res;
-
-	blk_snap_class = class_create(THIS_MODULE, THIS_MODULE->name);
-	if (IS_ERR(blk_snap_class)) {
-		res = PTR_ERR(blk_snap_class);
-
-		pr_err("Bad class create. errno=%d\n", abs(res));
-		return res;
-	}
-
-	pr_info("Create 'major' sysfs attribute\n");
-	res = class_create_file(blk_snap_class, &class_attr_major);
-	if (res) {
-		pr_err("Failed to create 'major' sysfs file\n");
-
-		class_destroy(blk_snap_class);
-		blk_snap_class = NULL;
-		return res;
-	}
-
-	dev = device_create(blk_snap_class, NULL,
-			    MKDEV(get_blk_snap_major(), 0), NULL,
-			    THIS_MODULE->name);
-	if (IS_ERR(dev)) {
-		res = PTR_ERR(dev);
-		pr_err("Failed to create device, errno=%d\n", abs(res));
-
-		class_remove_file(blk_snap_class, &class_attr_major);
-		class_destroy(blk_snap_class);
-		blk_snap_class = NULL;
-		return res;
-	}
-
-	blk_snap_device = dev;
-	return res;
-}
-
-void sysfs_finalize(void)
-{
-	pr_info("Cleanup sysfs\n");
-
-	if (blk_snap_device) {
-		device_unregister(blk_snap_device);
-		blk_snap_device = NULL;
-	}
-
-	if (blk_snap_class != NULL) {
-		class_remove_file(blk_snap_class, &class_attr_major);
-		class_destroy(blk_snap_class);
-		blk_snap_class = NULL;
-	}
-}
diff --git a/drivers/block/blksnap/sysfs.h b/drivers/block/blksnap/sysfs.h
deleted file mode 100644
index 5fc200d3678964..00000000000000
--- a/drivers/block/blksnap/sysfs.h
+++ /dev/null
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __BLK_SNAP_SYSFS_H
-#define __BLK_SNAP_SYSFS_H
-
-int sysfs_initialize(void);
-void sysfs_finalize(void);
-#endif /* __BLK_SNAP_SYSFS_H */