[PATCH 0/5] DRM Cleanups

[PATCH 0/5] DRM Cleanups

[David Herrmann]

Hey

Just a set of 5 random cleanup patches for DRM-core: Simplifying
drm_core_init/exit(), dropping drm_core.h and fixing the mode-object ID
allocations.

Thanks
David

David Herrmann (5):
  drm: simplify drm_sysfs_destroy() via IS_ERR_OR_NULL()
  drm: move drm_class into drm_sysfs.c
  drm: cleanup drm_core_{init,exit}()
  drm: drop obsolete drm_core.h
  drm: allocate kernel mode-object IDs in cyclic fashion

 drivers/gpu/drm/drm_crtc.c     | 37 +++++++++++++++++++++++----
 drivers/gpu/drm/drm_drv.c      | 50 +++++++++++++++++-------------------
 drivers/gpu/drm/drm_internal.h |  5 +++-
 drivers/gpu/drm/drm_ioc32.c    |  1 -
 drivers/gpu/drm/drm_ioctl.c    |  1 -
 drivers/gpu/drm/drm_sysfs.c    | 57 +++++++++++++++++-------------------------
 include/drm/drm_core.h         | 34 -------------------------
 7 files changed, 82 insertions(+), 103 deletions(-)
 delete mode 100644 include/drm/drm_core.h

-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm: simplify drm_sysfs_destroy() via IS_ERR_OR_NULL()

[David Herrmann]

Simplify `foo == NULL || IS_ERR(foo)` via IS_ERR_OR_NULL(). This is
pretty commonly used all over the kernel, especially for debugfs/sysfs
cleanup paths.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 0f6cd33..3f66cb0 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -156,7 +156,7 @@ err_out:
  */
 void drm_sysfs_destroy(void)
 {
-	if ((drm_class == NULL) || (IS_ERR(drm_class)))
+	if (IS_ERR_OR_NULL(drm_class))
 		return;
 	class_remove_file(drm_class, &class_attr_version.attr);
 	class_destroy(drm_class);
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm: move drm_class into drm_sysfs.c

[David Herrmann]

Right now, drm_sysfs_create() returns the newly allocated "struct class"
to the caller (which is drm_core_init()), which then has to set the
global variable 'drm_class'. During cleanup, though, we call
drm_sysfs_destroy() which implicitly uses the global 'drm_class'. This is
confusing, as ownership of the global 'drm_class' is non-obvious.

This patch changes drm_sysfs_create() to drm_sysfs_init() and makes it
initialize the 'drm_class' object directly, rather than returning it.
This way, both drm_sysfs_init() and drm_sysfs_destroy() work in a similar
fashion and manage the global drm class.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_drv.c      |  6 ++----
 drivers/gpu/drm/drm_internal.h |  2 +-
 drivers/gpu/drm/drm_sysfs.c    | 47 +++++++++++++++++++-----------------------
 3 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 53d09a1..58299f7 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -55,7 +55,6 @@ module_param_named(debug, drm_debug, int, 0600);
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
 
-struct class *drm_class;
 static struct dentry *drm_debugfs_root;
 
 void drm_err(const char *format, ...)
@@ -839,10 +838,9 @@ static int __init drm_core_init(void)
 	if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops))
 		goto err_p1;
 
