[PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[Antonino A. Daplas]

The frambuffer console is still writing to the graphics hardware even if the
hardware is not owned anymore by the console.  This can lead to crashes with
some hardware.

A trace of the source indicates that it comes from almost anywhere, anytime.

The fix is to return immediately if vc_mode != KD_TEXT.  The test is placed in 
the following functions if not already present.

fbcon_cursor()
fbcon_putcs()
fbcon_clear()
fbcon_bmove() 
fbcon_blank() 

Tony

Signed-off-by: Antonino Daplas <adaplas@pol.net>

 fbcon.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff -Nru a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
--- a/drivers/video/console/fbcon.c	2004-10-30 19:10:42 +08:00
+++ b/drivers/video/console/fbcon.c	2004-11-02 18:58:15 +08:00
@@ -983,6 +983,9 @@
 	if (info->state != FBINFO_STATE_RUNNING)
 		return;
 
+	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
+		return;
+
 	if (!height || !width)
 		return;
 
@@ -1007,6 +1010,7 @@
 
 	if (!info->fbops->fb_blank && console_blanked)
 		return;
+
 	if (info->state != FBINFO_STATE_RUNNING)
 		return;
 
@@ -1042,6 +1046,9 @@
 	int y = real_y(p, vc->vc_y);
  	int c = scr_readw((u16 *) vc->vc_pos);
 
+	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
+		return;
+
 	ops->cursor_flash = 1;
 	if (mode & CM_SOFTBACK) {
 		mode &= ~CM_SOFTBACK;
@@ -1690,6 +1697,9 @@
 	if (!info->fbops->fb_blank && console_blanked)
 		return;
 
+	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
+		return;
+
 	if (!width || !height)
 		return;
 
@@ -1952,6 +1962,7 @@
 	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
 	struct fbcon_ops *ops = (struct fbcon_ops *) info->fbcon_par;
 	struct display *p = &fb_display[vc->vc_num];
+	int retval = 0;
 
 	if (mode_switch) {
 		struct fb_var_screeninfo var = info->var;
@@ -1968,19 +1979,16 @@
 		if (info->flags & FBINFO_MISC_MODESWITCHLATE)
 			info->flags |= FBINFO_MISC_MODESWITCH;
 
-		if (blank) {
-			fbcon_cursor(vc, CM_ERASE);
-			return 0;
-		}
-
-		if (!(info->flags & FBINFO_MISC_MODESWITCHLATE)) {
+		if (!blank && !(info->flags & FBINFO_MISC_MODESWITCHLATE)) {
 			var.activate = FB_ACTIVATE_NOW | FB_ACTIVATE_FORCE;
 			fb_set_var(info, &var);
 		}
+
+		return 0;
 	}
 
-	fbcon_cursor(vc, blank ? CM_ERASE : CM_DRAW);
 	ops->cursor_flash = (!blank);
+	fbcon_cursor(vc, blank ? CM_ERASE : CM_DRAW);
 
 	if (!info->fbops->fb_blank) {
 		if (blank) {
@@ -2001,9 +2009,10 @@
 			vc->vc_video_erase_char = oldc;
 		} else
 			update_screen(vc->vc_num);
-		return 0;
-	} else
-		return fb_blank(info, blank);
+	} else if (vt_cons[vc->vc_num]->vc_mode == KD_TEXT)
+		retval = fb_blank(info, blank);
+
+	return retval;
 }
 
 static void fbcon_free_font(struct display *p)




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: [PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[Benjamin Herrenschmidt]

On Tue, 2004-11-02 at 19:46 +0800, Antonino A. Daplas wrote:
> The frambuffer console is still writing to the graphics hardware even if the
> hardware is not owned anymore by the console.  This can lead to crashes with
> some hardware.
> 
> A trace of the source indicates that it comes from almost anywhere, anytime.
> 
> The fix is to return immediately if vc_mode != KD_TEXT.  The test is placed in 
> the following functions if not already present.
> 
> fbcon_cursor()
> fbcon_putcs()
> fbcon_clear()
> fbcon_bmove() 
> fbcon_blank() 

Since the exact same thing must be done if the fbdev are "asleep", can't
we just have a kind of inline helper fbcon_useable() that does both test
and make sure every place is properly dealt with ?

Ben.




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: [PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[Antonino A. Daplas]

On Wednesday 03 November 2004 06:27, Benjamin Herrenschmidt wrote:
> On Tue, 2004-11-02 at 19:46 +0800, Antonino A. Daplas wrote:
> > The frambuffer console is still writing to the graphics hardware even if
> > the hardware is not owned anymore by the console.  This can lead to
> > crashes with some hardware.
> >
> > A trace of the source indicates that it comes from almost anywhere,
> > anytime.
> >
> > The fix is to return immediately if vc_mode != KD_TEXT.  The test is
> > placed in the following functions if not already present.
> >
> > fbcon_cursor()
> > fbcon_putcs()
> > fbcon_clear()
> > fbcon_bmove()
> > fbcon_blank()
>
> Since the exact same thing must be done if the fbdev are "asleep", can't

There is already a test that checks for console_blanked and FBINFO_STATE_RUNNING
in all of the above.  Yes, I can combine all three in one inline helper so it's cleaner.

Tony




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

[PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[Antonino A. Daplas]

On Wednesday 03 November 2004 06:27, Benjamin Herrenschmidt wrote:
> On Tue, 2004-11-02 at 19:46 +0800, Antonino A. Daplas wrote:
>
> Since the exact same thing must be done if the fbdev are "asleep", can't
> we just have a kind of inline helper fbcon_useable() that does both test
> and make sure every place is properly dealt with ?
>

Hi Ben, Andrew,

Sigh, this patch uncovered a can of worms.  I tested different combinations,
those with/without xxxfb_blank implementation, framebuffers in directcolor
or truecolor, etc. I find that there is a problem unblanking if the hardware
has no xxxfb_blank() implementation, and also that the generic fb_blank() in
fbmem.c is problematic with drivers in directcolor or pseudocolor mode when
called by an fb application such as X.

Display blanking is implemented in three ways:

    1. using the drivers blanking implementation - info->fbops->fb_blank()
    2. clearing the screen with the console erase character - fbcon_blank()
    3. setting the color map to all black - fb_blank()

The third method is problematic for these reasons:

    - Setting the colormap to all black will not work in truecolor mode
    - In directcolor or pseudocolor, it will overwrite the fb application's
      color map, producing wrong colors.

So, remove the generic implementation in fb_blank() and just return -EINVAL
if there is no hardware implementation.  This will be only used by apps doing
an FBIO_BLANK ioctl, and is a more robust approach.

Other changes:

- Consolidated all tests and created an inlined helper function to check if
  the framebuffer console is inactive or not.
- fix unblanking if driver has no xxxfb_blank() hook.

I'm probably missing a few more things, but this patch should be generally
correct. Please apply after the entire patch series to avoid rejects.

Signed-off-by: Antonino Daplas <adaplas@pol.net>
---
 console/fbcon.c |   63 +++++++++++++++++++++++---------------------------------
 fbmem.c         |   25 ++--------------------
 2 files changed, 29 insertions(+), 59 deletions(-)

diff -Nru a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
--- a/drivers/video/console/fbcon.c	2004-11-01 13:45:03 +08:00
+++ b/drivers/video/console/fbcon.c	2004-11-03 10:19:37 +08:00
@@ -200,6 +200,13 @@
 }
 #endif
 
+static inline int fbcon_is_inactive(struct vc_data *vc, struct fb_info *info)
+{
+	return (info->state != FBINFO_STATE_RUNNING ||
+		vt_cons[vc->vc_num]->vc_mode != KD_TEXT ||
+		(console_blanked && info->fbops->fb_blank));
+}
+
 static inline int get_color(struct vc_data *vc, struct fb_info *info,
 	      u16 c, int is_fg)
 {
@@ -252,9 +259,8 @@
 	if (ops->currcon != -1)
 		vc = vc_cons[ops->currcon].d;
 
-	if (info->state != FBINFO_STATE_RUNNING ||
-	    !vc || !CON_IS_VISIBLE(vc) ||
-	    vt_cons[vc->vc_num]->vc_mode != KD_TEXT ||
+	if (!vc || !CON_IS_VISIBLE(vc) ||
+	    fbcon_is_inactive(vc, info) ||
  	    registered_fb[con2fb_map[vc->vc_num]] != info)
 		return;
 	acquire_console_sem();
@@ -988,12 +994,7 @@
 	struct display *p = &fb_display[vc->vc_num];
 	u_int y_break;
 
-	if (!info->fbops->fb_blank && console_blanked)
-		return;
-	if (info->state != FBINFO_STATE_RUNNING)
-		return;
-
-	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
+	if (fbcon_is_inactive(vc, info))
 		return;
 
 	if (!height || !width)
@@ -1018,18 +1019,10 @@
 	struct display *p = &fb_display[vc->vc_num];
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	if (!info->fbops->fb_blank && console_blanked)
-		return;
-
-	if (info->state != FBINFO_STATE_RUNNING)
-		return;
-
-	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
-		return;
-
-	ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
-		   get_color(vc, info, scr_readw(s), 1),
-		   get_color(vc, info, scr_readw(s), 0));
+	if (!fbcon_is_inactive(vc, info))
+		ops->putcs(vc, info, s, count, real_y(p, ypos), xpos,
+			   get_color(vc, info, scr_readw(s), 1),
+			   get_color(vc, info, scr_readw(s), 0));
 }
 
 static void fbcon_putc(struct vc_data *vc, int c, int ypos, int xpos)
@@ -1045,7 +1038,8 @@
 	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	ops->clear_margins(vc, info, bottom_only);
+	if (!fbcon_is_inactive(vc, info))
+		ops->clear_margins(vc, info, bottom_only);
 }
 
 static void fbcon_cursor(struct vc_data *vc, int mode)
@@ -1056,7 +1050,7 @@
 	int y = real_y(p, vc->vc_y);
  	int c = scr_readw((u16 *) vc->vc_pos);
 
-	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
+	if (fbcon_is_inactive(vc, info))
 		return;
 
 	ops->cursor_flash = 1;
@@ -1511,11 +1505,8 @@
 	struct fbcon_ops *ops = info->fbcon_par;
 	int scroll_partial = info->flags & FBINFO_PARTIAL_PAN_OK;
 
-	if (!info->fbops->fb_blank && console_blanked)
-		return 0;
-
-	if (!count || vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
-		return 0;
+	if (fbcon_is_inactive(vc, info))
+		return -EINVAL;
 
 	fbcon_cursor(vc, CM_ERASE);
 
@@ -1704,12 +1695,9 @@
 	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
 	struct display *p = &fb_display[vc->vc_num];
 	
-	if (!info->fbops->fb_blank && console_blanked)
+	if (fbcon_is_inactive(vc, info))
 		return;
-
-	if (vt_cons[vc->vc_num]->vc_mode != KD_TEXT)
-		return;
-
+	    
 	if (!width || !height)
 		return;
 
@@ -1999,8 +1987,8 @@
 		return 0;
 	}
 
-	ops->cursor_flash = (!blank);
 	fbcon_cursor(vc, blank ? CM_ERASE : CM_DRAW);
+	ops->cursor_flash = (!blank);
 
 	if (!info->fbops->fb_blank) {
 		if (blank) {
@@ -2019,10 +2007,10 @@
 			} else
 				fbcon_clear(vc, 0, 0, height, vc->vc_cols);
 			vc->vc_video_erase_char = oldc;
-		} else
+ 		} else if (!fbcon_is_inactive(vc, info))
 			update_screen(vc->vc_num);
 	} else if (vt_cons[vc->vc_num]->vc_mode == KD_TEXT)
