* [RFC v2 1/6] percpu_rw_semaphore: export symbols for modules
2013-10-21 22:33 [RFC v2 0/6] DRM revoke support David Herrmann
@ 2013-10-21 22:33 ` David Herrmann
2013-10-21 22:33 ` [RFC v2 2/6] percpu_rw_semaphore: add percpu_down_read_trylock() David Herrmann
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: David Herrmann @ 2013-10-21 22:33 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, Dave Airlie, David Herrmann
DRM could make great use of percpu-rwsems to track active users and wait
for them during hw hotplugging. Export symbols and allow using them in
runtime loadable modules.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
lib/Makefile | 2 +-
lib/percpu-rwsem.c | 7 +++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/Makefile b/lib/Makefile
index f3bb2cb..c1572be 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -45,7 +45,7 @@ obj-$(CONFIG_DEBUG_LOCKING_API_SELFTESTS) += locking-selftest.o
obj-$(CONFIG_DEBUG_SPINLOCK) += spinlock_debug.o
lib-$(CONFIG_RWSEM_GENERIC_SPINLOCK) += rwsem-spinlock.o
lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
-lib-$(CONFIG_PERCPU_RWSEM) += percpu-rwsem.o
+obj-$(CONFIG_PERCPU_RWSEM) += percpu-rwsem.o
CFLAGS_hweight.o = $(subst $(quote),,$(CONFIG_ARCH_HWEIGHT_CFLAGS))
obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
diff --git a/lib/percpu-rwsem.c b/lib/percpu-rwsem.c
index 652a8ee..893586c 100644
--- a/lib/percpu-rwsem.c
+++ b/lib/percpu-rwsem.c
@@ -7,6 +7,7 @@
#include <linux/rcupdate.h>
#include <linux/sched.h>
#include <linux/errno.h>
+#include <linux/export.h>
int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
const char *name, struct lock_class_key *rwsem_key)
@@ -22,12 +23,14 @@ int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
init_waitqueue_head(&brw->write_waitq);
return 0;
}
+EXPORT_SYMBOL(__percpu_init_rwsem);
void percpu_free_rwsem(struct percpu_rw_semaphore *brw)
{
free_percpu(brw->fast_read_ctr);
brw->fast_read_ctr = NULL; /* catch use after free bugs */
}
+EXPORT_SYMBOL(percpu_free_rwsem);
/*
* This is the fast-path for down_read/up_read, it only needs to ensure
@@ -87,6 +90,7 @@ void percpu_down_read(struct percpu_rw_semaphore *brw)
/* avoid up_read()->rwsem_release() */
__up_read(&brw->rw_sem);
}
+EXPORT_SYMBOL(percpu_down_read);
void percpu_up_read(struct percpu_rw_semaphore *brw)
{
@@ -99,6 +103,7 @@ void percpu_up_read(struct percpu_rw_semaphore *brw)
if (atomic_dec_and_test(&brw->slow_read_ctr))
wake_up_all(&brw->write_waitq);
}
+EXPORT_SYMBOL(percpu_up_read);
static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
{
@@ -150,6 +155,7 @@ void percpu_down_write(struct percpu_rw_semaphore *brw)
/* wait for all readers to complete their percpu_up_read() */
wait_event(brw->write_waitq, !atomic_read(&brw->slow_read_ctr));
}
+EXPORT_SYMBOL(percpu_down_write);
void percpu_up_write(struct percpu_rw_semaphore *brw)
{
@@ -163,3 +169,4 @@ void percpu_up_write(struct percpu_rw_semaphore *brw)
/* the last writer unblocks update_fast_ctr() */
atomic_dec(&brw->write_ctr);
}
+EXPORT_SYMBOL(percpu_up_write);
--
1.8.4.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC v2 2/6] percpu_rw_semaphore: add percpu_down_read_trylock()
2013-10-21 22:33 [RFC v2 0/6] DRM revoke support David Herrmann
2013-10-21 22:33 ` [RFC v2 1/6] percpu_rw_semaphore: export symbols for modules David Herrmann
@ 2013-10-21 22:33 ` David Herrmann
2013-10-21 22:33 ` [RFC v2 3/6] drm: split drm_release() David Herrmann
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: David Herrmann @ 2013-10-21 22:33 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, Dave Airlie, David Herrmann
This is basically a copy of percpu_down_read() but does not sleep if a
writer is already active. Same semantics as classic down_read_trylock().
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/linux/percpu-rwsem.h | 1 +
lib/percpu-rwsem.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index 3e88c9a..2f752bd 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -16,6 +16,7 @@ struct percpu_rw_semaphore {
};
extern void percpu_down_read(struct percpu_rw_semaphore *);
+extern int percpu_down_read_trylock(struct percpu_rw_semaphore *);
extern void percpu_up_read(struct percpu_rw_semaphore *);
extern void percpu_down_write(struct percpu_rw_semaphore *);
diff --git a/lib/percpu-rwsem.c b/lib/percpu-rwsem.c
index 893586c..596f44b 100644
--- a/lib/percpu-rwsem.c
+++ b/lib/percpu-rwsem.c
@@ -92,6 +92,26 @@ void percpu_down_read(struct percpu_rw_semaphore *brw)
}
EXPORT_SYMBOL(percpu_down_read);
+int percpu_down_read_trylock(struct percpu_rw_semaphore *brw)
+{
+ int r;
+
+ if (likely(update_fast_ctr(brw, +1))) {
+ rwsem_acquire_read(&brw->rw_sem.dep_map, 0, 0, _RET_IP_);
+ return 1;
+ }
+
+ r = down_read_trylock(&brw->rw_sem);
+ if (r) {
+ atomic_inc(&brw->slow_read_ctr);
+ /* avoid up_read()->rwsem_release() */
+ __up_read(&brw->rw_sem);
+ }
+
+ return r;
+}
+EXPORT_SYMBOL(percpu_down_read_trylock);
+
void percpu_up_read(struct percpu_rw_semaphore *brw)
{
rwsem_release(&brw->rw_sem.dep_map, 1, _RET_IP_);
--
1.8.4.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC v2 3/6] drm: split drm_release()
2013-10-21 22:33 [RFC v2 0/6] DRM revoke support David Herrmann
2013-10-21 22:33 ` [RFC v2 1/6] percpu_rw_semaphore: export symbols for modules David Herrmann
2013-10-21 22:33 ` [RFC v2 2/6] percpu_rw_semaphore: add percpu_down_read_trylock() David Herrmann
@ 2013-10-21 22:33 ` David Herrmann
2013-10-21 22:33 ` [RFC v2 4/6] drm: make dev->unplugged reliable David Herrmann
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: David Herrmann @ 2013-10-21 22:33 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, Dave Airlie, David Herrmann
This splits off real file-destruction into drm_close(). drm_release() now
calls drm_close() and then takes care of any device-cleanup which isn't
strictly related to file-destruction.
Besides making the code more readable, this is needed if we want to close
open-files during GPU unplug. We could simply fake a drm_close() on each
open-file now to logically close them.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/gpu/drm/drm_fops.c | 65 +++++++++++++++++++++++++++-------------------
include/drm/drmP.h | 1 +
2 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index d0e2766..ecdd647 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -450,34 +450,26 @@ int drm_lastclose(struct drm_device * dev)
}
/**
- * Release file.
+ * drm_close - Close open DRM file
+ * @file_priv: Open DRM-file to close
*
- * \param inode device inode
- * \param file_priv DRM file private.
- * \return zero on success or a negative number on failure.
+ * This does the real work of drm_release(). All allocated data is freed and
+ * locks are released. drm_global_mutex must be locked by the caller. The
+ * drm_file object is not freed and @file_priv->minor->dev stays valid.
*
- * If the hardware lock is held then free it, and take it again for the kernel
- * context since it's necessary to reclaim buffers. Unlink the file private
- * data from its list and free it. Decreases the open count and if it reaches
- * zero calls drm_lastclose().
+ * This does not call drm_lastclose() or any device-cleanup helpers. It's the
+ * callers responsibility to do this. Drivers must not call this directly. Use
+ * drm_release() instead.
*/
-int drm_release(struct inode *inode, struct file *filp)
+void drm_close(struct drm_file *file_priv)
{
- struct drm_file *file_priv = filp->private_data;
struct drm_device *dev = file_priv->minor->dev;
- int retcode = 0;
-
- mutex_lock(&drm_global_mutex);
DRM_DEBUG("open_count = %d\n", dev->open_count);
if (dev->driver->preclose)
dev->driver->preclose(dev, file_priv);
- /* ========================================================
- * Begin inline drm_release
- */
-
DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
task_pid_nr(current),
(long)old_encode_dev(file_priv->minor->device),
@@ -490,7 +482,7 @@ int drm_release(struct inode *inode, struct file *filp)
/* if the master has gone away we can't do anything with the lock */
if (file_priv->minor->master)
- drm_master_release(dev, filp);
+ drm_master_release(dev, file_priv->filp);
if (drm_core_check_feature(dev, DRIVER_HAVE_DMA))
drm_core_reclaim_buffers(dev, file_priv);
@@ -573,25 +565,44 @@ int drm_release(struct inode *inode, struct file *filp)
drm_prime_destroy_file_private(&file_priv->prime);
put_pid(file_priv->pid);
- kfree(file_priv);
+}
- /* ========================================================
- * End inline drm_release
- */
+/**
+ * drm_release - Release open DRM file
+ * @inode: device inode
+ * @file_priv: DRM file
+ *
+ * Release an open DRM file. This is the default callback for DRM fops.
+ * We free all internally allocated data, release held locks and unlink the
+ * open-file from the DRM device. If this is the last user of the unplugged
+ * device, we also release the device.
+ *
+ * RETURNS:
+ * Returns always zero.
+ */
+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;
+
+ mutex_lock(&drm_global_mutex);
+
+ drm_close(file_priv);
+ kfree(file_priv);
if (!--dev->open_count) {
- if (atomic_read(&dev->ioctl_count)) {
+ if (atomic_read(&dev->ioctl_count))
DRM_ERROR("Device busy: %d\n",
atomic_read(&dev->ioctl_count));
- retcode = -EBUSY;
- } else
- retcode = drm_lastclose(dev);
+ else
+ drm_lastclose(dev);
if (drm_device_is_unplugged(dev))
drm_put_dev(dev);
}
+
mutex_unlock(&drm_global_mutex);
- return retcode;
+ return 0;
}
EXPORT_SYMBOL(drm_release);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 19b8082..16ff7c4 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1268,6 +1268,7 @@ extern int drm_open(struct inode *inode, struct file *filp);
extern int drm_stub_open(struct inode *inode, struct file *filp);
extern ssize_t drm_read(struct file *filp, char __user *buffer,
size_t count, loff_t *offset);
+extern void drm_close(struct drm_file *file_priv);
extern int drm_release(struct inode *inode, struct file *filp);
/* Mapping support (drm_vm.h) */
--
1.8.4.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC v2 4/6] drm: make dev->unplugged reliable
2013-10-21 22:33 [RFC v2 0/6] DRM revoke support David Herrmann
` (2 preceding siblings ...)
2013-10-21 22:33 ` [RFC v2 3/6] drm: split drm_release() David Herrmann
@ 2013-10-21 22:33 ` David Herrmann
2013-10-21 22:33 ` [RFC v2 5/6] drm: make drm_dev_unregister() immediate David Herrmann
2013-10-21 22:33 ` [RFC v2 6/6] drm: zap mmaps for dead devices David Herrmann
5 siblings, 0 replies; 7+ messages in thread
From: David Herrmann @ 2013-10-21 22:33 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, Dave Airlie, David Herrmann
If we unplug a DRM device, we currently set dev->unplugged and then wait
for all open-files to close. Beside rather subtle race-conditions, the
main disadvantage is that most DRM device resources will stay allocated
and registered. This means the device cannot be re-used by another driver
until all user-space processes close()ed their DRM fds.
This patch changes the way we track pending fops. Instead of checking for
dev->unplugged before each fop, we now call drm_dev_get_active(). This
includes a test for dev->unplugged but also marks the device as active.
Once the fop is done, we call drm_dev_put_active() and the active-count is
decreased.
This allows us at _any_ time to see whether there is any running fop.
Furthermore, we can mark the device as unplugged and wait for these to
finish. So instead of waiting for all users to close() their fds, it is
now enough to wait for all running fops to finish. No new fops can be
started as the drm_dev_get_active() call fails once the device is
unplugged. If no DRM fop sleeps for an indefinite amount of time, this
effectively allows us to unregister and free DRM device resource _before_
all DRM fds are closed. Thus preventing user-space from delaying resource
destruction.
However, this patch doesn't enforce this, yet. Instead, we only introduce
the fops tracking. The new drm_dev_shutdown() helper can be used to do a
synchronous shutdown. However, this isn't enforced on hotplug-capable
drivers, yet. Instead, all drivers using drm_unplug_dev() will still wait
for all fds to close before destroying the device (to not break possibly
buggy existing drivers).
However, for all other drivers we now forcibly wait for all fops to finish
during drm_dev_unregister(). (for hotplug-capable drivers this is a no-op
as all fds are already closed, anyway).
We used to panic if a driver is unloaded while there're active users so
this is just a protection against _some_ of the existing races. To allow
proper unplugging of _any_ driver, some more protections will follow.
Note: The use of percpu_rw_semaphore is an implementation detail. The same
effect could be achieved with a spinlock+atomic_t+wait_queue. The
percpu_rw_sempahore is only slightly heavier but gives us much better
performance and is a lot more convenient to use.
Note2: This only fixes the global DRM fops to use drm_dev_get_active().
For drivers to allows full hotplugging, all their level-1 fops also need
to be fixed. This can be done separately for each driver, though.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/drm_drv.c | 5 +-
drivers/gpu/drm/drm_fops.c | 25 +++++--
drivers/gpu/drm/drm_gem.c | 10 +--
drivers/gpu/drm/drm_stub.c | 136 +++++++++++++++++++++++++++++++++---
drivers/gpu/drm/drm_vm.c | 3 +-
drivers/gpu/drm/udl/udl_connector.c | 2 +-
drivers/gpu/drm/udl/udl_fb.c | 4 +-
include/drm/drmP.h | 25 +++----
9 files changed, 171 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 955555d..0795558 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -11,6 +11,7 @@ menuconfig DRM
select I2C
select I2C_ALGOBIT
select DMA_SHARED_BUFFER
+ select PERCPU_RWSEM
help
Kernel-level support for the Direct Rendering Infrastructure (DRI)
introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b55f138..21cd5f5 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -312,7 +312,7 @@ long drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
- if (drm_device_is_unplugged(dev))
+ if (!drm_dev_get_active(dev))
return -ENODEV;
atomic_inc(&dev->ioctl_count);
@@ -403,7 +403,10 @@ long drm_ioctl(struct file *filp,
if (kdata != stack_kdata)
kfree(kdata);
+
atomic_dec(&dev->ioctl_count);
+ drm_dev_put_active(dev);
+
if (retcode)
DRM_DEBUG("ret = %d\n", retcode);
return retcode;
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index ecdd647..3884185 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -94,7 +94,7 @@ int drm_open(struct inode *inode, struct file *filp)
if (!(dev = minor->dev))
return -ENODEV;
- if (drm_device_is_unplugged(dev))
+ if (!drm_dev_get_active(dev))
return -ENODEV;
if (!dev->open_count++)
@@ -118,7 +118,9 @@ int drm_open(struct inode *inode, struct file *filp)
if (retcode)
goto err_undo;
}
- return 0;
+
+ retcode = 0;
+ goto out_active;
err_undo:
mutex_lock(&dev->struct_mutex);
@@ -128,6 +130,8 @@ err_undo:
dev->dev_mapping = old_mapping;
mutex_unlock(&dev->struct_mutex);
dev->open_count--;
+out_active:
+ drm_dev_put_active(dev);
return retcode;
}
EXPORT_SYMBOL(drm_open);
@@ -159,9 +163,6 @@ int drm_stub_open(struct inode *inode, struct file *filp)
if (!(dev = minor->dev))
goto out;
- if (drm_device_is_unplugged(dev))
- goto out;
-
old_fops = filp->f_op;
filp->f_op = fops_get(dev->driver->fops);
if (filp->f_op == NULL) {
@@ -596,7 +597,7 @@ int drm_release(struct inode *inode, struct file *filp)
atomic_read(&dev->ioctl_count));
else
drm_lastclose(dev);
- if (drm_device_is_unplugged(dev))
+ if (drm_dev_is_unplugged(dev))
drm_put_dev(dev);
}
@@ -639,14 +640,18 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
size_t count, loff_t *offset)
{
struct drm_file *file_priv = filp->private_data;
+ struct drm_device *dev = file_priv->minor->dev;
struct drm_pending_event *e;
size_t total;
ssize_t ret;
ret = wait_event_interruptible(file_priv->event_wait,
- !list_empty(&file_priv->event_list));
+ !list_empty(&file_priv->event_list) ||
+ drm_dev_is_unplugged(dev));
if (ret < 0)
return ret;
+ if (!drm_dev_get_active(dev))
+ return -ENODEV;
total = 0;
while (drm_dequeue_event(file_priv, total, count, &e)) {
@@ -660,6 +665,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
e->destroy(e);
}
+ drm_dev_put_active(dev);
return total;
}
EXPORT_SYMBOL(drm_read);
@@ -667,10 +673,15 @@ EXPORT_SYMBOL(drm_read);
unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait)
{
struct drm_file *file_priv = filp->private_data;
+ struct drm_device *dev = file_priv->minor->dev;
unsigned int mask = 0;
poll_wait(filp, &file_priv->event_wait, wait);
+ /* signal HUP on device removal */
+ if (drm_dev_is_unplugged(dev))
+ mask |= POLLHUP;
+
if (!list_empty(&file_priv->event_list))
mask |= POLLIN | POLLRDNORM;
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 4761ade..152a7661 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -838,7 +838,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
struct drm_vma_offset_node *node;
int ret = 0;
- if (drm_device_is_unplugged(dev))
+ if (!drm_dev_get_active(dev))
return -ENODEV;
mutex_lock(&dev->struct_mutex);
@@ -847,17 +847,19 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
vma_pages(vma));
if (!node) {
mutex_unlock(&dev->struct_mutex);
+ drm_dev_put_active(dev);
return drm_mmap(filp, vma);
} else if (!drm_vma_node_is_allowed(node, filp)) {
- mutex_unlock(&dev->struct_mutex);
- return -EACCES;
+ ret = -EACCES;
+ goto unlock;
}
obj = container_of(node, struct drm_gem_object, vma_node);
ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT, vma);
+unlock:
mutex_unlock(&dev->struct_mutex);
-
+ drm_dev_put_active(dev);
return ret;
}
EXPORT_SYMBOL(drm_gem_mmap);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index b4c51d0..9218f17 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -370,6 +370,27 @@ static void drm_put_minor(struct drm_minor *minor)
}
/**
+ * drm_dev_shutdown - Shutdown a DRM device synchronously
+ * @dev: Device to shutdown
+ *
+ * This marks a device as unplugged, waits for all active fops to finish and
+ * then returns. Once the device is marked as unplugged, any further call to
+ * drm_dev_get_active() will fail so no new users can enter any DRM fops on the
+ * device.
+ * This must not be called with drm_global_mutex held! Otherwise, calls like
+ * drm_ioctl() might dead-lock. Instead, the caller must guarantee that this is
+ * never called multiple times in parallel. Repeated calls are allowed and
+ * detected properly.
+ */
+static void drm_dev_shutdown(struct drm_device *dev)
+{
+ if (!dev->unplugged) {
+ dev->unplugged = true;
+ percpu_down_write(&dev->active);
+ }
+}
+
+/**
* Called via drm_exit() at module unload time or when pci device is
* unplugged.
*
@@ -399,14 +420,14 @@ void drm_unplug_dev(struct drm_device *dev)
drm_unplug_minor(dev->render);
drm_unplug_minor(dev->primary);
- mutex_lock(&drm_global_mutex);
-
- drm_device_set_unplugged(dev);
+ /* TODO: We should call drm_dev_shutdown() here. But to protect against
+ * buggy drivers, we don't do any synchronous shutdown and instead wait
+ * for users to go away. */
+ dev->unplugged = true;
+ mb();
- if (dev->open_count == 0) {
+ if (dev->open_count == 0)
drm_put_dev(dev);
- }
- mutex_unlock(&drm_global_mutex);
}
EXPORT_SYMBOL(drm_unplug_dev);
@@ -446,9 +467,12 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
mutex_init(&dev->struct_mutex);
mutex_init(&dev->ctxlist_mutex);
- if (drm_ht_create(&dev->map_hash, 12))
+ if (percpu_init_rwsem(&dev->active))
goto err_free;
+ if (drm_ht_create(&dev->map_hash, 12))
+ goto err_rwsem;
+
ret = drm_ctxbitmap_init(dev);
if (ret) {
DRM_ERROR("Cannot allocate memory for context bitmap.\n");
@@ -469,6 +493,8 @@ err_ctxbitmap:
drm_ctxbitmap_cleanup(dev);
err_ht:
drm_ht_remove(&dev->map_hash);
+err_rwsem:
+ percpu_free_rwsem(&dev->active);
err_free:
kfree(dev);
return NULL;
@@ -497,6 +523,7 @@ void drm_dev_free(struct drm_device *dev)
drm_ctxbitmap_cleanup(dev);
drm_ht_remove(&dev->map_hash);
+ percpu_free_rwsem(&dev->active);
kfree(dev->devname);
kfree(dev);
}
@@ -575,14 +602,17 @@ EXPORT_SYMBOL(drm_dev_register);
* drm_dev_unregister - Unregister DRM device
* @dev: Device to unregister
*
- * 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.
+ * Mark DRM device as unplugged, wait for any pending user request and then
+ * unregister the DRM device from the system. This does the reverse of
+ * drm_dev_register(). The caller is responsible of freeing the device via
+ * drm_dev_free() once this returns.
*/
void drm_dev_unregister(struct drm_device *dev)
{
struct drm_map_list *r_list, *list_temp;
+ drm_dev_shutdown(dev);
+
drm_lastclose(dev);
if (dev->driver->unload)
@@ -603,3 +633,89 @@ void drm_dev_unregister(struct drm_device *dev)
list_del(&dev->driver_item);
}
EXPORT_SYMBOL(drm_dev_unregister);
+
+/**
+ * drm_dev_is_unplugged - Test whether device is unplugged
+ * @dev: Device to test
+ *
+ * This returns true if the device is unplugged and should not be used. However,
+ * compared to drm_dev_get_active() it does not acquire the device. Therefore,
+ * this can only be used to test whether a device is dead, not whether it's
+ * alive. Once a device is dead it will never get active again so you can rely
+ * on the value to stay.
+ *
+ * This helper is mainly meant for wait_*() calls used as wake-up condition to
+ * get notified once a device gets unplugged. This call is unlocked so you need
+ * to make sure you use proper barriers (mostly implicitly via wait() calls).
+ *
+ * You're highly recommended to not use this helper. Use drm_dev_get_active()
+ * and drm_dev_put_active() instead.
+ *
+ * RETURNS:
+ * True iff the device is unplugged and about to be removed.
+ */
+bool drm_dev_is_unplugged(struct drm_device *dev)
+{
+ return dev->unplugged;
+}
+EXPORT_SYMBOL(drm_dev_is_unplugged);
+
+/**
+ * drm_dev_get_active - Mark device as active
+ * @dev: Device to mark
+ *
+ * Whenever a DRM driver performs an action on behalf of user-space, it should
+ * mark the DRM device as active. Once it is done, call drm_dev_put_active() to
+ * release that mark. This allows DRM core to wait for pending user-space
+ * actions before unplugging a device. But this also means, user-space must
+ * not sleep for an indefinite period while a device is marked active.
+ * If you have to sleep for an indefinite period, call drm_dev_put_active() and
+ * try to reacquire the device once you wake up.
+ *
+ * Recursive calls are allowed but may fail if the device is about to go away.
+ *
+ * This is implemented via percpu-semaphores and can safely be used in
+ * fast-paths and atomic-contexts. Internally we use read-locks to count all
+ * active readers. As down_read() is not safe to be called recursively we use
+ * down_read_trylock() instead. Once a device gets unplugged we call
+ * down_write() and wait for all readers to finish. This causes any further
+ * down_read_trylock() to fail so no new users will occur. Once we get the
+ * write-lock, we never release it. See drm_dev_unregister() for more.
+ *
+ * RETURNS:
+ * True iff the device was marked active and can be used. False if the device
+ * was unplugged and must not be used, anymore.
+ */
+bool drm_dev_get_active(struct drm_device *dev)
+{
+ if (!percpu_down_read_trylock(&dev->active))
+ return false;
+
+ /* TODO: If we strictly enforce synchronous shutdown, we will be unable
+ * to lock dev->active if @unplugged is true. But buggy drivers are
+ * allowed to loosen this strict enforcement. Hence, lets try our best
+ * here and protect against use once @unplugged is set.
+ * This can be simplified once drm_dev_shutdown() is always forced. */
+
+ if (!dev->unplugged)
+ return true;
+
+ percpu_up_read(&dev->active);
+ return false;
+}
+EXPORT_SYMBOL(drm_dev_get_active);
+
+/**
+ * drm_dev_put_active - Unmark active device
+ * @dev: Active device to unmark
+ *
+ * This finishes a call to drm_dev_get_active(). You must not call it if
+ * drm_dev_get_active() failed.
+ * This marks the device as inactive again, iff no other user currently has the
+ * device marked as active. See drm_dev_get_active().
+ */
+void drm_dev_put_active(struct drm_device *dev)
+{
+ percpu_up_read(&dev->active);
+}
+EXPORT_SYMBOL(drm_dev_put_active);
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index b5c5af7..abdcf1a 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -658,13 +658,14 @@ int drm_mmap(struct file *filp, struct vm_area_struct *vma)
struct drm_device *dev = priv->minor->dev;
int ret;
- if (drm_device_is_unplugged(dev))
+ if (!drm_dev_get_active(dev))
return -ENODEV;
mutex_lock(&dev->struct_mutex);
ret = drm_mmap_locked(filp, vma);
mutex_unlock(&dev->struct_mutex);
+ drm_dev_put_active(dev);
return ret;
}
EXPORT_SYMBOL(drm_mmap);
diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index b44d548..b013022 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -96,7 +96,7 @@ static int udl_mode_valid(struct drm_connector *connector,
static enum drm_connector_status
udl_detect(struct drm_connector *connector, bool force)
{
- if (drm_device_is_unplugged(connector->dev))
+ if (drm_dev_is_unplugged(connector->dev))
return connector_status_disconnected;
return connector_status_connected;
}
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 97e9d61..c328826 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -323,10 +323,9 @@ static int udl_fb_open(struct fb_info *info, int user)
{
struct udl_fbdev *ufbdev = info->par;
struct drm_device *dev = ufbdev->ufb.base.dev;
- struct udl_device *udl = dev->dev_private;
/* If the USB device is gone, we don't accept new opens */
- if (drm_device_is_unplugged(udl->ddev))
+ if (!drm_dev_get_active(dev))
return -ENODEV;
ufbdev->fb_count++;
@@ -350,6 +349,7 @@ static int udl_fb_open(struct fb_info *info, int user)
pr_notice("open /dev/fb%d user=%d fb_info=%p count=%d\n",
info->node, user, info, ufbdev->fb_count);
+ drm_dev_put_active(dev);
return 0;
}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 16ff7c4..d066a58 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -47,6 +47,7 @@
#include <linux/fs.h>
#include <linux/init.h>
#include <linux/file.h>
+#include <linux/percpu-rwsem.h>
#include <linux/platform_device.h>
#include <linux/pci.h>
#include <linux/jiffies.h>
@@ -1209,7 +1210,11 @@ struct drm_device {
/*@} */
int switch_power_state;
- atomic_t unplugged; /* device has been unplugged or gone away */
+ /** \name Hotplug Management */
+ /*@{ */
+ struct percpu_rw_semaphore active; /**< protect active fops */
+ bool unplugged; /**< device unplugged? */
+ /*@} */
};
#define DRM_SWITCH_POWER_ON 0
@@ -1228,19 +1233,6 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
return dev->driver->bus->get_irq(dev);
}
-static inline void drm_device_set_unplugged(struct drm_device *dev)
-{
- smp_wmb();
- atomic_set(&dev->unplugged, 1);
-}
-
-static inline int drm_device_is_unplugged(struct drm_device *dev)
-{
- int ret = atomic_read(&dev->unplugged);
- smp_rmb();
- return ret;
-}
-
static inline bool drm_modeset_is_locked(struct drm_device *dev)
{
return mutex_is_locked(&dev->mode_config.mutex);
@@ -1642,6 +1634,11 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
void drm_dev_free(struct drm_device *dev);
int drm_dev_register(struct drm_device *dev, unsigned long flags);
void drm_dev_unregister(struct drm_device *dev);
+
+bool drm_dev_is_unplugged(struct drm_device *dev);
+bool drm_dev_get_active(struct drm_device *dev);
+void drm_dev_put_active(struct drm_device *dev);
+
/*@}*/
/* PCI section */
--
1.8.4.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC v2 5/6] drm: make drm_dev_unregister() immediate
2013-10-21 22:33 [RFC v2 0/6] DRM revoke support David Herrmann
` (3 preceding siblings ...)
2013-10-21 22:33 ` [RFC v2 4/6] drm: make dev->unplugged reliable David Herrmann
@ 2013-10-21 22:33 ` David Herrmann
2013-10-21 22:33 ` [RFC v2 6/6] drm: zap mmaps for dead devices David Herrmann
5 siblings, 0 replies; 7+ messages in thread
From: David Herrmann @ 2013-10-21 22:33 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, Dave Airlie, David Herrmann
Drivers which support unplugging currently use drm_unplug_dev() which
waits for all open files to be closed before unregistering a device. All
other drivers just immediately unregister the device if unplugged or
unloaded, which results in panics if there're pending open files.
This patch implements proper revoke support for DRM devices. We remove
drm_unplug_dev() and make drm_put_dev() an alias for drm_dev_unregister().
Instead of plainly unregistering the device, we now mark the device as
dead, close all open files and then unregister the device. This guarantees
that all pending actions are done, no new actions can happen and the
device is properly unregistered and destroyed.
As the underlying VFS layer doesn't provide revoke support for us, we need
some special workarounds to support it properly. Any open file still has
the file->f_private pointer set to their drm_file object. We cannot reset
this pointer so we must keep it allocated. Fortunately, any f_op accesses
((drm_file*)file->f_private)->minor->dev and checks whether the given
drm_device is still active before accessing anything else. So what we do
during device shutdown is:
1: Mark the device as dead
2: Wait for all pending f_ops to finish
3: Call drm_close() for all open files to fake a close()
4: Only if no (dead) open-file remains, we free the drm_device
This procedure guarantees that any pending open-file is now dead but not
freed. So any further f_op will return ENODEV as drm_dev_get_active() will
fail. Any following real close() call will notice that the device is dead
and skip the call to drm_close(). Instead, it simply frees the drm_file
private data and decreases the open-count. Once the open-count drops to 0
and the device is already unplugged, the last drm_release() will free the
DRM-device.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/gpu/drm/drm_fops.c | 29 +++++++++++++-----
drivers/gpu/drm/drm_stub.c | 68 ++++++++++++++++++++++++++-----------------
drivers/gpu/drm/udl/udl_drv.c | 2 +-
include/drm/drmP.h | 1 -
4 files changed, 65 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 3884185..c860376 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -585,22 +585,37 @@ 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;
+ bool active, alive;
mutex_lock(&drm_global_mutex);
- drm_close(file_priv);
+ /* The additional test against dev->filelist is required as
+ * drm_dev_unregister() cannot hold drm_global_mutex while locking
+ * dev->active. So if we're called in the short window between
+ * dev->unplugged being set but drm_global_mutex not yet locked in
+ * drm_dev_unregister(), we notice it as dev->filelist cannot be empty.
+ * We treat it as if the device was still active as drm_dev_unregister()
+ * will wait for us to drop drm_global_mutex, anyway. */
+ active = drm_dev_get_active(dev);
+ alive = active || !list_empty(&dev->filelist);
+
+ if (alive)
+ drm_close(file_priv);
kfree(file_priv);
if (!--dev->open_count) {
- if (atomic_read(&dev->ioctl_count))
- DRM_ERROR("Device busy: %d\n",
- atomic_read(&dev->ioctl_count));
- else
+ /* If the device is still alive, only call last-close. If it is
+ * already unregistered (!alive), then free the device as we're
+ * the last user. */
+ if (alive)
drm_lastclose(dev);
- if (drm_dev_is_unplugged(dev))
- drm_put_dev(dev);
+ else
+ drm_dev_free(dev);
}
+ if (active)
+ drm_dev_put_active(dev);
+
mutex_unlock(&drm_global_mutex);
return 0;
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 9218f17..e363b72 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -407,30 +407,9 @@ void drm_put_dev(struct drm_device *dev)
}
drm_dev_unregister(dev);
- drm_dev_free(dev);
}
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);
-
- /* TODO: We should call drm_dev_shutdown() here. But to protect against
- * buggy drivers, we don't do any synchronous shutdown and instead wait
- * for users to go away. */
- dev->unplugged = true;
- mb();
-
- if (dev->open_count == 0)
- drm_put_dev(dev);
-}
-EXPORT_SYMBOL(drm_unplug_dev);
-
/**
* drm_dev_alloc - Allocate new drm device
* @driver: DRM driver to allocate device for
@@ -508,8 +487,8 @@ EXPORT_SYMBOL(drm_dev_alloc);
* 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 must not be called once the device got registered. See
+ * drm_dev_unregister().
*/
void drm_dev_free(struct drm_device *dev)
{
@@ -535,7 +514,8 @@ EXPORT_SYMBOL(drm_dev_free);
*
* Register the DRM device @dev with the system, advertise device to user-space
* and start normal device operation. @dev must be allocated via drm_dev_alloc()
- * previously.
+ * previously. You must not call drm_dev_free() if this succeeds. Use
+ * drm_dev_unregister() instead.
*
* Never call this twice on any device!
*
@@ -604,15 +584,45 @@ EXPORT_SYMBOL(drm_dev_register);
*
* Mark DRM device as unplugged, wait for any pending user request and then
* unregister the DRM device from the system. This does the reverse of
- * drm_dev_register(). The caller is responsible of freeing the device via
- * drm_dev_free() once this returns.
+ * drm_dev_register().
+ *
+ * This also forcibly closes all open files on the device. The open-files are
+ * marked dead and get unregistered and unlinked (but kept allocated) so any
+ * new fop from user-space will result in ENODEV.
+ *
+ * The caller must not hold drm_global_mutex! The caller also must not access
+ * the device after this returns. If the device is unused, it's immediately
+ * freed, otherwise the last drm_release() will free the now dead device.
*/
void drm_dev_unregister(struct drm_device *dev)
{
struct drm_map_list *r_list, *list_temp;
+ struct drm_file *file;
+ bool dead;
drm_dev_shutdown(dev);
+ /* We cannot hold drm_global_mutex during drm_dev_shutdown() as it might
+ * dead-lock. Hence, there's a small race between drm_dev_shutdown() and
+ * us locking drm_global_mutex which drm_release() might trigger. To fix
+ * it, drm_release() tests for list_empty(&dev->filelist) additionally
+ * to the device being unplugged. */
+
+ mutex_lock(&drm_global_mutex);
+
+ /* Close all open DRM files. As the device is marked dead, any DRM fop
+ * will fail calling drm_dev_get_device() so they cannot access the DRM
+ * device anymore. We keep the dead drm_file allocated so new fops can
+ * access it. The real drm_release() will notice the device is dead and
+ * just free the dead drm_file for real. */
+ while (!list_empty(&dev->filelist)) {
+ file = list_entry(dev->filelist.next, struct drm_file, lhead);
+
+ /* wake up drm_read() and drm_poll() */
+ wake_up_interruptible(&file->event_wait);
+ drm_close(file);
+ }
+
drm_lastclose(dev);
if (dev->driver->unload)
@@ -631,6 +641,12 @@ void drm_dev_unregister(struct drm_device *dev)
drm_unplug_minor(dev->primary);
list_del(&dev->driver_item);
+ dead = !dev->open_count;
+
+ mutex_unlock(&drm_global_mutex);
+
+ if (dead)
+ drm_dev_free(dev);
}
EXPORT_SYMBOL(drm_dev_unregister);
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 3ddd6cd..2cb65d1 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -48,7 +48,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
drm_connector_unplug_all(dev);
udl_fbdev_unplug(dev);
udl_drop_usb(dev);
- drm_unplug_dev(dev);
+ drm_dev_unregister(dev);
}
static const struct vm_operations_struct udl_gem_vm_ops = {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d066a58..6b22f15 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1441,7 +1441,6 @@ extern struct drm_master *drm_master_get(struct drm_master *master);
extern void drm_master_put(struct drm_master **master);
extern void drm_put_dev(struct drm_device *dev);
-extern void drm_unplug_dev(struct drm_device *dev);
extern unsigned int drm_debug;
extern unsigned int drm_rnodes;
--
1.8.4.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [RFC v2 6/6] drm: zap mmaps for dead devices
2013-10-21 22:33 [RFC v2 0/6] DRM revoke support David Herrmann
` (4 preceding siblings ...)
2013-10-21 22:33 ` [RFC v2 5/6] drm: make drm_dev_unregister() immediate David Herrmann
@ 2013-10-21 22:33 ` David Herrmann
5 siblings, 0 replies; 7+ messages in thread
From: David Herrmann @ 2013-10-21 22:33 UTC (permalink / raw)
To: dri-devel; +Cc: linux-kernel, Dave Airlie, David Herrmann
Once a DRM device is unregistered, user-space must not access any existing
mmaps, anymore. As we cannot rely on this, we now zap all of them in
drm_dev_unregister().
Any driver which wants to support that needs to protect their fault()
and mmap() handlers via drm_dev_get_active(), otherwise users can create
new mmaps after/during drm_dev_unregister().
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/gpu/drm/drm_stub.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index e363b72..274a005 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -602,6 +602,10 @@ void drm_dev_unregister(struct drm_device *dev)
drm_dev_shutdown(dev);
+ /* zap all memory mappings (drm_global_mutex must not be locked) */
+ if (dev->dev_mapping)
+ unmap_mapping_range(dev->dev_mapping, 0, LLONG_MAX, 1);
+
/* We cannot hold drm_global_mutex during drm_dev_shutdown() as it might
* dead-lock. Hence, there's a small race between drm_dev_shutdown() and
* us locking drm_global_mutex which drm_release() might trigger. To fix
--
1.8.4.1
^ permalink raw reply [flat|nested] 7+ messages in thread