[PATCH 00/13] DRM Reliable Minor-IDs

[PATCH 00/13] DRM Reliable Minor-IDs

[David Herrmann]

Hi

I was looking into our minor-allocation code and it has one major drawback:
char-dev minor-IDs are unreliable. The <id>+128 hacks we use in user-space to
calculate render-node IDs based on the original card just does not work.

Instead of allocating dummy IDs for each driver, I went ahead and tried to fix
it properly. So this series changes the minor-ID management to just allocate a
single dev->minor_base ID instead of one ID per "drm_minor". Now we can use this
base to calculate the correct offset minor-ID for each existing DRM-minor.

While at it, I introduced drm-refcounts to make minor-lookup independent of
drm_global_mutex. This is still not finished (and dev->open_count still exists)
but I already have the next patches waiting here.

Comments welcome!

Branch is also available here:
  http://cgit.freedesktop.org/~dvdhrm/linux/log/?h=minor
If someone could pull this into their tree and push to Fengguang's
test-framework, I'd appreciate it a lot! I'm still waiting for a reply from him.

Thanks
David


David Herrmann (13):
  drm: group dev-lifetime related members
  drm: skip redundant minor-lookup in open path
  drm: remove unused DRM_MINOR_UNASSIGNED
  drm: turn DRM_MINOR_* into enum
  drm: provide device-refcount
  drm: add minor-lookup/release helpers
  drm: allocate minors early
  drm: move drm_put_minor() to drm_minor_free()
  drm: rename drm_unplug/get_minor() to drm_minor_register/unregister()
  drm: remove unneeded #ifdef CONFIG_DEBUGFS
  drm: remove redundant minor->device field
  drm: make minor-IDs reliable
  drm: remove redundant minor->index

 drivers/gpu/drm/drm_drv.c                       |   4 +-
 drivers/gpu/drm/drm_fops.c                      |  74 ++---
 drivers/gpu/drm/drm_info.c                      |   2 +-
 drivers/gpu/drm/drm_pci.c                       |   4 +-
 drivers/gpu/drm/drm_platform.c                  |   4 +-
 drivers/gpu/drm/drm_stub.c                      | 418 ++++++++++++++++--------
 drivers/gpu/drm/drm_sysfs.c                     |   8 +-
 drivers/gpu/drm/drm_usb.c                       |   4 +-
 drivers/gpu/drm/i915/i915_gpu_error.c           |   2 +-
 drivers/gpu/drm/i915/i915_trace.h               |  20 +-
 drivers/gpu/drm/msm/msm_fbdev.c                 |   2 +-
 drivers/gpu/drm/omapdrm/omap_fbdev.c            |   2 +-
 drivers/gpu/drm/radeon/atombios_encoders.c      |   2 +-
 drivers/gpu/drm/radeon/radeon_legacy_encoders.c |   2 +-
 drivers/gpu/drm/radeon/radeon_trace.h           |   2 +-
 drivers/gpu/drm/tegra/bus.c                     |   4 +-
 include/drm/drmP.h                              |  44 ++-
 17 files changed, 382 insertions(+), 216 deletions(-)

-- 
1.8.5.3

[PATCH 01/13] drm: group dev-lifetime related members

[David Herrmann]

These members are all managed by DRM-core, lets group them together so
they're not split across the whole device.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/drm/drmP.h | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 04086c5..6bd2d74 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1095,6 +1095,18 @@ struct drm_device {
 	char *devname;			/**< For /proc/interrupts */
 	int if_version;			/**< Highest interface version set */
 
+	/** \name Lifetime Management */
+	/*@{ */
+	struct device *dev;		/**< Device structure of bus-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 */
+	struct drm_minor *control;		/**< Control node */
+	struct drm_minor *primary;		/**< Primary node */
+	struct drm_minor *render;		/**< Render node */
+	atomic_t unplugged;			/**< Flag whether dev is dead */
+	/*@} */
+
 	/** \name Locks */
 	/*@{ */
 	spinlock_t count_lock;		/**< For inuse, drm_device::open_count, drm_device::buf_use */
@@ -1168,7 +1180,6 @@ struct drm_device {
 
 	struct drm_agp_head *agp;	/**< AGP data */
 
-	struct device *dev;             /**< Device structure */
 	struct pci_dev *pdev;		/**< PCI device structure */
 #ifdef __alpha__
 	struct pci_controller *hose;
@@ -1179,17 +1190,11 @@ struct drm_device {
 
 	struct drm_sg_mem *sg;	/**< Scatter gather memory */
 	unsigned int num_crtcs;                  /**< Number of CRTCs on this device */
-	void *dev_private;		/**< device private data */
-	struct address_space *dev_mapping;
 	struct drm_sigdata sigdata;	   /**< For block_all_signals */
 	sigset_t sigmask;
 
-	struct drm_driver *driver;
 	struct drm_local_map *agp_buffer_map;
 	unsigned int agp_buffer_token;
-	struct drm_minor *control;		/**< Control node for card */
-	struct drm_minor *primary;		/**< render type primary screen head */
-	struct drm_minor *render;		/**< render node for card */
 
         struct drm_mode_config mode_config;	/**< Current mode config */
 
@@ -1200,8 +1205,6 @@ struct drm_device {
 	struct drm_vma_offset_manager *vma_offset_manager;
 	/*@} */
 	int switch_power_state;
-
-	atomic_t unplugged; /* device has been unplugged or gone away */
 };
 
 #define DRM_SWITCH_POWER_ON 0
-- 
1.8.5.3

[PATCH 02/13] drm: skip redundant minor-lookup in open path

[David Herrmann]

The drm_open_helper() function is only used internally for drm_open() so
we can safely pass in the minor-object directly instead of the minor-id.
This way, we avoid the additional minor IDR lookup, which we already do
twice in drm_stub_open() and drm_open().

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_fops.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 7f2af9a..6466cb5 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -44,7 +44,7 @@ DEFINE_MUTEX(drm_global_mutex);
 EXPORT_SYMBOL(drm_global_mutex);
 
 static int drm_open_helper(struct inode *inode, struct file *filp,
-			   struct drm_device * dev);
+			   struct drm_minor *minor);
 
 static int drm_setup(struct drm_device * dev)
 {
@@ -110,7 +110,7 @@ int drm_open(struct inode *inode, struct file *filp)
 	filp->f_mapping = dev->dev_mapping;
 	mutex_unlock(&dev->struct_mutex);
 
-	retcode = drm_open_helper(inode, filp, dev);
+	retcode = drm_open_helper(inode, filp, minor);
 	if (retcode)
 		goto err_undo;
 	if (need_setup) {
@@ -196,16 +196,16 @@ static int drm_cpu_valid(void)
  *
  * \param inode device inode.
  * \param filp file pointer.
- * \param dev device.
+ * \param minor acquired minor-object.
  * \return zero on success or a negative number on failure.
  *
  * Creates and initializes a drm_file structure for the file private data in \p
  * filp and add it into the double linked list in \p dev.
  */
 static int drm_open_helper(struct inode *inode, struct file *filp,
-			   struct drm_device * dev)
+			   struct drm_minor *minor)
 {
-	int minor_id = iminor(inode);
+	struct drm_device *dev = minor->dev;
 	struct drm_file *priv;
 	int ret;
 
@@ -216,7 +216,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 	if (dev->switch_power_state != DRM_SWITCH_POWER_ON && dev->switch_power_state != DRM_SWITCH_POWER_DYNAMIC_OFF)
 		return -EINVAL;
 
-	DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor_id);
+	DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor->index);
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -226,11 +226,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 	priv->filp = filp;
 	priv->uid = current_euid();
 	priv->pid = get_pid(task_pid(current));
-	priv->minor = idr_find(&drm_minors_idr, minor_id);
-	if (!priv->minor) {
-		ret = -ENODEV;
-		goto out_put_pid;
-	}
+	priv->minor = minor;
 
 	/* for compatibility root is always authenticated */
 	priv->always_authenticated = capable(CAP_SYS_ADMIN);
@@ -336,7 +332,6 @@ out_prime_destroy:
 		drm_prime_destroy_file_private(&priv->prime);
 	if (dev->driver->driver_features & DRIVER_GEM)
 		drm_gem_release(dev, priv);
-out_put_pid:
 	put_pid(priv->pid);
 	kfree(priv);
 	filp->private_data = NULL;
-- 
1.8.5.3

[PATCH 03/13] drm: remove unused DRM_MINOR_UNASSIGNED

[David Herrmann]

This constant is unused, remove it.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/drm/drmP.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 6bd2d74..89b9d58 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1005,7 +1005,6 @@ struct drm_driver {
 	struct list_head legacy_dev_list;
 };
 
-#define DRM_MINOR_UNASSIGNED 0
 #define DRM_MINOR_LEGACY 1
 #define DRM_MINOR_CONTROL 2
 #define DRM_MINOR_RENDER 3
-- 
1.8.5.3

[PATCH 04/13] drm: turn DRM_MINOR_* into enum

[David Herrmann]

Use enum for DRM_MINOR_* constants to avoid hard-coding the IDs.
Furthermore, add a DRM_MINOR_CNT so we can perform range-checks in
follow-ups.

This changes the IDs of the minor-types by -1, but they're not used as
indices so this is fine.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/drm/drmP.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 89b9d58..c3aaf2d 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1005,9 +1005,12 @@ struct drm_driver {
 	struct list_head legacy_dev_list;
 };
 
-#define DRM_MINOR_LEGACY 1
-#define DRM_MINOR_CONTROL 2
-#define DRM_MINOR_RENDER 3
+enum drm_minor_type {
+	DRM_MINOR_LEGACY,
+	DRM_MINOR_CONTROL,
+	DRM_MINOR_RENDER,
+	DRM_MINOR_CNT,
+};
 
 /**
  * Info file list entry. This structure represents a debugfs or proc file to
-- 
1.8.5.3

[PATCH 05/13] drm: provide device-refcount

[David Herrmann]

Lets not trick ourselves into thinking "drm_device" objects are not
ref-counted. That's just utterly stupid. We manage "drm_minor" objects on
each drm-device and each minor can have an unlimited number of open
handles. Each of these handles has the drm_minor (and thus the drm_device)
as private-data in the file-handle. Therefore, we may not destroy
"drm_device" until all these handles are closed.

It is *not* possible to reset all these pointers atomically and restrict
access to them, and this is *not* how this is done! Instead, we use
ref-counts to make sure the object is valid and not freed.

Note that we currently use "dev->open_count" for that, which is *exactly*
the same as a reference-count, just open coded. So this patch doesn't
change any semantics on DRM devices (well, this patch just introduces the
ref-count, anyway. Follow-up patches will replace open_count by it).

Also note that generic VFS revoke support could allow us to drop this
ref-count again. We could then just synchronously disable any fops->xy()
calls. However, this is not the case, yet, and no such patches are
in sight (and I seriously question the idea of dropping the ref-cnt
again).

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_pci.c      |  2 +-
 drivers/gpu/drm/drm_platform.c |  2 +-
 drivers/gpu/drm/drm_stub.c     | 57 +++++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/drm_usb.c      |  2 +-
 drivers/gpu/drm/tegra/bus.c    |  2 +-
 include/drm/drmP.h             |  5 +++-
 6 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 5736aaa..9ded847 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -351,7 +351,7 @@ err_agp:
 	drm_pci_agp_destroy(dev);
 	pci_disable_device(pdev);
 err_free:
-	drm_dev_free(dev);
+	drm_dev_unref(dev);
 	return ret;
 }
 EXPORT_SYMBOL(drm_get_pci_dev);
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 21fc820..319ff53 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -64,7 +64,7 @@ static int drm_get_platform_dev(struct platform_device *platdev,
 	return 0;
 
 err_free:
-	drm_dev_free(dev);
+	drm_dev_unref(dev);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 98a33c580..c51333e 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -392,7 +392,7 @@ void drm_put_dev(struct drm_device *dev)
 	}
 
 	drm_dev_unregister(dev);
-	drm_dev_free(dev);
+	drm_dev_unref(dev);
 }
 EXPORT_SYMBOL(drm_put_dev);
 
@@ -410,7 +410,7 @@ void drm_unplug_dev(struct drm_device *dev)
 	drm_device_set_unplugged(dev);
 
 	if (dev->open_count == 0) {
-		drm_put_dev(dev);
+		drm_dev_unref(dev);
 	}
 	mutex_unlock(&drm_global_mutex);
 }
@@ -425,6 +425,9 @@ EXPORT_SYMBOL(drm_unplug_dev);
  * Call drm_dev_register() to advertice the device to user space and register it
  * with other core subsystems.
  *
+ * The initial ref-count of the object is 1. Use drm_dev_ref() and
+ * drm_dev_unref() to take and drop further ref-counts.
+ *
  * RETURNS:
  * Pointer to new DRM device, or NULL if out of memory.
  */
@@ -438,6 +441,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 	if (!dev)
 		return NULL;
 
+	kref_init(&dev->ref);
 	dev->dev = parent;
 	dev->driver = driver;
 
@@ -486,12 +490,10 @@ EXPORT_SYMBOL(drm_dev_alloc);
  * @dev: DRM device to free
  *
  * Free a DRM device that has previously been allocated via drm_dev_alloc().
- * You must not use kfree() instead or you will leak memory.
- *
- * This must not be called once the device got registered. Use drm_put_dev()
- * instead, which then calls drm_dev_free().
+ * This may not be called directly, you must use drm_dev_ref() and
+ * drm_dev_unref() to gain and drop references to the object.
  */
-void drm_dev_free(struct drm_device *dev)
+static void drm_dev_free(struct drm_device *dev)
 {
 	drm_put_minor(dev->control);
 	drm_put_minor(dev->render);
@@ -506,7 +508,44 @@ void drm_dev_free(struct drm_device *dev)
 	kfree(dev->devname);
 	kfree(dev);
 }
-EXPORT_SYMBOL(drm_dev_free);
+
+static void drm_dev_release(struct kref *ref)
+{
+	drm_dev_free(container_of(ref, struct drm_device, ref));
+}
+
+/**
+ * drm_dev_ref - Take reference of a DRM device
+ * @dev: device to take reference of or NULL
+ *
+ * This increases the ref-count of @dev by one. You *must* already own a
+ * reference when calling this. Use drm_dev_unref() to drop this reference
+ * again.
+ *
+ * This function never fails. However, this function does not provide *any*
+ * guarantee whether the device is alive or running. It only provides a
+ * reference to the object and the memory associated with it.
+ */
+void drm_dev_ref(struct drm_device *dev)
+{
+	if (dev)
+		kref_get(&dev->ref);
+}
+EXPORT_SYMBOL(drm_dev_ref);
+
+/**
+ * drm_dev_unref - Drop reference of a DRM device
+ * @dev: device to drop reference of or NULL
+ *
+ * This decreases the ref-count of @dev by one. The device is destroyed if the
+ * ref-count drops to zero.
+ */
+void drm_dev_unref(struct drm_device *dev)
+{
+	if (dev)
+		kref_put(&dev->ref, drm_dev_release);
+}
+EXPORT_SYMBOL(drm_dev_unref);
 
 /**
  * drm_dev_register - Register DRM device
@@ -581,7 +620,7 @@ EXPORT_SYMBOL(drm_dev_register);
  *
  * Unregister the DRM device from the system. This does the reverse of
  * drm_dev_register() but does not deallocate the device. The caller must call
- * drm_dev_free() to free all resources.
+ * drm_dev_unref() to drop their final reference.
  */
 void drm_dev_unregister(struct drm_device *dev)
 {
diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
index 0f8cb1a..c3406aa 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -30,7 +30,7 @@ int drm_get_usb_dev(struct usb_interface *interface,
 	return 0;
 
 err_free:
-	drm_dev_free(dev);
+	drm_dev_unref(dev);
 	return ret;
 
 }
diff --git a/drivers/gpu/drm/tegra/bus.c b/drivers/gpu/drm/tegra/bus.c
index e38e596..71cef5c 100644
--- a/drivers/gpu/drm/tegra/bus.c
+++ b/drivers/gpu/drm/tegra/bus.c
@@ -63,7 +63,7 @@ int drm_host1x_init(struct drm_driver *driver, struct host1x_device *device)
 	return 0;
 
 err_free:
-	drm_dev_free(drm);
+	drm_dev_unref(drm);
 	return ret;
 }
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index c3aaf2d..2c58e94 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -43,6 +43,7 @@
 #include <asm/current.h>
 #endif				/* __alpha__ */
 #include <linux/kernel.h>
+#include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -1099,6 +1100,7 @@ struct drm_device {
 
 	/** \name Lifetime Management */
 	/*@{ */
+	struct kref ref;		/**< Object ref-count */
 	struct device *dev;		/**< Device structure of bus-device */
 	struct drm_driver *driver;	/**< DRM driver managing the device */
 	void *dev_private;		/**< DRM driver private data */
@@ -1663,7 +1665,8 @@ static __inline__ void drm_core_dropmap(struct drm_local_map *map)
 
 struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 				 struct device *parent);
