Regression X Hangs at bootup -- PATCH

From: Florian Mickler
Date: Mon Apr 06 2009 - 16:56:12 EST


[resent, because i got the lkml-adress wrong the first time]
Hi Eric!

To put this mail into context:
http://bugs.freedesktop.org/show_bug.cgi?id=20985

I finally poked a little bit at the code, since i figured you would be
glad if i came up with something.

I think I understood the problem and made a correct patch, but kernel
is new territory for me. So please doublecheck if i made the correct
choices for the error-returns.

Can you make shure that this patch (if acceptable) goes into mainline?

Sincerely,
Florian


p.s.: does somebody know where the actual implementation of
copy_[from/to]_user for my core2duo @64bit is? it is a mistery!

From 95d4c8702dbd0bbc06291105c3fbfe1927b13f2d Mon Sep 17 00:00:00 2001
From: Florian Mickler <florian@xxxxxxxxxxx>
Date: Mon, 6 Apr 2009 21:35:33 +0200
Subject: [PATCH] Fix: use of uninitialized var

i915_gem_put_relocs_to_user returned an uninitialized value which
got returned to userspace. This caused libdrm in my setup to never
get out of a do{}while() loop retrying i915_gem_execbuffer.

result was hanging X, overheating of cpu and 2-3gb of logfile-spam.

This patch adresses the issue by
1. initializing vars in this file where necessary
2. correcting wrongly interpreted return values of
copy_[from/to]_user

Nr. 2 helps libdrm from getting out of its loop and Nr. 1 helps
i915_gem_execbuffer to not think there was an error.

Signed-off-by: Florian Mickler <florian@xxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 63 +++++++++++++++++++++++++-------------
1 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1449b45..99c01f5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -151,6 +151,7 @@ fast_shmem_read(struct page **pages,
ret = __copy_to_user_inatomic(data, vaddr + page_offset, length);
kunmap_atomic(vaddr, KM_USER0);

+ /* return a number of bytes */
return ret;
}

@@ -2976,7 +2977,7 @@ i915_gem_get_relocs_from_user(struct drm_i915_gem_exec_object *exec_list,
struct drm_i915_gem_relocation_entry **relocs)
{
uint32_t reloc_count = 0, reloc_index = 0, i;
- int ret;
+ int ret = 0;

*relocs = NULL;
for (i = 0; i < buffer_count; i++) {
@@ -3002,13 +3003,14 @@ i915_gem_get_relocs_from_user(struct drm_i915_gem_exec_object *exec_list,
drm_free(*relocs, reloc_count * sizeof(**relocs),
DRM_MEM_DRIVER);
*relocs = NULL;
- return ret;
+ return -EFAULT;
}

reloc_index += exec_list[i].relocation_count;
}

- return ret;
+ /* success */
+ return 0;
}

static int
@@ -3017,14 +3019,14 @@ i915_gem_put_relocs_to_user(struct drm_i915_gem_exec_object *exec_list,
struct drm_i915_gem_relocation_entry *relocs)
{
uint32_t reloc_count = 0, i;
- int ret;
+ int ret = 0;

for (i = 0; i < buffer_count; i++) {
struct drm_i915_gem_relocation_entry __user *user_relocs;

user_relocs = (void __user *)(uintptr_t)exec_list[i].relocs_ptr;

- if (ret == 0) {
+ if ( ret == 0) {
ret = copy_to_user(user_relocs,
&relocs[reloc_count],
exec_list[i].relocation_count *
@@ -3036,6 +3038,12 @@ i915_gem_put_relocs_to_user(struct drm_i915_gem_exec_object *exec_list,

drm_free(relocs, reloc_count * sizeof(*relocs), DRM_MEM_DRIVER);

+ /* copy_to_user returns a number of bytes */
+ if (ret != 0) {
+ ret = -EFAULT;
+ }
+
+
return ret;
}

@@ -3052,11 +3060,14 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
struct drm_i915_gem_object *obj_priv;
struct drm_clip_rect *cliprects = NULL;
struct drm_i915_gem_relocation_entry *relocs;
- int ret, ret2, i, pinned = 0;
+ int ret, error, i, pinned;
uint64_t exec_offset;
uint32_t seqno, flush_domains, reloc_index;
int pin_tries;

+ error = 0;
+ pinned = 0;
+
#if WATCH_EXEC
DRM_INFO("buffers_ptr %d buffer_count %d len %08x\n",
(int) args->buffers_ptr, args->buffer_count, args->batch_len);
@@ -3075,7 +3086,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
DRM_ERROR("Failed to allocate exec or object list "
"for %d buffers\n",
args->buffer_count);
- ret = -ENOMEM;
+ error = -ENOMEM;
goto pre_mutex_err;
}
ret = copy_from_user(exec_list,
@@ -3085,6 +3096,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
if (ret != 0) {
DRM_ERROR("copy %d exec entries failed %d\n",
args->buffer_count, ret);
+ error = ret;
goto pre_mutex_err;
}

@@ -3101,15 +3113,17 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
if (ret != 0) {
DRM_ERROR("copy %d cliprects failed: %d\n",
args->num_cliprects, ret);
+ error = ret;
goto pre_mutex_err;
}
}

ret = i915_gem_get_relocs_from_user(exec_list, args->buffer_count,
&relocs);
- if (ret != 0)
+ if (ret != 0) {
+ error = ret;
goto pre_mutex_err;
-
+ }
mutex_lock(&dev->struct_mutex);

i915_verify_inactive(dev, __FILE__, __LINE__);
@@ -3117,14 +3131,14 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
if (dev_priv->mm.wedged) {
DRM_ERROR("Execbuf while wedged\n");
mutex_unlock(&dev->struct_mutex);
- ret = -EIO;
+ error = -EIO;
goto pre_mutex_err;
}

if (dev_priv->mm.suspended) {
DRM_ERROR("Execbuf while VT-switched.\n");
mutex_unlock(&dev->struct_mutex);
- ret = -EBUSY;
+ error = -EBUSY;
goto pre_mutex_err;
}

@@ -3135,7 +3149,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
if (object_list[i] == NULL) {
DRM_ERROR("Invalid object handle %d at index %d\n",
exec_list[i].handle, i);
- ret = -EBADF;
+ error = -EBADF;
goto err;
}

@@ -3143,7 +3157,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
if (obj_priv->in_execbuffer) {
DRM_ERROR("Object %p appears more than once in object list\n",
object_list[i]);
- ret = -EBADF;
+ error = -EBADF;
goto err;
}
obj_priv->in_execbuffer = true;
@@ -3174,6 +3188,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
if (ret != -ENOMEM || pin_tries >= 1) {
if (ret != -ERESTARTSYS)
DRM_ERROR("Failed to pin buffers %d\n", ret);
+ error = ret;
goto err;
}

@@ -3184,8 +3199,10 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,

/* evict everyone we can from the aperture */
ret = i915_gem_evict_everything(dev);
- if (ret)
+ if (ret){
+ error = ret;
goto err;
+ }
}

/* Set the pending read domains for the batch buffer to COMMAND */
@@ -3253,6 +3270,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
ret = i915_dispatch_gem_execbuffer(dev, args, cliprects, exec_offset);
if (ret) {
DRM_ERROR("dispatch failed %d\n", ret);
+ error = ret;
goto err;
}

@@ -3308,10 +3326,12 @@ err:
(uintptr_t) args->buffers_ptr,
exec_list,
sizeof(*exec_list) * args->buffer_count);
- if (ret)
+ if (ret) {
+ error = -EFAULT;
DRM_ERROR("failed to copy %d exec entries "
"back to user (%d)\n",
args->buffer_count, ret);
+ }
}

/* Copy the updated relocations out regardless of current error
@@ -3319,13 +3339,12 @@ err:
* time userland calls execbuf, it would do so with presumed offset
* state that didn't match the actual object state.
*/
- ret2 = i915_gem_put_relocs_to_user(exec_list, args->buffer_count,
+ ret = i915_gem_put_relocs_to_user(exec_list, args->buffer_count,
relocs);
- if (ret2 != 0) {
- DRM_ERROR("Failed to copy relocations back out: %d\n", ret2);
-
- if (ret == 0)
- ret = ret2;
+ if (ret != 0) {
+ DRM_ERROR("Failed to copy relocations back out\n");
+ if (error == 0)
+ error = ret;
}

pre_mutex_err:
@@ -3336,7 +3355,7 @@ pre_mutex_err:
drm_free(cliprects, sizeof(*cliprects) * args->num_cliprects,
DRM_MEM_DRIVER);

- return ret;
+ return error;
}

int
--
1.6.2

Attachment: signature.asc
Description: PGP signature