-	drm_class = drm_sysfs_create(THIS_MODULE, "drm");
-	if (IS_ERR(drm_class)) {
+	ret = drm_sysfs_init();
+	if (ret < 0) {
 		printk(KERN_ERR "DRM: Error creating drm class.\n");
-		ret = PTR_ERR(drm_class);
 		goto err_p2;
 	}
 
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 059af01..43cbda3 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -73,7 +73,7 @@ int drm_authmagic(struct drm_device *dev, void *data,
 /* drm_sysfs.c */
 extern struct class *drm_class;
 
-struct class *drm_sysfs_create(struct module *owner, char *name);
+int drm_sysfs_init(void);
 void drm_sysfs_destroy(void);
 struct device *drm_sysfs_minor_alloc(struct drm_minor *minor);
 int drm_sysfs_connector_add(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 3f66cb0..f08873f 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -30,6 +30,8 @@ static struct device_type drm_sysfs_device_minor = {
 	.name = "drm_minor"
 };
 
+struct class *drm_class;
+
 /**
  * __drm_class_suspend - internal DRM class suspend routine
  * @dev: Linux device to suspend
@@ -112,41 +114,34 @@ static CLASS_ATTR_STRING(version, S_IRUGO,
 		CORE_DATE);
 
 /**
- * drm_sysfs_create - create a struct drm_sysfs_class structure
- * @owner: pointer to the module that is to "own" this struct drm_sysfs_class
- * @name: pointer to a string for the name of this class.
+ * drm_sysfs_init - initialize sysfs helpers
+ *
+ * This is used to create the DRM class, which is the implicit parent of any
+ * other top-level DRM sysfs objects.
  *
- * This is used to create DRM class pointer that can then be used
- * in calls to drm_sysfs_device_add().
+ * You must call drm_sysfs_destroy() to release the allocated resources.
  *
- * Note, the pointer created here is to be destroyed when finished by making a
- * call to drm_sysfs_destroy().
+ * Return: 0 on success, negative error code on failure.
  */
-struct class *drm_sysfs_create(struct module *owner, char *name)
+int drm_sysfs_init(void)
 {
-	struct class *class;
 	int err;
 
-	class = class_create(owner, name);
-	if (IS_ERR(class)) {
-		err = PTR_ERR(class);
-		goto err_out;
-	}
-
-	class->pm = &drm_class_dev_pm_ops;
+	drm_class = class_create(THIS_MODULE, "drm");
+	if (IS_ERR(drm_class))
+		return PTR_ERR(drm_class);
 
-	err = class_create_file(class, &class_attr_version.attr);
-	if (err)
-		goto err_out_class;
+	drm_class->pm = &drm_class_dev_pm_ops;
 
-	class->devnode = drm_devnode;
-
-	return class;
+	err = class_create_file(drm_class, &class_attr_version.attr);
+	if (err) {
+		class_destroy(drm_class);
+		drm_class = NULL;
+		return err;
+	}
 
-err_out_class:
-	class_destroy(class);
-err_out:
-	return ERR_PTR(err);
+	drm_class->devnode = drm_devnode;
+	return 0;
 }
 
 /**
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: move drm_class into drm_sysfs.c

[Daniel Vetter]

On Wed, Sep 09, 2015 at 02:21:30PM +0200, David Herrmann wrote:
> Right now, drm_sysfs_create() returns the newly allocated "struct class"
> to the caller (which is drm_core_init()), which then has to set the
> global variable 'drm_class'. During cleanup, though, we call
> drm_sysfs_destroy() which implicitly uses the global 'drm_class'. This is
> confusing, as ownership of the global 'drm_class' is non-obvious.
> 
> This patch changes drm_sysfs_create() to drm_sysfs_init() and makes it
> initialize the 'drm_class' object directly, rather than returning it.
> This way, both drm_sysfs_init() and drm_sysfs_destroy() work in a similar
> fashion and manage the global drm class.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

First 2 patches applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_drv.c      |  6 ++----
>  drivers/gpu/drm/drm_internal.h |  2 +-
>  drivers/gpu/drm/drm_sysfs.c    | 47 +++++++++++++++++++-----------------------
>  3 files changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 53d09a1..58299f7 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -55,7 +55,6 @@ module_param_named(debug, drm_debug, int, 0600);
>  static DEFINE_SPINLOCK(drm_minor_lock);
>  static struct idr drm_minors_idr;
>  
> -struct class *drm_class;
>  static struct dentry *drm_debugfs_root;
>  
>  void drm_err(const char *format, ...)
> @@ -839,10 +838,9 @@ static int __init drm_core_init(void)
>  	if (register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops))
>  		goto err_p1;
>  
> -	drm_class = drm_sysfs_create(THIS_MODULE, "drm");
> -	if (IS_ERR(drm_class)) {
> +	ret = drm_sysfs_init();
> +	if (ret < 0) {
>  		printk(KERN_ERR "DRM: Error creating drm class.\n");
> -		ret = PTR_ERR(drm_class);
>  		goto err_p2;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 059af01..43cbda3 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -73,7 +73,7 @@ int drm_authmagic(struct drm_device *dev, void *data,
>  /* drm_sysfs.c */
>  extern struct class *drm_class;
>  
> -struct class *drm_sysfs_create(struct module *owner, char *name);
> +int drm_sysfs_init(void);
>  void drm_sysfs_destroy(void);
>  struct device *drm_sysfs_minor_alloc(struct drm_minor *minor);
>  int drm_sysfs_connector_add(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 3f66cb0..f08873f 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -30,6 +30,8 @@ static struct device_type drm_sysfs_device_minor = {
>  	.name = "drm_minor"
>  };
>  
> +struct class *drm_class;
> +
>  /**
>   * __drm_class_suspend - internal DRM class suspend routine
>   * @dev: Linux device to suspend
> @@ -112,41 +114,34 @@ static CLASS_ATTR_STRING(version, S_IRUGO,
>  		CORE_DATE);
>  
>  /**
> - * drm_sysfs_create - create a struct drm_sysfs_class structure
> - * @owner: pointer to the module that is to "own" this struct drm_sysfs_class
> - * @name: pointer to a string for the name of this class.
> + * drm_sysfs_init - initialize sysfs helpers
> + *
> + * This is used to create the DRM class, which is the implicit parent of any
> + * other top-level DRM sysfs objects.
>   *
> - * This is used to create DRM class pointer that can then be used
> - * in calls to drm_sysfs_device_add().
> + * You must call drm_sysfs_destroy() to release the allocated resources.
>   *
> - * Note, the pointer created here is to be destroyed when finished by making a
> - * call to drm_sysfs_destroy().
> + * Return: 0 on success, negative error code on failure.
>   */
> -struct class *drm_sysfs_create(struct module *owner, char *name)
> +int drm_sysfs_init(void)
>  {
> -	struct class *class;
>  	int err;
>  
> -	class = class_create(owner, name);
> -	if (IS_ERR(class)) {
> -		err = PTR_ERR(class);
> -		goto err_out;
> -	}
> -
> -	class->pm = &drm_class_dev_pm_ops;
> +	drm_class = class_create(THIS_MODULE, "drm");
> +	if (IS_ERR(drm_class))
> +		return PTR_ERR(drm_class);
>  
> -	err = class_create_file(class, &class_attr_version.attr);
> -	if (err)
> -		goto err_out_class;
> +	drm_class->pm = &drm_class_dev_pm_ops;
>  
> -	class->devnode = drm_devnode;
> -
> -	return class;
> +	err = class_create_file(drm_class, &class_attr_version.attr);
> +	if (err) {
> +		class_destroy(drm_class);
> +		drm_class = NULL;
> +		return err;
> +	}
>  
> -err_out_class:
> -	class_destroy(class);
> -err_out:
> -	return ERR_PTR(err);
> +	drm_class->devnode = drm_devnode;
> +	return 0;
>  }
>  
>  /**
> -- 
> 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

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

[David Herrmann]

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;
 }
 
 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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

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

[David Herrmann]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm: drop obsolete drm_core.h

[David Herrmann]

The drm_core.h header contains a set of constants meant to be used
throughout DRM. However, as it turns out, they're each used just once and
don't bring any benefit. They're also grossly mis-named and lack
name-spacing. This patch inlines them, or moves them into drm_internal.h
as appropriate:

 - CORE_AUTHOR and CORE_DESC are inlined into corresponding MODULE_*()
   macros. It's just confusing having to follow 2 pointers when trying to
   find the definition of these fields. Grep'ping for MODULE_AUTHOR()
   should reveal the full information, if there's no strong reason not to.

 - CORE_NAME, CORE_DATE, CORE_MAJOR, CORE_MINOR, and CORE_PATCHLEVEL are
   inlined into the sysfs 'version' attribute. They're stripped
   everywhere else (which is just some printk() statements). CORE_NAME
   just doesn't make *any* sense, as we hard-code it in many places,
   anyway. The other constants are outdated and just serve
   binary-compatibility purposes. Hence, inline them in 'version' sysfs
   attribute (we might even try dropping it..).

 - DRM_IF_MAJOR and DRM_IF_MINOR are moved into drm_internal.h as they're
   only used by the global ioctl handlers. Furthermore, versioning
   interfaces breaks backports and as such is deprecated, anyway. We just
   keep them for historic reasons. I doubt anyone will ever modify them
   again.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_drv.c      |  8 +++-----
 drivers/gpu/drm/drm_internal.h |  3 +++
 drivers/gpu/drm/drm_ioc32.c    |  1 -
 drivers/gpu/drm/drm_ioctl.c    |  1 -
 drivers/gpu/drm/drm_sysfs.c    |  8 +-------
 include/drm/drm_core.h         | 34 ----------------------------------
 6 files changed, 7 insertions(+), 48 deletions(-)
 delete mode 100644 include/drm/drm_core.h

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index bf2924b..d16aec1 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -33,7 +33,6 @@
 #include <linux/mount.h>
 #include <linux/slab.h>
 #include <drm/drmP.h>
-#include <drm/drm_core.h>
 #include "drm_legacy.h"
 #include "drm_internal.h"
 
@@ -42,8 +41,8 @@ EXPORT_SYMBOL(drm_debug);
 
 bool drm_atomic = 0;
 
-MODULE_AUTHOR(CORE_AUTHOR);
-MODULE_DESCRIPTION(CORE_DESC);
+MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon Smirl");
+MODULE_DESCRIPTION("DRM shared core routines");
 MODULE_LICENSE("GPL and additional rights");
 MODULE_PARM_DESC(debug, "Enable debug output");
 MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs] (0: never disable, <0: disable immediately)");
@@ -852,8 +851,7 @@ static int __init drm_core_init(void)
 	if (ret < 0)
 		goto error;
 
-	DRM_INFO("Initialized %s %d.%d.%d %s\n",
-		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
+	DRM_INFO("Initialized\n");
 	return 0;
 
 error:
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 43cbda3..6c4c3d4 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -21,6 +21,9 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#define DRM_IF_MAJOR 1
+#define DRM_IF_MINOR 4
+
 /* drm_irq.c */
 extern unsigned int drm_timestamp_monotonic;
 
diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
index ddfa601..0538b87 100644
--- a/drivers/gpu/drm/drm_ioc32.c
+++ b/drivers/gpu/drm/drm_ioc32.c
@@ -32,7 +32,6 @@
 #include <linux/export.h>
 
 #include <drm/drmP.h>
-#include <drm/drm_core.h>
 
 #define DRM_IOCTL_VERSION32		DRM_IOWR(0x00, drm_version32_t)
 #define DRM_IOCTL_GET_UNIQUE32		DRM_IOWR(0x01, drm_unique32_t)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 9a860ca..7436fc5 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -29,7 +29,6 @@
  */
 
 #include <drm/drmP.h>
-#include <drm/drm_core.h>
 #include "drm_legacy.h"
 #include "drm_internal.h"
 #include "drm_crtc_internal.h"
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index f08873f..865a49c 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -19,7 +19,6 @@
 #include <linux/export.h>
 
 #include <drm/drm_sysfs.h>
-#include <drm/drm_core.h>
 #include <drm/drmP.h>
 #include "drm_internal.h"
 
@@ -106,12 +105,7 @@ static char *drm_devnode(struct device *dev, umode_t *mode)
 	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
 }
 
-static CLASS_ATTR_STRING(version, S_IRUGO,
-		CORE_NAME " "
-		__stringify(CORE_MAJOR) "."
-		__stringify(CORE_MINOR) "."
-		__stringify(CORE_PATCHLEVEL) " "
-		CORE_DATE);
+static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
 
 /**
  * drm_sysfs_init - initialize sysfs helpers
diff --git a/include/drm/drm_core.h b/include/drm/drm_core.h
deleted file mode 100644
index 4e75238..0000000
--- a/include/drm/drm_core.h
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * Copyright 2004 Jon Smirl <jonsmirl@gmail.com>
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sub license,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the
- * next paragraph) shall be included in all copies or substantial portions
- * of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
- * VIA, S3 GRAPHICS, AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
- * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- */
-#define CORE_AUTHOR		"Gareth Hughes, Leif Delgass, José Fonseca, Jon Smirl"
-
-#define CORE_NAME		"drm"
-#define CORE_DESC		"DRM shared core routines"
-#define CORE_DATE		"20060810"
-
-#define DRM_IF_MAJOR	1
-#define DRM_IF_MINOR	4
-
-#define CORE_MAJOR	1
-#define CORE_MINOR	1
-#define CORE_PATCHLEVEL 0
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm: allocate kernel mode-object IDs in cyclic fashion

[David Herrmann]

Now that we support connector hotplugging, user-space might see
mode-object IDs coming and going asynchronously. Therefore, we must make
sure to not re-use object IDs, so to not confuse user-space and introduce
races. Therefore, all kernel-allocated objects will no longer recycle
IDs. Instead, we use the cyclic idr allocator (which performs still fine
for reasonable allocation schemes).

However, for user-space allocated objects like framebuffers, we don't
want to risk allowing malicious users to screw with the ID space.
Furthermore, those objects happen to not be subject to ID hotplug races,
as they're allocated and freed explicitly. Hence, we still recycle IDs
for these objects (which are just framebuffers so far).

For atomic-modesetting, objects are looked up by the kernel without
specifying an object-type. Hence, we have a theoretical race where a
framebuffer recycles a previous connector ID. However, user-allocated
objects are never returned by drm_mode_object_find() (as they need
separate ref-count handling), so this race cannot happen with the
currently available objects. Even if we add ref-counting to other
objects, all we need to make sure is to never lookup user-controlled
objects with the same function as kernel-controlled objects, but not
specifying the type. This sounds highly unlikely to happen ever, so we
should be safe, anyway.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 33d877c..fd8a2e2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -269,18 +269,42 @@ const char *drm_get_format_name(uint32_t format)
 EXPORT_SYMBOL(drm_get_format_name);
 
 /*
+ * Flags for drm_mode_object_get_reg():
+ * DRM_MODE_OBJECT_ID_UNLINKED:	Allocate the object ID, but do not store the
+ * 				object pointer. Hence, the object is not
+ * 				registered but needs to be inserted manually.
+ * 				This must be used for hotplugged objects.
+ * DRM_MODE_OBJECT_ID_RECYCLE:	Allow recycling previously allocated IDs. If
+ * 				not set, the IDs space is allocated in a cyclic
+ * 				fashion. This should be the default for all
+ * 				kernel allocated objects, to not confuse
+ * 				user-space on hotplug. This must not be used
+ * 				for user-allocated objects, though.
+ */
+enum {
+	DRM_MODE_OBJECT_ID_UNLINKED		= (1U << 0),
+	DRM_MODE_OBJECT_ID_RECYCLE			= (1U << 1),
+};
+
+/*
  * Internal function to assign a slot in the object idr and optionally
  * register the object into the idr.
  */
 static int drm_mode_object_get_reg(struct drm_device *dev,
 				   struct drm_mode_object *obj,
 				   uint32_t obj_type,
-				   bool register_obj)
+				   unsigned int flags)
 {
+	void *ptr = (flags & DRM_MODE_OBJECT_ID_UNLINKED) ? NULL : obj;
 	int ret;
 
 	mutex_lock(&dev->mode_config.idr_mutex);
-	ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_KERNEL);
+	if (flags & DRM_MODE_OBJECT_ID_RECYCLE)
+		ret = idr_alloc(&dev->mode_config.crtc_idr, ptr, 1, 0,
+				GFP_KERNEL);
+	else
+		ret = idr_alloc_cyclic(&dev->mode_config.crtc_idr, ptr, 1, 0,
+				       GFP_KERNEL);
 	if (ret >= 0) {
 		/*
 		 * Set up the object linking under the protection of the idr
@@ -312,7 +336,7 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
 int drm_mode_object_get(struct drm_device *dev,
 			struct drm_mode_object *obj, uint32_t obj_type)
 {
-	return drm_mode_object_get_reg(dev, obj, obj_type, true);
+	return drm_mode_object_get_reg(dev, obj, obj_type, 0);
 }
 
 static void drm_mode_object_register(struct drm_device *dev,
@@ -414,7 +438,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	fb->dev = dev;
 	fb->funcs = funcs;
 
-	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
+	ret = drm_mode_object_get_reg(dev, &fb->base, DRM_MODE_OBJECT_FB,
+				      DRM_MODE_OBJECT_ID_RECYCLE);
 	if (ret)
 		goto out;
 
@@ -872,7 +897,9 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_modeset_lock_all(dev);
 
-	ret = drm_mode_object_get_reg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, false);
+	ret = drm_mode_object_get_reg(dev, &connector->base,
+				      DRM_MODE_OBJECT_CONNECTOR,
+				      DRM_MODE_OBJECT_ID_UNLINKED);
 	if (ret)
 		goto out_unlock;
 
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm: allocate kernel mode-object IDs in cyclic fashion

[Daniel Vetter]

On Wed, Sep 09, 2015 at 02:21:33PM +0200, David Herrmann wrote:
> Now that we support connector hotplugging, user-space might see
> mode-object IDs coming and going asynchronously. Therefore, we must make
> sure to not re-use object IDs, so to not confuse user-space and introduce
> races. Therefore, all kernel-allocated objects will no longer recycle
> IDs. Instead, we use the cyclic idr allocator (which performs still fine
> for reasonable allocation schemes).
> 
> However, for user-space allocated objects like framebuffers, we don't
> want to risk allowing malicious users to screw with the ID space.
> Furthermore, those objects happen to not be subject to ID hotplug races,
> as they're allocated and freed explicitly. Hence, we still recycle IDs
> for these objects (which are just framebuffers so far).

Since atomic also blob properties can be created by userspace. And there
has actually been trouble once with some attempt in SNA to cache the edid
blob without realizing that they get reused. So working userspace _always_
has to re-read the blob prop to make sure it has the current one anyway.
-Daniel

> 
> For atomic-modesetting, objects are looked up by the kernel without
> specifying an object-type. Hence, we have a theoretical race where a
> framebuffer recycles a previous connector ID. However, user-allocated
> objects are never returned by drm_mode_object_find() (as they need
> separate ref-count handling), so this race cannot happen with the
> currently available objects. Even if we add ref-counting to other
> objects, all we need to make sure is to never lookup user-controlled
> objects with the same function as kernel-controlled objects, but not
> specifying the type. This sounds highly unlikely to happen ever, so we
> should be safe, anyway.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 33d877c..fd8a2e2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -269,18 +269,42 @@ const char *drm_get_format_name(uint32_t format)
>  EXPORT_SYMBOL(drm_get_format_name);
>  
>  /*
> + * Flags for drm_mode_object_get_reg():
> + * DRM_MODE_OBJECT_ID_UNLINKED:	Allocate the object ID, but do not store the
> + * 				object pointer. Hence, the object is not
> + * 				registered but needs to be inserted manually.
> + * 				This must be used for hotplugged objects.
> + * DRM_MODE_OBJECT_ID_RECYCLE:	Allow recycling previously allocated IDs. If
> + * 				not set, the IDs space is allocated in a cyclic
> + * 				fashion. This should be the default for all
> + * 				kernel allocated objects, to not confuse
> + * 				user-space on hotplug. This must not be used
> + * 				for user-allocated objects, though.
> + */
> +enum {
> +	DRM_MODE_OBJECT_ID_UNLINKED		= (1U << 0),
> +	DRM_MODE_OBJECT_ID_RECYCLE			= (1U << 1),
> +};
> +
> +/*
>   * Internal function to assign a slot in the object idr and optionally
>   * register the object into the idr.
>   */
>  static int drm_mode_object_get_reg(struct drm_device *dev,
>  				   struct drm_mode_object *obj,
>  				   uint32_t obj_type,
> -				   bool register_obj)
> +				   unsigned int flags)
>  {
> +	void *ptr = (flags & DRM_MODE_OBJECT_ID_UNLINKED) ? NULL : obj;
>  	int ret;
>  
>  	mutex_lock(&dev->mode_config.idr_mutex);
> -	ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_KERNEL);
> +	if (flags & DRM_MODE_OBJECT_ID_RECYCLE)
> +		ret = idr_alloc(&dev->mode_config.crtc_idr, ptr, 1, 0,
> +				GFP_KERNEL);
> +	else
> +		ret = idr_alloc_cyclic(&dev->mode_config.crtc_idr, ptr, 1, 0,
> +				       GFP_KERNEL);
>  	if (ret >= 0) {
>  		/*
>  		 * Set up the object linking under the protection of the idr
> @@ -312,7 +336,7 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
>  int drm_mode_object_get(struct drm_device *dev,
>  			struct drm_mode_object *obj, uint32_t obj_type)
>  {
> -	return drm_mode_object_get_reg(dev, obj, obj_type, true);
> +	return drm_mode_object_get_reg(dev, obj, obj_type, 0);
>  }
>  
>  static void drm_mode_object_register(struct drm_device *dev,
> @@ -414,7 +438,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  	fb->dev = dev;
>  	fb->funcs = funcs;
>  
> -	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
> +	ret = drm_mode_object_get_reg(dev, &fb->base, DRM_MODE_OBJECT_FB,
> +				      DRM_MODE_OBJECT_ID_RECYCLE);
>  	if (ret)
>  		goto out;
>  
> @@ -872,7 +897,9 @@ int drm_connector_init(struct drm_device *dev,
>  
>  	drm_modeset_lock_all(dev);
>  
> -	ret = drm_mode_object_get_reg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, false);
> +	ret = drm_mode_object_get_reg(dev, &connector->base,
> +				      DRM_MODE_OBJECT_CONNECTOR,
> +				      DRM_MODE_OBJECT_ID_UNLINKED);
>  	if (ret)
>  		goto out_unlock;
>  
> -- 
> 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

Re: [PATCH 5/5] drm: allocate kernel mode-object IDs in cyclic fashion

[David Herrmann]

Hi

On Wed, Sep 9, 2015 at 3:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 09, 2015 at 02:21:33PM +0200, David Herrmann wrote:
>> Now that we support connector hotplugging, user-space might see
>> mode-object IDs coming and going asynchronously. Therefore, we must make
>> sure to not re-use object IDs, so to not confuse user-space and introduce
>> races. Therefore, all kernel-allocated objects will no longer recycle
>> IDs. Instead, we use the cyclic idr allocator (which performs still fine
>> for reasonable allocation schemes).
>>
>> However, for user-space allocated objects like framebuffers, we don't
>> want to risk allowing malicious users to screw with the ID space.
>> Furthermore, those objects happen to not be subject to ID hotplug races,
>> as they're allocated and freed explicitly. Hence, we still recycle IDs
>> for these objects (which are just framebuffers so far).
>
> Since atomic also blob properties can be created by userspace. And there
> has actually been trouble once with some attempt in SNA to cache the edid
> blob without realizing that they get reused. So working userspace _always_
> has to re-read the blob prop to make sure it has the current one anyway.

I see. So lets include the blobs in recycle, too? This makes the patch
a bit more clunky as we need to distinguish kernel-generated blobs and
user-space generated blobs. I'll see how that works out.

Thanks
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm: allocate kernel mode-object IDs in cyclic fashion

[Daniel Vetter]

On Fri, Sep 11, 2015 at 11:47 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 3:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Sep 09, 2015 at 02:21:33PM +0200, David Herrmann wrote:
>>> Now that we support connector hotplugging, user-space might see
>>> mode-object IDs coming and going asynchronously. Therefore, we must make
>>> sure to not re-use object IDs, so to not confuse user-space and introduce
>>> races. Therefore, all kernel-allocated objects will no longer recycle
>>> IDs. Instead, we use the cyclic idr allocator (which performs still fine
>>> for reasonable allocation schemes).
>>>
>>> However, for user-space allocated objects like framebuffers, we don't
>>> want to risk allowing malicious users to screw with the ID space.
>>> Furthermore, those objects happen to not be subject to ID hotplug races,
>>> as they're allocated and freed explicitly. Hence, we still recycle IDs
>>> for these objects (which are just framebuffers so far).
>>
>> Since atomic also blob properties can be created by userspace. And there
>> has actually been trouble once with some attempt in SNA to cache the edid
>> blob without realizing that they get reused. So working userspace _always_
>> has to re-read the blob prop to make sure it has the current one anyway.
>
> I see. So lets include the blobs in recycle, too? This makes the patch
> a bit more clunky as we need to distinguish kernel-generated blobs and
> user-space generated blobs. I'll see how that works out.

What I meant to say is that userspace already must be able to cope
with kernel-generated edid blobs recycling ids. Which means you
actually don't need to make this distinction. We just need to add
blobs to the type of objects where we should recycle. Aside from that
we also have kernel-generated framebuffers (e.g. fbdev or takeover
from bios or universal cursors) with the same problems of potentially
confusing userspace. So blobs aren't really any different than
framebuffers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel