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

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

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