-		retval = fb_blank(info, blank);
+ 		retval = info->fbops->fb_blank(blank, info);
 
 	return retval;
 }
@@ -2328,8 +2316,9 @@
 	int i, j, k, depth;
 	u8 val;
 
-	if (!info->fbops->fb_blank && console_blanked)
+	if (fbcon_is_inactive(vc, info))
 		return -EINVAL;
+
 	depth = fb_get_color_depth(info);
 	if (depth > 3) {
 		for (i = j = 0; i < 16; i++) {
diff -Nru a/drivers/video/fbmem.c b/drivers/video/fbmem.c
--- a/drivers/video/fbmem.c	2004-10-27 14:58:07 +08:00
+++ b/drivers/video/fbmem.c	2004-11-03 10:04:59 +08:00
@@ -743,29 +743,10 @@
 int
 fb_blank(struct fb_info *info, int blank)
 {	
-	/* ??? Variable sized stack allocation.  */
-	struct fb_cmap cmap;
-	u16 *black = NULL;
-	int err = 0;
+	int err = -EINVAL;
 	
-	if (info->fbops->fb_blank && !info->fbops->fb_blank(blank, info))
-		return 0;
-
-	cmap = info->cmap;
-
-	if (blank) { 
-		black = kmalloc(sizeof(u16) * info->cmap.len, GFP_KERNEL);
-		if (black) {
-			memset(black, 0, info->cmap.len * sizeof(u16));
-			cmap.red = cmap.green = cmap.blue = black;
-			cmap.transp = info->cmap.transp ? black : NULL;
-			cmap.start = info->cmap.start;
-			cmap.len = info->cmap.len;
-		}
-	}
-
-	err = fb_set_cmap(&cmap, info);
-	kfree(black);
+	if (info->fbops->fb_blank)
+		err = info->fbops->fb_blank(blank, info);
 
 	return err;
 }





-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: [PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[Andrew Morton]

"Antonino A. Daplas" <adaplas@hotpop.com> wrote:
>
> Sigh, this patch uncovered a can of worms.  I tested different combinations,
> those with/without xxxfb_blank implementation, framebuffers in directcolor
> or truecolor, etc. I find that there is a problem unblanking if the hardware
> has no xxxfb_blank() implementation, and also that the generic fb_blank() in
> fbmem.c is problematic with drivers in directcolor or pseudocolor mode when
> called by an fb application such as X.
> 
> Display blanking is implemented in three ways:
> 
>     1. using the drivers blanking implementation - info->fbops->fb_blank()
>     2. clearing the screen with the console erase character - fbcon_blank()
>     3. setting the color map to all black - fb_blank()
> 
> The third method is problematic for these reasons:
> 
>     - Setting the colormap to all black will not work in truecolor mode
>     - In directcolor or pseudocolor, it will overwrite the fb application's
>       color map, producing wrong colors.
> 
> So, remove the generic implementation in fb_blank() and just return -EINVAL
> if there is no hardware implementation.  This will be only used by apps doing
> an FBIO_BLANK ioctl, and is a more robust approach.

This patch conflicts with Ben's "fbdev: workaround for broken X servers"
patch, which is in Linus's tree now.

So fb_blank() will now look like this:

int
fb_blank(struct fb_info *info, int blank)
{	
	int err = -EINVAL;
	
	/* Workaround for broken X servers */
	if (blank > VESA_POWERDOWN)
		blank = VESA_POWERDOWN;

	if (info->fbops->fb_blank)
 		err = info->fbops->fb_blank(blank, info);

	return err;
}



-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: [PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[Benjamin Herrenschmidt]

On Wed, 2004-11-03 at 15:52 -0800, Andrew Morton wrote:

> This patch conflicts with Ben's "fbdev: workaround for broken X servers"
> patch, which is in Linus's tree now.

Sorry, Antonio, forgot to CC you ...

Ben.




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: [PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[Antonino A. Daplas]

On Thursday 04 November 2004 08:16, Benjamin Herrenschmidt wrote:
> On Wed, 2004-11-03 at 15:52 -0800, Andrew Morton wrote:
> > This patch conflicts with Ben's "fbdev: workaround for broken X servers"
> > patch, which is in Linus's tree now.
>
> Sorry, Antonio, forgot to CC you ...
>

No harm done. My fault too, I already saw your patch but I was too slow to
respond.

Tony




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: [PATCH 1/8] fbcon: Do not touch hardware if vc_mode != KD_TEXT

[Antonino A. Daplas]

On Thursday 04 November 2004 07:52, Andrew Morton wrote:
> "Antonino A. Daplas" <adaplas@hotpop.com> wrote:
> This patch conflicts with Ben's "fbdev: workaround for broken X servers"
> patch, which is in Linus's tree now.
>
> So fb_blank() will now look like this:
>
> int
> fb_blank(struct fb_info *info, int blank)
> {
> 	int err = -EINVAL;
>
> 	/* Workaround for broken X servers */
> 	if (blank > VESA_POWERDOWN)
> 		blank = VESA_POWERDOWN;
>
> 	if (info->fbops->fb_blank)
>  		err = info->fbops->fb_blank(blank, info);
>
> 	return err;
> }

Yes, that's correct.  Thanks.

Tony




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click