Commit ab5c54cb authored by Rob Clark's avatar Rob Clark
Browse files

drm/msm: Protect obj->active_count under obj lock 



Previously we only held obj lock in the _active_get() path, and relied
on atomic_dec_return() to not be racy in the _active_put() path where
obj lock was not held.

But this is a false sense of security.  Unlike obj lifetime refcnt,
where you do not expect to *increase* the refcnt after the last put
(which would mean that something has gone horribly wrong with the
object liveness reference counting), the active_count can increase
again from zero.  Racing _active_put()s and _active_get()s could leave
the obj on the wrong mm list.

But in the retire path, immediately after the _active_put(), the
_unpin_iova() would acquire obj lock.  So just move the locking earlier
and rely on that to protect obj->active_count.

Fixes: c5c1643c ("drm/msm: Drop struct_mutex from the retire path")
Signed-off-by: default avatarRob Clark <robdclark@chromium.org>
parent e8c76581

drivers/gpu/drm/msm/msm_gem.c

+6 −5
Original line number Diff line number Diff line
@@ -769,7 +769,7 @@ void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu) 
	WARN_ON(!msm_gem_is_locked(obj));

	WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);



	if (!atomic_fetch_inc(&msm_obj->active_count)) {

	if (msm_obj->active_count++ == 0) {

		mutex_lock(&priv->mm_lock);

		list_del_init(&msm_obj->mm_list);

		list_add_tail(&msm_obj->mm_list, &gpu->active_list);
@@ -783,8 +783,9 @@ void msm_gem_active_put(struct drm_gem_object *obj) 
	struct msm_drm_private *priv = obj->dev->dev_private;



	might_sleep();

	WARN_ON(!msm_gem_is_locked(obj));



	if (!atomic_dec_return(&msm_obj->active_count)) {

	if (--msm_obj->active_count == 0) {

		mutex_lock(&priv->mm_lock);

		list_del_init(&msm_obj->mm_list);

		list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
@@ -935,15 +936,15 @@ void msm_gem_free_object(struct drm_gem_object *obj) 
	struct drm_device *dev = obj->dev;

	struct msm_drm_private *priv = dev->dev_private;



	/* object should not be on active list: */

	WARN_ON(is_active(msm_obj));



	mutex_lock(&priv->mm_lock);

	list_del(&msm_obj->mm_list);

	mutex_unlock(&priv->mm_lock);



	msm_gem_lock(obj);



	/* object should not be on active list: */

	WARN_ON(is_active(msm_obj));



	put_iova(obj);



	if (obj->import_attach) {

drivers/gpu/drm/msm/msm_gem.h

+3 −2
Original line number Diff line number Diff line
@@ -87,7 +87,7 @@ struct msm_gem_object { 


	char name[32]; /* Identifier to print for the debugfs files */



	atomic_t active_count;

	int active_count;

};

#define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
@@ -185,7 +185,8 @@ msm_gem_is_locked(struct drm_gem_object *obj) 


static inline bool is_active(struct msm_gem_object *msm_obj)

{

	return atomic_read(&msm_obj->active_count);

	WARN_ON(!msm_gem_is_locked(&msm_obj->base));

	return msm_obj->active_count;

}



static inline bool is_purgeable(struct msm_gem_object *msm_obj)

drivers/gpu/drm/msm/msm_gpu.c

+6 −4
Original line number Diff line number Diff line
@@ -719,11 +719,13 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring, 
		stats->alwayson_start, stats->alwayson_end);



	for (i = 0; i < submit->nr_bos; i++) {

		struct msm_gem_object *msm_obj = submit->bos[i].obj;

		struct drm_gem_object *obj = &submit->bos[i].obj->base;



		msm_gem_active_put(&msm_obj->base);

		msm_gem_unpin_iova(&msm_obj->base, submit->aspace);

		drm_gem_object_put(&msm_obj->base);

		msm_gem_lock(obj);

		msm_gem_active_put(obj);

		msm_gem_unpin_iova_locked(obj, submit->aspace);

		msm_gem_unlock(obj);

		drm_gem_object_put(obj);

	}



	pm_runtime_mark_last_busy(&gpu->pdev->dev);