Regression X Hangs at bootup -- PATCH

[Florian Mickler <florian@mickler.org>]

[-- Attachment #1.1: Type: text/plain, Size: 670 bytes --]

[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!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Fix-use-of-uninitialized-var.patch --]
[-- Type: text/x-patch, Size: 7432 bytes --]

From 95d4c8702dbd0bbc06291105c3fbfe1927b13f2d Mon Sep 17 00:00:00 2001
From: Florian Mickler <florian@mickler.org>
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@mickler.org>
---
 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]