* Re: [PATCH] drivers: video: Remove useless checks
2011-05-19 18:27 [PATCH] drivers: video: Remove useless checks Maxin B John
@ 2011-05-22 8:06 ` Maxin B John
2011-05-25 12:47 ` Tim Gardner
2011-06-03 9:29 ` Maxin B John
2 siblings, 0 replies; 8+ messages in thread
From: Maxin B John @ 2011-05-22 8:06 UTC (permalink / raw)
To: linux-kernel
Cc: akpm, tim.gardner, airlied, lethal, james, error27, randy.dunlap
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 <maxin.john@gmail.com> 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 <maxin.john@gmail.com>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drivers: video: Remove useless checks
2011-05-19 18:27 [PATCH] drivers: video: Remove useless checks Maxin B John
2011-05-22 8:06 ` Maxin B John
@ 2011-05-25 12:47 ` Tim Gardner
2011-05-26 9:30 ` Maxin B John
2011-06-03 9:29 ` Maxin B John
2 siblings, 1 reply; 8+ messages in thread
From: Tim Gardner @ 2011-05-25 12:47 UTC (permalink / raw)
To: Maxin B John
Cc: linux-kernel, akpm, airlied, lethal, james, error27, randy.dunlap
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<maxin.john@gmail.com>
> ---
> 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 tim.gardner@canonical.com
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drivers: video: Remove useless checks
2011-05-25 12:47 ` Tim Gardner
@ 2011-05-26 9:30 ` Maxin B John
2011-05-26 13:43 ` Tim Gardner
0 siblings, 1 reply; 8+ messages in thread
From: Maxin B John @ 2011-05-26 9:30 UTC (permalink / raw)
To: tim.gardner
Cc: linux-kernel, akpm, airlied, lethal, james, error27, randy.dunlap
Hi,
On Wed, May 25, 2011 at 1:47 PM, Tim Gardner <tim.gardner@canonical.com> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drivers: video: Remove useless checks
2011-05-26 9:30 ` Maxin B John
@ 2011-05-26 13:43 ` Tim Gardner
2011-05-26 15:31 ` Maxin B John
0 siblings, 1 reply; 8+ messages in thread
From: Tim Gardner @ 2011-05-26 13:43 UTC (permalink / raw)
To: Maxin B John
Cc: linux-kernel, akpm, airlied, lethal, james, error27, randy.dunlap
On 05/26/2011 03:30 AM, Maxin B John wrote:
> Hi,
>
> On Wed, May 25, 2011 at 1:47 PM, Tim Gardner<tim.gardner@canonical.com> 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 tim.gardner@canonical.com
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drivers: video: Remove useless checks
2011-05-26 13:43 ` Tim Gardner
@ 2011-05-26 15:31 ` Maxin B John
0 siblings, 0 replies; 8+ messages in thread
From: Maxin B John @ 2011-05-26 15:31 UTC (permalink / raw)
To: tim.gardner
Cc: linux-kernel, akpm, airlied, lethal, james, error27, randy.dunlap
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers: video: Remove useless checks
2011-05-19 18:27 [PATCH] drivers: video: Remove useless checks Maxin B John
2011-05-22 8:06 ` Maxin B John
2011-05-25 12:47 ` Tim Gardner
@ 2011-06-03 9:29 ` Maxin B John
2011-06-03 20:11 ` Joe Perches
2 siblings, 1 reply; 8+ messages in thread
From: Maxin B John @ 2011-06-03 9:29 UTC (permalink / raw)
To: linux-kernel
Cc: akpm, tim.gardner, airlied, lethal, james, error27, randy.dunlap
ping ...
On Thu, May 19, 2011 at 7:27 PM, Maxin B John <maxin.john@gmail.com> 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 <maxin.john@gmail.com>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drivers: video: Remove useless checks
2011-06-03 9:29 ` Maxin B John
@ 2011-06-03 20:11 ` Joe Perches
0 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2011-06-03 20:11 UTC (permalink / raw)
To: Maxin B John
Cc: linux-kernel, akpm, tim.gardner, airlied, lethal, james, error27,
randy.dunlap
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 <maxin.john@gmail.com> 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
^ permalink raw reply [flat|nested] 8+ messages in thread