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@xxxxxxxxx>
> ---
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
