2011-05-19 18:27:14

by Maxin B. John

Subject: [PATCH] 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]>
---
diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
index 5c3960d..15cf8fc 100644
--- a/drivers/video/fbcmap.c
+++ b/drivers/video/fbcmap.c
@@ -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;
}
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 5aac00e..9edca4d 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -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);

2011-05-22 08:06:25

by Maxin B. John

Subject: Re: [PATCH] drivers: video: Remove useless checks

Hi Paul,

Please let me know your comments on this patch.

Best Regards,
Maxin

On Thu, May 19, 2011 at 9:27 PM, Maxin B John <[email protected]> wrote:
> 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]>
> ---
> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
> index 5c3960d..15cf8fc 100644
> --- a/drivers/video/fbcmap.c
> +++ b/drivers/video/fbcmap.c
> @@ -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;
> ? ? ? ?}
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 5aac00e..9edca4d 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -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);
>

2011-05-25 12:47:55

by Tim Gardner

Subject: Re: [PATCH] drivers: video: Remove useless checks

On 05/19/2011 12:27 PM, Maxin B John wrote:
> 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]>
> ---
> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
> index 5c3960d..15cf8fc 100644
> --- a/drivers/video/fbcmap.c
> +++ b/drivers/video/fbcmap.c
> @@ -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;
> }
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 5aac00e..9edca4d 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -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
--
Tim Gardner [email protected]

2011-05-26 09:30:06

by Maxin B. John

Subject: Re: [PATCH] drivers: video: Remove useless checks

Hi,

On Wed, May 25, 2011 at 1:47 PM, Tim Gardner <[email protected]> wrote:
> On 05/19/2011 12:27 PM, Maxin B John wrote:
>>
>> 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

2011-05-26 13:43:24

by Tim Gardner

Subject: Re: [PATCH] drivers: video: Remove useless checks

On 05/26/2011 03:30 AM, Maxin B John wrote:
> Hi,
>
> On Wed, May 25, 2011 at 1:47 PM, Tim Gardner<[email protected]> wrote:
>> On 05/19/2011 12:27 PM, Maxin B John wrote:
>>>
>>> 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
>

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
--
Tim Gardner [email protected]

2011-05-26 15:31:30

by Maxin B. John

Subject: Re: [PATCH] drivers: video: Remove useless checks

Hi,

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

> 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

2011-06-03 09:29:51

by Maxin B. John

Subject: Re: [PATCH] drivers: video: Remove useless checks

ping ...

On Thu, May 19, 2011 at 7:27 PM, Maxin B John <[email protected]> wrote:
> 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]>
> ---
> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
> index 5c3960d..15cf8fc 100644
> --- a/drivers/video/fbcmap.c
> +++ b/drivers/video/fbcmap.c
> @@ -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;
> ? ? ? ?}
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 5aac00e..9edca4d 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -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);
>

2011-06-03 20:11:10

by Joe Perches

Subject: Re: [PATCH] drivers: video: Remove useless checks

On Fri, 2011-06-03 at 10:29 +0100, Maxin B John wrote:
> ping ...
> On Thu, May 19, 2011 at 7:27 PM, Maxin B John <[email protected]> wrote:
> > 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