[PATCH] i810fb: Fix console switch regression

[PATCH] i810fb: Fix console switch regression

[Stefan Bauer]

From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>

Commit eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! versus & precedence in 
various places") introduced a regression in console switching when using 
i810fb. Every 5th to 10th console switch causes 'pixel waste' - the same line 
of multi-colored pixels repeated over the whole screen.
This reverts eaa0ff1 for i810_main.c.

Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
Cc: Antonino Daplas <adaplas@pol.net>

---
As I'm not subscribed to the LKML, please CC me, thanks.

--- linux-2.6/drivers/video/i810/i810_main.c.orig
+++ linux-2.6/drivers/video/i810/i810_main.c
@@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
	struct i810fb_par *par = info->par;
	u8 __iomem *mmio = par->mmio_start_virtual;

-	if (!(par->dev_flags & LOCKUP))
+	if (!par->dev_flags & LOCKUP)
		return -ENXIO;

	if (cursor->image.width > 64 || cursor->image.height > 64)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] i810fb: Fix console switch regression

[Geert Uytterhoeven]

On Sun, 16 Mar 2008, Stefan Bauer wrote:
> From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> 
> Commit eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! versus & precedence in 
> various places") introduced a regression in console switching when using 
> i810fb. Every 5th to 10th console switch causes 'pixel waste' - the same line 
> of multi-colored pixels repeated over the whole screen.
> This reverts eaa0ff1 for i810_main.c.
> 
> Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> Cc: Antonino Daplas <adaplas@pol.net>
> 
> ---
> As I'm not subscribed to the LKML, please CC me, thanks.
> 
> --- linux-2.6/drivers/video/i810/i810_main.c.orig
> +++ linux-2.6/drivers/video/i810/i810_main.c
> @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
> 	struct i810fb_par *par = info->par;
> 	u8 __iomem *mmio = par->mmio_start_virtual;
> 
> -	if (!(par->dev_flags & LOCKUP))
> +	if (!par->dev_flags & LOCKUP)
> 		return -ENXIO;

However, the original expression didn't make sense, as LOCKUP is 8 and
!par->dev_flags is either 0 or 1, so `!par->dev_flags & LOCKUP' is
always 0.

I took a quick look at the usage of the LOCKUP flag. Apparently when a
lock-up is detected, this flag is set, and the driver will fall back to
software operations instead of hardware accelerated operations.

Is it possible the intended code was

	if (par->dev_flags & LOCKUP)
		return -ENXIO;

?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] i810fb: Fix console switch regression

[Stefan Bauer]

Am Sonntag, 16. März 2008 20:48 schrieb Geert Uytterhoeven:
> On Sun, 16 Mar 2008, Stefan Bauer wrote:
> > --- linux-2.6/drivers/video/i810/i810_main.c.orig
> > +++ linux-2.6/drivers/video/i810/i810_main.c
> > @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info
> > 	struct i810fb_par *par = info->par;
> > 	u8 __iomem *mmio = par->mmio_start_virtual;
> >
> > -	if (!(par->dev_flags & LOCKUP))
> > +	if (!par->dev_flags & LOCKUP)
> > 		return -ENXIO;
>
> However, the original expression didn't make sense, as LOCKUP is 8 and
> !par->dev_flags is either 0 or 1, so `!par->dev_flags & LOCKUP' is
> always 0.
>
> I took a quick look at the usage of the LOCKUP flag. Apparently when a
> lock-up is detected, this flag is set, and the driver will fall back to
> software operations instead of hardware accelerated operations.
>
> Is it possible the intended code was
>
> 	if (par->dev_flags & LOCKUP)
> 		return -ENXIO;
>
> ?

