Re: [PATCH 3/5] drm: cleanup drm_core_{init,exit}()

[Daniel Vetter <daniel@ffwll.ch>]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel