| Message ID | 1374673438-26251-9-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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); > /*@}*/ >
Hi On Wed, Jul 24, 2013 at 4:03 PM, Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote: > 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. It is. See this scenario: drm_dev_unregister() sets "plugged = true;" in write_lock(). This guarantees, there is currently no other pending user in any file-ops or mmap-fault handler (assuming the mmap fault handler is wrapped in drm_dev_get_active(), drm_dev_put_active()). After that, I call unmap_mapping_range() which resets all DRM-mapped PTEs of user-space processes. So if a mmap is accessed afterwards, it will trap and the fault() handler will fail with SIGBUS because drm_dev_get_active() will return false. New mmaps cannot be created, because drm_mmap() and alike will also be protected by drm_dev_get_active(). I don't see any race here, do you? > 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. Is this somehow faster/better than wrapping fault callbacks in get_active/put_active? > 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. I have 4 UDL cards at home and I dislike that all cards are blocked for a quite long time if I unplug just one card. As long as we have drm_global_mutex with this broad use, we cannot fix it. But I thought, I'd avoid introducing more global locks so maybe I can some day improve that, too. So I disagree, once we reduce drm_global_mutex, a single global rwsem _will_ have a negative effect. Besides, if you have only 1 GPU, it doesn't matter as you still only get a single rwsem. > > 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. If we want to fix the races completely, I don't think using i_mmap_mutex helps us. Regarding drm_global_mutex, as I said, my intention is to reduce this lock to a minimum. The only place where we need it is minor-allocation and driver-device-lists (ignoring the legacy users like drm-lock). For both, we could easily use spinlocks. So it's the same reason as above: I'd like to avoid contention on drm_global_mutex. Besides, I dislike broad locks. It's not like we safe a lot of memory here if we have one spinlock per driver instead of a global drm mutex. And if we have small-ranging locks, it is much easier to review them (at least to me). Cheers David
Op 24-07-13 16:24, David Herrmann schreef: > Hi > > On Wed, Jul 24, 2013 at 4:03 PM, Maarten Lankhorst > <maarten.lankhorst@canonical.com> wrote: >> 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. > It is. See this scenario: > > drm_dev_unregister() sets "plugged = true;" in write_lock(). This > guarantees, there is currently no other pending user in any file-ops > or mmap-fault handler (assuming the mmap fault handler is wrapped in > drm_dev_get_active(), drm_dev_put_active()). > > After that, I call unmap_mapping_range() which resets all DRM-mapped > PTEs of user-space processes. So if a mmap is accessed afterwards, it > will trap and the fault() handler will fail with SIGBUS because > drm_dev_get_active() will return false. New mmaps cannot be created, > because drm_mmap() and alike will also be protected by > drm_dev_get_active(). > > I don't see any race here, do you? > >> 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. > Is this somehow faster/better than wrapping fault callbacks in > get_active/put_active? Did you test the last patch with PROVE_LOCKING? >> 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. > I have 4 UDL cards at home and I dislike that all cards are blocked > for a quite long time if I unplug just one card. As long as we have > drm_global_mutex with this broad use, we cannot fix it. But I thought, > I'd avoid introducing more global locks so maybe I can some day > improve that, too. > > So I disagree, once we reduce drm_global_mutex, a single global rwsem > _will_ have a negative effect. Besides, if you have only 1 GPU, it > doesn't matter as you still only get a single rwsem. > >> 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. > If we want to fix the races completely, I don't think using > i_mmap_mutex helps us. > > Regarding drm_global_mutex, as I said, my intention is to reduce this > lock to a minimum. The only place where we need it is minor-allocation > and driver-device-lists (ignoring the legacy users like drm-lock). For > both, we could easily use spinlocks. So it's the same reason as above: > I'd like to avoid contention on drm_global_mutex. > Besides, I dislike broad locks. It's not like we safe a lot of memory > here if we have one spinlock per driver instead of a global drm mutex. > And if we have small-ranging locks, it is much easier to review them > (at least to me). > > Cheers > David >
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); /*@}*/
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@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(-)