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

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: michalj@gmail.com, Rolf Eike Beer <eike-kernel@sf-tec.de>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org,
	Linux Fbdev development list <linux-fbdev@vger.kernel.org>
Subject: abs() vs. abs64() (was: Re: [PATCH] fbdev: fix nearest mode search)
Date: Thu, 18 Nov 2010 07:40:19 +0100	[thread overview]
Message-ID: <AANLkTimYDAJr9x00sjGGnDF2vOVXU12qmbnmfuzA5_0h@mail.gmail.com> (raw)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 6070 bytes --]

[Don't use the obsolete linux-fbdev-devel address]

On Thu, Nov 18, 2010 at 00:08, Michal Januszewski <michalj@gmail.com> 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@gmail.com>
> ---
> 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@linux-m68k.org

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Ç+‰·Ÿ®‰†+%ŠËÿ±éÝ¶\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dÊ‡Ú™ë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