abs() vs. abs64() (was: Re: [PATCH] fbdev: fix nearest mode search)

[Geert Uytterhoeven]

[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
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i