drivers: video: Remove useless checks

PATCH In review Maxin B John 3 participants 8 messages May 19, 2011 – Jun 3, 2011 Framebuffer layer

lore

Share & tools

lore

Maxin B John

author

2011-05-19 18:27 UTC #0 lore raw

PATCH 1/1 drivers: video: Remove useless checks

Comparing unsigned less than zero will never be true. Removing similar checks from 'fbmem.c' and 'fbcmap.c'.

Please let me know your comments.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/video/fbcmap.c +1 -2

index 5c3960d..15cf8fc 100644
@@ -285,8 +285,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 		rc = -ENODEV;
 		goto out;
 	}
-	if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-				!info->fbops->fb_setcmap)) {
+	if (!info->fbops->fb_setcolreg && !info->fbops->fb_setcmap) {
 		rc = -EINVAL;
 		goto out1;
 	}

drivers/video/fbmem.c +1 -1

index 5aac00e..9edca4d 100644
@@ -1143,7 +1143,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			return -EFAULT;
 		if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
 			return -EINVAL;
-		if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
+		if (con2fb.framebuffer >= FB_MAX)
 			return -EINVAL;
 		if (!registered_fb[con2fb.framebuffer])
 			request_module("fb%d", con2fb.framebuffer);

Maxin B John

author

2011-05-22 08:06 UTC #1 lore raw

Hi Paul,

Please let me know your comments on this patch.

Best Regards,
Maxin

Quoted 35 lines · Maxin B John

Comparing unsigned less than zero will never be true. Removing similar checks from 'fbmem.c' and 'fbcmap.c'.

Please let me know your comments.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/video/fbcmap.c +1 -2

index 5c3960d..15cf8fc 100644
@@ -285,8 +285,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
                rc = -ENODEV;
                goto out;
        }
-       if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-                               !info->fbops->fb_setcmap)) {
+       if (!info->fbops->fb_setcolreg && !info->fbops->fb_setcmap) {
                rc = -EINVAL;
                goto out1;
        }

drivers/video/fbmem.c +1 -1

index 5aac00e..9edca4d 100644
@@ -1143,7 +1143,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
                        return -EFAULT;
                if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
                        return -EINVAL;
-               if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
+               if (con2fb.framebuffer >= FB_MAX)
                        return -EINVAL;
                if (!registered_fb[con2fb.framebuffer])
                        request_module("fb%d", con2fb.framebuffer);

Tim Gardner

2011-05-25 12:47 UTC #2 lore raw

In reply to Maxin B John

Quoted 27 lines · Maxin B John

Comparing unsigned less than zero will never be true. Removing similar checks from 'fbmem.c' and 'fbcmap.c'.
Please let me know your comments.
Signed-off-by Maxin B. John<[email protected]>
---
drivers/video/fbcmap.c +1 -2
index 5c3960d..15cf8fc 100644
@@ -285,8 +285,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
  		rc = -ENODEV;
  		goto out;
  	}
