[PATCH -mm] fb: modedb uses wrong default_mode

[PATCH -mm] fb: modedb uses wrong default_mode

[Tero Roponen]

It seems that default_mode is always overwritten in
fb_find_mode() if caller gives its own modedb; this
patch should fix it.

dmesg diff before and after the following patch:

 neofb: mapped framebuffer at c4a80000
 -Mode (640x400) won't display properly on LCD
 -Mode (640x400) won't display properly on LCD
 -neofb v0.4.2: 2048kB VRAM, using 640x480, 31.469kHz, 59Hz
 -Console: switching to colour frame buffer device 80x30
 +neofb v0.4.2: 2048kB VRAM, using 800x600, 37.878kHz, 60Hz
 +Console: switching to colour frame buffer device 100x37
  fb0: MagicGraph 128XD frame buffer device

Signed-off-by: Tero Roponen <teanropo@jyu.fi>
---

--- linux-2.6.19-rc5-mm2/drivers/video/modedb.c.orig	2006-11-15 19:03:03.000000000 +0200
+++ linux-2.6.19-rc5-mm2/drivers/video/modedb.c	2006-11-15 19:02:57.000000000 +0200
@@ -507,7 +507,7 @@ int fb_find_mode(struct fb_var_screeninf
     }
     if (!default_mode && db != modedb)
 	default_mode = &db[0];
-    else
+    else if (!default_mode)
 	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
 
     if (!default_bpp)

Re: [PATCH -mm] fb: modedb uses wrong default_mode

[Andrew Morton]

On Wed, 15 Nov 2006 19:43:16 +0200 (EET)
Tero Roponen <teanropo@jyu.fi> wrote:

> 
> It seems that default_mode is always overwritten in
> fb_find_mode() if caller gives its own modedb; this
> patch should fix it.
> 
> dmesg diff before and after the following patch:
> 
>  neofb: mapped framebuffer at c4a80000
>  -Mode (640x400) won't display properly on LCD
>  -Mode (640x400) won't display properly on LCD
>  -neofb v0.4.2: 2048kB VRAM, using 640x480, 31.469kHz, 59Hz
>  -Console: switching to colour frame buffer device 80x30
>  +neofb v0.4.2: 2048kB VRAM, using 800x600, 37.878kHz, 60Hz
>  +Console: switching to colour frame buffer device 100x37
>   fb0: MagicGraph 128XD frame buffer device
> 
> Signed-off-by: Tero Roponen <teanropo@jyu.fi>
> ---
> 
> --- linux-2.6.19-rc5-mm2/drivers/video/modedb.c.orig	2006-11-15 19:03:03.000000000 +0200
> +++ linux-2.6.19-rc5-mm2/drivers/video/modedb.c	2006-11-15 19:02:57.000000000 +0200
> @@ -507,7 +507,7 @@ int fb_find_mode(struct fb_var_screeninf
>      }
>      if (!default_mode && db != modedb)
>  	default_mode = &db[0];
> -    else
> +    else if (!default_mode)
>  	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
>  
>      if (!default_bpp)

Sigh.

2.6.19-rc5 has:

    if (!default_mode)
	default_mode = &modedb[DEFAULT_MODEDB_INDEX];

and Jordan changed it to

    if (!default_mode && db != modedb)
	default_mode = &db[0];
    else
	default_mode = &modedb[DEFAULT_MODEDB_INDEX];

and you want to change it to

    if (!default_mode && db != modedb)
	default_mode = &db[0];
    else if (!default_mode)
	default_mode = &modedb[DEFAULT_MODEDB_INDEX];

which is actually a complicated way of doing

    if (!default_mode)
	default_mode = &db[DEFAULT_MODEDB_INDEX];

So let's do that.

Re: fb: modedb uses wrong default_mode

[Jordan Crouse]

On 15/11/06 15:29 -0800, Andrew Morton wrote:
> On Wed, 15 Nov 2006 19:43:16 +0200 (EET)
> Tero Roponen <teanropo@jyu.fi> wrote:
> 
> > 
> > It seems that default_mode is always overwritten in
> > fb_find_mode() if caller gives its own modedb; this
> > patch should fix it.

> Sigh.
> 
> 2.6.19-rc5 has:
> 
>     if (!default_mode)
> 	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
> 
> and Jordan changed it to
> 
>     if (!default_mode && db != modedb)
> 	default_mode = &db[0];
>     else
> 	default_mode = &modedb[DEFAULT_MODEDB_INDEX];


