[PATCH 12/13] drm: make minor-IDs reliable

29 Jan 2014

A DRM device may (or may not!) have multiple char-devs registered. For
each char-dev type, we use a different minor-ID range. However, imagine
the following situation:
UDL device:
 - card0 as: 0
i915 device:
 - card1 as: 1
 - renderD128 as: 128
Due to dynamic bus-probing we have no guarantee of driver loading.
Therefore, in this situation, the render-node of i915 has an offset of 127
instead of 128. The UDL device does not allocate any render-node, so it
also does not allocate a render-node minor.
Hence, instead of allocating dynamic minor-IDs for each drm-minor, we now
only allocate a base. This base in then used to calculate minor-IDs for
each DRM-minor on-the-fly.
While at it, this patch also adds fine-grained locking around minor-lookup
and allocation. Instead of relying on drm_global_mutex, we now have a
separate spin-lock for minor allocation and lookup. This further reduces
the scope of the global lock so we may some day be able to have parallel
driver loading.
One short note on a semantic change: We now allocate minor-IDs during
device-allocation, not registration. This means, a lookup *could* return a
minor of an unregistered device. To avoid that, we store NULL as pointer
until the device is registered.
Device unregistration is still the same.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
---
 drivers/gpu/drm/drm_fops.c |   2 +-
 drivers/gpu/drm/drm_stub.c | 150 ++++++++++++++++++++++++++++++++-------------
 include/drm/drmP.h         |   1 +
 3 files changed, 111 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 4ce5318..fdf35cd 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -39,7 +39,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
-/* from BKL pushdown: note that nothing else serializes idr_find() */
+/* from BKL pushdown */
 DEFINE_MUTEX(drm_global_mutex);
 EXPORT_SYMBOL(drm_global_mutex);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index fa9542e..0f84bf6 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -70,6 +70,7 @@ module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
 module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600);
 module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
+static DEFINE_SPINLOCK(drm_minor_lock);
 struct idr drm_minors_idr;
struct class *drm_class;
@@ -117,26 +118,6 @@ void drm_ut_debug_printk(unsigned int request_level,
 }
 EXPORT_SYMBOL(drm_ut_debug_printk);
-static int drm_minor_get_id(struct drm_device *dev, int type)
-{
-	int ret;
-	int base = 0, limit = 63;
-
-	if (type == DRM_MINOR_CONTROL) {
-		base += 64;
-		limit = base + 63;
-	} else if (type == DRM_MINOR_RENDER) {
-		base += 128;
-		limit = base + 63;
-	}
-
-	mutex_lock(&dev->struct_mutex);
-	ret = idr_alloc(&drm_minors_idr, NULL, base, limit, GFP_KERNEL);
-	mutex_unlock(&dev->struct_mutex);
-
-	return ret == -ENOSPC ? -EINVAL : ret;
-}
-
 struct drm_master *drm_master_create(struct drm_minor *minor)
 {
    struct drm_master *master;
@@ -260,6 +241,80 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
    return 0;
 }
+/*
+ * DRM Minors
+ * A DRM device can provide several char-dev interfaces on the DRM-Major. Each
+ * of them is represented by a drm_minor object. Depending on the capabilities
+ * of the device-driver, different interfaces are registered. To guarantee that
+ * user-space can figure out char-dev relations via simple minor-calculations,
+ * we guarantee that each minor has a fixed offset to the device's minor base.
+ *
+ * dev->minor_base contains the base number for the minor. Each registered real
+ * minor adds "64 * minor" to this base. The legacy/primary minor-id is equal to
+ * the base. This allows user-space to use "primary + 128" to get the minor-ID
+ * of a possible render-node of this device. If the device has no render-node,
+ * this ID is guaranteed to stay unused.
+ *
+ * Minors can be accessed via dev->$minor_name. This pointer is either
+ * NULL or a valid drm_minor pointer and stays valid as long as the device is
+ * valid. This means, DRM minors have the same life-time as the underlying
+ * device. However, this doesn't mean that the minor is active. Minors are
+ * registered and unregistered dynamically according to device-state.
+ */
+
+static int drm_minor_alloc_base(struct drm_device *dev)
+{
+	unsigned long flags;
+	int id;
+
+	idr_preload(GFP_KERNEL);
+	spin_lock_irqsave(&drm_minor_lock, flags);
+	id = idr_alloc(&drm_minors_idr, NULL, 0, 64, GFP_NOWAIT);
+	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	idr_preload_end();
+
+	if (id < 0)
+		return id;
+
+	dev->minor_base = id;
+	return 0;
+}
+
+static void drm_minor_free_base(struct drm_device *dev)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&drm_minor_lock, flags);
+	idr_remove(&drm_minors_idr, dev->minor_base);
+	spin_unlock_irqrestore(&drm_minor_lock, flags);
+}
+
+static void drm_minor_activate_all(struct drm_device *dev)
+{
+	unsigned long flags;
+
+	/* replace NULL with @dev so lookups will succeed from now on */
+	spin_lock_irqsave(&drm_minor_lock, flags);
+	idr_replace(&drm_minors_idr, dev, dev->minor_base);
+	spin_unlock_irqrestore(&drm_minor_lock, flags);
+}
+
+static unsigned int drm_minor_type_to_id(struct drm_device *dev,
+					 unsigned int type)
+{
+	return dev->minor_base + 64 * type;
+}
+
+static int drm_minor_id_to_type(unsigned int id)
+{
+	return id / 64;
+}
+
+static unsigned int drm_minor_id_to_base(unsigned int id)
+{
+	return id % 64;
+}
+
 static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
    				     unsigned int type)
 {
@@ -284,6 +339,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
    	return -ENOMEM;
minor->type = type;
+	minor->index = drm_minor_type_to_id(dev, type);
    minor->dev = dev;
    INIT_LIST_HEAD(&minor->master_list);
@@ -306,7 +362,6 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 {
    struct drm_minor *new_minor;
    int ret;
-	int minor_id;
DRM_DEBUG("\n");
@@ -314,35 +369,23 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
    if (!new_minor)
    	return 0;
-	minor_id = drm_minor_get_id(dev, type);
-	if (minor_id < 0)
-		return minor_id;
-
-	new_minor->index = minor_id;
-
-	idr_replace(&drm_minors_idr, new_minor, minor_id);
-
-	ret = drm_debugfs_init(new_minor, minor_id, drm_debugfs_root);
+	ret = drm_debugfs_init(new_minor, new_minor->index, drm_debugfs_root);
    if (ret) {
    	DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
-		goto err_mem;
+		return ret;
    }
ret = drm_sysfs_device_add(new_minor);
    if (ret) {
-		printk(KERN_ERR
-		       "DRM: Error sysfs_device_add.\n");
+		DRM_ERROR("DRM: Error sysfs_device_add.\n");
    	goto err_debugfs;
    }
-	DRM_DEBUG("new minor assigned %d\n", minor_id);
+	DRM_DEBUG("new minor assigned %d\n", new_minor->index);
    return 0;
-
 err_debugfs:
    drm_debugfs_cleanup(new_minor);
-err_mem:
-	idr_remove(&drm_minors_idr, minor_id);
    return ret;
 }
@@ -356,7 +399,6 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
drm_debugfs_cleanup(minor);
    drm_sysfs_device_remove(minor);
-	idr_remove(&drm_minors_idr, minor->index);
 }