-	if (cmap->start<  0 || (!info->fbops->fb_setcolreg&&
-				!info->fbops->fb_setcmap)) {
+	if (!info->fbops->fb_setcolreg&&  !info->fbops->fb_setcmap) {
  		rc = -EINVAL;
  		goto out1;
  	}
drivers/video/fbmem.c +0 -0
index 5aac00e..9edca4d 100644

drivers/video/fbmem.c static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, · line 1149

Quoted 8 lines

drivers/video/fbmem.c +1 -1

@@ -1143,7 +1143,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
  			return -EFAULT;
  		if (con2fb.console<  1 || con2fb.console>  MAX_NR_CONSOLES)
  			return -EINVAL;
-		if (con2fb.framebuffer<  0 || con2fb.framebuffer>= FB_MAX)
+		if (con2fb.framebuffer>= FB_MAX)
  			return -EINVAL;
  		if (!registered_fb[con2fb.framebuffer])
  			request_module("fb%d", con2fb.framebuffer);

Looks right to me, though there are other places that suffer from the same issue. See fb_set_cmap() and its use of 'int start' and cmap->start.

rtg

Signature · 1 line

Tim Gardner [email protected]

Maxin B John

author

2011-05-26 09:30 UTC #3 lore raw

Hi,

Quoted 7 lines · Tim Gardner

Quoted 3 lines · Maxin B John
Comparing unsigned less than zero will never be true. Removing similar checks from 'fbmem.c' and 'fbcmap.c'.
Looks right to me, though there are other places that suffer from the same issue. See fb_set_cmap() and its use of 'int start' and cmap->start.

Thank you very much for reviewing the patch. As per your suggestion, I have checked
the scenario is fb_set_cmap() and the use of 'int start' and 'cmap->start'.

IMHO, that scenario doesn't fall under the comparison of unsigned int < 0. That scenario
looks similar to the below given code to me:
/*---------------------------------*/
#include <stdio.h>

int
main ()
{
unsigned int u_int = -1;
int s_int = 0;
s_int = u_int;
if (s_int < 0)

    printf ("s_int is less than 0\n");

return 0;
}
/*---------------------------------*/

Please let me know your comments.

Best Regards,
Maxin

Tim Gardner

2011-05-26 13:43 UTC #4 lore raw

Quoted 31 lines · Maxin B John

Hi,
Quoted 7 lines · Tim Gardner
Quoted 3 lines · Maxin B John
Comparing unsigned less than zero will never be true. Removing similar checks from 'fbmem.c' and 'fbcmap.c'.
Looks right to me, though there are other places that suffer from the same issue. See fb_set_cmap() and its use of 'int start' and cmap->start.
Thank you very much for reviewing the patch. As per your suggestion, I have checked
the scenario is fb_set_cmap() and the use of 'int start' and 'cmap->start'.
IMHO, that scenario doesn't fall under the comparison of unsigned int < 0. That scenario
looks similar to the below given code to me:
/*---------------------------------*/
#include<stdio.h>
int
main ()
{
   unsigned int u_int = -1;
   int s_int = 0;
   s_int = u_int;
   if (s_int<  0)
     printf ("s_int is less than 0\n");
   return 0;
}

Quoted 7 lines

/*---------------------------------*/
Please let me know your comments.
Best Regards,
Maxin

IMHO mixing signed and unsigned comparisons like this is just wrong. Its unnecessarily complicated and misleading, especially for a device driver. I'm a firm believer in the KISS philosophy for driver development.

Its likely that the reason the fb code got into this situation is because a type was changed from signed to unsigned whence long ago, and nobody has bothered to clean it up.

rtg

Signature · 1 line

Tim Gardner [email protected]

Maxin B John

author

2011-05-26 15:31 UTC #5 lore raw

Hi,

Quoted 2 lines

IMHO mixing signed and unsigned comparisons like this is just wrong. Its unnecessarily complicated and misleading, especially for a device driver.

Yes. I do agree with you.

Quoted 3 lines

Its likely that the reason the fb code got into this situation is because a type was changed from signed to unsigned whence long ago, and nobody has bothered to clean it up.

Thats possible. Similar useless checks are present in other subsystems of the kernel as well.

WBR,
Maxin

Maxin B John

author

2011-06-03 09:29 UTC #6 lore raw

In reply to Maxin B John

ping ...

Quoted 35 lines · Maxin B John

Comparing unsigned less than zero will never be true. Removing similar checks from 'fbmem.c' and 'fbcmap.c'.

Please let me know your comments.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/video/fbcmap.c +1 -2

index 5c3960d..15cf8fc 100644
@@ -285,8 +285,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
                rc = -ENODEV;
                goto out;
        }
-       if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
-                               !info->fbops->fb_setcmap)) {
+       if (!info->fbops->fb_setcolreg && !info->fbops->fb_setcmap) {
                rc = -EINVAL;
                goto out1;
        }

drivers/video/fbmem.c +1 -1

index 5aac00e..9edca4d 100644
@@ -1143,7 +1143,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
                        return -EFAULT;
                if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
                        return -EINVAL;
-               if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
+               if (con2fb.framebuffer >= FB_MAX)
                        return -EINVAL;
                if (!registered_fb[con2fb.framebuffer])
                        request_module("fb%d", con2fb.framebuffer);

Joe Perches

2011-06-03 20:11 UTC #7 lore raw

Quoted 5 lines · Maxin B John

ping ...
Quoted 3 lines · Maxin B John
Comparing unsigned less than zero will never be true. Removing similar checks from 'fbmem.c' and 'fbcmap.c'. Please let me know your comments.

http://lkml.org/lkml/2004/11/23/171