If we want to support real hotplugging, we need to block new syscalls from
entering any drm_* fops. We also need to wait for these to finish before
unregistering a device.
This patch introduces drm_dev_get/put_active() which mark a device as
active during file-ops. If a device is unplugged, drm_dev_get_active()
will fail and prevent users from using this device.
Internally, we use rwsems to implement this. It allows simultaneous users
(down_read) and we can block on them (down_write) to wait until they are
done. This way, a drm_dev_unregister() can be called at any time and does
not have to wait for the last drm_release() call.
Note that the current "atomic_t unplugged" approach is not safe. Imagine
an unplugged device but a user-space context which already is beyong the
"drm_device_is_unplugged()" check. We have no way to prevent any following
mmap operation or buffer access. The percpu-rwsem avoids this by
protecting a whole file-op call and waiting with unplugging a device until
all pending calls are finished.
FIXME: We still need to update all the driver's fops in case they don't
use the DRM core stubs. A quick look showed only custom mmap callbacks.
Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/drm_drv.c | 6 +++--
drivers/gpu/drm/drm_fops.c | 31 +++++++++++++++++-----
drivers/gpu/drm/drm_stub.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
include/drm/drmP.h | 6 +++++
5 files changed, 96 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index a7c54c8..e2a399e 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 c294e89..db5e57b 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -318,7 +318,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);
@@ -412,7 +412,9 @@ 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 f8a3ebc..b75af7d 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -122,7 +122,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++)
@@ -147,7 +147,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);
@@ -157,6 +159,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);
@@ -188,9 +192,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) {
@@ -654,14 +655,24 @@ 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;
+ /* No locking needed around "unplugged" as we sleep during wait_event()
+ * below, anyway. We acquire the active device after we woke up so we
+ * never use unplugged devices. */
+ if (dev->unplugged)
+ return -ENODEV;
+
ret = wait_event_interruptible(file_priv->event_wait,
- !list_empty(&file_priv->event_list));
+ !list_empty(&file_priv->event_list) ||
+ dev->unplugged);
if (ret < 0)
return ret;
+ if (!drm_dev_get_active(dev))
+ return -ENODEV;
total = 0;
while (drm_dequeue_event(file_priv, total, count, &e)) {
@@ -675,6 +686,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);
@@ -682,13 +694,20 @@ 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;
+ /* signal HUP/IN on device removal */
+ if (!drm_dev_get_active(dev))
+ return POLLHUP | POLLIN | POLLRDNORM;
+
poll_wait(filp, &file_priv->event_wait, wait);
if (!list_empty(&file_priv->event_list))
mask |= POLLIN | POLLRDNORM;
+ drm_dev_put_active(dev);
+
return mask;
}
EXPORT_SYMBOL(drm_poll);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 4ff1227..c0e76c0 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -449,9 +449,12 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
dev->types[4] = _DRM_STAT_LOCKS;
dev->types[5] = _DRM_STAT_UNLOCKS;
- 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");
@@ -472,6 +475,8 @@ err_ctxbitmap:
drm_ctxbitmap_cleanup(dev);
err_map_hash:
drm_ht_remove(&dev->map_hash);
+err_rwsem:
+ percpu_free_rwsem(&dev->active);
err_free:
kfree(dev);
return NULL;
@@ -496,6 +501,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);
}
@@ -576,14 +582,21 @@ 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().
*/
void drm_dev_unregister(struct drm_device *dev)
{
struct drm_map_list *r_list, *list_temp;
+ /* Wait for pending users and then mark the device as unplugged. We
+ * must not hold the global-mutex while doing this. Otherwise, calls
+ * like drm_ioctl() or drm_lock() will dead-lock. */
+ percpu_down_write(&dev->active);
+ dev->unplugged = true;
+ percpu_up_write(&dev->active);
+
drm_lastclose(dev);
if (dev->driver->unload)
@@ -605,3 +618,46 @@ void drm_dev_unregister(struct drm_device *dev)
list_del(&dev->driver_item);
}
EXPORT_SYMBOL(drm_dev_unregister);
+
+/**
+ * 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 not allowed! They will dead-lock!
+ *
+ * RETURNS:
+ * True iff the device was marked active and can be used. False if the device
+ * was unplugged and must not be used.
+ */
+bool drm_dev_get_active(struct drm_device *dev)
+{
+ percpu_down_read(&dev->active);
+ 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 finished 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/include/drm/drmP.h b/include/drm/drmP.h
index 381679e..9689173 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1209,7 +1209,11 @@ struct drm_device {
/*@} */
int switch_power_state;
+ /** \name Hotplug Management */
+ /*@{ */
+ struct percpu_rw_semaphore active; /**< protect active users */
atomic_t unplugged; /* device has been unplugged or gone away */
+ /*@} */
};
#define DRM_SWITCH_POWER_ON 0
@@ -1710,6 +1714,8 @@ 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);
void drm_dev_unregister(struct drm_device *dev);
+bool drm_dev_get_active(struct drm_device *dev);
+void drm_dev_put_active(struct drm_device *dev);
int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type);
/*@}*/
--
1.8.3.3
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