> and you want to change it to
> 
>     if (!default_mode && db != modedb)
> 	default_mode = &db[0];
>     else if (!default_mode)
> 	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
> 
> which is actually a complicated way of doing
> 
>     if (!default_mode)
> 	default_mode = &db[DEFAULT_MODEDB_INDEX];

Unless DEFAULT_MODEDB_INDEX for some reason gets set to non-zero, then
it could be dangerous. If we agree that the default entry should aways be 
at 0, then nuke the define and hard code the zero.  That way, nobody will be 
tempted to change it.

Jordan

-- 
Jordan Crouse
Senior Linux Engineer
Advanced Micro Devices, Inc.
<www.amd.com/embeddedprocessors>

Re: [Linux-fbdev-devel] fb: modedb uses wrong default_mode

[James Simmons]

> > > It seems that default_mode is always overwritten in
> > > fb_find_mode() if caller gives its own modedb; this
> > > patch should fix it.
> 
> > Sigh.
> > 
> > 2.6.19-rc5 has:
> > 
> >     if (!default_mode)
> > 	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
> > 
> > and Jordan changed it to
> > 
> >     if (!default_mode && db != modedb)
> > 	default_mode = &db[0];
> >     else
> > 	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
> 
> 
> > and you want to change it to
> > 
> >     if (!default_mode && db != modedb)
> > 	default_mode = &db[0];
> >     else if (!default_mode)
> > 	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
> > 
> > which is actually a complicated way of doing
> > 
> >     if (!default_mode)
> > 	default_mode = &db[DEFAULT_MODEDB_INDEX];
> 
> Unless DEFAULT_MODEDB_INDEX for some reason gets set to non-zero, then
> it could be dangerous. If we agree that the default entry should aways be 
> at 0, then nuke the define and hard code the zero.  That way, nobody will be 
> tempted to change it.

Taking a look at modedb.c and neofb.c I noticed the bug is in the neofb.c 
driver. The problem is the confusion with the fb_find_mode function 
itself.

int fb_find_mode(struct fb_var_screeninfo *var,
                 struct fb_info *info, const char *mode_option,
                 const struct fb_videomode *db, unsigned int dbsize,
                 const struct fb_videomode *default_mode,
                 unsigned int default_bpp)

db is the database but default_mode is the mode we want to run. As you 
can see neofb.c does

	if (!fb_find_mode(&info->var, info, mode_option, NULL, 0,
                        info->monspecs.modedb, 16)) {
                printk(KERN_ERR "neofb: Unable to find usable video mode.\n");
                goto err_map_video;
        }