/**
@@ -377,13 +419,31 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
  */
 struct drm_minor *drm_minor_acquire(unsigned int minor_id)
 {
+	struct drm_device *dev;
    struct drm_minor *minor;
+	unsigned int minor_base;
+	unsigned long flags;
+	int type;
-	minor = idr_find(&drm_minors_idr, minor_id);
-	if (!minor)
+	minor_base = drm_minor_id_to_base(minor_id);
+	type = drm_minor_id_to_type(minor_id);
+	if (type >= DRM_MINOR_CNT)
    	return ERR_PTR(-ENODEV);
-	drm_dev_ref(minor->dev);
+	spin_lock_irqsave(&drm_minor_lock, flags);
+	dev = idr_find(&drm_minors_idr, minor_base);
+	drm_dev_ref(dev);
+	spin_unlock_irqrestore(&drm_minor_lock, flags);
+
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	minor = *drm_minor_get_slot(dev, type);
+	if (drm_device_is_unplugged(dev) || !minor) {
+		drm_dev_unref(dev);
+		return ERR_PTR(-ENODEV);
+	}
+
    return minor;
 }
@@ -477,6 +537,10 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
    mutex_init(&dev->struct_mutex);
    mutex_init(&dev->ctxlist_mutex);
+	ret = drm_minor_alloc_base(dev);
+	if (ret)
+		goto err_free;
+
    if (drm_core_check_feature(dev, DRIVER_MODESET)) {
    	ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL);
    	if (ret)
@@ -520,6 +584,8 @@ err_minors:
    drm_minor_free(dev, DRM_MINOR_LEGACY);
    drm_minor_free(dev, DRM_MINOR_RENDER);
    drm_minor_free(dev, DRM_MINOR_CONTROL);
+	drm_minor_free_base(dev);
+err_free:
    kfree(dev);
    return NULL;
 }
@@ -544,6 +610,7 @@ static void drm_dev_free(struct drm_device *dev)
    drm_minor_free(dev, DRM_MINOR_LEGACY);
    drm_minor_free(dev, DRM_MINOR_RENDER);
    drm_minor_free(dev, DRM_MINOR_CONTROL);
+	drm_minor_free_base(dev);
kfree(dev->devname);
    kfree(dev);
@@ -632,6 +699,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
    		goto err_unload;
    }
+	drm_minor_activate_all(dev);
    ret = 0;
    goto out_unlock;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 114d46f..cdc5362 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1104,6 +1104,7 @@ struct drm_device {
    struct drm_driver *driver;	/**< DRM driver managing the device */
    void *dev_private;		/**< DRM driver private data */
    struct address_space *dev_mapping;	/**< Private addr-space just for the device */
+	unsigned int minor_base;	/**< DRM minor base for dev */
    struct drm_minor *control;		/**< Control node */
    struct drm_minor *primary;		/**< Primary node */
    struct drm_minor *render;		/**< Render node */
-- 
1.8.5.3


    

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

[PATCH 12/13] drm: make minor-IDs reliable