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

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