| Message ID | 1441801293-1440-4-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 09, 2015 at 02:21:31PM +0200, David Herrmann wrote: > Various cleanups to the DRM core initialization and exit handlers: > > - Register chrdev last: Once register_chrdev() returns, open() will > succeed on the given chrdevs. This is usually not an issue, as no > chardevs are registered, yet. However, nodes can be created by > user-space via mknod(2), even though such major/minor combinations are > unknown to the kernel. Avoid calling into drm_stub_open() in those > cases. > Again, drm_stub_open() would just bail out as the inode is unknown, > but it's really non-obvious if you hack on drm_stub_open(). > > - Unify error-paths into just one label. All the error-path helpers can > be called even though the constructors were not called yet, or failed. > Hence, just call all cleanups unconditionally. > > - Call into drm_global_release(). This is a no-op, but provides > debugging helpers in case there're GLOBALS left on module unload. This > function was unused until now. > > - Use DRM_ERROR() instead of printk(), and also print the error-code on > failure (even if it is static!). > > - Don't throw away error-codes of register_chrdev()! > > - Don't hardcode -1 as errno. This is just plain wrong. > > - Order exit-handlers in the exact reverse order of initialization > (except if the order actually matters for syncing-reasons, which is > not the case here, though). > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/gpu/drm/drm_drv.c | 36 ++++++++++++++++++------------------ > 1 file changed, 18 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 58299f7..bf2924b 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -829,50 +829,50 @@ static const struct file_operations drm_stub_fops = { > > static int __init drm_core_init(void) > { > - int ret = -ENOMEM; > + int ret; > > drm_global_init(); > drm_connector_ida_init(); > idr_init(&drm_minors_idr); > > - if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops)) > - goto err_p1; > - > ret = drm_sysfs_init(); > if (ret < 0) { > - printk(KERN_ERR "DRM: Error creating drm class.\n"); > - goto err_p2; > + DRM_ERROR("Cannot create DRM class: %d\n", ret); > + goto error; > } > > drm_debugfs_root = debugfs_create_dir("dri", NULL); > if (!drm_debugfs_root) { > - DRM_ERROR("Cannot create /sys/kernel/debug/dri\n"); > - ret = -1; > - goto err_p3; > + ret = -ENOMEM; > + DRM_ERROR("Cannot create debugfs-root: %d\n", ret); > + goto error; > } > > + ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops); > + if (ret < 0) > + goto error; > + > DRM_INFO("Initialized %s %d.%d.%d %s\n", > CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE); > return 0; > -err_p3: > - drm_sysfs_destroy(); > -err_p2: > - unregister_chrdev(DRM_MAJOR, "drm"); > > +error: > + debugfs_remove(drm_debugfs_root); > + drm_sysfs_destroy(); > idr_destroy(&drm_minors_idr); > -err_p1: > + drm_connector_ida_destroy(); > + drm_global_release(); > return ret; Since unregister_chardev can cope if we haven't registered the range yet, can't we just call drm_core_exit here for simplicity and robustness? -Daniel > } > > static void __exit drm_core_exit(void) > { > + unregister_chrdev(DRM_MAJOR, "drm"); > debugfs_remove(drm_debugfs_root); > drm_sysfs_destroy(); > - > - unregister_chrdev(DRM_MAJOR, "drm"); > - > - drm_connector_ida_destroy(); > idr_destroy(&drm_minors_idr); > + drm_connector_ida_destroy(); > + drm_global_release(); > } > > module_init(drm_core_init); > -- > 2.5.1 >
Hi On Wed, Sep 9, 2015 at 3:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Sep 09, 2015 at 02:21:31PM +0200, David Herrmann wrote: >> Various cleanups to the DRM core initialization and exit handlers: >> >> - Register chrdev last: Once register_chrdev() returns, open() will >> succeed on the given chrdevs. This is usually not an issue, as no >> chardevs are registered, yet. However, nodes can be created by >> user-space via mknod(2), even though such major/minor combinations are >> unknown to the kernel. Avoid calling into drm_stub_open() in those >> cases. >> Again, drm_stub_open() would just bail out as the inode is unknown, >> but it's really non-obvious if you hack on drm_stub_open(). >> >> - Unify error-paths into just one label. All the error-path helpers can >> be called even though the constructors were not called yet, or failed. >> Hence, just call all cleanups unconditionally. >> >> - Call into drm_global_release(). This is a no-op, but provides >> debugging helpers in case there're GLOBALS left on module unload. This >> function was unused until now. >> >> - Use DRM_ERROR() instead of printk(), and also print the error-code on >> failure (even if it is static!). >> >> - Don't throw away error-codes of register_chrdev()! >> >> - Don't hardcode -1 as errno. This is just plain wrong. >> >> - Order exit-handlers in the exact reverse order of initialization >> (except if the order actually matters for syncing-reasons, which is >> not the case here, though). >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> --- >> drivers/gpu/drm/drm_drv.c | 36 ++++++++++++++++++------------------ >> 1 file changed, 18 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index 58299f7..bf2924b 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -829,50 +829,50 @@ static const struct file_operations drm_stub_fops = { >> >> static int __init drm_core_init(void) >> { >> - int ret = -ENOMEM; >> + int ret; >> >> drm_global_init(); >> drm_connector_ida_init(); >> idr_init(&drm_minors_idr); >> >> - if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops)) >> - goto err_p1; >> - >> ret = drm_sysfs_init(); >> if (ret < 0) { >> - printk(KERN_ERR "DRM: Error creating drm class.\n"); >> - goto err_p2; >> + DRM_ERROR("Cannot create DRM class: %d\n", ret); >> + goto error; >> } >> >> drm_debugfs_root = debugfs_create_dir("dri", NULL); >> if (!drm_debugfs_root) { >> - DRM_ERROR("Cannot create /sys/kernel/debug/dri\n"); >> - ret = -1; >> - goto err_p3; >> + ret = -ENOMEM; >> + DRM_ERROR("Cannot create debugfs-root: %d\n", ret); >> + goto error; >> } >> >> + ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops); >> + if (ret < 0) >> + goto error; >> + >> DRM_INFO("Initialized %s %d.%d.%d %s\n", >> CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE); >> return 0; >> -err_p3: >> - drm_sysfs_destroy(); >> -err_p2: >> - unregister_chrdev(DRM_MAJOR, "drm"); >> >> +error: >> + debugfs_remove(drm_debugfs_root); >> + drm_sysfs_destroy(); >> idr_destroy(&drm_minors_idr); >> -err_p1: >> + drm_connector_ida_destroy(); >> + drm_global_release(); >> return ret; > > Since unregister_chardev can cope if we haven't registered the range yet, > can't we just call drm_core_exit here for simplicity and robustness? Unlike the other cleanups, unregister_chrdev() deals with global resources. So if some other module had those majors registered, we'd 'steal' them here. Unfortunately, register_chrdev() doesn't return any pointer to us, which we could use to pass to unregister_chrdev().. I don't mind changing it, your call. Thanks David
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 58299f7..bf2924b 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -829,50 +829,50 @@ static const struct file_operations drm_stub_fops = { static int __init drm_core_init(void) { - int ret = -ENOMEM; + int ret; drm_global_init(); drm_connector_ida_init(); idr_init(&drm_minors_idr); - if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops)) - goto err_p1; - ret = drm_sysfs_init(); if (ret < 0) { - printk(KERN_ERR "DRM: Error creating drm class.\n"); - goto err_p2; + DRM_ERROR("Cannot create DRM class: %d\n", ret); + goto error; } drm_debugfs_root = debugfs_create_dir("dri", NULL); if (!drm_debugfs_root) { - DRM_ERROR("Cannot create /sys/kernel/debug/dri\n"); - ret = -1; - goto err_p3; + ret = -ENOMEM; + DRM_ERROR("Cannot create debugfs-root: %d\n", ret); + goto error; } + ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops); + if (ret < 0) + goto error; + DRM_INFO("Initialized %s %d.%d.%d %s\n", CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE); return 0; -err_p3: - drm_sysfs_destroy(); -err_p2: - unregister_chrdev(DRM_MAJOR, "drm"); +error: + debugfs_remove(drm_debugfs_root); + drm_sysfs_destroy(); idr_destroy(&drm_minors_idr); -err_p1: + drm_connector_ida_destroy(); + drm_global_release(); return ret; } static void __exit drm_core_exit(void) { + unregister_chrdev(DRM_MAJOR, "drm"); debugfs_remove(drm_debugfs_root); drm_sysfs_destroy(); - - unregister_chrdev(DRM_MAJOR, "drm"); - - drm_connector_ida_destroy(); idr_destroy(&drm_minors_idr); + drm_connector_ida_destroy(); + drm_global_release(); } module_init(drm_core_init);
Various cleanups to the DRM core initialization and exit handlers: - Register chrdev last: Once register_chrdev() returns, open() will succeed on the given chrdevs. This is usually not an issue, as no chardevs are registered, yet. However, nodes can be created by user-space via mknod(2), even though such major/minor combinations are unknown to the kernel. Avoid calling into drm_stub_open() in those cases. Again, drm_stub_open() would just bail out as the inode is unknown, but it's really non-obvious if you hack on drm_stub_open(). - Unify error-paths into just one label. All the error-path helpers can be called even though the constructors were not called yet, or failed. Hence, just call all cleanups unconditionally. - Call into drm_global_release(). This is a no-op, but provides debugging helpers in case there're GLOBALS left on module unload. This function was unused until now. - Use DRM_ERROR() instead of printk(), and also print the error-code on failure (even if it is static!). - Don't throw away error-codes of register_chrdev()! - Don't hardcode -1 as errno. This is just plain wrong. - Order exit-handlers in the exact reverse order of initialization (except if the order actually matters for syncing-reasons, which is not the case here, though). Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_drv.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)