-void drm_dev_free(struct drm_device *dev);
+void drm_dev_ref(struct drm_device *dev);
+void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
 /*@}*/
-- 
1.8.5.3

Re: [PATCH 05/13] drm: provide device-refcount

[Daniel Vetter]

On Wed, Jan 29, 2014 at 03:01:52PM +0100, David Herrmann wrote:
> Lets not trick ourselves into thinking "drm_device" objects are not
> ref-counted. That's just utterly stupid. We manage "drm_minor" objects on
> each drm-device and each minor can have an unlimited number of open
> handles. Each of these handles has the drm_minor (and thus the drm_device)
> as private-data in the file-handle. Therefore, we may not destroy
> "drm_device" until all these handles are closed.
> 
> It is *not* possible to reset all these pointers atomically and restrict
> access to them, and this is *not* how this is done! Instead, we use
> ref-counts to make sure the object is valid and not freed.
> 
> Note that we currently use "dev->open_count" for that, which is *exactly*
> the same as a reference-count, just open coded. So this patch doesn't
> change any semantics on DRM devices (well, this patch just introduces the
> ref-count, anyway. Follow-up patches will replace open_count by it).
> 
> Also note that generic VFS revoke support could allow us to drop this
> ref-count again. We could then just synchronously disable any fops->xy()
> calls. However, this is not the case, yet, and no such patches are
> in sight (and I seriously question the idea of dropping the ref-cnt
> again).
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

[snip]

> +/**
> + * drm_dev_ref - Take reference of a DRM device
> + * @dev: device to take reference of or NULL
> + *
> + * This increases the ref-count of @dev by one. You *must* already own a
> + * reference when calling this. Use drm_dev_unref() to drop this reference
> + * again.
> + *
> + * This function never fails. However, this function does not provide *any*
> + * guarantee whether the device is alive or running. It only provides a
> + * reference to the object and the memory associated with it.
> + */
> +void drm_dev_ref(struct drm_device *dev)
> +{
> +	if (dev)

This check here (and below in the unref code) look funny. What's the
reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
pretty serious bug to me. This is in contrast to kfree(NULL) which imo
makes sense - freeing nothing is a legitimate operation imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 05/13] drm: provide device-refcount

[David Herrmann]

Hi

