From 6796cb16c088905bf3af40548fda68c09e6f6ee5 Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Fri, 3 Jan 2014 14:24:19 +0100 Subject: drm: use anon-inode instead of relying on cdevs DRM drivers share a common address_space across all character-devices of a single DRM device. This allows simple buffer eviction and mapping-control. However, DRM core currently waits for the first ->open() on any char-dev to mark the underlying inode as backing inode of the device. This delayed initialization causes ugly conditions all over the place: if (dev->dev_mapping) do_sth(); To avoid delayed initialization and to stop reusing the inode of the char-dev, we allocate an anonymous inode for each DRM device and reset filp->f_mapping to it on ->open(). Signed-off-by: David Herrmann --- drivers/gpu/drm/drm_fops.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) (limited to 'drivers/gpu/drm/drm_fops.c') diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 7f2af9aca038..147a84d9da9b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -84,8 +84,6 @@ int drm_open(struct inode *inode, struct file *filp) struct drm_minor *minor; int retcode = 0; int need_setup = 0; - struct address_space *old_mapping; - struct address_space *old_imapping; minor = idr_find(&drm_minors_idr, minor_id); if (!minor) @@ -99,16 +97,9 @@ int drm_open(struct inode *inode, struct file *filp) if (!dev->open_count++) need_setup = 1; - mutex_lock(&dev->struct_mutex); - old_imapping = inode->i_mapping; - old_mapping = dev->dev_mapping; - if (old_mapping == NULL) - dev->dev_mapping = &inode->i_data; - /* ihold ensures nobody can remove inode with our i_data */ - ihold(container_of(dev->dev_mapping, struct inode, i_data)); - inode->i_mapping = dev->dev_mapping; - filp->f_mapping = dev->dev_mapping; - mutex_unlock(&dev->struct_mutex); + + /* share address_space across all char-devs of a single device */ + filp->f_mapping = dev->anon_inode->i_mapping; retcode = drm_open_helper(inode, filp, dev); if (retcode) @@ -121,12 +112,6 @@ int drm_open(struct inode *inode, struct file *filp) return 0; err_undo: - mutex_lock(&dev->struct_mutex); - filp->f_mapping = old_imapping; - inode->i_mapping = old_imapping; - iput(container_of(dev->dev_mapping, struct inode, i_data)); - dev->dev_mapping = old_mapping; - mutex_unlock(&dev->struct_mutex); dev->open_count--; return retcode; } @@ -434,7 +419,6 @@ int drm_lastclose(struct drm_device * dev) drm_legacy_dma_takedown(dev); - dev->dev_mapping = NULL; mutex_unlock(&dev->struct_mutex); drm_legacy_dev_reinit(dev); @@ -549,9 +533,6 @@ int drm_release(struct inode *inode, struct file *filp) } } - BUG_ON(dev->dev_mapping == NULL); - iput(container_of(dev->dev_mapping, struct inode, i_data)); - /* drop the reference held my the file priv */ if (file_priv->master) drm_master_put(&file_priv->master); -- cgit v1.2.3 From f4aede2e3291896e7cb42755ecc5b6815b6cac97 Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Wed, 29 Jan 2014 10:18:02 +0100 Subject: drm: skip redundant minor-lookup in open path 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 Reviewed-by: Daniel Vetter --- drivers/gpu/drm/drm_fops.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) (limited to 'drivers/gpu/drm/drm_fops.c') diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 7f2af9aca038..6466cb5d8b1f 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; -- cgit v1.2.3 From 1616c525b98deb34b8f4b02eccf0ae3a1310fa27 Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Wed, 29 Jan 2014 10:49:19 +0100 Subject: drm: add minor-lookup/release helpers 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 Reviewed-by: Daniel Vetter --- 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(-) (limited to 'drivers/gpu/drm/drm_fops.c') diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 6466cb5d8b1f..79478191404a 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 f2f0249304b7..269048215e82 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -355,6 +355,45 @@ static void drm_unplug_minor(struct drm_minor *minor) idr_remove(&drm_minors_idr, minor->index); } +/** + * 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 4e53f1607355..82963167f161 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1672,6 +1672,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 */ -- cgit v1.2.3 From 5817878c6f4221c3ace4af63260080635063371e Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Wed, 29 Jan 2014 13:12:31 +0100 Subject: drm: remove redundant minor->device field 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 Reviewed-by: Daniel Vetter --- 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(-) (limited to 'drivers/gpu/drm/drm_fops.c') diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 345be03c23db..ec651be2f3cb 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 79478191404a..4ce5318d14bc 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 fe9595b750ea..96fe5dec3822 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 82963167f161..538079030be0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1043,7 +1043,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; -- cgit v1.2.3 From 0d639883ee26359e1bf38195df1dbca0f879e239 Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Mon, 24 Feb 2014 15:53:25 +0100 Subject: drm: make minors independent of global lock We used to protect minor-lookup and setup by the global drm lock. To continue our attempts of dropping drm_global_mutex, this patch makes the minor management independent of it. Furthermore, we make it all atomic and switch to spin-locks instead of a mutex. Now that minor-lookup is independent, we also move the "drm_is_unplugged()" test into the minor-lookup path. There is no reason to ever return a minor for unplugged objects, so keep that logic internal. Signed-off-by: David Herrmann --- drivers/gpu/drm/drm_fops.c | 11 +--------- drivers/gpu/drm/drm_stub.c | 51 ++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 46 insertions(+), 16 deletions(-) (limited to 'drivers/gpu/drm/drm_fops.c') diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 4ce5318d14bc..8f46fe273ba3 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -39,7 +39,7 @@ #include #include -/* from BKL pushdown: note that nothing else serializes idr_find() */ +/* from BKL pushdown */ DEFINE_MUTEX(drm_global_mutex); EXPORT_SYMBOL(drm_global_mutex); @@ -91,11 +91,6 @@ int drm_open(struct inode *inode, struct file *filp) return PTR_ERR(minor); dev = minor->dev; - if (drm_device_is_unplugged(dev)) { - retcode = -ENODEV; - goto err_release; - } - if (!dev->open_count++) need_setup = 1; mutex_lock(&dev->struct_mutex); @@ -127,7 +122,6 @@ err_undo: dev->dev_mapping = old_mapping; mutex_unlock(&dev->struct_mutex); dev->open_count--; -err_release: drm_minor_release(minor); return retcode; } @@ -157,9 +151,6 @@ int drm_stub_open(struct inode *inode, struct file *filp) goto out_unlock; dev = minor->dev; - if (drm_device_is_unplugged(dev)) - goto out_release; - new_fops = fops_get(dev->driver->fops); if (!new_fops) goto out_release; diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 83ef4a63358c..c23eaf6442ff 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; @@ -240,6 +241,19 @@ 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. + * + * 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 struct drm_minor **drm_minor_get_slot(struct drm_device *dev, unsigned int type) { @@ -285,6 +299,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type) static int drm_minor_register(struct drm_device *dev, unsigned int type) { struct drm_minor *new_minor; + unsigned long flags; int ret; int minor_id; @@ -294,19 +309,21 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) if (!new_minor) return 0; + idr_preload(GFP_KERNEL); + spin_lock_irqsave(&drm_minor_lock, flags); minor_id = idr_alloc(&drm_minors_idr, NULL, 64 * type, 64 * (type + 1), - GFP_KERNEL); + GFP_NOWAIT); + spin_unlock_irqrestore(&drm_minor_lock, flags); + idr_preload_end(); 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); if (ret) { DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n"); @@ -319,27 +336,40 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) goto err_debugfs; } + /* replace NULL with @minor so lookups will succeed from now on */ + spin_lock_irqsave(&drm_minor_lock, flags); + idr_replace(&drm_minors_idr, new_minor, new_minor->index); + spin_unlock_irqrestore(&drm_minor_lock, flags); + DRM_DEBUG("new minor assigned %d\n", minor_id); return 0; err_debugfs: drm_debugfs_cleanup(new_minor); err_id: + spin_lock_irqsave(&drm_minor_lock, flags); idr_remove(&drm_minors_idr, minor_id); + spin_unlock_irqrestore(&drm_minor_lock, flags); + new_minor->index = 0; return ret; } static void drm_minor_unregister(struct drm_device *dev, unsigned int type) { struct drm_minor *minor; + unsigned long flags; minor = *drm_minor_get_slot(dev, type); if (!minor || !minor->kdev) return; + spin_lock_irqsave(&drm_minor_lock, flags); + idr_remove(&drm_minors_idr, minor->index); + spin_unlock_irqrestore(&drm_minor_lock, flags); + minor->index = 0; + drm_debugfs_cleanup(minor); drm_sysfs_device_remove(minor); - idr_remove(&drm_minors_idr, minor->index); } /** @@ -361,12 +391,21 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type) struct drm_minor *drm_minor_acquire(unsigned int minor_id) { struct drm_minor *minor; + unsigned long flags; + spin_lock_irqsave(&drm_minor_lock, flags); minor = idr_find(&drm_minors_idr, minor_id); - if (!minor) + if (minor) + drm_dev_ref(minor->dev); + spin_unlock_irqrestore(&drm_minor_lock, flags); + + if (!minor) { + return ERR_PTR(-ENODEV); + } else if (drm_device_is_unplugged(minor->dev)) { + drm_dev_unref(minor->dev); return ERR_PTR(-ENODEV); + } - drm_dev_ref(minor->dev); return minor; } -- cgit v1.2.3 From ac05dbc57ef2b8709bf48693bb25e16a63e8e71f Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Wed, 19 Feb 2014 14:21:48 +0100 Subject: drm: Make control nodes master-less v3 Like for render-nodes, there is no point in maintaining the master concept for control nodes, so set the struct drm_file::master pointer to NULL. At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always allowed when called through the control node. Previously the caller also needed to be master. v2: Adapt to refactoring of ioctl permission check. v3: Formatting of logical expression. Use drm_is_control_client() instead of drm_is_control(). Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul Reviewed-by: David Herrmann --- drivers/gpu/drm/drm_drv.c | 7 ++++--- drivers/gpu/drm/drm_fops.c | 6 ++++-- include/drm/drmP.h | 5 +++++ 3 files changed, 13 insertions(+), 5 deletions(-) (limited to 'drivers/gpu/drm/drm_fops.c') diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 05e30530c025..cf2dfb790bf1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -306,13 +306,14 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) !file_priv->authenticated)) return -EACCES; - /* MASTER is only for master */ - if (unlikely((flags & DRM_MASTER) && !file_priv->is_master)) + /* MASTER is only for master or control clients */ + if (unlikely((flags & DRM_MASTER) && !file_priv->is_master && + !drm_is_control_client(file_priv))) return -EACCES; /* Control clients must be explicitly allowed */ if (unlikely(!(flags & DRM_CONTROL_ALLOW) && - file_priv->minor->type == DRM_MINOR_CONTROL)) + drm_is_control_client(file_priv))) return -EACCES; /* Render clients must be explicitly allowed */ diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 9b02f126fb0d..5432a1a61c15 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -232,7 +232,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* if there is no current master make this fd it, but do not create * any master object for render clients */ mutex_lock(&dev->struct_mutex); - if (!priv->minor->master && !drm_is_render_client(priv)) { + if (!priv->minor->master && !drm_is_render_client(priv) && + !drm_is_control_client(priv)) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -270,7 +271,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, goto out_close; } } - } else if (!drm_is_render_client(priv)) { + } else if (!drm_is_render_client(priv) && + !drm_is_control_client(priv)) { /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2242968e7deb..3cf9f46ce2e6 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1207,6 +1207,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_RENDER; } +static inline bool drm_is_control_client(const struct drm_file *file_priv) +{ + return file_priv->minor->type == DRM_MINOR_CONTROL; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/ -- cgit v1.2.3 From 436830571eb9045d563979dc6185b1d5145ca4b6 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 13 Mar 2014 11:07:44 +0100 Subject: drm: Improve on minor type helpers v3 Add a drm_is_legacy() helper, constify argument to drm_is_render_client(), and use / change helpers where appropriate. v2: s/drm_is_legacy/drm_is_legacy_client/ and adapt to new code context. v3: s/legacy_client/primary_client/ Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/drm_crtc.c | 4 ++-- drivers/gpu/drm/drm_fops.c | 6 ++---- include/drm/drmP.h | 7 ++++++- 3 files changed, 10 insertions(+), 7 deletions(-) (limited to 'drivers/gpu/drm/drm_fops.c') diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5fb02d57dae4..960ca987c20f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1492,7 +1492,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, mutex_unlock(&file_priv->fbs_lock); drm_modeset_lock_all(dev); - if (file_priv->minor->type != DRM_MINOR_LEGACY) { + if (!drm_is_primary_client(file_priv)) { mode_group = NULL; list_for_each(lh, &dev->mode_config.crtc_list) @@ -2848,7 +2848,7 @@ int drm_mode_getfb(struct drm_device *dev, r->pitch = fb->pitches[0]; if (fb->funcs->create_handle) { if (file_priv->is_master || capable(CAP_SYS_ADMIN) || - file_priv->minor->type == DRM_MINOR_CONTROL) { + drm_is_control_client(file_priv)) { ret = fb->funcs->create_handle(fb, file_priv, &r->handle); } else { diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 5432a1a61c15..c7792b1d1773 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -232,8 +232,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* if there is no current master make this fd it, but do not create * any master object for render clients */ mutex_lock(&dev->struct_mutex); - if (!priv->minor->master && !drm_is_render_client(priv) && - !drm_is_control_client(priv)) { + if (drm_is_primary_client(priv) && !priv->minor->master) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -271,8 +270,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, goto out_close; } } - } else if (!drm_is_render_client(priv) && - !drm_is_control_client(priv)) { + } else if (drm_is_primary_client(priv)) { /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3cf9f46ce2e6..3d06f71bc691 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1202,7 +1202,7 @@ static inline bool drm_modeset_is_locked(struct drm_device *dev) return mutex_is_locked(&dev->mode_config.mutex); } -static inline bool drm_is_render_client(struct drm_file *file_priv) +static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; } @@ -1212,6 +1212,11 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_CONTROL; } +static inline bool drm_is_primary_client(const struct drm_file *file_priv) +{ + return file_priv->minor->type == DRM_MINOR_LEGACY; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/ -- cgit v1.2.3 From c996fd0b956450563454e7ccc97a82ca31f9d043 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Tue, 25 Feb 2014 19:57:44 +0100 Subject: drm: Protect the master management with a drm_device::master_mutex v3 The master management was previously protected by the drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separate master_mutex. Locking order is master_mutex -> struct_mutex. Also remove drm_master::blocked since it's not used. v2: Add an inline comment about what drm_device::master_mutex is protecting. v3: Remove unneeded struct_mutex locks. Fix error returns in drm_setmaster_ioctl(). Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul Reviewed-by: David Herrmann Acked-by: Daniel Vetter --- drivers/gpu/drm/drm_fops.c | 22 +++++++++++----------- drivers/gpu/drm/drm_stub.c | 43 ++++++++++++++++++++++++++++--------------- include/drm/drmP.h | 46 +++++++++++++++++++++++++--------------------- 3 files changed, 64 insertions(+), 47 deletions(-) (limited to 'drivers/gpu/drm/drm_fops.c') diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c7792b1d1773..a0ce39c96f8e 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -231,12 +231,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* if there is no current master make this fd it, but do not create * any master object for render clients */ - mutex_lock(&dev->struct_mutex); + mutex_lock(&dev->master_mutex); if (drm_is_primary_client(priv) && !priv->minor->master) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { - mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; goto out_close; } @@ -244,29 +243,23 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->is_master = 1; /* take another reference for the copy in the local file priv */ priv->master = drm_master_get(priv->minor->master); - priv->authenticated = 1; - mutex_unlock(&dev->struct_mutex); if (dev->driver->master_create) { ret = dev->driver->master_create(dev, priv->master); if (ret) { - mutex_lock(&dev->struct_mutex); /* drop both references if this fails */ drm_master_put(&priv->minor->master); drm_master_put(&priv->master); - mutex_unlock(&dev->struct_mutex); goto out_close; } } - mutex_lock(&dev->struct_mutex); if (dev->driver->master_set) { ret = dev->driver->master_set(dev, priv, true); if (ret) { /* drop both references if this fails */ drm_master_put(&priv->minor->master); drm_master_put(&priv->master); - mutex_unlock(&dev->struct_mutex); goto out_close; } } @@ -274,7 +267,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&dev->master_mutex); mutex_lock(&dev->struct_mutex); list_add(&priv->lhead, &dev->filelist); @@ -302,6 +295,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, return 0; out_close: + mutex_unlock(&dev->master_mutex); if (dev->driver->postclose) dev->driver->postclose(dev, priv); out_prime_destroy: @@ -489,11 +483,13 @@ int drm_release(struct inode *inode, struct file *filp) } mutex_unlock(&dev->ctxlist_mutex); - mutex_lock(&dev->struct_mutex); + mutex_lock(&dev->master_mutex); if (file_priv->is_master) { struct drm_master *master = file_priv->master; struct drm_file *temp; + + mutex_lock(&dev->struct_mutex); list_for_each_entry(temp, &dev->filelist, lhead) { if ((temp->master == file_priv->master) && (temp != file_priv)) @@ -512,6 +508,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv = NULL; wake_up_interruptible_all(&master->lock.lock_queue); } + mutex_unlock(&dev->struct_mutex); if (file_priv->minor->master == file_priv->master) { /* drop the reference held my the minor */ @@ -521,10 +518,13 @@ int drm_release(struct inode *inode, struct file *filp) } } - /* drop the reference held my the file priv */ + /* drop the master reference held by the file priv */ if (file_priv->master) drm_master_put(&file_priv->master); file_priv->is_master = 0; + mutex_unlock(&dev->master_mutex); + + mutex_lock(&dev->struct_mutex); list_del(&file_priv->lhead); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index a378af49ed2e..fac6f9834257 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -144,6 +144,7 @@ static void drm_master_destroy(struct kref *kref) struct drm_device *dev = master->minor->dev; struct drm_map_list *r_list, *list_temp; + mutex_lock(&dev->struct_mutex); if (dev->driver->master_destroy) dev->driver->master_destroy(dev, master); @@ -171,6 +172,7 @@ static void drm_master_destroy(struct kref *kref) drm_ht_remove(&master->magiclist); + mutex_unlock(&dev->struct_mutex); kfree(master); } @@ -186,19 +188,20 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, { int ret = 0; + mutex_lock(&dev->master_mutex); if (file_priv->is_master) - return 0; + goto out_unlock; - if (file_priv->minor->master && file_priv->minor->master != file_priv->master) - return -EINVAL; - - if (!file_priv->master) - return -EINVAL; + if (file_priv->minor->master) { + ret = -EINVAL; + goto out_unlock; + } - if (file_priv->minor->master) - return -EINVAL; + if (!file_priv->master) { + ret = -EINVAL; + goto out_unlock; + } - mutex_lock(&dev->struct_mutex); file_priv->minor->master = drm_master_get(file_priv->master); file_priv->is_master = 1; if (dev->driver->master_set) { @@ -208,27 +211,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, drm_master_put(&file_priv->minor->master); } } - mutex_unlock(&dev->struct_mutex); +out_unlock: + mutex_unlock(&dev->master_mutex); return ret; } int drm_dropmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + int ret = -EINVAL; + + mutex_lock(&dev->master_mutex); if (!file_priv->is_master) - return -EINVAL; + goto out_unlock; if (!file_priv->minor->master) - return -EINVAL; + goto out_unlock; - mutex_lock(&dev->struct_mutex); + ret = 0; if (dev->driver->master_drop) dev->driver->master_drop(dev, file_priv, false); drm_master_put(&file_priv->minor->master); file_priv->is_master = 0; - mutex_unlock(&dev->struct_mutex); - return 0; + +out_unlock: + mutex_unlock(&dev->master_mutex); + return ret; } /* @@ -559,6 +568,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->ctxlist_mutex); + mutex_init(&dev->master_mutex); dev->anon_inode = drm_fs_inode_new(); if (IS_ERR(dev->anon_inode)) { @@ -612,6 +622,7 @@ err_minors: drm_minor_free(dev, DRM_MINOR_CONTROL); drm_fs_inode_free(dev->anon_inode); err_free: + mutex_destroy(&dev->master_mutex); kfree(dev); return NULL; } @@ -633,6 +644,8 @@ static void drm_dev_release(struct kref *ref) drm_minor_free(dev, DRM_MINOR_CONTROL); kfree(dev->devname); + + mutex_destroy(&dev->master_mutex); kfree(dev); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3d594ca7fa62..4e24a1a0daeb 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -405,7 +405,8 @@ struct drm_prime_file_private { struct drm_file { unsigned always_authenticated :1; unsigned authenticated :1; - unsigned is_master :1; /* this file private is a master for a minor */ + /* Whether we're master for a minor. Protected by master_mutex */ + unsigned is_master :1; /* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1; @@ -684,28 +685,29 @@ struct drm_gem_object { #include -/* per-master structure */ +/** + * struct drm_master - drm master structure + * + * @refcount: Refcount for this master object. + * @minor: Link back to minor char device we are master for. Immutable. + * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. + * @unique_len: Length of unique field. Protected by drm_global_mutex. + * @unique_size: Amount allocated. Protected by drm_global_mutex. + * @magiclist: Hash of used authentication tokens. Protected by struct_mutex. + * @magicfree: List of used authentication tokens. Protected by struct_mutex. + * @lock: DRI lock information. + * @driver_priv: Pointer to driver-private information. + */ struct drm_master { - - struct kref refcount; /* refcount for this master */ - - struct drm_minor *minor; /**< link back to minor we are a master for */ - - char *unique; /**< Unique identifier: e.g., busid */ - int unique_len; /**< Length of unique field */ - int unique_size; /**< amount allocated */ - - int blocked; /**< Blocked due to VC switch? */ - - /** \name Authentication */ - /*@{ */ + struct kref refcount; + struct drm_minor *minor; + char *unique; + int unique_len; + int unique_size; struct drm_open_hash magiclist; struct list_head magicfree; - /*@} */ - - struct drm_lock_data lock; /**< Information on hardware lock */ - - void *driver_priv; /**< Private structure for driver to use */ + struct drm_lock_data lock; + void *driver_priv; }; /* Size of ringbuffer for vblank timestamps. Just double-buffer @@ -1020,7 +1022,8 @@ struct drm_minor { struct list_head debugfs_list; struct mutex debugfs_lock; /* Protects debugfs_list. */ - struct drm_master *master; /* currently active master for this node */ + /* currently active master for this node. Protected by master_mutex */ + struct drm_master *master; struct drm_mode_group mode_group; }; @@ -1070,6 +1073,7 @@ struct drm_device { /*@{ */ spinlock_t count_lock; /**< For inuse, drm_device::open_count, drm_device::buf_use */ struct mutex struct_mutex; /**< For others */ + struct mutex master_mutex; /**< For drm_minor::master and drm_file::is_master */ /*@} */ /** \name Usage Counters */ -- cgit v1.2.3