[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()

[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()

[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/