[PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Antonino A. Daplas]

The field info->modelist is initialized during register_framebuffer.  This
field is also referred to in fb_set_var().  Thus a call to fb_set_var()
before register_framebuffer() will cause a crash.  A few drivers do this,
notably controlfb.  (This might fix reports of controlfb crashing in
powermacs).

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

 fbmem.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)

diff -Nru a/drivers/video/fbmem.c b/drivers/video/fbmem.c
--- a/drivers/video/fbmem.c	2004-11-23 21:20:13 +08:00
+++ b/drivers/video/fbmem.c	2004-11-25 01:09:22 +08:00
@@ -725,7 +725,10 @@
 			fb_set_cmap(&info->cmap, info);
 
 			fb_var_to_videomode(&mode, &info->var);
-			fb_add_videomode(&mode, &info->modelist);
+
+			if (info->modelist.prev && info->modelist.next &&
+			    !list_empty(&info->modelist))
+				fb_add_videomode(&mode, &info->modelist);
 
 			if (info->flags & FBINFO_MISC_MODECHANGEUSER) {
 				struct fb_event event;

Re: [PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Mario Gaucher]

> The field info->modelist is initialized during register_framebuffer.  This
> field is also referred to in fb_set_var().  Thus a call to fb_set_var()
> before register_framebuffer() will cause a crash.  A few drivers do this,
> notably controlfb.  (This might fix reports of controlfb crashing in
> powermacs).


this patch works well... I can now boot my PowerMac 7300 using
2.6.10-rc2-bk8 (that I got on kernel.org) with this patch...

but I still has some problem with my Matrox Millenium PCI card using
matroxfb driver... the kernel boot... but I get corrupted characters on
the console... X load ok and display ok...
It works really well with the onboard video (controlfb driver)

matroxfb driver works fine on 2.6.8

Re: [PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Antonino A. Daplas]

On Friday 26 November 2004 08:20, Mario Gaucher wrote:
> > The field info->modelist is initialized during register_framebuffer. 
> > This field is also referred to in fb_set_var().  Thus a call to
> > fb_set_var() before register_framebuffer() will cause a crash.  A few
> > drivers do this, notably controlfb.  (This might fix reports of controlfb
> > crashing in powermacs).
>
> this patch works well... I can now boot my PowerMac 7300 using
> 2.6.10-rc2-bk8 (that I got on kernel.org) with this patch...

That's good.

>
> but I still has some problem with my Matrox Millenium PCI card using
> matroxfb driver... the kernel boot... but I get corrupted characters on
> the console... X load ok and display ok...

Try this first.

1. Open drivers/video/matrox/matrofb_accel.c
2. At the end of the file is this function:

static void matroxfb_imageblit(struct fb_info* info, const struct fb_image* image) {
	MINFO_FROM_INFO(info);

	DBG_HEAVY(__FUNCTION__);

	if (image->depth == 1) {
		u_int32_t fgx, bgx;

		fgx = ((u_int32_t*)info->pseudo_palette)[image->fg_color];
		bgx = ((u_int32_t*)info->pseudo_palette)[image->bg_color];
		matroxfb_1bpp_imageblit(PMINFO fgx, bgx, image->data, image->width, image->height, image->dy, image->dx);
	} else {
		/* Danger! image->depth is useless: logo painting code always
		   passes framebuffer color depth here, although logo data are
		   always 8bpp and info->pseudo_palette is changed to contain
		   logo palette to be used (but only for true/direct-color... sic...).
		   So do it completely in software... */
		cfb_imageblit(info, image);
	}
}

3. Replace the above function to use software drawing so it becomes like this:

static void matroxfb_imageblit(struct fb_info* info, const struct fb_image* image) {
	cfb_imageblit(info, image);
}

Tony

Re: [PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Benjamin Herrenschmidt]

On Thu, 2004-11-25 at 01:15 +0800, Antonino A. Daplas wrote:
> The field info->modelist is initialized during register_framebuffer.  This
> field is also referred to in fb_set_var().  Thus a call to fb_set_var()
> before register_framebuffer() will cause a crash.  A few drivers do this,
> notably controlfb.  (This might fix reports of controlfb crashing in
> powermacs).
>
> .../...

Hi Antonio, I "missed" this modelist thingy. Can you explain what is the
logic ? There is now a modelist attached to fb_info used for mode
matching ? In this case, it makes sense to be able to add things to it
before register_framebuffer(). I mean, I would expect drivers to probe
at least their default head modes before registering the fb, since the
later will cause fbcon to try to setup a mode ...

Ben.




-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

Re: [PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Mario Gaucher]

From: "Antonino A. Daplas" <adaplas@hotpop.com>
> > but I still has some problem with my Matrox Millenium PCI card using
> > matroxfb driver... the kernel boot... but I get corrupted characters on
> > the console... X load ok and display ok...
> 
> Try this first.
> 
> 1. Open drivers/video/matrox/matrofb_accel.c
> 2. At the end of the file is this function:
> 
> static void matroxfb_imageblit(struct fb_info* info, const struct
fb_image* image) {
> 	MINFO_FROM_INFO(info);
> 
> 	DBG_HEAVY(__FUNCTION__);
> 
> 	if (image->depth == 1) {
> 		u_int32_t fgx, bgx;
> 
> 		fgx = ((u_int32_t*)info->pseudo_palette)[image->fg_color];
> 		bgx = ((u_int32_t*)info->pseudo_palette)[image->bg_color];
> 		matroxfb_1bpp_imageblit(PMINFO fgx, bgx, image->data, image->width,
image->height, image->dy, image->dx);
> 	} else {
> 		/* Danger! image->depth is useless: logo painting code always
> 		   passes framebuffer color depth here, although logo data are
> 		   always 8bpp and info->pseudo_palette is changed to contain
> 		   logo palette to be used (but only for true/direct-color... sic...).
> 		   So do it completely in software... */
> 		cfb_imageblit(info, image);
> 	}
> }
> 
> 3. Replace the above function to use software drawing so it becomes like
this:
> 
> static void matroxfb_imageblit(struct fb_info* info, const struct
fb_image* image) {
> 	cfb_imageblit(info, image);
> }


this patch works fine on my PowerMac 7300 with my Matrox Millenium PCI
card... 

so I think that both patch you sent me should be included in the kernel... 

thank you

Re: [PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Antonino A. Daplas]

On Saturday 27 November 2004 07:17, Mario Gaucher wrote:
> From: "Antonino A. Daplas" <adaplas@hotpop.com>
>
> > > but I still has some problem with my Matrox Millenium PCI card using
> > > matroxfb driver... the kernel boot... but I get corrupted characters on
> > > the console... X load ok and display ok...
> >
> > Try this first.
> >
> > 1. Open drivers/video/matrox/matrofb_accel.c
> > 2. At the end of the file is this function:
> >
> > static void matroxfb_imageblit(struct fb_info* info, const struct
>
> fb_image* image) {
>
> > 	MINFO_FROM_INFO(info);
> >
> > 	DBG_HEAVY(__FUNCTION__);
> >
> > 	if (image->depth == 1) {
> > 		u_int32_t fgx, bgx;
> >
> > 		fgx = ((u_int32_t*)info->pseudo_palette)[image->fg_color];
> > 		bgx = ((u_int32_t*)info->pseudo_palette)[image->bg_color];
> > 		matroxfb_1bpp_imageblit(PMINFO fgx, bgx, image->data, image->width,
>
> image->height, image->dy, image->dx);
>
> > 	} else {
> > 		/* Danger! image->depth is useless: logo painting code always
> > 		   passes framebuffer color depth here, although logo data are
> > 		   always 8bpp and info->pseudo_palette is changed to contain
> > 		   logo palette to be used (but only for true/direct-color... sic...).
> > 		   So do it completely in software... */
> > 		cfb_imageblit(info, image);
> > 	}
> > }
> >
> > 3. Replace the above function to use software drawing so it becomes like
>
> this:
> > static void matroxfb_imageblit(struct fb_info* info, const struct
>
> fb_image* image) {
>
> > 	cfb_imageblit(info, image);
> > }
>
> this patch works fine on my PowerMac 7300 with my Matrox Millenium PCI
> card...
>
> so I think that both patch you sent me should be included in the kernel...

No, this modification is for testing purposes only.  It means that hardware
color expansion in matroxfb for powerpc's got broken.  CC'ed Petr.

Tony

Re: [PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Antonino A. Daplas]

On Saturday 27 November 2004 06:38, Benjamin Herrenschmidt wrote:
> On Thu, 2004-11-25 at 01:15 +0800, Antonino A. Daplas wrote:
> > The field info->modelist is initialized during register_framebuffer. 
> > This field is also referred to in fb_set_var().  Thus a call to
> > fb_set_var() before register_framebuffer() will cause a crash.  A few
> > drivers do this, notably controlfb.  (This might fix reports of controlfb
> > crashing in powermacs).
> >
> > .../...
>
> Hi Antonio, I "missed" this modelist thingy. Can you explain what is the
> logic ? There is now a modelist attached to fb_info used for mode
> matching ? In this case, it makes sense to be able to add things to it
> before register_framebuffer(). I mean, I would expect drivers to probe
> at least their default head modes before registering the fb, since the
> later will cause fbcon to try to setup a mode ...

Yep, this came about because there were two problems in terms of mode
setting before I added the modelist:

1. Lack of per-display var which prevents full restore of the console on 
mode switch
2. the stty utility does not work correctly for drivers that do not have
mode validation

The modelist functions as a private mode database, meaning the entries are
always correct for the driver/graphics/display combination in question.

The modelist can be filled up in a number of ways:

1.  By default, upon register_framebuffer(), if info->modelist is empty or
not initialized, it will be filled up with mode timings derived from
info->var.  In effect, most drivers will have at least one correct entry in
info->modelist.

2. Drivers can choose to fill up info->modelist, perhaps from timings
derived from EDID.  There is one function in fbmem.c,
fb_videomode_to_modelist() which converts a modedb array to a 
modelist.

3.  For each correct fbset, (accepted by both driver and user), if
the timings are unique, this will be also be added to the modelist. Also,
in the unfortunate case that an illegal mode timing is accidentally entered
into the modelist, there is a mechanism to remove that entry.

Therefore, when doing an stty, fbcon will only look at info->modelist,
finds the best matching mode, builds a var from that particular entry
and passes the var to the driver.  There is no guesswork involved, and
drivers will always operate on known working mode timings.

Also, the graphics states per console are also preserved.  However, instead
of saving a var per console, we only save a part of the var, the graphics
state. The mode timings are saved as pointers to entries in info->modelist.
In effect, we have the full functionality of a per-display var, but we save
half the memory, that's around 5K.

You'll probably notice that with recent kernels, if you switch consoles,
your previous console settings are restored.  Also, doing an stty should, in
theory, never produce a corrupt console, even if you enter insane numbers.
Of course, the effectivity of stty depends on the number of entries in
info->modelist.

Anyway, the goal here is to fix as many regressions as possible.  And I
think the 2.6 framebuffer is now behaving in almost the same manner as 2.4,
with a few minor exceptions. 

BTW, a few drivers do fill up info->modelist before register_framebuffer(),
rivafb and savagefb.

Tony




-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

Re: [PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Benjamin Herrenschmidt]

On Sat, 2004-11-27 at 10:56 +0800, Antonino A. Daplas wrote:

> 1. Lack of per-display var which prevents full restore of the console on 
> mode switch
> 2. the stty utility does not work correctly for drivers that do not have
> mode validation

Ok, good.

 .../...

> 3.  For each correct fbset, (accepted by both driver and user), if
> the timings are unique, this will be also be added to the modelist. Also,
> in the unfortunate case that an illegal mode timing is accidentally entered
> into the modelist, there is a mechanism to remove that entry.

Ok, though I'm not too fan of this one, I'd rather have an explicit flag
in the var passed by fbset trigger the "remember" thing... oh well,
we'll see how things goes.

> Therefore, when doing an stty, fbcon will only look at info->modelist,
> finds the best matching mode, builds a var from that particular entry
> and passes the var to the driver.  There is no guesswork involved, and
> drivers will always operate on known working mode timings.

Yah, I know that problem, this looks like a good solution.

> Also, the graphics states per console are also preserved.  However, instead
> of saving a var per console, we only save a part of the var, the graphics
> state. The mode timings are saved as pointers to entries in info->modelist.
> In effect, we have the full functionality of a per-display var, but we save
> half the memory, that's around 5K.

I dislike the pointers tho. Remember that one day, we'll do monitor
hotswap, which means that at one point, the driver will
remove/add/rebuild modelist... 

> You'll probably notice that with recent kernels, if you switch consoles,
> your previous console settings are restored.  Also, doing an stty should, in
> theory, never produce a corrupt console, even if you enter insane numbers.
> Of course, the effectivity of stty depends on the number of entries in
> info->modelist.

Yah, sounds good.

> Anyway, the goal here is to fix as many regressions as possible.  And I
> think the 2.6 framebuffer is now behaving in almost the same manner as 2.4,
> with a few minor exceptions. 
> 
> BTW, a few drivers do fill up info->modelist before register_framebuffer(),
> rivafb and savagefb.

I'll add that to radeonfb too, I'm working on a big update of this
driver adding power management support for newer apple laptops plus a
few other things, I'll include that as well.

Ben.




-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

Re: [PATCH] fbdev: Fix crash if fb_set_var() called before register_framebuffer()

[Antonino A. Daplas]

On Saturday 27 November 2004 12:15, Benjamin Herrenschmidt wrote:
> On Sat, 2004-11-27 at 10:56 +0800, Antonino A. Daplas wrote:
> > 1. Lack of per-display var which prevents full restore of the console on
> > mode switch
> > 2. the stty utility does not work correctly for drivers that do not have
> > mode validation
>
> Ok, good.
>
>  .../...
>
> > 3.  For each correct fbset, (accepted by both driver and user), if
> > the timings are unique, this will be also be added to the modelist. Also,
> > in the unfortunate case that an illegal mode timing is accidentally
> > entered into the modelist, there is a mechanism to remove that entry.
>
> Ok, though I'm not too fan of this one, I'd rather have an explicit flag
> in the var passed by fbset trigger the "remember" thing... oh well,
> we'll see how things goes.

You mean the mode entry deletion? That's okay, the code is minor, still
unused, and was submitted as optional, but still got merged.  It can be
easily removed.

>
> > Therefore, when doing an stty, fbcon will only look at info->modelist,
> > finds the best matching mode, builds a var from that particular entry
> > and passes the var to the driver.  There is no guesswork involved, and
> > drivers will always operate on known working mode timings.
>
> Yah, I know that problem, this looks like a good solution.
>
> > Also, the graphics states per console are also preserved.  However,
> > instead of saving a var per console, we only save a part of the var, the
> > graphics state. The mode timings are saved as pointers to entries in
> > info->modelist. In effect, we have the full functionality of a
> > per-display var, but we save half the memory, that's around 5K.
>
> I dislike the pointers tho. Remember that one day, we'll do monitor
> hotswap, which means that at one point, the driver will
> remove/add/rebuild modelist...

Removing/adding/rebuilding the modelist should be doable with pointers. But
in the case people don't want pointers, this can be easily changed.  The
conversion code is entirely in display_to_var() and var_to_display() helpers.
  
Tony




-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/