Hi
On Wed, Sep 9, 2015 at 3:11 PM, Daniel Vetter <daniel@xxxxxxxx> 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@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?
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