On Wed, Feb 12, 2014 at 2:25 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 29, 2014 at 03:01:52PM +0100, David Herrmann wrote:
>> Lets not trick ourselves into thinking "drm_device" objects are not
>> ref-counted. That's just utterly stupid. We manage "drm_minor" objects on
>> each drm-device and each minor can have an unlimited number of open
>> handles. Each of these handles has the drm_minor (and thus the drm_device)
>> as private-data in the file-handle. Therefore, we may not destroy
>> "drm_device" until all these handles are closed.
>>
>> It is *not* possible to reset all these pointers atomically and restrict
>> access to them, and this is *not* how this is done! Instead, we use
>> ref-counts to make sure the object is valid and not freed.
>>
>> Note that we currently use "dev->open_count" for that, which is *exactly*
>> the same as a reference-count, just open coded. So this patch doesn't
>> change any semantics on DRM devices (well, this patch just introduces the
>> ref-count, anyway. Follow-up patches will replace open_count by it).
>>
>> Also note that generic VFS revoke support could allow us to drop this
>> ref-count again. We could then just synchronously disable any fops->xy()
>> calls. However, this is not the case, yet, and no such patches are
>> in sight (and I seriously question the idea of dropping the ref-cnt
>> again).
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> [snip]
>
>> +/**
>> + * drm_dev_ref - Take reference of a DRM device
>> + * @dev: device to take reference of or NULL
>> + *
>> + * This increases the ref-count of @dev by one. You *must* already own a
>> + * reference when calling this. Use drm_dev_unref() to drop this reference
>> + * again.
>> + *
>> + * This function never fails. However, this function does not provide *any*
>> + * guarantee whether the device is alive or running. It only provides a
>> + * reference to the object and the memory associated with it.
>> + */
>> +void drm_dev_ref(struct drm_device *dev)
>> +{
>> +     if (dev)
>
> This check here (and below in the unref code) look funny. What's the
> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
> makes sense - freeing nothing is a legitimate operation imo.

I added it mainly to simplify cleanup-code paths. You can then just
call unref() and set it to NULL regardless whether you actually hold a
reference or not. For ref() I don't really care but I think the
NULL-test doesn't hurt either.

I copied this behavior from get_device() and put_device(), btw.
Similar to these functions, I think a lot more will go wrong if the
NULL pointer is not intentional. Imo, ref-counting on a NULL object
just means "no object", so it shouldn't do anything.

Thanks
David

Re: [PATCH 05/13] drm: provide device-refcount

[Daniel Vetter]

On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> +/**
>>> + * drm_dev_ref - Take reference of a DRM device
>>> + * @dev: device to take reference of or NULL
>>> + *
>>> + * This increases the ref-count of @dev by one. You *must* already own a
>>> + * reference when calling this. Use drm_dev_unref() to drop this reference
>>> + * again.
>>> + *
>>> + * This function never fails. However, this function does not provide *any*
>>> + * guarantee whether the device is alive or running. It only provides a
>>> + * reference to the object and the memory associated with it.
>>> + */
>>> +void drm_dev_ref(struct drm_device *dev)
>>> +{
>>> +     if (dev)
>>
>> This check here (and below in the unref code) look funny. What's the
>> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
>> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
>> makes sense - freeing nothing is a legitimate operation imo.
>
> I added it mainly to simplify cleanup-code paths. You can then just
> call unref() and set it to NULL regardless whether you actually hold a
> reference or not. For ref() I don't really care but I think the
> NULL-test doesn't hurt either.
>
> I copied this behavior from get_device() and put_device(), btw.
> Similar to these functions, I think a lot more will go wrong if the
> NULL pointer is not intentional. Imo, ref-counting on a NULL object
> just means "no object", so it shouldn't do anything.

My fear with this kind of magic is that someone accidentally exchanges
the pointer clearing to NULL (or assignement when grabbing a ref) with
the unref/ref call and then we have a very subtle bug at hand. If we
don't accept NULL objects the failure will be much more obvious.

The entire kernel kobject stuff is very consistent about this, but I
couldn't find a reason for it - all the NULL checks predate git
history. Greg can you please shed some lights on best practice here
and whether my fears are justified given your experience with shoddy
drivers in general?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 05/13] drm: provide device-refcount

[Daniel Vetter]

On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> +/**
>>> + * drm_dev_ref - Take reference of a DRM device
>>> + * @dev: device to take reference of or NULL
>>> + *
>>> + * This increases the ref-count of @dev by one. You *must* already own a
>>> + * reference when calling this. Use drm_dev_unref() to drop this reference
>>> + * again.
>>> + *
>>> + * This function never fails. However, this function does not provide *any*
>>> + * guarantee whether the device is alive or running. It only provides a
>>> + * reference to the object and the memory associated with it.
>>> + */
>>> +void drm_dev_ref(struct drm_device *dev)
>>> +{
>>> +     if (dev)
>>
>> This check here (and below in the unref code) look funny. What's the
>> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
>> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
>> makes sense - freeing nothing is a legitimate operation imo.
>
> I added it mainly to simplify cleanup-code paths. You can then just
> call unref() and set it to NULL regardless whether you actually hold a
> reference or not. For ref() I don't really care but I think the
> NULL-test doesn't hurt either.
>
> I copied this behavior from get_device() and put_device(), btw.
> Similar to these functions, I think a lot more will go wrong if the
> NULL pointer is not intentional. Imo, ref-counting on a NULL object
> just means "no object", so it shouldn't do anything.

My fear with this kind of magic is that someone accidentally exchanges
the pointer clearing to NULL (or assignement when grabbing a ref) with
the unref/ref call and then we have a very subtle bug at hand. If we
don't accept NULL objects the failure will be much more obvious.

The entire kernel kobject stuff is very consistent about this, but I
couldn't find a reason for it - all the NULL checks predate git
history. Greg can you please shed some lights on best practice here
and whether my fears are justified given your experience with shoddy
drivers in general?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 05/13] drm: provide device-refcount

[Greg KH]

On Wed, Feb 12, 2014 at 05:26:57PM +0100, Daniel Vetter wrote:
> On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> >>> +/**
> >>> + * drm_dev_ref - Take reference of a DRM device
> >>> + * @dev: device to take reference of or NULL
> >>> + *
> >>> + * This increases the ref-count of @dev by one. You *must* already own a
> >>> + * reference when calling this. Use drm_dev_unref() to drop this reference
> >>> + * again.
> >>> + *
> >>> + * This function never fails. However, this function does not provide *any*
> >>> + * guarantee whether the device is alive or running. It only provides a
> >>> + * reference to the object and the memory associated with it.
> >>> + */
> >>> +void drm_dev_ref(struct drm_device *dev)
> >>> +{
> >>> +     if (dev)
> >>
> >> This check here (and below in the unref code) look funny. What's the
> >> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
> >> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
> >> makes sense - freeing nothing is a legitimate operation imo.
> >
> > I added it mainly to simplify cleanup-code paths. You can then just
> > call unref() and set it to NULL regardless whether you actually hold a
> > reference or not. For ref() I don't really care but I think the
> > NULL-test doesn't hurt either.
> >
> > I copied this behavior from get_device() and put_device(), btw.
> > Similar to these functions, I think a lot more will go wrong if the
> > NULL pointer is not intentional. Imo, ref-counting on a NULL object
> > just means "no object", so it shouldn't do anything.
> 
> My fear with this kind of magic is that someone accidentally exchanges
> the pointer clearing to NULL (or assignement when grabbing a ref) with
> the unref/ref call and then we have a very subtle bug at hand. If we
> don't accept NULL objects the failure will be much more obvious.
> 
> The entire kernel kobject stuff is very consistent about this, but I
> couldn't find a reason for it - all the NULL checks predate git
> history. Greg can you please shed some lights on best practice here
> and whether my fears are justified given your experience with shoddy
> drivers in general?

Yes, the driver core does test for NULL here, as sometimes you are
passing in a "parent" pointer, and don't really care if it is NULL or
not, so just treating it as if you really do have a reference is usually
fine.

But, for a subsystem where you "know" you will not be doing anything as
foolish as that, I'd not allow that :)

So I'd recommend taking those checks out of the drm code.

thanks,

greg k-h

Re: [PATCH 05/13] drm: provide device-refcount

[David Herrmann]

Hi

On Wed, Feb 12, 2014 at 5:40 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Feb 12, 2014 at 05:26:57PM +0100, Daniel Vetter wrote:
>> On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> >>> +/**
>> >>> + * drm_dev_ref - Take reference of a DRM device
>> >>> + * @dev: device to take reference of or NULL
>> >>> + *
>> >>> + * This increases the ref-count of @dev by one. You *must* already own a
>> >>> + * reference when calling this. Use drm_dev_unref() to drop this reference
>> >>> + * again.
>> >>> + *
>> >>> + * This function never fails. However, this function does not provide *any*
>> >>> + * guarantee whether the device is alive or running. It only provides a
>> >>> + * reference to the object and the memory associated with it.
>> >>> + */
>> >>> +void drm_dev_ref(struct drm_device *dev)
>> >>> +{
>> >>> +     if (dev)
>> >>
>> >> This check here (and below in the unref code) look funny. What's the
>> >> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
>> >> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
>> >> makes sense - freeing nothing is a legitimate operation imo.
>> >
>> > I added it mainly to simplify cleanup-code paths. You can then just
>> > call unref() and set it to NULL regardless whether you actually hold a
>> > reference or not. For ref() I don't really care but I think the
>> > NULL-test doesn't hurt either.
>> >
>> > I copied this behavior from get_device() and put_device(), btw.
>> > Similar to these functions, I think a lot more will go wrong if the
>> > NULL pointer is not intentional. Imo, ref-counting on a NULL object
>> > just means "no object", so it shouldn't do anything.
>>
>> My fear with this kind of magic is that someone accidentally exchanges
>> the pointer clearing to NULL (or assignement when grabbing a ref) with
>> the unref/ref call and then we have a very subtle bug at hand. If we
>> don't accept NULL objects the failure will be much more obvious.
>>
>> The entire kernel kobject stuff is very consistent about this, but I
>> couldn't find a reason for it - all the NULL checks predate git
>> history. Greg can you please shed some lights on best practice here
>> and whether my fears are justified given your experience with shoddy
>> drivers in general?
>
> Yes, the driver core does test for NULL here, as sometimes you are
> passing in a "parent" pointer, and don't really care if it is NULL or
> not, so just treating it as if you really do have a reference is usually
> fine.
>
> But, for a subsystem where you "know" you will not be doing anything as
> foolish as that, I'd not allow that :)
>
> So I'd recommend taking those checks out of the drm code.

Ok, for _ref() I'm fine dropping it, but for _unref() I really don't
understand the concerns. I like to follow the principle of making
teardown-functions work with partially initialized objects. A caller
shouldn't be required to reverse all it's setup functions if one last
step of object-initialization fails. It's much easier if they can just
call the destructor which figures itself out which parts are
initialized. Obviously, this isn't always possible, but checking for
NULL in _unref() or _put() paths simplifies this a lot and avoids
non-sense if(obj) unref(obj);

For instance for drm_minor objects we only initialize the minors that
are enabled by the specific driver. However, it's enough to test for
the flags during device-initialization. device-registration,
-deregistration and -teardown just call _free/unref on all possible
minors. Allowing NULL avoids testing for these flags in every path but
the initialization.

Anyhow, shared code -> many opinions, so if people agree on dropping
it, I will do so.

Thanks
David

Re: [PATCH 05/13] drm: provide device-refcount

[David Herrmann]

Hi

On Wed, Feb 12, 2014 at 5:40 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Feb 12, 2014 at 05:26:57PM +0100, Daniel Vetter wrote:
>> On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> >>> +/**
>> >>> + * drm_dev_ref - Take reference of a DRM device
>> >>> + * @dev: device to take reference of or NULL
>> >>> + *
>> >>> + * This increases the ref-count of @dev by one. You *must* already own a
>> >>> + * reference when calling this. Use drm_dev_unref() to drop this reference
>> >>> + * again.
>> >>> + *
>> >>> + * This function never fails. However, this function does not provide *any*
>> >>> + * guarantee whether the device is alive or running. It only provides a
>> >>> + * reference to the object and the memory associated with it.
>> >>> + */
>> >>> +void drm_dev_ref(struct drm_device *dev)
>> >>> +{
>> >>> +     if (dev)
>> >>
>> >> This check here (and below in the unref code) look funny. What's the
>> >> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
>> >> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
>> >> makes sense - freeing nothing is a legitimate operation imo.
>> >
>> > I added it mainly to simplify cleanup-code paths. You can then just
>> > call unref() and set it to NULL regardless whether you actually hold a
>> > reference or not. For ref() I don't really care but I think the
>> > NULL-test doesn't hurt either.
>> >
>> > I copied this behavior from get_device() and put_device(), btw.
>> > Similar to these functions, I think a lot more will go wrong if the
>> > NULL pointer is not intentional. Imo, ref-counting on a NULL object
>> > just means "no object", so it shouldn't do anything.
>>
>> My fear with this kind of magic is that someone accidentally exchanges
>> the pointer clearing to NULL (or assignement when grabbing a ref) with
>> the unref/ref call and then we have a very subtle bug at hand. If we
>> don't accept NULL objects the failure will be much more obvious.
>>
>> The entire kernel kobject stuff is very consistent about this, but I
>> couldn't find a reason for it - all the NULL checks predate git
>> history. Greg can you please shed some lights on best practice here
>> and whether my fears are justified given your experience with shoddy
>> drivers in general?
>
> Yes, the driver core does test for NULL here, as sometimes you are
> passing in a "parent" pointer, and don't really care if it is NULL or
> not, so just treating it as if you really do have a reference is usually
> fine.
>
> But, for a subsystem where you "know" you will not be doing anything as
> foolish as that, I'd not allow that :)
>
> So I'd recommend taking those checks out of the drm code.

Ok, for _ref() I'm fine dropping it, but for _unref() I really don't
understand the concerns. I like to follow the principle of making
teardown-functions work with partially initialized objects. A caller
shouldn't be required to reverse all it's setup functions if one last
step of object-initialization fails. It's much easier if they can just
call the destructor which figures itself out which parts are
initialized. Obviously, this isn't always possible, but checking for
NULL in _unref() or _put() paths simplifies this a lot and avoids
non-sense if(obj) unref(obj);

For instance for drm_minor objects we only initialize the minors that
are enabled by the specific driver. However, it's enough to test for
the flags during device-initialization. device-registration,
-deregistration and -teardown just call _free/unref on all possible
minors. Allowing NULL avoids testing for these flags in every path but
the initialization.

Anyhow, shared code -> many opinions, so if people agree on dropping
it, I will do so.

Thanks
David

Re: [PATCH 05/13] drm: provide device-refcount

[Daniel Vetter]

On Wed, Feb 12, 2014 at 06:48:50PM +0100, David Herrmann wrote:
> On Wed, Feb 12, 2014 at 5:40 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Feb 12, 2014 at 05:26:57PM +0100, Daniel Vetter wrote:
> >> On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> >> >>> +/**
> >> >>> + * drm_dev_ref - Take reference of a DRM device
> >> >>> + * @dev: device to take reference of or NULL
> >> >>> + *
> >> >>> + * This increases the ref-count of @dev by one. You *must* already own a
> >> >>> + * reference when calling this. Use drm_dev_unref() to drop this reference
> >> >>> + * again.
> >> >>> + *
> >> >>> + * This function never fails. However, this function does not provide *any*
> >> >>> + * guarantee whether the device is alive or running. It only provides a
> >> >>> + * reference to the object and the memory associated with it.
> >> >>> + */
> >> >>> +void drm_dev_ref(struct drm_device *dev)
> >> >>> +{
> >> >>> +     if (dev)
> >> >>
> >> >> This check here (and below in the unref code) look funny. What's the
> >> >> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
> >> >> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
> >> >> makes sense - freeing nothing is a legitimate operation imo.
> >> >
> >> > I added it mainly to simplify cleanup-code paths. You can then just
> >> > call unref() and set it to NULL regardless whether you actually hold a
> >> > reference or not. For ref() I don't really care but I think the
> >> > NULL-test doesn't hurt either.
> >> >
> >> > I copied this behavior from get_device() and put_device(), btw.
> >> > Similar to these functions, I think a lot more will go wrong if the
> >> > NULL pointer is not intentional. Imo, ref-counting on a NULL object
> >> > just means "no object", so it shouldn't do anything.
> >>
> >> My fear with this kind of magic is that someone accidentally exchanges
> >> the pointer clearing to NULL (or assignement when grabbing a ref) with
> >> the unref/ref call and then we have a very subtle bug at hand. If we
> >> don't accept NULL objects the failure will be much more obvious.
> >>
> >> The entire kernel kobject stuff is very consistent about this, but I
> >> couldn't find a reason for it - all the NULL checks predate git
> >> history. Greg can you please shed some lights on best practice here
> >> and whether my fears are justified given your experience with shoddy
> >> drivers in general?
> >
> > Yes, the driver core does test for NULL here, as sometimes you are
> > passing in a "parent" pointer, and don't really care if it is NULL or
> > not, so just treating it as if you really do have a reference is usually
> > fine.
> >
> > But, for a subsystem where you "know" you will not be doing anything as
> > foolish as that, I'd not allow that :)
> >
> > So I'd recommend taking those checks out of the drm code.
> 
> Ok, for _ref() I'm fine dropping it, but for _unref() I really don't
> understand the concerns. I like to follow the principle of making
> teardown-functions work with partially initialized objects. A caller
> shouldn't be required to reverse all it's setup functions if one last
> step of object-initialization fails. It's much easier if they can just
> call the destructor which figures itself out which parts are
> initialized. Obviously, this isn't always possible, but checking for
> NULL in _unref() or _put() paths simplifies this a lot and avoids
> non-sense if(obj) unref(obj);
> 
> For instance for drm_minor objects we only initialize the minors that
> are enabled by the specific driver. However, it's enough to test for
> the flags during device-initialization. device-registration,
> -deregistration and -teardown just call _free/unref on all possible
> minors. Allowing NULL avoids testing for these flags in every path but
> the initialization.
> 
> Anyhow, shared code -> many opinions, so if people agree on dropping
> it, I will do so.

I might have missed it, but afaics both drm_minor_free and unregister
already have NULL pointer checks at the beginning for other reasons. And
the _unref in the drm unload paths also should never see a NULL
drm_device. So I think with your current patch series we're already
covered and there's no need for any additional NULL checks, hence also
none for the drm_dev_unref function.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 05/13] drm: provide device-refcount

[Daniel Vetter]

On Wed, Feb 12, 2014 at 06:48:50PM +0100, David Herrmann wrote:
> On Wed, Feb 12, 2014 at 5:40 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Feb 12, 2014 at 05:26:57PM +0100, Daniel Vetter wrote:
> >> On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> >> >>> +/**
> >> >>> + * drm_dev_ref - Take reference of a DRM device
> >> >>> + * @dev: device to take reference of or NULL
> >> >>> + *
> >> >>> + * This increases the ref-count of @dev by one. You *must* already own a
> >> >>> + * reference when calling this. Use drm_dev_unref() to drop this reference
> >> >>> + * again.
> >> >>> + *
> >> >>> + * This function never fails. However, this function does not provide *any*
> >> >>> + * guarantee whether the device is alive or running. It only provides a
> >> >>> + * reference to the object and the memory associated with it.
> >> >>> + */
> >> >>> +void drm_dev_ref(struct drm_device *dev)
> >> >>> +{
> >> >>> +     if (dev)
> >> >>
> >> >> This check here (and below in the unref code) look funny. What's the
> >> >> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
> >> >> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
> >> >> makes sense - freeing nothing is a legitimate operation imo.
> >> >
> >> > I added it mainly to simplify cleanup-code paths. You can then just
> >> > call unref() and set it to NULL regardless whether you actually hold a
> >> > reference or not. For ref() I don't really care but I think the
> >> > NULL-test doesn't hurt either.
> >> >
> >> > I copied this behavior from get_device() and put_device(), btw.
> >> > Similar to these functions, I think a lot more will go wrong if the
> >> > NULL pointer is not intentional. Imo, ref-counting on a NULL object
> >> > just means "no object", so it shouldn't do anything.
> >>
> >> My fear with this kind of magic is that someone accidentally exchanges
> >> the pointer clearing to NULL (or assignement when grabbing a ref) with
> >> the unref/ref call and then we have a very subtle bug at hand. If we
> >> don't accept NULL objects the failure will be much more obvious.
> >>
> >> The entire kernel kobject stuff is very consistent about this, but I
> >> couldn't find a reason for it - all the NULL checks predate git
> >> history. Greg can you please shed some lights on best practice here
> >> and whether my fears are justified given your experience with shoddy
> >> drivers in general?
> >
> > Yes, the driver core does test for NULL here, as sometimes you are
> > passing in a "parent" pointer, and don't really care if it is NULL or
> > not, so just treating it as if you really do have a reference is usually
> > fine.
> >
> > But, for a subsystem where you "know" you will not be doing anything as
> > foolish as that, I'd not allow that :)
> >
> > So I'd recommend taking those checks out of the drm code.
> 
> Ok, for _ref() I'm fine dropping it, but for _unref() I really don't
> understand the concerns. I like to follow the principle of making
> teardown-functions work with partially initialized objects. A caller
> shouldn't be required to reverse all it's setup functions if one last
> step of object-initialization fails. It's much easier if they can just
> call the destructor which figures itself out which parts are
> initialized. Obviously, this isn't always possible, but checking for
> NULL in _unref() or _put() paths simplifies this a lot and avoids
> non-sense if(obj) unref(obj);
> 
> For instance for drm_minor objects we only initialize the minors that
> are enabled by the specific driver. However, it's enough to test for
> the flags during device-initialization. device-registration,
> -deregistration and -teardown just call _free/unref on all possible
> minors. Allowing NULL avoids testing for these flags in every path but
> the initialization.
> 
> Anyhow, shared code -> many opinions, so if people agree on dropping
> it, I will do so.

I might have missed it, but afaics both drm_minor_free and unregister
already have NULL pointer checks at the beginning for other reasons. And
the _unref in the drm unload paths also should never see a NULL
drm_device. So I think with your current patch series we're already
covered and there's no need for any additional NULL checks, hence also
none for the drm_dev_unref function.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 05/13] drm: provide device-refcount

[Greg KH]

On Wed, Feb 12, 2014 at 06:48:50PM +0100, David Herrmann wrote:
> Hi
> 
> On Wed, Feb 12, 2014 at 5:40 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Feb 12, 2014 at 05:26:57PM +0100, Daniel Vetter wrote:
> >> On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> >> >>> +/**
> >> >>> + * drm_dev_ref - Take reference of a DRM device
> >> >>> + * @dev: device to take reference of or NULL
> >> >>> + *
> >> >>> + * This increases the ref-count of @dev by one. You *must* already own a
> >> >>> + * reference when calling this. Use drm_dev_unref() to drop this reference
> >> >>> + * again.
> >> >>> + *
> >> >>> + * This function never fails. However, this function does not provide *any*
> >> >>> + * guarantee whether the device is alive or running. It only provides a
> >> >>> + * reference to the object and the memory associated with it.
> >> >>> + */
> >> >>> +void drm_dev_ref(struct drm_device *dev)
> >> >>> +{
> >> >>> +     if (dev)
> >> >>
> >> >> This check here (and below in the unref code) look funny. What's the
> >> >> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
> >> >> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
> >> >> makes sense - freeing nothing is a legitimate operation imo.
> >> >
> >> > I added it mainly to simplify cleanup-code paths. You can then just
> >> > call unref() and set it to NULL regardless whether you actually hold a
> >> > reference or not. For ref() I don't really care but I think the
> >> > NULL-test doesn't hurt either.
> >> >
> >> > I copied this behavior from get_device() and put_device(), btw.
> >> > Similar to these functions, I think a lot more will go wrong if the
> >> > NULL pointer is not intentional. Imo, ref-counting on a NULL object
> >> > just means "no object", so it shouldn't do anything.
> >>
> >> My fear with this kind of magic is that someone accidentally exchanges
> >> the pointer clearing to NULL (or assignement when grabbing a ref) with
> >> the unref/ref call and then we have a very subtle bug at hand. If we
> >> don't accept NULL objects the failure will be much more obvious.
> >>
> >> The entire kernel kobject stuff is very consistent about this, but I
> >> couldn't find a reason for it - all the NULL checks predate git
> >> history. Greg can you please shed some lights on best practice here
> >> and whether my fears are justified given your experience with shoddy
> >> drivers in general?
> >
> > Yes, the driver core does test for NULL here, as sometimes you are
> > passing in a "parent" pointer, and don't really care if it is NULL or
> > not, so just treating it as if you really do have a reference is usually
> > fine.
> >
> > But, for a subsystem where you "know" you will not be doing anything as
> > foolish as that, I'd not allow that :)
> >
> > So I'd recommend taking those checks out of the drm code.
> 
> Ok, for _ref() I'm fine dropping it, but for _unref() I really don't
> understand the concerns. I like to follow the principle of making
> teardown-functions work with partially initialized objects. A caller
> shouldn't be required to reverse all it's setup functions if one last
> step of object-initialization fails. It's much easier if they can just
> call the destructor which figures itself out which parts are
> initialized. Obviously, this isn't always possible, but checking for
> NULL in _unref() or _put() paths simplifies this a lot and avoids
> non-sense if(obj) unref(obj);
> 
> For instance for drm_minor objects we only initialize the minors that
> are enabled by the specific driver. However, it's enough to test for
> the flags during device-initialization. device-registration,
> -deregistration and -teardown just call _free/unref on all possible
> minors. Allowing NULL avoids testing for these flags in every path but
> the initialization.

I have no objection to that justification for it.

greg k-h

Re: [PATCH 05/13] drm: provide device-refcount

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 920 bytes --]

On Wed, Jan 29, 2014 at 03:01:52PM +0100, David Herrmann wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
[...]
> @@ -486,12 +490,10 @@ EXPORT_SYMBOL(drm_dev_alloc);
>   * @dev: DRM device to free
>   *
>   * Free a DRM device that has previously been allocated via drm_dev_alloc().
> - * You must not use kfree() instead or you will leak memory.
> - *
> - * This must not be called once the device got registered. Use drm_put_dev()
> - * instead, which then calls drm_dev_free().
> + * This may not be called directly, you must use drm_dev_ref() and
> + * drm_dev_unref() to gain and drop references to the object.
>   */
> -void drm_dev_free(struct drm_device *dev)
> +static void drm_dev_free(struct drm_device *dev)

With the function turning static it can't be called from anywhere else
but this file, so perhaps the last sentence of the comment can now go
away as well?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 05/13] drm: provide device-refcount

[David Herrmann]

Hi

On Fri, Feb 21, 2014 at 8:01 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Jan 29, 2014 at 03:01:52PM +0100, David Herrmann wrote:
> [...]
>> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> [...]
>> @@ -486,12 +490,10 @@ EXPORT_SYMBOL(drm_dev_alloc);
>>   * @dev: DRM device to free
>>   *
>>   * Free a DRM device that has previously been allocated via drm_dev_alloc().
>> - * You must not use kfree() instead or you will leak memory.
>> - *
>> - * This must not be called once the device got registered. Use drm_put_dev()
>> - * instead, which then calls drm_dev_free().
>> + * This may not be called directly, you must use drm_dev_ref() and
>> + * drm_dev_unref() to gain and drop references to the object.
>>   */
>> -void drm_dev_free(struct drm_device *dev)
>> +static void drm_dev_free(struct drm_device *dev)
>
> With the function turning static it can't be called from anywhere else
> but this file, so perhaps the last sentence of the comment can now go
> away as well?

Yepp, fixed.

Thanks
David

[PATCH 06/13] drm: add minor-lookup/release helpers

[David Herrmann]

Instead of accessing drm_minors_idr directly, this adds a small helper to
hide the internals. This will help us later to remove the drm_global_mutex
requirement for minor-lookup.

Furthermore, this also makes sure that minor->dev is always valid and
takes a reference-count to the device as long as the minor is used in an
open-file. This way, "struct file*"->private_data->dev is guaranteed to be
valid (which it has to, as we cannot reset it).

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_fops.c | 50 +++++++++++++++++++++++++---------------------
 drivers/gpu/drm/drm_stub.c | 39 ++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h         |  4 ++++
 3 files changed, 70 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 6466cb5..7947819 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -79,23 +79,22 @@ static int drm_setup(struct drm_device * dev)
  */
 int drm_open(struct inode *inode, struct file *filp)
 {
-	struct drm_device *dev = NULL;
-	int minor_id = iminor(inode);
+	struct drm_device *dev;
 	struct drm_minor *minor;
-	int retcode = 0;
+	int retcode;
 	int need_setup = 0;
 	struct address_space *old_mapping;
 	struct address_space *old_imapping;
 
-	minor = idr_find(&drm_minors_idr, minor_id);
-	if (!minor)
-		return -ENODEV;
-
-	if (!(dev = minor->dev))
-		return -ENODEV;
+	minor = drm_minor_acquire(iminor(inode));
+	if (IS_ERR(minor))
+		return PTR_ERR(minor);
 
-	if (drm_device_is_unplugged(dev))
-		return -ENODEV;
+	dev = minor->dev;
+	if (drm_device_is_unplugged(dev)) {
+		retcode = -ENODEV;
+		goto err_release;
+	}
 
 	if (!dev->open_count++)
 		need_setup = 1;
@@ -128,6 +127,8 @@ err_undo:
 	dev->dev_mapping = old_mapping;
 	mutex_unlock(&dev->struct_mutex);
 	dev->open_count--;
+err_release:
+	drm_minor_release(minor);
 	return retcode;
 }
 EXPORT_SYMBOL(drm_open);
@@ -143,33 +144,33 @@ EXPORT_SYMBOL(drm_open);
  */
 int drm_stub_open(struct inode *inode, struct file *filp)
 {
-	struct drm_device *dev = NULL;
+	struct drm_device *dev;
 	struct drm_minor *minor;
-	int minor_id = iminor(inode);
 	int err = -ENODEV;
 	const struct file_operations *new_fops;
 
 	DRM_DEBUG("\n");
 
 	mutex_lock(&drm_global_mutex);
-	minor = idr_find(&drm_minors_idr, minor_id);
-	if (!minor)
-		goto out;
-
-	if (!(dev = minor->dev))
-		goto out;
+	minor = drm_minor_acquire(iminor(inode));
+	if (IS_ERR(minor))
+		goto out_unlock;
 
+	dev = minor->dev;
 	if (drm_device_is_unplugged(dev))
-		goto out;
+		goto out_release;
 
 	new_fops = fops_get(dev->driver->fops);
 	if (!new_fops)
-		goto out;
+		goto out_release;
 
 	replace_fops(filp, new_fops);
 	if (filp->f_op->open)
 		err = filp->f_op->open(inode, filp);
-out:
+
+out_release:
+	drm_minor_release(minor);
+out_unlock:
 	mutex_unlock(&drm_global_mutex);
 	return err;
 }
@@ -453,7 +454,8 @@ int drm_lastclose(struct drm_device * dev)
 int drm_release(struct inode *inode, struct file *filp)
 {
 	struct drm_file *file_priv = filp->private_data;
-	struct drm_device *dev = file_priv->minor->dev;
+	struct drm_minor *minor = file_priv->minor;
+	struct drm_device *dev = minor->dev;
 	int retcode = 0;
 
 	mutex_lock(&drm_global_mutex);
@@ -575,6 +577,8 @@ int drm_release(struct inode *inode, struct file *filp)
 	}
 	mutex_unlock(&drm_global_mutex);
 
+	drm_minor_release(minor);
+
 	return retcode;
 }
 EXPORT_SYMBOL(drm_release);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index c51333e..d3232b6 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -356,6 +356,45 @@ static void drm_unplug_minor(struct drm_minor *minor)
 }
 
 /**
+ * drm_minor_acquire - Acquire a DRM minor
+ * @minor_id: Minor ID of the DRM-minor
+ *
+ * Looks up the given minor-ID and returns the respective DRM-minor object. The
+ * refence-count of the underlying device is increased so you must release this
+ * object with drm_minor_release().
+ *
+ * As long as you hold this minor, it is guaranteed that the object and the
+ * minor->dev pointer will stay valid! However, the device may get unplugged and
+ * unregistered while you hold the minor.
+ *
+ * Returns:
+ * Pointer to minor-object with increased device-refcount, or PTR_ERR on
+ * failure.
+ */
+struct drm_minor *drm_minor_acquire(unsigned int minor_id)
+{
+	struct drm_minor *minor;
+
+	minor = idr_find(&drm_minors_idr, minor_id);
+	if (!minor)
+		return ERR_PTR(-ENODEV);
+
+	drm_dev_ref(minor->dev);
+	return minor;
+}
+
+/**
+ * drm_minor_release - Release DRM minor
+ * @minor: Pointer to DRM minor object
+ *
+ * Release a minor that was previously acquired via drm_minor_acquire().
+ */
+void drm_minor_release(struct drm_minor *minor)
+{
+	drm_dev_unref(minor->dev);
+}
+
+/**
  * drm_put_minor - Destroy DRM minor
  * @minor: Minor to destroy
  *
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2c58e94..3c02cad 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1669,6 +1669,10 @@ void drm_dev_ref(struct drm_device *dev);
 void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
+
+struct drm_minor *drm_minor_acquire(unsigned int minor_id);
+void drm_minor_release(struct drm_minor *minor);
+
 /*@}*/
 
 /* PCI section */
-- 
1.8.5.3

Re: [PATCH 06/13] drm: add minor-lookup/release helpers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1473 bytes --]

