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 <[email protected]>
---
--- 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)
On Wed, 15 Nov 2006 19:43:16 +0200 (EET)
Tero Roponen <[email protected]> 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 <[email protected]>
> ---
>
> --- 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.
On 15/11/06 15:29 -0800, Andrew Morton wrote:
> On Wed, 15 Nov 2006 19:43:16 +0200 (EET)
> Tero Roponen <[email protected]> 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.
<http://www.amd.com/embeddedprocessors>
> > > 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.
On Fri, 17 Nov 2006 20:08:47 +0000 (GMT)
James Simmons <[email protected]> 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 <[email protected]>
If no default mode is specified, it should be grabbed from the supplied
database, not the default one.
[[email protected]: fix it]
[[email protected]: simplify it]
[[email protected]: remove pointless DEFAULT_MODEDB_INDEX]
Signed-off-by: Jordan Crouse <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: "Antonino A. Daplas" <[email protected]>
Signed-off-by: Tero Roponen <[email protected]>
Cc: James Simmons <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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;
_
On Fri, 17 Nov 2006, Andrew Morton wrote:
>
> Here's the current version of this monster patch:
>
> From: Jordan Crouse <[email protected]>
>
> If no default mode is specified, it should be grabbed from the supplied
> database, not the default one.
>
> [[email protected]: fix it]
> [[email protected]: simplify it]
> [[email protected]: remove pointless DEFAULT_MODEDB_INDEX]
> Signed-off-by: Jordan Crouse <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: "Antonino A. Daplas" <[email protected]>
> Signed-off-by: Tero Roponen <[email protected]>
> Cc: James Simmons <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> 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
> James Simmons <[email protected]> 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;
On Mon, 20 Nov 2006 16:48:05 +0000 (GMT)
James Simmons <[email protected]> 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.
> > 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.