it should be
	if (!fb_find_mode(&info->var, info, mode_option, info->monspecs.modedb,
			0, NULL, 16)) {
		
Who knows how many drivers get this wrong. BTW Jordan is right. 
DEFAULT_MODEDB_INDEX is unless. Also we don't need dbsize anymore. 
Jordan did point out a error in fb_find_mode. It should be

	if (!db)
	    db = modedb;
	dbsize = ARRAY_SIZE(modedb);

	if (!default_mode)
	    default_mode = &db[DEFAULT_MODEDB_INDEX];
	if (!default_bpp)
	    default_bpp = 8;

db will always be set.


 

Re: [Linux-fbdev-devel] fb: modedb uses wrong default_mode

[Andrew Morton]

On Fri, 17 Nov 2006 20:08:47 +0000 (GMT)
James Simmons <jsimmons@infradead.org> wrote:

> Who knows how many drivers get this wrong. BTW Jordan is right. 
> DEFAULT_MODEDB_INDEX is unless. Also we don't need dbsize anymore. 
> Jordan did point out a error in fb_find_mode. It should be
> 
> 	if (!db)
> 	    db = modedb;
> 	dbsize = ARRAY_SIZE(modedb);
> 
> 	if (!default_mode)
> 	    default_mode = &db[DEFAULT_MODEDB_INDEX];
> 	if (!default_bpp)
> 	    default_bpp = 8;
> 
> db will always be set.

I think we do need dbsize, and that the code which I have now:

int fb_find_mode(struct fb_var_screeninfo *var,
		 struct fb_info *info, const char *mode_option,
		 const struct fb_videomode *db, unsigned int dbsize,
		 const struct fb_videomode *default_mode,
		 unsigned int default_bpp)
{
    int i;

    /* Set up defaults */
    if (!db) {
	db = modedb;
	dbsize = ARRAY_SIZE(modedb);
    }

    if (!default_mode)
	default_mode = &db[0];

    if (!default_bpp)
	default_bpp = 8;


is appropriate?


Here's the current version of this monster patch:

From: Jordan Crouse <jordan.crouse@amd.com>

If no default mode is specified, it should be grabbed from the supplied
database, not the default one.

[teanropo@jyu.fi: fix it]
[akpm@osdl.org: simplify it]
[akpm@osdl.org: remove pointless DEFAULT_MODEDB_INDEX]
Signed-off-by: Jordan Crouse <jordan.crouse@amd.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: "Antonino A. Daplas" <adaplas@pol.net>
Signed-off-by: Tero Roponen <teanropo@jyu.fi>
Cc: James Simmons <jsimmons@infradead.org>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 drivers/video/modedb.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff -puN drivers/video/modedb.c~video-get-the-default-mode-from-the-right-database drivers/video/modedb.c
--- a/drivers/video/modedb.c~video-get-the-default-mode-from-the-right-database
+++ a/drivers/video/modedb.c
@@ -34,8 +34,6 @@ const char *global_mode_option;
      *  Standard video mode definitions (taken from XFree86)
      */
 
-#define DEFAULT_MODEDB_INDEX	0
-
 static const struct fb_videomode modedb[] = {
     {
 	/* 640x400 @ 70 Hz, 31.5 kHz hsync */
@@ -505,8 +503,10 @@ int fb_find_mode(struct fb_var_screeninf
 	db = modedb;
 	dbsize = ARRAY_SIZE(modedb);
     }
+
     if (!default_mode)
-	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
+	default_mode = &db[0];
+
     if (!default_bpp)
 	default_bpp = 8;
 
_

Re: [Linux-fbdev-devel] fb: modedb uses wrong default_mode

[Tero Roponen]

On Fri, 17 Nov 2006, Andrew Morton wrote:
> 
> Here's the current version of this monster patch:
> 
> From: Jordan Crouse <jordan.crouse@amd.com>
> 
> If no default mode is specified, it should be grabbed from the supplied
> database, not the default one.
> 
> [teanropo@jyu.fi: fix it]
> [akpm@osdl.org: simplify it]
> [akpm@osdl.org: remove pointless DEFAULT_MODEDB_INDEX]
> Signed-off-by: Jordan Crouse <jordan.crouse@amd.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: "Antonino A. Daplas" <adaplas@pol.net>
> Signed-off-by: Tero Roponen <teanropo@jyu.fi>
> Cc: James Simmons <jsimmons@infradead.org>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> ---
> 
>  drivers/video/modedb.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff -puN drivers/video/modedb.c~video-get-the-default-mode-from-the-right-database drivers/video/modedb.c
> --- a/drivers/video/modedb.c~video-get-the-default-mode-from-the-right-database
> +++ a/drivers/video/modedb.c
> @@ -34,8 +34,6 @@ const char *global_mode_option;
>       *  Standard video mode definitions (taken from XFree86)
>       */
>  
> -#define DEFAULT_MODEDB_INDEX	0
> -
>  static const struct fb_videomode modedb[] = {
>      {
>  	/* 640x400 @ 70 Hz, 31.5 kHz hsync */
> @@ -505,8 +503,10 @@ int fb_find_mode(struct fb_var_screeninf
>  	db = modedb;
>  	dbsize = ARRAY_SIZE(modedb);
>      }
> +
>      if (!default_mode)
> -	default_mode = &modedb[DEFAULT_MODEDB_INDEX];
> +	default_mode = &db[0];
> +
>      if (!default_bpp)
>  	default_bpp = 8;
>  
> _

I'm using neofb and this Works For Me (TM).

Thanks,
Tero Roponen

Re: [Linux-fbdev-devel] fb: modedb uses wrong default_mode

[James Simmons]

> James Simmons <jsimmons@infradead.org> wrote:
> 
> > Who knows how many drivers get this wrong. BTW Jordan is right. 
> > DEFAULT_MODEDB_INDEX is unless. Also we don't need dbsize anymore. 
> > Jordan did point out a error in fb_find_mode. It should be
> > 
> > 	if (!db)
> > 	    db = modedb;
> > 	dbsize = ARRAY_SIZE(modedb);
> > 
> > 	if (!default_mode)
> > 	    default_mode = &db[DEFAULT_MODEDB_INDEX];
> > 	if (!default_bpp)
> > 	    default_bpp = 8;
> > 
> > db will always be set.
> 
> I think we do need dbsize, and that the code which I have now:

I really don't trust dbsize. The driver writer can pass in the wrong 
number. Whereas ARRAY_SIZE will always be correct. Lets take the position 
that we use dbsize then we need to test if dbsize is greater than the 
really size of the modedb. The dbsize parameter was for the days before we
had ARRAY_SIZE.

> int fb_find_mode(struct fb_var_screeninfo *var,
> 		 struct fb_info *info, const char *mode_option,
> 		 const struct fb_videomode *db, unsigned int dbsize,
> 		 const struct fb_videomode *default_mode,
> 		 unsigned int default_bpp)
> {
>     int i;
> 
>     /* Set up defaults */
>     if (!db) {
> 	db = modedb;
> 	dbsize = ARRAY_SIZE(modedb);
>     }

      if (dbsize > ARRAY_SIZE(db))
	dbsize = ARRAY_SIZE(db);

>     if (!default_mode)
> 	default_mode = &db[0];
> 
>     if (!default_bpp)
> 	default_bpp = 8;

Re: [Linux-fbdev-devel] fb: modedb uses wrong default_mode

[Andrew Morton]

On Mon, 20 Nov 2006 16:48:05 +0000 (GMT)
James Simmons <jsimmons@infradead.org> wrote:

> > > 	    db = modedb;
> > > 	dbsize = ARRAY_SIZE(modedb);
> > > 
> > > 	if (!default_mode)
> > > 	    default_mode = &db[DEFAULT_MODEDB_INDEX];
> > > 	if (!default_bpp)
> > > 	    default_bpp = 8;
> > > 
> > > db will always be set.
> > 
> > I think we do need dbsize, and that the code which I have now:
> 
> I really don't trust dbsize. The driver writer can pass in the wrong 
> number.

That would be a bug.

> Whereas ARRAY_SIZE will always be correct. Lets take the position 
> that we use dbsize then we need to test if dbsize is greater than the 
> really size of the modedb. The dbsize parameter was for the days before we
> had ARRAY_SIZE.
> 
> > int fb_find_mode(struct fb_var_screeninfo *var,
> > 		 struct fb_info *info, const char *mode_option,
> > 		 const struct fb_videomode *db, unsigned int dbsize,
> > 		 const struct fb_videomode *default_mode,
> > 		 unsigned int default_bpp)
> > {
> >     int i;
> > 
> >     /* Set up defaults */
> >     if (!db) {
> > 	db = modedb;
> > 	dbsize = ARRAY_SIZE(modedb);
> >     }
> 
>       if (dbsize > ARRAY_SIZE(db))
> 	dbsize = ARRAY_SIZE(db);

We can't do ARRAY_SIZE on a random pointer like this: the compiler needs to
see the full definition of the array itself, and that is back in the
caller's compilation unit.

Re: [Linux-fbdev-devel] fb: modedb uses wrong default_mode

[James Simmons]

> > I really don't trust dbsize. The driver writer can pass in the wrong 
> > number.
> 
> That would be a bug.

Excuse my paranoia about what driver writers will do :-/

> > Whereas ARRAY_SIZE will always be correct. Lets take the position 
> > that we use dbsize then we need to test if dbsize is greater than the 
> > really size of the modedb. The dbsize parameter was for the days before we
> > had ARRAY_SIZE.
> > 
> > > int fb_find_mode(struct fb_var_screeninfo *var,
> > > 		 struct fb_info *info, const char *mode_option,
> > > 		 const struct fb_videomode *db, unsigned int dbsize,
> > > 		 const struct fb_videomode *default_mode,
> > > 		 unsigned int default_bpp)
> > > {
> > >     int i;
> > > 
> > >     /* Set up defaults */
> > >     if (!db) {
> > > 	db = modedb;
> > > 	dbsize = ARRAY_SIZE(modedb);
> > >     }
> > 
> >       if (dbsize > ARRAY_SIZE(db))
> > 	dbsize = ARRAY_SIZE(db);
> 
> We can't do ARRAY_SIZE on a random pointer like this: the compiler needs to
> see the full definition of the array itself, and that is back in the
> caller's compilation unit.

Good point about the pointer being valid. In that case we have to deal 
with dbsize. Still nervous about going out of bounds of the array.