[PATCH] radeon: Fix 3 regressions (was: glxgears crashes radeon driver)

[Jean Delvare]

Yes, it's me again ;)

On Sun, 9 May 2010 22:58:30 +0200, Jean Delvare wrote:
> Bisection says that the faulty commit is:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=b4fe945405e477cded91772b4fec854705443dd5
> 
> It doesn't revert cleanly from 2.6.34-rc6+, I'll try a manual revert
> tomorrow. My device is a Radeon 9200, so r200, if it helps.

I can confirm that manually reverting the patch above helps somewhat.
The crash is gone. Now I get an essentially black image in glxgears,
which may be a different bug.

(gdb) file drivers/gpu/drm/radeon/radeon.o
Reading symbols from /home/khali/src/linux-2.6.34-rc6/drivers/gpu/drm/radeon/radeon.o...done.
(gdb) list *(radeon_cp_cmdbuf+0x15e)
0xa58e is in radeon_cp_cmdbuf (drivers/gpu/drm/radeon/radeon_state.c:2921).
2916	
2917			drm_radeon_cmd_header_t *header;
2918			header = drm_buffer_read_object(cmdbuf->buffer,
2919					sizeof(stack_header), &stack_header);
2920	
2921			switch (header->header.cmd_type) {
2922			case RADEON_CMD_PACKET:
2923				DRM_DEBUG("RADEON_CMD_PACKET\n");
2924				if (radeon_emit_packets
2925				    (dev_priv, file_priv, *header, cmdbuf)) {
(gdb) 

I took a look at the above commit and found 3 bugs in it, one of which
is directly responsible for my crash. Candidate patch below.

Note 1: Why one would call radeon_cp_cmdbuf() with an empty buffer is
beyond me, but hey, what do I know about graphics drivers.
Note 2: Even with this patch, glxgears is all black unless I move the
window around, so there's one more bug to track. I guess I'm up for one
more bisection afternoon.

* * * * *

From: Jean Delvare <khali at linux-fr.org>
Subject: radeon: Fix 3 regressions

Commit b4fe945405e477cded91772b4fec854705443dd5 introduced 3 bugs,
fix them:

* Use the right command dword for second packet offset in
  RADEON_CNTL_PAINT/BITBLT_MULTI.
* Don't leak memory if drm_buffer_copy_from_user() fails.
* Don't call drm_buffer_unprocessed() unless drm_buffer_alloc() and
  drm_buffer_copy_from_user() have been called successfully first.

Signed-off-by: Jean Delvare <khali at linux-fr.org>
Cc: Pauli Nieminen <suokkos at gmail.com>
Cc: Dave Airlie <airlied at redhat.com>
---
I can certainly provide 3 separate patches for clarity if you prefer.

 drivers/gpu/drm/radeon/radeon_state.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

--- linux-2.6.34-rc6.orig/drivers/gpu/drm/radeon/radeon_state.c	2010-05-10 09:20:07.000000000 +0200
+++ linux-2.6.34-rc6/drivers/gpu/drm/radeon/radeon_state.c	2010-05-10 15:19:19.000000000 +0200
@@ -424,7 +424,7 @@ static __inline__ int radeon_check_and_f
 		if ((*cmd & RADEON_GMC_SRC_PITCH_OFFSET_CNTL) &&
 		    (*cmd & RADEON_GMC_DST_PITCH_OFFSET_CNTL)) {
 			u32 *cmd3 = drm_buffer_pointer_to_dword(cmdbuf->buffer, 3);
-			offset = *cmd << 10;
+			offset = *cmd3 << 10;
 			if (radeon_check_and_fixup_offset
 			    (dev_priv, file_priv, &offset)) {
 				DRM_ERROR("Invalid second packet offset\n");
@@ -2895,9 +2895,12 @@ static int radeon_cp_cmdbuf(struct drm_d
 			return rv;
 		rv = drm_buffer_copy_from_user(cmdbuf->buffer, buffer,
 						cmdbuf->bufsz);
-		if (rv)
+		if (rv) {
+			drm_buffer_free(cmdbuf->buffer);
 			return rv;
-	}
+		}
+	} else
+		goto done;
 
 	orig_nbox = cmdbuf->nbox;
 
@@ -2905,8 +2908,7 @@ static int radeon_cp_cmdbuf(struct drm_d
 		int temp;
 		temp = r300_do_cp_cmdbuf(dev, file_priv, cmdbuf);
 
-		if (cmdbuf->bufsz != 0)
-			drm_buffer_free(cmdbuf->buffer);
+		drm_buffer_free(cmdbuf->buffer);
 
 		return temp;
 	}
@@ -3012,16 +3014,15 @@ static int radeon_cp_cmdbuf(struct drm_d
 		}
 	}
 
-	if (cmdbuf->bufsz != 0)
-		drm_buffer_free(cmdbuf->buffer);
+	drm_buffer_free(cmdbuf->buffer);
 
+      done:
 	DRM_DEBUG("DONE\n");
 	COMMIT_RING();
 	return 0;
 
       err:
-	if (cmdbuf->bufsz != 0)
-		drm_buffer_free(cmdbuf->buffer);
+	drm_buffer_free(cmdbuf->buffer);
 	return -EINVAL;
 }
 

-- 
Jean Delvare