On Wed, Jan 29, 2014 at 03:01:53PM +0100, David Herrmann wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index c51333e..d3232b6 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -356,6 +356,45 @@ static void drm_unplug_minor(struct drm_minor *minor)
>  }
>  
>  /**
> + * drm_minor_acquire - Acquire a DRM minor
> + * @minor_id: Minor ID of the DRM-minor
> + *
> + * Looks up the given minor-ID and returns the respective DRM-minor object. The
> + * refence-count of the underlying device is increased so you must release this
> + * object with drm_minor_release().
> + *
> + * As long as you hold this minor, it is guaranteed that the object and the
> + * minor->dev pointer will stay valid! However, the device may get unplugged and
> + * unregistered while you hold the minor.
> + *
> + * Returns:
> + * Pointer to minor-object with increased device-refcount, or PTR_ERR on
> + * failure.
> + */
> +struct drm_minor *drm_minor_acquire(unsigned int minor_id)
> +{
> +	struct drm_minor *minor;
> +
> +	minor = idr_find(&drm_minors_idr, minor_id);
> +	if (!minor)
> +		return ERR_PTR(-ENODEV);
> +
> +	drm_dev_ref(minor->dev);

Is it possible that somebody would drop the last reference on the device
right between the idr_find() call and drm_dev_ref()? In which case both
the device and the minor will have become invalid when drm_dev_ref() is
called.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 06/13] drm: add minor-lookup/release helpers

