[Don't use the obsolete linux-fbdev-devel address] On Thu, Nov 18, 2010 at 00:08, Michal Januszewski <michalj@xxxxxxxxx> wrote: > In the framebuffer subsystem the abs() macro is often used as a part of > the calculation of a Manhattan metric, which in turn is used as a measure > of similarity between video modes. ÂThe arguments of abs() are sometimes > unsigned numbers. ÂThis worked fine until commit a49c59c0, which changed > the definition of abs() to prevent truncation. ÂAs a result of this > change, in the following piece of code: > > Âu32 a = 0, b = 1; > Âu32 c = abs(a - b); Indeed, the difference of 2 numbers is unsigned, as per C. > 'c' will end up with a value of 0xffffffff instead of the expected 0x1. This happens on 64-bit only, right? Anyway, I think commit a49c59c0 is wrong: abs() operates on signed (32-bit) numbers. For larger (64-bit signed) numbers, we have abs64(). > A problem caused by this change and visible by the end user is that > framebuffer drivers relying on functions from modedb.c will fail to > find high resolution video modes similar to that explicitly requested > by the user if an exact match cannot be found (see e.g. > https://bugs.gentoo.org/show_bug.cgi?id=296539). > > Fix this problem by casting all arguments of abs() to an int prior > to the macro evaluation in modedb.c and uvesafb.c. > > Signed-off-by: Michal Januszewski <michalj@xxxxxxxxx> > --- > diff --git a/drivers/video/modedb.c b/drivers/video/modedb.c > index 0a4dbdc..878bea1 100644 > --- a/drivers/video/modedb.c > +++ b/drivers/video/modedb.c > @@ -636,8 +636,10 @@ done: >            Âif (refresh_specified && db[i].refresh == refresh) { >                Âreturn 1; >            Â} else { > -                if (abs(db[i].refresh - refresh) < diff) { > -                    diff = abs(db[i].refresh - refresh); > +                if (abs((int)(db[i].refresh - refresh)) < > +                  diff) { > +                    diff = abs((int)(db[i].refresh - > +                        refresh)); >                    Âbest = i; >                Â} >            Â} > @@ -654,8 +656,8 @@ done: >    Âfor (i = 0; i < dbsize; i++) { >        ÂDPRINTK("Trying %ix%i\n", db[i].xres, db[i].yres); >        Âif (!fb_try_mode(var, info, &db[i], bpp)) { > -            tdiff = abs(db[i].xres - xres) + > -                abs(db[i].yres - yres); > +            tdiff = abs((int)(db[i].xres - xres)) + > +                abs((int)(db[i].yres - yres)); > >            Â/* >             * Penalize modes with resolutions smaller > @@ -851,13 +853,13 @@ const struct fb_videomode *fb_find_nearest_mode(const struct fb_videomode *mode, >        Âmodelist = list_entry(pos, struct fb_modelist, list); >        Âcmode = &modelist->mode; > > -        d = abs(cmode->xres - mode->xres) + > -            abs(cmode->yres - mode->yres); > +        d = abs((int)(cmode->xres - mode->xres)) + > +            abs((int)(cmode->yres - mode->yres)); >        Âif (diff > d) { >            Âdiff = d; >            Âbest = cmode; >        Â} else if (diff == d) { > -            d = abs(cmode->refresh - mode->refresh); > +            d = abs((int)(cmode->refresh - mode->refresh)); >            Âif (diff_refresh > d) { >                Âdiff_refresh = d; >                Âbest = cmode; > diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c > index 7b8839e..6621427 100644 > --- a/drivers/video/uvesafb.c > +++ b/drivers/video/uvesafb.c > @@ -320,9 +320,9 @@ static int uvesafb_vbe_find_mode(struct uvesafb_par *par, >    Âint i, match = -1, h = 0, d = 0x7fffffff; > >    Âfor (i = 0; i < par->vbe_modes_cnt; i++) { > -        h = abs(par->vbe_modes[i].x_res - xres) + > -          abs(par->vbe_modes[i].y_res - yres) + > -          abs(depth - par->vbe_modes[i].depth); > +        h = abs((int)(par->vbe_modes[i].x_res - xres)) + > +          abs((int)(par->vbe_modes[i].y_res - yres)) + > +          abs((int)(depth - par->vbe_modes[i].depth)); > >        Â/* >         * We have an exact match in terms of resolution > @@ -1375,7 +1375,7 @@ static int uvesafb_check_var(struct fb_var_screeninfo *var, >     * which is theoretically incorrect, but which we'll try to handle >     * here. >     */ > -    if (depth == 0 || abs(depth - var->bits_per_pixel) >= 8) > +    if (depth == 0 || abs((int)(depth - var->bits_per_pixel)) >= 8) >        Âdepth = var->bits_per_pixel; > >    Âmatch = uvesafb_vbe_find_mode(par, var->xres, var->yres, depth, Gr{oetje,eeting}s,             Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.              Â Â -- Linus Torvalds ÿô.nlj·Ÿ®‰†+%ŠË±é¥Šwÿº{.nlj·¥Š{±ýžØn‡r¡öë¨è&£ûz¹Þúzf£¢·hšˆ§~††Ûÿÿïÿ‘ê_èæ+v‰¨þ)ßø
