drivers/video/riva/fbdev.c broken on x86_64

drivers/video/riva/fbdev.c broken on x86_64

[J. Ryan Earl]

The following snippet is from drivers/video/riva/fbdev.c  I'm put arrows 
on the lines I think break cursor loading.  It does segfault, but boy 
does the cursor look weird.  The code in this function is so confusing, 
I have no idea what was going on or how to fix it:

/**
 * rivafb_load_cursor_image - load cursor image to hardware
 * @data: address to monochrome bitmap (1 = foreground color, 0 = 
background)
 * @par:  pointer to private data
 * @w:    width of cursor image in pixels
 * @h:    height of cursor image in scanlines
 * @bg:   background color (ARGB1555) - alpha bit determines opacity
 * @fg:   foreground color (ARGB1555)
 *
 * DESCRIPTiON:
 * Loads cursor image based on a monochrome source and mask bitmap.  The
 * image bits determines the color of the pixel, 0 for background, 1 for
 * foreground.  Only the affected region (as determined by @w and @h
 * parameters) will be updated.
 *
 * CALLED FROM:
 * rivafb_cursor()
 */
static void rivafb_load_cursor_image(struct riva_par *par, u8 *data,
                                     u8 *mask, u16 bg, u16 fg, u32 w, u32 h)
{
        int i, j, k = 0;
        u32 b, m, tmp;

        for (i = 0; i < h; i++) {
->             b = *((u32 *)data);
                b = (u32)((u32 *)b + 1);
->              m = *((u32 *)mask);
                m = (u32)((u32 *)m + 1);
                reverse_order(&b);


-ryan

Re: drivers/video/riva/fbdev.c broken on x86_64

[Andreas Schwab]

"J. Ryan Earl" <heretic@clanhk.org> writes:

> static void rivafb_load_cursor_image(struct riva_par *par, u8 *data,
>                                      u8 *mask, u16 bg, u16 fg, u32 w, u32 h)
> {
>         int i, j, k = 0;
>         u32 b, m, tmp;
>
>         for (i = 0; i < h; i++) {
> ->             b = *((u32 *)data);
>                 b = (u32)((u32 *)b + 1);
> ->              m = *((u32 *)mask);
>                 m = (u32)((u32 *)m + 1);

It appears that someone tried to fix the use of cast as lvalue and failed
miserably.  Untested patch ahead.

--- linux-2.6.5/drivers/video/riva/fbdev.c.~1~	2004-04-04 05:37:37.000000000 +0200
+++ linux-2.6.5/drivers/video/riva/fbdev.c	2004-05-14 23:23:38.092744302 +0200
@@ -500,9 +500,9 @@ static void rivafb_load_cursor_image(str
 
 	for (i = 0; i < h; i++) {
 		b = *((u32 *)data);
-		b = (u32)((u32 *)b + 1);
+		data = (u8 *)((u32 *)data + 1);
 		m = *((u32 *)mask);
-		m = (u32)((u32 *)m + 1);
+		mask = (u8 *)((u32 *)mask + 1);
 		reverse_order(&b);
 		
 		for (j = 0; j < w/2; j++) {

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: drivers/video/riva/fbdev.c broken on x86_64

[Andrew Morton]

Andreas Schwab <schwab@suse.de> wrote:
>
>          for (i = 0; i < h; i++) {
> > ->             b = *((u32 *)data);
> >                 b = (u32)((u32 *)b + 1);
> > ->              m = *((u32 *)mask);
> >                 m = (u32)((u32 *)m + 1);
> 
> It appears that someone tried to fix the use of cast as lvalue and failed
> miserably.

That would be me.

How about we simplify things a bit?




---

 25-akpm/drivers/video/riva/fbdev.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff -puN drivers/video/riva/fbdev.c~fbdev-lval-fix drivers/video/riva/fbdev.c
--- 25/drivers/video/riva/fbdev.c~fbdev-lval-fix	Fri May 14 16:34:10 2004
+++ 25-akpm/drivers/video/riva/fbdev.c	Fri May 14 16:35:32 2004
@@ -492,17 +492,17 @@ static inline void reverse_order(u32 *l)
  * CALLED FROM:
  * rivafb_cursor()
  */
-static void rivafb_load_cursor_image(struct riva_par *par, u8 *data, 
-				     u8 *mask, u16 bg, u16 fg, u32 w, u32 h)
+static void rivafb_load_cursor_image(struct riva_par *par, u8 *data8,
+				     u8 *mask8, u16 bg, u16 fg, u32 w, u32 h)
 {
 	int i, j, k = 0;
 	u32 b, m, tmp;
+	u32 *data = (u32 *)data8;
+	u32 *mask = (u32 *)mask8;
 
 	for (i = 0; i < h; i++) {
-		b = *((u32 *)data);
-		b = (u32)((u32 *)b + 1);
-		m = *((u32 *)mask);
-		m = (u32)((u32 *)m + 1);
+		b = *data++;
+		m = *mask++;
 		reverse_order(&b);
 		
 		for (j = 0; j < w/2; j++) {

_

Re: drivers/video/riva/fbdev.c broken on x86_64

[J. Ryan Earl]

Andreas' patch fixed part of the cursor problem, at least on the shell 
it's horizontal and not vertical.  However, it's still glitchy, in 
particular in a 'make menuconfig' the cursor blinks with random garbage 
in it.  I think it has to do with this warning:

  CC      drivers/video/fbmem.o
drivers/video/fbmem.c: In function `fb_cursor':
drivers/video/fbmem.c:917: warning: passing arg 1 of `copy_from_user' 
discards qualifiers from pointer target type

This line:
                if (copy_from_user(cursor.image.data, 
sprite->image.data, size) ||

-ryan

Andrew Morton wrote:

>Andreas Schwab <schwab@suse.de> wrote:
>  
>
>>         for (i = 0; i < h; i++) {
>>    
>>
>>>->             b = *((u32 *)data);
>>>                b = (u32)((u32 *)b + 1);
>>>->              m = *((u32 *)mask);
>>>                m = (u32)((u32 *)m + 1);
>>>      
>>>
>>It appears that someone tried to fix the use of cast as lvalue and failed
>>miserably.
>>    
>>
>
>That would be me.
>
>How about we simplify things a bit?
>
>
>
>
>---
>
> 25-akpm/drivers/video/riva/fbdev.c |   12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
>diff -puN drivers/video/riva/fbdev.c~fbdev-lval-fix drivers/video/riva/fbdev.c
>--- 25/drivers/video/riva/fbdev.c~fbdev-lval-fix	Fri May 14 16:34:10 2004
>+++ 25-akpm/drivers/video/riva/fbdev.c	Fri May 14 16:35:32 2004
>@@ -492,17 +492,17 @@ static inline void reverse_order(u32 *l)
>  * CALLED FROM:
>  * rivafb_cursor()
>  */
>-static void rivafb_load_cursor_image(struct riva_par *par, u8 *data, 
>-				     u8 *mask, u16 bg, u16 fg, u32 w, u32 h)
>+static void rivafb_load_cursor_image(struct riva_par *par, u8 *data8,
>+				     u8 *mask8, u16 bg, u16 fg, u32 w, u32 h)
> {
> 	int i, j, k = 0;
> 	u32 b, m, tmp;
>+	u32 *data = (u32 *)data8;
>+	u32 *mask = (u32 *)mask8;
> 
> 	for (i = 0; i < h; i++) {
>-		b = *((u32 *)data);
>-		b = (u32)((u32 *)b + 1);
>-		m = *((u32 *)mask);
>-		m = (u32)((u32 *)m + 1);
>+		b = *data++;
>+		m = *mask++;
> 		reverse_order(&b);
> 		
> 		for (j = 0; j < w/2; j++) {
>
>_
>
>  
>