[David Herrmann]

Hi

On Fri, Feb 21, 2014 at 8:09 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Jan 29, 2014 at 03:01:53PM +0100, David Herrmann wrote:
> [...]
>> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
>> index c51333e..d3232b6 100644
>> --- a/drivers/gpu/drm/drm_stub.c
>> +++ b/drivers/gpu/drm/drm_stub.c
>> @@ -356,6 +356,45 @@ static void drm_unplug_minor(struct drm_minor *minor)
>>  }
>>
>>  /**
>> + * drm_minor_acquire - Acquire a DRM minor
>> + * @minor_id: Minor ID of the DRM-minor
>> + *
>> + * Looks up the given minor-ID and returns the respective DRM-minor object. The
>> + * refence-count of the underlying device is increased so you must release this
>> + * object with drm_minor_release().
>> + *
>> + * As long as you hold this minor, it is guaranteed that the object and the
>> + * minor->dev pointer will stay valid! However, the device may get unplugged and
>> + * unregistered while you hold the minor.
>> + *
>> + * Returns:
>> + * Pointer to minor-object with increased device-refcount, or PTR_ERR on
>> + * failure.
>> + */
>> +struct drm_minor *drm_minor_acquire(unsigned int minor_id)
>> +{
>> +     struct drm_minor *minor;
>> +
>> +     minor = idr_find(&drm_minors_idr, minor_id);
>> +     if (!minor)
>> +             return ERR_PTR(-ENODEV);
>> +
>> +     drm_dev_ref(minor->dev);
>
> Is it possible that somebody would drop the last reference on the device
> right between the idr_find() call and drm_dev_ref()? In which case both
> the device and the minor will have become invalid when drm_dev_ref() is
> called.

No, it's locked via the drm_global_mutex. And later I introduce a
spinlock here that protects this in case we remove the global lock.

Thanks
David

Re: [PATCH 06/13] drm: add minor-lookup/release helpers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1962 bytes --]

On Fri, Feb 21, 2014 at 08:34:15AM +0100, David Herrmann wrote:
> Hi
> 
> On Fri, Feb 21, 2014 at 8:09 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Jan 29, 2014 at 03:01:53PM +0100, David Herrmann wrote:
> > [...]
> >> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> >> index c51333e..d3232b6 100644
> >> --- a/drivers/gpu/drm/drm_stub.c
> >> +++ b/drivers/gpu/drm/drm_stub.c
> >> @@ -356,6 +356,45 @@ static void drm_unplug_minor(struct drm_minor *minor)
> >>  }
> >>
> >>  /**
> >> + * drm_minor_acquire - Acquire a DRM minor
> >> + * @minor_id: Minor ID of the DRM-minor
> >> + *
> >> + * Looks up the given minor-ID and returns the respective DRM-minor object. The
> >> + * refence-count of the underlying device is increased so you must release this
> >> + * object with drm_minor_release().
> >> + *
> >> + * As long as you hold this minor, it is guaranteed that the object and the
> >> + * minor->dev pointer will stay valid! However, the device may get unplugged and
> >> + * unregistered while you hold the minor.
> >> + *
> >> + * Returns:
> >> + * Pointer to minor-object with increased device-refcount, or PTR_ERR on
> >> + * failure.
> >> + */
> >> +struct drm_minor *drm_minor_acquire(unsigned int minor_id)
> >> +{
> >> +     struct drm_minor *minor;
> >> +
> >> +     minor = idr_find(&drm_minors_idr, minor_id);
> >> +     if (!minor)
> >> +             return ERR_PTR(-ENODEV);
> >> +
> >> +     drm_dev_ref(minor->dev);
> >
> > Is it possible that somebody would drop the last reference on the device
> > right between the idr_find() call and drm_dev_ref()? In which case both
> > the device and the minor will have become invalid when drm_dev_ref() is
> > called.
> 
> No, it's locked via the drm_global_mutex. And later I introduce a
> spinlock here that protects this in case we remove the global lock.

Indeed. Thanks for clarifying.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 07/13] drm: allocate minors early

[David Herrmann]

Instead of waiting for device-registration, we now allocate minor-objects
during device allocation. The minors are not registered or assigned an ID.
This is still postponed to device-registration.

While at it, remove the superfluous output-parameter in drm_get_minor().

The reason for this early allocation is to make
dev->primary/control/render available atomically. So once the device is
alive, all of them are already set and we never have the situation where
one of them is set after another (they're either NULL or set, but never
changed). This will eventually allow us to reduce minor-ID allocation to
one base-ID instead of a single ID for each.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_stub.c | 110 +++++++++++++++++++++++++++++----------------
 1 file changed, 71 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index d3232b6..9c2da5f 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -260,21 +260,49 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 	return 0;
 }
 
+static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
+					     unsigned int type)
+{
+	switch (type) {
+	case DRM_MINOR_LEGACY:
+		return &dev->primary;
+	case DRM_MINOR_RENDER:
+		return &dev->render;
+	case DRM_MINOR_CONTROL:
+		return &dev->control;
+	default:
+		return NULL;
+	}
+}
+
+static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
+{
+	struct drm_minor *minor;
+
+	minor = kzalloc(sizeof(*minor), GFP_KERNEL);
+	if (!minor)
+		return -ENOMEM;
+
+	minor->type = type;
+	minor->dev = dev;
+	INIT_LIST_HEAD(&minor->master_list);
+
+	*drm_minor_get_slot(dev, type) = minor;
+	return 0;
+}
+
 /**
- * drm_get_minor - Allocate and register new DRM minor
+ * drm_get_minor - Register DRM minor
  * @dev: DRM device
- * @minor: Pointer to where new minor is stored
  * @type: Type of minor
  *
- * Allocate a new minor of the given type and register it. A pointer to the new
- * minor is returned in @minor.
+ * Register minor of given type.
  * Caller must hold the global DRM mutex.
  *
  * RETURNS:
  * 0 on success, negative error code on failure.
  */
