Re: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Jun 12, 2023 at 04:13:22PM +0200, Peter Zijlstra wrote:
> > Mind if I try this series to convert a more "normal" driver to see how
> > it works with that?  That's going to be the true test, see if the
> > changes make sense to someone who doesn't really know the internals of
> > the driver core like this...
> 
> Not at all, feel tree to have a go at that. I picked code I was familiar
> with, but it would ofc. be good to have others give it a spin too.

Ok, here's an attempt to clean up misc.c a bit.  It does two things,
adds a guard for the global misc_mtx mutex when used (sometimes, it's
not always paired with a release.)

Then in the last part of the file, I abuse the DEFINE_FREE() to handle a
special case of removing a proc file if things go bad (and add a
DEFINE_FREE() for class_destroy(), which should go into
include/device/class.h.

I've only test-built it, but is this the proper use of DEFINE_FREE()?
There wasn't much documentation :)

To be fair the end-result of misc_init() is much nicer and cleaner and
"obviously correct", which is good, even with the crazy proc file mess
in it.  So I like the idea overall, need to figure out when to use
DEFINE_CLASS() vs. DEFINE_FREE(), that isn't obvious to me.

Also, you can't put a DEFINE_FREE() within a function declaration, which
I guess makes sense, but the build warning is very odd when you attempt
it, mentioning an "invalid storage class".  Is that supposed to be able
to work?

thanks,

greg k-h


---
 drivers/char/misc.c | 69 +++++++++++++++++----------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 1c44c29a666e..a203b17a2b82 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -123,10 +123,9 @@ static int misc_open(struct inode *inode, struct file *file)
 {
 	int minor = iminor(inode);
 	struct miscdevice *c = NULL, *iter;
-	int err = -ENODEV;
 	const struct file_operations *new_fops = NULL;
 
-	mutex_lock(&misc_mtx);
+	guard(mutex)(&misc_mtx);
 
 	list_for_each_entry(iter, &misc_list, list) {
 		if (iter->minor != minor)
@@ -149,7 +148,7 @@ static int misc_open(struct inode *inode, struct file *file)
 			break;
 		}
 		if (!new_fops)
-			goto fail;
+			return -ENODEV;
 	}
 
 	/*
@@ -159,13 +158,11 @@ static int misc_open(struct inode *inode, struct file *file)
 	 */
 	file->private_data = c;
 
-	err = 0;
 	replace_fops(file, new_fops);
 	if (file->f_op->open)
-		err = file->f_op->open(inode, file);
-fail:
-	mutex_unlock(&misc_mtx);
-	return err;
+		return file->f_op->open(inode, file);
+
+	return 0;
 }
 
 static struct class *misc_class;
@@ -197,29 +194,24 @@ static const struct file_operations misc_fops = {
 int misc_register(struct miscdevice *misc)
 {
 	dev_t dev;
-	int err = 0;
 	bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
 
 	INIT_LIST_HEAD(&misc->list);
 
-	mutex_lock(&misc_mtx);
+	guard(mutex)(&misc_mtx);
 
 	if (is_dynamic) {
 		int i = misc_minor_alloc();
 
-		if (i < 0) {
-			err = -EBUSY;
-			goto out;
-		}
+		if (i < 0)
+			return -EBUSY;
 		misc->minor = i;
 	} else {
 		struct miscdevice *c;
 
 		list_for_each_entry(c, &misc_list, list) {
-			if (c->minor == misc->minor) {
-				err = -EBUSY;
-				goto out;
-			}
+			if (c->minor == misc->minor)
+				return -EBUSY;
 		}
 	}
 
@@ -233,8 +225,7 @@ int misc_register(struct miscdevice *misc)
 			misc_minor_free(misc->minor);
 			misc->minor = MISC_DYNAMIC_MINOR;
 		}
-		err = PTR_ERR(misc->this_device);
-		goto out;
+		return PTR_ERR(misc->this_device);
 	}
 
 	/*
@@ -242,9 +233,7 @@ int misc_register(struct miscdevice *misc)
 	 * earlier defaults
 	 */
 	list_add(&misc->list, &misc_list);
- out:
-	mutex_unlock(&misc_mtx);
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(misc_register);
 
@@ -261,11 +250,10 @@ void misc_deregister(struct miscdevice *misc)
 	if (WARN_ON(list_empty(&misc->list)))
 		return;
 
-	mutex_lock(&misc_mtx);
+	guard(mutex)(&misc_mtx);
 	list_del(&misc->list);
 	device_destroy(misc_class, MKDEV(MISC_MAJOR, misc->minor));
 	misc_minor_free(misc->minor);
-	mutex_unlock(&misc_mtx);
 }
 EXPORT_SYMBOL(misc_deregister);
 
@@ -280,29 +268,24 @@ static char *misc_devnode(const struct device *dev, umode_t *mode)
 	return NULL;
 }
 
+DEFINE_FREE(class_destroy, struct class *, if (_T) class_destroy(_T));
+DEFINE_FREE(remove_proc, struct proc_dir_entry *, if (_T) remove_proc_entry("misc", NULL));
 static int __init misc_init(void)
 {
-	int err;
-	struct proc_dir_entry *ret;
+	struct proc_dir_entry *ret __free(remove_proc) = proc_create_seq("misc", 0, NULL, &misc_seq_ops);
+	struct class *c __free(class_destroy) = class_create("misc");
 
-	ret = proc_create_seq("misc", 0, NULL, &misc_seq_ops);
-	misc_class = class_create("misc");
-	err = PTR_ERR(misc_class);
-	if (IS_ERR(misc_class))
-		goto fail_remove;
+	if (IS_ERR(c))
+		return PTR_ERR(c);
 
-	err = -EIO;
 	if (register_chrdev(MISC_MAJOR, "misc", &misc_fops))
-		goto fail_printk;
-	misc_class->devnode = misc_devnode;
-	return 0;
+		return -EIO;
 
-fail_printk:
-	pr_err("unable to get major %d for misc devices\n", MISC_MAJOR);
-	class_destroy(misc_class);
-fail_remove:
-	if (ret)
-		remove_proc_entry("misc", NULL);
-	return err;
+	c->devnode = misc_devnode;
+
+	misc_class = no_free_ptr(c);
+	no_free_ptr(ret);
+
+	return 0;
 }
 subsys_initcall(misc_init);
-- 
2.41.0