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

[James Simmons <jsimmons@infradead.org>]

> > > 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.


 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/