-static int drm_get_minor(struct drm_device *dev, struct drm_minor **minor,
-			 int type)
+static int drm_get_minor(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *new_minor;
 	int ret;
@@ -282,21 +310,16 @@ static int drm_get_minor(struct drm_device *dev, struct drm_minor **minor,
 
 	DRM_DEBUG("\n");
 
+	new_minor = *drm_minor_get_slot(dev, type);
+	if (!new_minor)
+		return 0;
+
 	minor_id = drm_minor_get_id(dev, type);
 	if (minor_id < 0)
 		return minor_id;
 
-	new_minor = kzalloc(sizeof(struct drm_minor), GFP_KERNEL);
-	if (!new_minor) {
-		ret = -ENOMEM;
-		goto err_idr;
-	}
-
-	new_minor->type = type;
 	new_minor->device = MKDEV(DRM_MAJOR, minor_id);
-	new_minor->dev = dev;
 	new_minor->index = minor_id;
-	INIT_LIST_HEAD(&new_minor->master_list);
 
 	idr_replace(&drm_minors_idr, new_minor, minor_id);
 
@@ -314,7 +337,6 @@ static int drm_get_minor(struct drm_device *dev, struct drm_minor **minor,
 		       "DRM: Error sysfs_device_add.\n");
 		goto err_debugfs;
 	}
-	*minor = new_minor;
 
 	DRM_DEBUG("new minor assigned %d\n", minor_id);
 	return 0;
@@ -325,10 +347,7 @@ err_debugfs:
 	drm_debugfs_cleanup(new_minor);
 err_mem:
 #endif
-	kfree(new_minor);
-err_idr:
 	idr_remove(&drm_minors_idr, minor_id);
-	*minor = NULL;
 	return ret;
 }
 
@@ -495,8 +514,24 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 	mutex_init(&dev->struct_mutex);
 	mutex_init(&dev->ctxlist_mutex);
 
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL);
+		if (ret)
+			goto err_minors;
+	}
+
+	if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) {
+		ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
+		if (ret)
+			goto err_minors;
+	}
+
+	ret = drm_minor_alloc(dev, DRM_MINOR_LEGACY);
+	if (ret)
+		goto err_minors;
+
 	if (drm_ht_create(&dev->map_hash, 12))
-		goto err_free;
+		goto err_minors;
 
 	ret = drm_ctxbitmap_init(dev);
 	if (ret) {
@@ -518,7 +553,10 @@ err_ctxbitmap:
 	drm_ctxbitmap_cleanup(dev);
 err_ht:
 	drm_ht_remove(&dev->map_hash);
-err_free:
+err_minors:
+	drm_put_minor(dev->control);
+	drm_put_minor(dev->render);
+	drm_put_minor(dev->primary);
 	kfree(dev);
 	return NULL;
 }
@@ -605,26 +643,22 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 
 	mutex_lock(&drm_global_mutex);
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
-		if (ret)
-			goto out_unlock;
-	}
+	ret = drm_get_minor(dev, DRM_MINOR_CONTROL);
+	if (ret)
+		goto err_minors;
 
-	if (drm_core_check_feature(dev, DRIVER_RENDER) && drm_rnodes) {
-		ret = drm_get_minor(dev, &dev->render, DRM_MINOR_RENDER);
-		if (ret)
-			goto err_control_node;
-	}
+	ret = drm_get_minor(dev, DRM_MINOR_RENDER);
+	if (ret)
+		goto err_minors;
 
-	ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY);
+	ret = drm_get_minor(dev, DRM_MINOR_LEGACY);
 	if (ret)
-		goto err_render_node;
+		goto err_minors;
 
 	if (dev->driver->load) {
 		ret = dev->driver->load(dev, flags);
 		if (ret)
-			goto err_primary_node;
+			goto err_minors;
 	}
 
 	/* setup grouping for legacy outputs */
@@ -641,12 +675,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 err_unload:
 	if (dev->driver->unload)
 		dev->driver->unload(dev);
-err_primary_node:
-	drm_unplug_minor(dev->primary);
-err_render_node:
-	drm_unplug_minor(dev->render);
-err_control_node:
+err_minors:
 	drm_unplug_minor(dev->control);
+	drm_unplug_minor(dev->render);
+	drm_unplug_minor(dev->primary);
 out_unlock:
 	mutex_unlock(&drm_global_mutex);
 	return ret;
-- 
1.8.5.3

[PATCH 08/13] drm: move drm_put_minor() to drm_minor_free()

[David Herrmann]

_put/get() are used for ref-counting, which we clearly don't do here.
Rename it to _free() and also use the common drm_minor_* prefix.
Furthermore, avoid passing the minor directly but instead use the type
like the other functions do, this allows us to reset the slot.

We also drop the redundant call to drm_unplug_minor() as drm_minor_free()
is only used from paths were that has already be called.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_stub.c | 45 ++++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 9c2da5f..1634a09 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -291,6 +291,17 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	return 0;
 }
 
+static void drm_minor_free(struct drm_device *dev, unsigned int type)
+{
+	struct drm_minor **slot;
+
+	slot = drm_minor_get_slot(dev, type);
+	if (*slot) {
+		kfree(*slot);
+		*slot = NULL;
+	}
+}
+
 /**
  * drm_get_minor - Register DRM minor
  * @dev: DRM device
@@ -414,26 +425,6 @@ void drm_minor_release(struct drm_minor *minor)
 }
 
 /**
- * drm_put_minor - Destroy DRM minor
- * @minor: Minor to destroy
- *
- * This calls drm_unplug_minor() on the given minor and then frees it. Nothing
- * is done if @minor is NULL. It is fine to call this on already unplugged
- * minors.
- * The global DRM mutex must be held by the caller.
- */
-static void drm_put_minor(struct drm_minor *minor)
-{
-	if (!minor)
-		return;
-
-	DRM_DEBUG("release secondary minor %d\n", minor->index);
-
-	drm_unplug_minor(minor);
-	kfree(minor);
-}
-
-/**
  * Called via drm_exit() at module unload time or when pci device is
  * unplugged.
  *
@@ -554,9 +545,9 @@ err_ctxbitmap:
 err_ht:
 	drm_ht_remove(&dev->map_hash);
 err_minors:
-	drm_put_minor(dev->control);
-	drm_put_minor(dev->render);
-	drm_put_minor(dev->primary);
+	drm_minor_free(dev, DRM_MINOR_LEGACY);
+	drm_minor_free(dev, DRM_MINOR_RENDER);
+	drm_minor_free(dev, DRM_MINOR_CONTROL);
 	kfree(dev);
 	return NULL;
 }
@@ -572,16 +563,16 @@ EXPORT_SYMBOL(drm_dev_alloc);
  */
 static void drm_dev_free(struct drm_device *dev)
 {
-	drm_put_minor(dev->control);
-	drm_put_minor(dev->render);
-	drm_put_minor(dev->primary);
-
 	if (dev->driver->driver_features & DRIVER_GEM)
 		drm_gem_destroy(dev);
 
 	drm_ctxbitmap_cleanup(dev);
 	drm_ht_remove(&dev->map_hash);
 
+	drm_minor_free(dev, DRM_MINOR_LEGACY);
+	drm_minor_free(dev, DRM_MINOR_RENDER);
+	drm_minor_free(dev, DRM_MINOR_CONTROL);
+
 	kfree(dev->devname);
 	kfree(dev);
 }
-- 
1.8.5.3

[PATCH 09/13] drm: rename drm_unplug/get_minor() to drm_minor_register/unregister()

[David Herrmann]

drm_get_minor() no longer allocates objects, and drm_unplug_minor() is now
the exact reverse of it. Rename it to _register/unregister() so their
name actually says what they do.

Furthermore, remove the direct minor-ptr and instead pass the minor-type.
This way we know the actualy slot of the minor and can reset it if
required.

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

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 1634a09..853026a 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -302,18 +302,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
 	}
 }
 
-/**
- * drm_get_minor - Register DRM minor
- * @dev: DRM device
- * @type: Type of minor
- *
- * Register minor of given type.
- * Caller must hold the global DRM mutex.
- *
- * RETURNS:
- * 0 on success, negative error code on failure.
- */
-static int drm_get_minor(struct drm_device *dev, unsigned int type)
+static int drm_minor_register(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *new_minor;
 	int ret;
@@ -362,18 +351,11 @@ err_mem:
 	return ret;
 }
 
-/**
- * drm_unplug_minor - Unplug DRM minor
- * @minor: Minor to unplug
- *
- * Unplugs the given DRM minor but keeps the object. So after this returns,
- * minor->dev is still valid so existing open-files can still access it to get
- * device information from their drm_file ojects.
- * If the minor is already unplugged or if @minor is NULL, nothing is done.
- * The global DRM mutex must be held by the caller.
- */
-static void drm_unplug_minor(struct drm_minor *minor)
+static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 {
+	struct drm_minor *minor;
+
+	minor = *drm_minor_get_slot(dev, type);
 	if (!minor || !minor->kdev)
 		return;
 
@@ -448,11 +430,9 @@ EXPORT_SYMBOL(drm_put_dev);
 void drm_unplug_dev(struct drm_device *dev)
 {
 	/* for a USB device */
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		drm_unplug_minor(dev->control);
-	if (dev->render)
-		drm_unplug_minor(dev->render);
-	drm_unplug_minor(dev->primary);
+	drm_minor_unregister(dev, DRM_MINOR_LEGACY);
+	drm_minor_unregister(dev, DRM_MINOR_RENDER);
+	drm_minor_unregister(dev, DRM_MINOR_CONTROL);
 
 	mutex_lock(&drm_global_mutex);
 
@@ -634,15 +614,15 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 
 	mutex_lock(&drm_global_mutex);
 
-	ret = drm_get_minor(dev, DRM_MINOR_CONTROL);
+	ret = drm_minor_register(dev, DRM_MINOR_CONTROL);
 	if (ret)
 		goto err_minors;
 
-	ret = drm_get_minor(dev, DRM_MINOR_RENDER);
+	ret = drm_minor_register(dev, DRM_MINOR_RENDER);
 	if (ret)
 		goto err_minors;
 
-	ret = drm_get_minor(dev, DRM_MINOR_LEGACY);
+	ret = drm_minor_register(dev, DRM_MINOR_LEGACY);
 	if (ret)
 		goto err_minors;
 
@@ -667,9 +647,9 @@ err_unload:
 	if (dev->driver->unload)
 		dev->driver->unload(dev);
 err_minors:
-	drm_unplug_minor(dev->control);
-	drm_unplug_minor(dev->render);
-	drm_unplug_minor(dev->primary);
+	drm_minor_unregister(dev, DRM_MINOR_LEGACY);
+	drm_minor_unregister(dev, DRM_MINOR_RENDER);
+	drm_minor_unregister(dev, DRM_MINOR_CONTROL);
 out_unlock:
 	mutex_unlock(&drm_global_mutex);
 	return ret;
@@ -701,8 +681,8 @@ void drm_dev_unregister(struct drm_device *dev)
 	list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head)
 		drm_rmmap(dev, r_list->map);
 
-	drm_unplug_minor(dev->control);
-	drm_unplug_minor(dev->render);
-	drm_unplug_minor(dev->primary);
+	drm_minor_unregister(dev, DRM_MINOR_LEGACY);
+	drm_minor_unregister(dev, DRM_MINOR_RENDER);
+	drm_minor_unregister(dev, DRM_MINOR_CONTROL);
 }
 EXPORT_SYMBOL(drm_dev_unregister);
-- 
1.8.5.3

Re: [PATCH 09/13] drm: rename drm_unplug/get_minor() to drm_minor_register/unregister()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 445 bytes --]

On Wed, Jan 29, 2014 at 03:01:56PM +0100, David Herrmann wrote:
> drm_get_minor() no longer allocates objects, and drm_unplug_minor() is now
> the exact reverse of it. Rename it to _register/unregister() so their
> name actually says what they do.
> 
> Furthermore, remove the direct minor-ptr and instead pass the minor-type.
> This way we know the actualy slot of the minor and can reset it if

Nit: "actualy" -> "actual".

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 09/13] drm: rename drm_unplug/get_minor() to drm_minor_register/unregister()

