diff mbox

drm/ttm: fix delayed ttm_bo_cleanup_refs_and_unlock delayed handling

Message ID 50D1F786.3020300@canonical.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maarten Lankhorst Dec. 19, 2012, 5:21 p.m. UTC
Op 19-12-12 15:18, Maarten Lankhorst schreef:
> Fix regression introduced by 85b144f860176
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>
>
Hey Paul Menzel,

I just wanted to have the fix out asap, and had to leave right after.
Updated commit message below:

Fix regression introduced by 85b144f860176
"drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v3"

Slowpath ttm_bo_cleanup_refs_and_unlock accidentally tried to increase
refcount on &bo->sync_obj instead of bo->sync_obj.

The compiler didn't complain since sync_obj_ref takes a void pointer,
so it was still valid c.

This could result in lockups, memory corruptions, and warnings like
these when graphics card VRAM usage is high:

------------[ cut here ]------------
WARNING: at include/linux/kref.h:42 radeon_fence_ref+0x2c/0x40()
Hardware name: System Product Name
Pid: 157, comm: X Not tainted 3.7.0-rc7-00520-g85b144f-dirty #174
Call Trace:
[<ffffffff81058c84>] ? warn_slowpath_common+0x74/0xb0
[<ffffffff8129273c>] ? radeon_fence_ref+0x2c/0x40
[<ffffffff8125e95c>] ? ttm_bo_cleanup_refs_and_unlock+0x18c/0x2d0
[<ffffffff8125f17c>] ? ttm_mem_evict_first+0x1dc/0x2a0
[<ffffffff81264452>] ? ttm_bo_man_get_node+0x62/0xb0
[<ffffffff8125f4ce>] ? ttm_bo_mem_space+0x28e/0x340
[<ffffffff8125fb0c>] ? ttm_bo_move_buffer+0xfc/0x170
[<ffffffff810de172>] ? kmem_cache_alloc+0xb2/0xc0
[<ffffffff8125fc15>] ? ttm_bo_validate+0x95/0x110
[<ffffffff8125ff7c>] ? ttm_bo_init+0x2ec/0x3b0
[<ffffffff8129419a>] ? radeon_bo_create+0x18a/0x200
[<ffffffff81293e80>] ? radeon_bo_clear_va+0x40/0x40
[<ffffffff812a5342>] ? radeon_gem_object_create+0x92/0x160
[<ffffffff812a575c>] ? radeon_gem_create_ioctl+0x6c/0x150
[<ffffffff812a529f>] ? radeon_gem_object_free+0x2f/0x40
[<ffffffff81246b60>] ? drm_ioctl+0x420/0x4f0
[<ffffffff812a56f0>] ? radeon_gem_pwrite_ioctl+0x20/0x20
[<ffffffff810f53a4>] ? do_vfs_ioctl+0x2e4/0x4e0
[<ffffffff810e5588>] ? vfs_read+0x118/0x160
[<ffffffff810f55ec>] ? sys_ioctl+0x4c/0xa0
[<ffffffff810e5851>] ? sys_read+0x51/0xa0
[<ffffffff814b0612>] ? system_call_fastpath+0x16/0x1b

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>
Message-ID: <20121217182752.GA351@x4>
---

Comments

Paul Menzel Dec. 19, 2012, 6:40 p.m. UTC | #1
Dear Maarten,


Am Mittwoch, den 19.12.2012, 18:21 +0100 schrieb Maarten Lankhorst:
> Op 19-12-12 15:18, Maarten Lankhorst schreef:
> > Fix regression introduced by 85b144f860176
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> > Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>

> I just wanted to have the fix out asap, and had to leave right after.

I see. Sorry for my message then!

> Updated commit message below:

Awesome! By the way, you can add --- 8< --- below the message and the
patch with commit message and `git am --scissors` will leave out the
reply in the commit message.

> Fix regression introduced by 85b144f860176
> "drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v3"
> 
> Slowpath ttm_bo_cleanup_refs_and_unlock accidentally tried to increase
> refcount on &bo->sync_obj instead of bo->sync_obj.
> 
> The compiler didn't complain since sync_obj_ref takes a void pointer,
> so it was still valid c.
> 
> This could result in lockups, memory corruptions, and warnings like
> these when graphics card VRAM usage is high:
> 
> ------------[ cut here ]------------
> WARNING: at include/linux/kref.h:42 radeon_fence_ref+0x2c/0x40()
> Hardware name: System Product Name
> Pid: 157, comm: X Not tainted 3.7.0-rc7-00520-g85b144f-dirty #174
> Call Trace:
> [<ffffffff81058c84>] ? warn_slowpath_common+0x74/0xb0
> [<ffffffff8129273c>] ? radeon_fence_ref+0x2c/0x40
> [<ffffffff8125e95c>] ? ttm_bo_cleanup_refs_and_unlock+0x18c/0x2d0
> [<ffffffff8125f17c>] ? ttm_mem_evict_first+0x1dc/0x2a0
> [<ffffffff81264452>] ? ttm_bo_man_get_node+0x62/0xb0
> [<ffffffff8125f4ce>] ? ttm_bo_mem_space+0x28e/0x340
> [<ffffffff8125fb0c>] ? ttm_bo_move_buffer+0xfc/0x170
> [<ffffffff810de172>] ? kmem_cache_alloc+0xb2/0xc0
> [<ffffffff8125fc15>] ? ttm_bo_validate+0x95/0x110
> [<ffffffff8125ff7c>] ? ttm_bo_init+0x2ec/0x3b0
> [<ffffffff8129419a>] ? radeon_bo_create+0x18a/0x200
> [<ffffffff81293e80>] ? radeon_bo_clear_va+0x40/0x40
> [<ffffffff812a5342>] ? radeon_gem_object_create+0x92/0x160
> [<ffffffff812a575c>] ? radeon_gem_create_ioctl+0x6c/0x150
> [<ffffffff812a529f>] ? radeon_gem_object_free+0x2f/0x40
> [<ffffffff81246b60>] ? drm_ioctl+0x420/0x4f0
> [<ffffffff812a56f0>] ? radeon_gem_pwrite_ioctl+0x20/0x20
> [<ffffffff810f53a4>] ? do_vfs_ioctl+0x2e4/0x4e0
> [<ffffffff810e5588>] ? vfs_read+0x118/0x160
> [<ffffffff810f55ec>] ? sys_ioctl+0x4c/0xa0
> [<ffffffff810e5851>] ? sys_read+0x51/0xa0
> [<ffffffff814b0612>] ? system_call_fastpath+0x16/0x1b
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>
> Message-ID: <20121217182752.GA351@x4>
> ---
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 0bf66f9..9f85418 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -579,7 +579,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
>  		 * at this point the buffer should be dead, so
>  		 * no new sync objects can be attached.
>  		 */
> -		sync_obj = driver->sync_obj_ref(&bo->sync_obj);
> +		sync_obj = driver->sync_obj_ref(bo->sync_obj);
>  		spin_unlock(&bdev->fence_lock);
>  
>  		atomic_set(&bo->reserved, 0);

Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>


Thanks,

Paul
diff mbox

Patch

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 0bf66f9..9f85418 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -579,7 +579,7 @@  static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 		 * at this point the buffer should be dead, so
 		 * no new sync objects can be attached.
 		 */
-		sync_obj = driver->sync_obj_ref(&bo->sync_obj);
+		sync_obj = driver->sync_obj_ref(bo->sync_obj);
 		spin_unlock(&bdev->fence_lock);
 
 		atomic_set(&bo->reserved, 0);