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

[David Herrmann]

Hi

On Wed, Sep 9, 2015 at 3:11 PM, Daniel Vetter <daniel at 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 at 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