[David Herrmann]

Hi

On Fri, Feb 21, 2014 at 8:21 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Jan 29, 2014 at 03:01:56PM +0100, David Herrmann wrote:
>> drm_get_minor() no longer allocates objects, and drm_unplug_minor() is now
>> the exact reverse of it. Rename it to _register/unregister() so their
>> name actually says what they do.
>>
>> Furthermore, remove the direct minor-ptr and instead pass the minor-type.
>> This way we know the actualy slot of the minor and can reset it if
>
> Nit: "actualy" -> "actual".

Fixed.

Thanks
David

[PATCH 10/13] drm: remove unneeded #ifdef CONFIG_DEBUGFS

[David Herrmann]

No need to check for DEBUGFS, we already have dummy-fallbacks in our
headers.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_stub.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 853026a..d564a5d 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -323,13 +323,11 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 
 	idr_replace(&drm_minors_idr, new_minor, minor_id);
 
-#if defined(CONFIG_DEBUG_FS)
 	ret = drm_debugfs_init(new_minor, minor_id, drm_debugfs_root);
 	if (ret) {
 		DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
 		goto err_mem;
 	}
-#endif
 
 	ret = drm_sysfs_device_add(new_minor);
 	if (ret) {
@@ -343,10 +341,8 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 
 
 err_debugfs:
-#if defined(CONFIG_DEBUG_FS)
 	drm_debugfs_cleanup(new_minor);
 err_mem:
-#endif
 	idr_remove(&drm_minors_idr, minor_id);
 	return ret;
 }
@@ -359,10 +355,7 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 	if (!minor || !minor->kdev)
 		return;
 
-#if defined(CONFIG_DEBUG_FS)
 	drm_debugfs_cleanup(minor);
-#endif
-
 	drm_sysfs_device_remove(minor);
 	idr_remove(&drm_minors_idr, minor->index);
 }
-- 
1.8.5.3

[PATCH 11/13] drm: remove redundant minor->device field

[David Herrmann]

Whenever we access minor->device, we are in a minor->kdev->...->fops
callback so the minor->kdev pointer *must* be valid. Thus, simply use
minor->kdev->devt instead of minor->device and remove the redundant field.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_drv.c  | 4 ++--
 drivers/gpu/drm/drm_fops.c | 2 +-
 drivers/gpu/drm/drm_stub.c | 1 -
 include/drm/drmP.h         | 1 -
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 345be03..ec651be 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -344,7 +344,7 @@ long drm_ioctl(struct file *filp,
 
 	DRM_DEBUG("pid=%d, dev=0x%lx, auth=%d, %s\n",
 		  task_pid_nr(current),
-		  (long)old_encode_dev(file_priv->minor->device),
+		  (long)old_encode_dev(file_priv->minor->kdev->devt),
 		  file_priv->authenticated, ioctl->name);
 
 	/* Do not trust userspace, use our own definition */
@@ -402,7 +402,7 @@ long drm_ioctl(struct file *filp,
 	if (!ioctl)
 		DRM_DEBUG("invalid ioctl: pid=%d, dev=0x%lx, auth=%d, cmd=0x%02x, nr=0x%02x\n",
 			  task_pid_nr(current),
-			  (long)old_encode_dev(file_priv->minor->device),
+			  (long)old_encode_dev(file_priv->minor->kdev->devt),
 			  file_priv->authenticated, cmd, nr);
 
 	if (kdata != stack_kdata)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 7947819..4ce5318 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -471,7 +471,7 @@ int drm_release(struct inode *inode, struct file *filp)
 
 	DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
 		  task_pid_nr(current),
-		  (long)old_encode_dev(file_priv->minor->device),
+		  (long)old_encode_dev(file_priv->minor->kdev->devt),
 		  dev->open_count);
 
 	/* Release any auth tokens that might point to this file_priv,
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index d564a5d..fa9542e 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -318,7 +318,6 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 	if (minor_id < 0)
 		return minor_id;
 
-	new_minor->device = MKDEV(DRM_MAJOR, minor_id);
 	new_minor->index = minor_id;
 
 	idr_replace(&drm_minors_idr, new_minor, minor_id);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c02cad..114d46f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1040,7 +1040,6 @@ struct drm_info_node {
 struct drm_minor {
 	int index;			/**< Minor device number */
 	int type;                       /**< Control or render */
-	dev_t device;			/**< Device number for mknod */
 	struct device *kdev;		/**< Linux device */
 	struct drm_device *dev;
 
-- 
1.8.5.3

Re: [PATCH 11/13] drm: remove redundant minor->device field

[Daniel Vetter]

On Wed, Jan 29, 2014 at 03:01:58PM +0100, David Herrmann wrote:
> Whenever we access minor->device, we are in a minor->kdev->...->fops
> callback so the minor->kdev pointer *must* be valid. Thus, simply use
> minor->kdev->devt instead of minor->device and remove the redundant field.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

I think this is simply compat cruft from the days when the drm core was
still shared with the *bsds. With the one patch I've commented on all
patches up to this one are

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

As discussed on irc I think we don't want to have stable minor ids really,
userspace simply needs to inquire udev to get at the right
render/control/legacy node it wants. There's also ongoing discussions
about how that probing should look like on e.g. ARM. So at least for now
Nack on those two from my side.

Wrt longer-term plans I think the minors (and also all the sysfs stuff
really) also need to grab references on the drm_device, so that unplugging
doesn't race with userspace. But I guess that this might lead to reference
loops for driver unloading, so maybe we need to postpone that a bit more.
But this is definitely a big step into the right direction.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 11/13] drm: remove redundant minor->device field

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1184 bytes --]

On Wed, Feb 12, 2014 at 02:36:24PM +0100, Daniel Vetter wrote:
> On Wed, Jan 29, 2014 at 03:01:58PM +0100, David Herrmann wrote:
> > Whenever we access minor->device, we are in a minor->kdev->...->fops
> > callback so the minor->kdev pointer *must* be valid. Thus, simply use
> > minor->kdev->devt instead of minor->device and remove the redundant field.
> > 
> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> 
> I think this is simply compat cruft from the days when the drm core was
> still shared with the *bsds. With the one patch I've commented on all
> patches up to this one are
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> As discussed on irc I think we don't want to have stable minor ids really,
> userspace simply needs to inquire udev to get at the right
> render/control/legacy node it wants.

Does that mean we should go all the way and don't keep the +64 (for
control) and +128 (for render nodes) offsets either? Should it be
possible to have a /dev/dri directory that looks somewhat like this:

	/dev/dri/card0    (GPU#0, legacy)
	/dev/dri/card1    (GPU#1, legacy)
	/dev/dri/render0  (GPU#1, render)

?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 11/13] drm: remove redundant minor->device field

[David Herrmann]

Hi

On Fri, Feb 21, 2014 at 8:30 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Feb 12, 2014 at 02:36:24PM +0100, Daniel Vetter wrote:
>> On Wed, Jan 29, 2014 at 03:01:58PM +0100, David Herrmann wrote:
>> > Whenever we access minor->device, we are in a minor->kdev->...->fops
>> > callback so the minor->kdev pointer *must* be valid. Thus, simply use
>> > minor->kdev->devt instead of minor->device and remove the redundant field.
>> >
>> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>
>> I think this is simply compat cruft from the days when the drm core was
>> still shared with the *bsds. With the one patch I've commented on all
>> patches up to this one are
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> As discussed on irc I think we don't want to have stable minor ids really,
>> userspace simply needs to inquire udev to get at the right
>> render/control/legacy node it wants.
>
> Does that mean we should go all the way and don't keep the +64 (for
> control) and +128 (for render nodes) offsets either? Should it be
> possible to have a /dev/dri directory that looks somewhat like this:
>
>         /dev/dri/card0    (GPU#0, legacy)
>         /dev/dri/card1    (GPU#1, legacy)
>         /dev/dri/render0  (GPU#1, render)

That might break backwards compat, but may be worth it. However, we
*have* to keep the +64 / +128 offsets for minor numbers. There's
already user-space using that for dev-type testing (which is fine!).

Thanks
David

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

[David Herrmann]

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

[PATCH 13/13] drm: remove redundant minor->index

[David Herrmann]

The index of a minor can be easily calculated, no reason to store it.
Furthermore, it's actually only really used in drm_sysfs.c during
device-registration, so we don't win anything by caching it. All other
uses of it are dev->primary->index accesses, which are equivalent to
dev->minor_base now.

Replace all occurrences and drop minor->index. A small helper
drm_minor_get_id() is added, so drm_sysfs.c does not have to hard-code the
+64 offset, which is an implementation detail of drm_stub.c.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_fops.c                      |  3 ++-
 drivers/gpu/drm/drm_info.c                      |  2 +-
 drivers/gpu/drm/drm_pci.c                       |  2 +-
 drivers/gpu/drm/drm_platform.c                  |  2 +-
 drivers/gpu/drm/drm_stub.c                      | 17 ++++++++++++++---
 drivers/gpu/drm/drm_sysfs.c                     |  8 +++++---
 drivers/gpu/drm/drm_usb.c                       |  2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c           |  2 +-
 drivers/gpu/drm/i915/i915_trace.h               | 20 ++++++++++----------
 drivers/gpu/drm/msm/msm_fbdev.c                 |  2 +-
 drivers/gpu/drm/omapdrm/omap_fbdev.c            |  2 +-
 drivers/gpu/drm/radeon/atombios_encoders.c      |  2 +-
 drivers/gpu/drm/radeon/radeon_legacy_encoders.c |  2 +-
 drivers/gpu/drm/radeon/radeon_trace.h           |  2 +-
 drivers/gpu/drm/tegra/bus.c                     |  2 +-
 include/drm/drmP.h                              |  2 +-
 16 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index fdf35cd..d37695e 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -217,7 +217,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 	if (dev->switch_power_state != DRM_SWITCH_POWER_ON && dev->switch_power_state != DRM_SWITCH_POWER_DYNAMIC_OFF)
 		return -EINVAL;
 
-	DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor->index);
+	DRM_DEBUG("pid = %d, devt = 0x%lx\n", task_pid_nr(current),
+		  (long)old_encode_dev(minor->kdev->devt));
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index 7473035..b5a4b0a 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -190,7 +190,7 @@ int drm_clients_info(struct seq_file *m, void *data)
 	list_for_each_entry(priv, &dev->filelist, lhead) {
 		seq_printf(m, "%c %3d %5d %5d %10u\n",
 			   priv->authenticated ? 'y' : 'n',
-			   priv->minor->index,
+			   drm_minor_get_id(priv->minor),
 			   pid_vnr(priv->pid),
 			   from_kuid_munged(seq_user_ns(m), priv->uid),
 			   priv->magic);
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 9ded847..4e6f5ea 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -338,7 +338,7 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
 
 	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
 		 driver->name, driver->major, driver->minor, driver->patchlevel,
-		 driver->date, pci_name(pdev), dev->primary->index);
+		 driver->date, pci_name(pdev), dev->minor_base);
 
 	/* No locking needed since shadow-attach is single-threaded since it may
 	 * only be called from the per-driver module init hook. */
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 319ff53..7521571 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -59,7 +59,7 @@ static int drm_get_platform_dev(struct platform_device *platdev,
 
 	DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n",
 		 driver->name, driver->major, driver->minor, driver->patchlevel,
-		 driver->date, dev->primary->index);
+		 driver->date, dev->minor_base);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 0f84bf6..2567ecb 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -339,7 +339,6 @@ 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);
 
@@ -369,7 +368,8 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 	if (!new_minor)
 		return 0;
 
-	ret = drm_debugfs_init(new_minor, new_minor->index, drm_debugfs_root);
+	ret = drm_debugfs_init(new_minor, drm_minor_get_id(new_minor),
+			       drm_debugfs_root);
 	if (ret) {
 		DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
 		return ret;
@@ -381,7 +381,7 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 		goto err_debugfs;
 	}
 
-	DRM_DEBUG("new minor assigned %d\n", new_minor->index);
+	DRM_DEBUG("new minor assigned %d\n", drm_minor_get_id(new_minor));
 	return 0;
 
 err_debugfs:
