On Tue, Apr 26, 2016 at 07:29:41PM +0200, Daniel Vetter wrote:
> amdgpu gained dev->struct_mutex usage, and that's because it's walking
> the dev->filelist list. Protect that list with it's own lock to take
> one more step towards getting rid of struct_mutex usage in drivers
> once and for all.
>
> While doing the conversion I noticed that 2 debugfs files in i915
> completely lacked appropriate locking. Fix that up too.
>
> v2: don't forget to switch to drm_gem_object_unreference_unlocked.
>
> Cc: Alex Deucher <alexander.deuc...@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
Ok, merged up to this patch to drm-misc, thanks a lot for feedback and
review.
-Daniel
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 +++++-----
> drivers/gpu/drm/drm_drv.c | 1 +
> drivers/gpu/drm/drm_fops.c | 9 ++++++---
> drivers/gpu/drm/drm_info.c | 4 ++--
> drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++--
> include/drm/drmP.h | 1 +
> 6 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index fa6a27bff298..a087b9638cde 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -93,7 +93,7 @@ void amdgpu_gem_force_release(struct amdgpu_device *adev)
> struct drm_device *ddev = adev->ddev;
> struct drm_file *file;
>
> - mutex_lock(&ddev->struct_mutex);
> + mutex_lock(&ddev->filelist_mutex);
>
> list_for_each_entry(file, &ddev->filelist, lhead) {
> struct drm_gem_object *gobj;
> @@ -103,13 +103,13 @@ void amdgpu_gem_force_release(struct amdgpu_device
> *adev)
> spin_lock(&file->table_lock);
> idr_for_each_entry(&file->object_idr, gobj, handle) {
> WARN_ONCE(1, "And also active allocations!\n");
> - drm_gem_object_unreference(gobj);
> + drm_gem_object_unreference_unlocked(gobj);
> }
> idr_destroy(&file->object_idr);
> spin_unlock(&file->table_lock);
> }
>
> - mutex_unlock(&ddev->struct_mutex);
> + mutex_unlock(&ddev->filelist_mutex);
> }
>
> /*
> @@ -769,7 +769,7 @@ static int amdgpu_debugfs_gem_info(struct seq_file *m,
> void *data)
> struct drm_file *file;
> int r;
>
> - r = mutex_lock_interruptible(&dev->struct_mutex);
> + r = mutex_lock_interruptible(&dev->filelist_mutex);
> if (r)
> return r;
>
> @@ -793,7 +793,7 @@ static int amdgpu_debugfs_gem_info(struct seq_file *m,
> void *data)
> spin_unlock(&file->table_lock);
> }
>
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&dev->filelist_mutex);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e1fb52d4f72c..bff89226a344 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -590,6 +590,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver
> *driver,
> spin_lock_init(&dev->buf_lock);
> spin_lock_init(&dev->event_lock);
> mutex_init(&dev->struct_mutex);
> + mutex_init(&dev->filelist_mutex);
> mutex_init(&dev->ctxlist_mutex);
> mutex_init(&dev->master_mutex);
>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index c3d0aaac0669..7af7f8bcb355 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -297,9 +297,9 @@ static int drm_open_helper(struct file *filp, struct
> drm_minor *minor)
> }
> mutex_unlock(&dev->master_mutex);
>
> - mutex_lock(&dev->struct_mutex);
> + mutex_lock(&dev->filelist_mutex);
> list_add(&priv->lhead, &dev->filelist);
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&dev->filelist_mutex);
>
> #ifdef __alpha__
> /*
> @@ -447,8 +447,11 @@ int drm_release(struct inode *inode, struct file *filp)
>
> DRM_DEBUG("open_count = %d\n", dev->open_count);
>
> - mutex_lock(&dev->struct_mutex);
> + mutex_lock(&dev->filelist_mutex);
> list_del(&file_priv->lhead);
> + mutex_unlock(&dev->filelist_mutex);
> +
> + mutex_lock(&dev->struct_mutex);
> if (file_priv->magic)
> idr_remove(&file_priv->master->magic_map, file_priv->magic);
> mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
> index cbb4fc0fc969..5d469b2f26f4 100644
> --- a/drivers/gpu/drm/drm_info.c
> +++ b/drivers/gpu/drm/drm_info.c
> @@ -174,7 +174,7 @@ int drm_clients_info(struct seq_file *m, void *data)
> /* dev->filelist is sorted youngest first, but we want to present
> * oldest first (i.e. kernel, servers, clients), so walk backwardss.
> */
> - mutex_lock(&dev->struct_mutex);
> + mutex_lock(&dev->filelist_mutex);
> list_for_each_entry_reverse(priv, &dev->filelist, lhead) {
> struct task_struct *task;
>
> @@ -190,7 +190,7 @@ int drm_clients_info(struct seq_file *m, void *data)
> priv->magic);
> rcu_read_unlock();
> }
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&dev->filelist_mutex);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 4950d05d2948..8b8d6f07d7aa 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -528,6 +528,10 @@ static int i915_gem_object_info(struct seq_file *m,
> void* data)
>
> seq_putc(m, '\n');
> print_batch_pool_stats(m, dev_priv);
> +
> + mutex_unlock(&dev->struct_mutex);
> +
> + mutex_lock(&dev->filelist_mutex);
> list_for_each_entry_reverse(file, &dev->filelist, lhead) {
> struct file_stats stats;
> struct task_struct *task;
> @@ -548,8 +552,7 @@ static int i915_gem_object_info(struct seq_file *m, void*
> data)
> print_file_stats(m, task ? task->comm : "<unknown>", stats);
> rcu_read_unlock();
> }
> -
> - mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&dev->filelist_mutex);
>
> return 0;
> }
> @@ -2354,6 +2357,7 @@ static int i915_ppgtt_info(struct seq_file *m, void
> *data)
> else if (INTEL_INFO(dev)->gen >= 6)
> gen6_ppgtt_info(m, dev);
>
> + mutex_lock(&dev->filelist_mutex);
> list_for_each_entry_reverse(file, &dev->filelist, lhead) {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> struct task_struct *task;
> @@ -2368,6 +2372,7 @@ static int i915_ppgtt_info(struct seq_file *m, void
> *data)
> idr_for_each(&file_priv->context_idr, per_file_ctx,
> (void *)(unsigned long)m);
> }
> + mutex_unlock(&dev->filelist_mutex);
>
> out_put:
> intel_runtime_pm_put(dev_priv);
> @@ -2403,6 +2408,8 @@ static int i915_rps_boost_info(struct seq_file *m, void
> *data)
> intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit),
> intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit),
> intel_gpu_freq(dev_priv, dev_priv->rps.max_freq));
> +
> + mutex_lock(&dev->filelist_mutex);
> spin_lock(&dev_priv->rps.client_lock);
> list_for_each_entry_reverse(file, &dev->filelist, lhead) {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> @@ -2425,6 +2432,7 @@ static int i915_rps_boost_info(struct seq_file *m, void
> *data)
> list_empty(&dev_priv->rps.mmioflips.link) ? "" : ", active");
> seq_printf(m, "Kernel boosts: %d\n", dev_priv->rps.boosts);
> spin_unlock(&dev_priv->rps.client_lock);
> + mutex_unlock(&dev->filelist_mutex);
>
> return 0;
> }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index f82979dee647..c81dd2250fc6 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -769,6 +769,7 @@ struct drm_device {
> atomic_t buf_alloc; /**< Buffer allocation in progress */
> /*@} */
>
> + struct mutex filelist_mutex;
> struct list_head filelist;
>
> /** \name Memory management */
> --
> 2.8.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