Yes, IMO you are right. 4c7ffe0 ("fbdev: prevent drivers that have hardware 
cursors from calling software cursor code") introduced the senseless code.

I'm going on testing today, but I think that's it.

Again, please CC me, thanks.

---
From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>

Since 4c7ffe0b9f7f40bd818fe3af51342f64c483908e ("fbdev: prevent drivers that 
have hardware cursors from calling software cursor code") every call of 
i810fb_cursor fails with -ENXIO because of a incorrect "!".

This hasn't striked until eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! 
versus & precedence in various places") surrounded the expression with 
braces, so that the intended behavior was inverted. That caused 'pixel 
waste' - the same line of multi-colored pixels repeated over the whole 
screen - during console switch.

This switches back to the original pre-4c7ffe0 behavior.

Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Antonino Daplas <adaplas@pol.net>

---
--- linux-2.6/drivers/video/i810/i810_main.c.orig
+++ linux-2.6/drivers/video/i810/i810_main.c
@@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info
        struct i810fb_par *par = info->par;
        u8 __iomem *mmio = par->mmio_start_virtual;

-       if (!(par->dev_flags & LOCKUP))
+       if (par->dev_flags & LOCKUP)
                return -ENXIO;

        if (cursor->image.width > 64 || cursor->image.height > 64)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] i810fb: Fix console switch regression

[Stefan Bauer]

I'm sorry for the spaces in the second patch, here's a clean one with tabs.

---
From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>

Since 4c7ffe0b9f7f40bd818fe3af51342f64c483908e ("fbdev: prevent drivers that 
have hardware cursors from calling software cursor code") every call of 
i810fb_cursor fails with -ENXIO because of a incorrect "!".

This hasn't striked until eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! 
versus & precedence in various places") surrounded the expression with 
braces, so that the intended behavior was inverted. That caused 'pixel 
waste' - the same line of multi-colored pixels repeated over the whole 
screen - during console switch.

This switches back to the original pre-4c7ffe0 behavior.

Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Antonino Daplas <adaplas@pol.net>

--- linux-2.6/drivers/video/i810/i810_main.c.orig
+++ linux-2.6/drivers/video/i810/i810_main.c
@@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
 	struct i810fb_par *par = info->par;
 	u8 __iomem *mmio = par->mmio_start_virtual;
 
-	if (!(par->dev_flags & LOCKUP))
+	if (par->dev_flags & LOCKUP)
 		return -ENXIO;
 
 	if (cursor->image.width > 64 || cursor->image.height > 64)

Re: [PATCH] i810fb: Fix console switch regression

[Andrew Morton]

On Mon, 17 Mar 2008 16:41:10 +0100 Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de> wrote:

> I'm sorry for the spaces in the second patch, here's a clean one with tabs.
> 
> ---
> From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> 
> Since 4c7ffe0b9f7f40bd818fe3af51342f64c483908e ("fbdev: prevent drivers that 
> have hardware cursors from calling software cursor code") every call of 
> i810fb_cursor fails with -ENXIO because of a incorrect "!".
> 
> This hasn't striked until eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! 
> versus & precedence in various places") surrounded the expression with 
> braces, so that the intended behavior was inverted. That caused 'pixel 
> waste' - the same line of multi-colored pixels repeated over the whole 
> screen - during console switch.
> 
> This switches back to the original pre-4c7ffe0 behavior.
> 
> Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Antonino Daplas <adaplas@pol.net>
> 
> --- linux-2.6/drivers/video/i810/i810_main.c.orig
> +++ linux-2.6/drivers/video/i810/i810_main.c
> @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
>  	struct i810fb_par *par = info->par;
>  	u8 __iomem *mmio = par->mmio_start_virtual;
>  
> -	if (!(par->dev_flags & LOCKUP))
> +	if (par->dev_flags & LOCKUP)
>  		return -ENXIO;
>  
>  	if (cursor->image.width > 64 || cursor->image.height > 64)

Can you please confirm that this actually fixes the bug?  That wasn't clear.

Thanks.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] i810fb: Fix console switch regression

[Stefan Bauer]

Am Dienstag, 18. März 2008 04:48 schrieb Andrew Morton:
> On Mon, 17 Mar 2008 16:41:10 +0100 Stefan Bauer 
<stefan.bauer@cs.tu-chemnitz.de> wrote:
> > From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> >
> > Since 4c7ffe0b9f7f40bd818fe3af51342f64c483908e ("fbdev: prevent drivers
> > that have hardware cursors from calling software cursor code") every call
> > of i810fb_cursor fails with -ENXIO because of a incorrect "!".
> >
> > This hasn't striked until eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix
> > ! versus & precedence in various places") surrounded the expression with
> > braces, so that the intended behavior was inverted. That caused 'pixel
> > waste' - the same line of multi-colored pixels repeated over the whole
> > screen - during console switch.
> >
> > This switches back to the original pre-4c7ffe0 behavior.
> >
> > Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Antonino Daplas <adaplas@pol.net>
> >
> > --- linux-2.6/drivers/video/i810/i810_main.c.orig
> > +++ linux-2.6/drivers/video/i810/i810_main.c
> > @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info
> >  	struct i810fb_par *par = info->par;
> >  	u8 __iomem *mmio = par->mmio_start_virtual;
> >
> > -	if (!(par->dev_flags & LOCKUP))
> > +	if (par->dev_flags & LOCKUP)
> >  		return -ENXIO;
> >
> >  	if (cursor->image.width > 64 || cursor->image.height > 64)
>
> Can you please confirm that this actually fixes the bug?  That wasn't
> clear.
>
> Thanks.

Yes, yesterday I worked the whole day with that machine running -rc6 + my 
patch. The regression definitely is fixed, fbi and fbset work w/o problems or 
new regressions.

	Regards
		Stefan

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] i810fb: Fix console switch regression

[Henrique de Moraes Holschuh]

On Sun, 16 Mar 2008, Stefan Bauer wrote:
> From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> 
> Commit eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! versus & precedence in 
> various places") introduced a regression in console switching when using 
> i810fb. Every 5th to 10th console switch causes 'pixel waste' - the same line 
> of multi-colored pixels repeated over the whole screen.
> This reverts eaa0ff1 for i810_main.c.
> 
> Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> Cc: Antonino Daplas <adaplas@pol.net>
> 
> ---
> As I'm not subscribed to the LKML, please CC me, thanks.
> 
> --- linux-2.6/drivers/video/i810/i810_main.c.orig
> +++ linux-2.6/drivers/video/i810/i810_main.c
> @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
> 	struct i810fb_par *par = info->par;
> 	u8 __iomem *mmio = par->mmio_start_virtual;
> 
> -	if (!(par->dev_flags & LOCKUP))
> +	if (!par->dev_flags & LOCKUP)
> 		return -ENXIO;
> 
> 	if (cursor->image.width > 64 || cursor->image.height > 64)
> --

I think you need to add something to that line that avoids someone making
the same change as eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 some years down
the road.

Might I suggest using "((!par->dev_flags) & LOCKUP)"?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] i810fb: Fix console switch regression

[Henrique de Moraes Holschuh]

On Sun, 16 Mar 2008, Henrique de Moraes Holschuh wrote:
> Might I suggest using "((!par->dev_flags) & LOCKUP)"?

Never mind, I read your other post about that line not even making any
sense the way it was written.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh