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