@@ -459,6 +459,17 @@ void drm_minor_release(struct drm_minor *minor)
 }
 
 /**
+ * drm_minor_get_id - Return minor-ID of DRM-minor object
+ * @minor: DRM-minor object
+ *
+ * Returns the minor-ID of the given DRM-minor object.
+ */
+unsigned int drm_minor_get_id(struct drm_minor *minor)
+{
+	return drm_minor_type_to_id(minor->dev, minor->type);
+}
+
+/**
  * Called via drm_exit() at module unload time or when pci device is
  * unplugged.
  *
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index c22c309..9be02d9 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -380,7 +380,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 
 	connector->kdev = device_create(drm_class, dev->primary->kdev,
 					0, connector, "card%d-%s",
-					dev->primary->index, drm_get_connector_name(connector));
+					dev->minor_base, drm_get_connector_name(connector));
 	DRM_DEBUG("adding \"%s\" to sysfs\n",
 		  drm_get_connector_name(connector));
 
@@ -505,6 +505,7 @@ static void drm_sysfs_release(struct device *dev)
  */
 int drm_sysfs_device_add(struct drm_minor *minor)
 {
+	unsigned int minor_id;
 	char *minor_str;
 	int r;
 
@@ -521,15 +522,16 @@ int drm_sysfs_device_add(struct drm_minor *minor)
 		goto error;
 	}
 
+	minor_id = drm_minor_get_id(minor);
 	device_initialize(minor->kdev);
-	minor->kdev->devt = MKDEV(DRM_MAJOR, minor->index);
+	minor->kdev->devt = MKDEV(DRM_MAJOR, minor_id);
 	minor->kdev->class = drm_class;
 	minor->kdev->type = &drm_sysfs_device_minor;
 	minor->kdev->parent = minor->dev->dev;
 	minor->kdev->release = drm_sysfs_release;
 	dev_set_drvdata(minor->kdev, minor);
 
-	r = dev_set_name(minor->kdev, minor_str, minor->index);
+	r = dev_set_name(minor->kdev, minor_str, minor_id);
 	if (r < 0)
 		goto error;
 
diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
index c3406aa..eb02ad3 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -25,7 +25,7 @@ int drm_get_usb_dev(struct usb_interface *interface,
 
 	DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n",
 		 driver->name, driver->major, driver->minor, driver->patchlevel,
-		 driver->date, dev->primary->index);
+		 driver->date, dev->minor_base);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index a707cca..07442fb 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -914,7 +914,7 @@ void i915_capture_error_state(struct drm_device *dev)
 	}
 
 	DRM_INFO("GPU crash dump saved to /sys/class/drm/card%d/error\n",
-		 dev->primary->index);
+		 dev->minor_base);
 	DRM_INFO("GPU hangs can indicate a bug anywhere in the entire gfx stack, including userspace.\n");
 	DRM_INFO("Please file a _new_ bug report on bugs.freedesktop.org against DRI -> DRM/Intel\n");
 	DRM_INFO("drm/i915 developers can then reassign to the right component if it's not a kernel issue.\n");
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 6e580c9..a03e4e2 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -207,7 +207,7 @@ TRACE_EVENT(i915_gem_evict,
 			    ),
 
 	    TP_fast_assign(
-			   __entry->dev = dev->primary->index;
+			   __entry->dev = dev->minor_base;
 			   __entry->size = size;
 			   __entry->align = align;
 			   __entry->mappable = mappable;
@@ -227,7 +227,7 @@ TRACE_EVENT(i915_gem_evict_everything,
 			    ),
 
 	    TP_fast_assign(
-			   __entry->dev = dev->primary->index;
+			   __entry->dev = dev->minor_base;
 			  ),
 
 	    TP_printk("dev=%d", __entry->dev)
@@ -245,7 +245,7 @@ TRACE_EVENT(i915_gem_evict_vm,
 			   __entry->vm = vm;
 			  ),
 
-	    TP_printk("dev=%d, vm=%p", __entry->vm->dev->primary->index, __entry->vm)
+	    TP_printk("dev=%d, vm=%p", __entry->vm->dev->minor_base, __entry->vm)
 );
 
 TRACE_EVENT(i915_gem_ring_sync_to,
@@ -262,7 +262,7 @@ TRACE_EVENT(i915_gem_ring_sync_to,
 			     ),
 
 	    TP_fast_assign(
-			   __entry->dev = from->dev->primary->index;
+			   __entry->dev = from->dev->minor_base;
 			   __entry->sync_from = from->id;
 			   __entry->sync_to = to->id;
 			   __entry->seqno = seqno;
@@ -286,7 +286,7 @@ TRACE_EVENT(i915_gem_ring_dispatch,
 			     ),
 
 	    TP_fast_assign(
-			   __entry->dev = ring->dev->primary->index;
+			   __entry->dev = ring->dev->minor_base;
 			   __entry->ring = ring->id;
 			   __entry->seqno = seqno;
 			   __entry->flags = flags;
@@ -309,7 +309,7 @@ TRACE_EVENT(i915_gem_ring_flush,
 			     ),
 
 	    TP_fast_assign(
-			   __entry->dev = ring->dev->primary->index;
+			   __entry->dev = ring->dev->minor_base;
 			   __entry->ring = ring->id;
 			   __entry->invalidate = invalidate;
 			   __entry->flush = flush;
@@ -331,7 +331,7 @@ DECLARE_EVENT_CLASS(i915_gem_request,
 			     ),
 
 	    TP_fast_assign(
-			   __entry->dev = ring->dev->primary->index;
+			   __entry->dev = ring->dev->minor_base;
 			   __entry->ring = ring->id;
 			   __entry->seqno = seqno;
 			   ),
@@ -356,7 +356,7 @@ TRACE_EVENT(i915_gem_request_complete,
 			     ),
 
 	    TP_fast_assign(
-			   __entry->dev = ring->dev->primary->index;
+			   __entry->dev = ring->dev->minor_base;
 			   __entry->ring = ring->id;
 			   __entry->seqno = ring->get_seqno(ring, false);
 			   ),
@@ -388,7 +388,7 @@ TRACE_EVENT(i915_gem_request_wait_begin,
 	     * less desirable.
 	     */
 	    TP_fast_assign(
-			   __entry->dev = ring->dev->primary->index;
+			   __entry->dev = ring->dev->minor_base;
 			   __entry->ring = ring->id;
 			   __entry->seqno = seqno;
 			   __entry->blocking = mutex_is_locked(&ring->dev->struct_mutex);
@@ -414,7 +414,7 @@ DECLARE_EVENT_CLASS(i915_ring,
 			     ),
 
 	    TP_fast_assign(
-			   __entry->dev = ring->dev->primary->index;
+			   __entry->dev = ring->dev->minor_base;
 			   __entry->ring = ring->id;
 			   ),
 
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index 6c6d7d4..5fc48b9 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -83,7 +83,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
 
 	/* allocate backing bo */
 	size = mode_cmd.pitches[0] * mode_cmd.height;
-	DBG("allocating %d bytes for fb %d", size, dev->primary->index);
+	DBG("allocating %d bytes for fb %d", size, dev->minor_base);
 	mutex_lock(&dev->struct_mutex);
 	fbdev->bo = msm_gem_new(dev, size, MSM_BO_SCANOUT | MSM_BO_WC);
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 002988d..bd9f9a7 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -176,7 +176,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	gsize = (union omap_gem_size){
 		.bytes = PAGE_ALIGN(mode_cmd.pitches[0] * mode_cmd.height),
 	};
-	DBG("allocating %d bytes for fb %d", gsize.bytes, dev->primary->index);
+	DBG("allocating %d bytes for fb %d", gsize.bytes, dev->minor_base);
 	fbdev->bo = omap_gem_new(dev, gsize, OMAP_BO_SCANOUT | OMAP_BO_WC);
 	if (!fbdev->bo) {
 		dev_err(dev->dev, "failed to allocate buffer object\n");
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
index a42d615..f5cc894 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -212,7 +212,7 @@ void radeon_atom_backlight_init(struct radeon_encoder *radeon_encoder,
 	props.max_brightness = RADEON_MAX_BL_LEVEL;
 	props.type = BACKLIGHT_RAW;
 	snprintf(bl_name, sizeof(bl_name),
-		 "radeon_bl%d", dev->primary->index);
+		 "radeon_bl%d", dev->minor_base);
 	bd = backlight_device_register(bl_name, drm_connector->kdev,
 				       pdata, &radeon_atom_backlight_ops, &props);
 	if (IS_ERR(bd)) {
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
index c89971d..ef82bdb 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
@@ -391,7 +391,7 @@ void radeon_legacy_backlight_init(struct radeon_encoder *radeon_encoder,
 	props.max_brightness = RADEON_MAX_BL_LEVEL;
 	props.type = BACKLIGHT_RAW;
 	snprintf(bl_name, sizeof(bl_name),
-		 "radeon_bl%d", dev->primary->index);
+		 "radeon_bl%d", dev->minor_base);
 	bd = backlight_device_register(bl_name, drm_connector->kdev,
 				       pdata, &radeon_backlight_ops, &props);
 	if (IS_ERR(bd)) {
diff --git a/drivers/gpu/drm/radeon/radeon_trace.h b/drivers/gpu/drm/radeon/radeon_trace.h
index 0473257..84b56df 100644
--- a/drivers/gpu/drm/radeon/radeon_trace.h
+++ b/drivers/gpu/drm/radeon/radeon_trace.h
@@ -116,7 +116,7 @@ DECLARE_EVENT_CLASS(radeon_fence_request,
 			     ),
 
 	    TP_fast_assign(
-			   __entry->dev = dev->primary->index;
+			   __entry->dev = dev->minor_base;
 			   __entry->seqno = seqno;
 			   ),
 
diff --git a/drivers/gpu/drm/tegra/bus.c b/drivers/gpu/drm/tegra/bus.c
index 71cef5c..7d2aace 100644
--- a/drivers/gpu/drm/tegra/bus.c
+++ b/drivers/gpu/drm/tegra/bus.c
@@ -58,7 +58,7 @@ int drm_host1x_init(struct drm_driver *driver, struct host1x_device *device)
 
 	DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n", driver->name,
 		 driver->major, driver->minor, driver->patchlevel,
-		 driver->date, drm->primary->index);
+		 driver->date, drm->minor_base);
 
 	return 0;
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index cdc5362..5339a9c 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1038,7 +1038,6 @@ struct drm_info_node {
  * DRM minor structure. This structure represents a drm minor number.
  */
 struct drm_minor {
-	int index;			/**< Minor device number */
 	int type;                       /**< Control or render */
 	struct device *kdev;		/**< Linux device */
 	struct drm_device *dev;
@@ -1670,6 +1669,7 @@ void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
 
+unsigned int drm_minor_get_id(struct drm_minor *minor);
 struct drm_minor *drm_minor_acquire(unsigned int minor_id);
 void drm_minor_release(struct drm_minor *minor);
 
-- 
1.8.5.3

Re: [PATCH 00/13] DRM Reliable Minor-IDs

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1202 bytes --]

On Wed, Jan 29, 2014 at 03:01:47PM +0100, David Herrmann wrote:
> Hi
> 
> I was looking into our minor-allocation code and it has one major drawback:
> char-dev minor-IDs are unreliable. The <id>+128 hacks we use in user-space to
> calculate render-node IDs based on the original card just does not work.
> 
> Instead of allocating dummy IDs for each driver, I went ahead and tried to fix
> it properly. So this series changes the minor-ID management to just allocate a
> single dev->minor_base ID instead of one ID per "drm_minor". Now we can use this
> base to calculate the correct offset minor-ID for each existing DRM-minor.
> 
> While at it, I introduced drm-refcounts to make minor-lookup independent of
> drm_global_mutex. This is still not finished (and dev->open_count still exists)
> but I already have the next patches waiting here.
> 
> Comments welcome!
> 
> Branch is also available here:
>   http://cgit.freedesktop.org/~dvdhrm/linux/log/?h=minor
> If someone could pull this into their tree and push to Fengguang's
> test-framework, I'd appreciate it a lot! I'm still waiting for a reply from him.

The series:

Tested-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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