[RFC 8/9] drm: track pending user-space actions
Maarten Lankhorst
maarten.lankhorst at canonical.com
Wed Jul 24 07:03:14 PDT 2013
Op 24-07-13 15:43, David Herrmann schreef:
> 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.
Meh, I don't think it's possible to make the mmaps completely race free.
I 'solved' this by taking mapping->i_mmap_mutex, it reduces the window,
because all the mmap callbacks are called while holding that lock, so at least
new mappings from splitting mappings up cannot happen.
Why did you make the percpu rwsem local to the device?
It's only going to be taking in write mode during device destruction, which
isn't exactly fast anyway. A single global rwsem wouldn't have any negative effect.
My original patch was locking mapping->i_mmap_mutex, then the rwsem in write mode, then the
global mutex lock during unplug, to ensure that all code saw the unplugged correctly.
That way using any of those 3 locks would be sufficient to check dev->unplugged safely.
> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
> ---
> 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);
> /*@}*/
>
More information about the dri-devel
mailing list