* [PATCH] omapfb: Wrong test on unsigned?
@ 2009-10-16 18:23 Roel Kluin
2009-10-16 20:56 ` Aguirre Rodriguez, Sergio Alberto
0 siblings, 1 reply; 5+ messages in thread
From: Roel Kluin @ 2009-10-16 18:23 UTC (permalink / raw)
To: Imre Deak, linux-fbdev-devel, linux-omap, Andrew Morton
Only if the test is signed negative values can be spotted.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Is this correct? please review.
diff --git a/drivers/video/omap/omapfb_main.c b/drivers/video/omap/omapfb_main.c
index 0d0c8c8..cc7dd93 100644
--- a/drivers/video/omap/omapfb_main.c
+++ b/drivers/video/omap/omapfb_main.c
@@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info *info, u_int regno, u_int red, u_int green,
if (r != 0)
break;
- if (regno < 0) {
+ if ((int)regno < 0) {
r = -EINVAL;
break;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH] omapfb: Wrong test on unsigned?
2009-10-16 18:23 [PATCH] omapfb: Wrong test on unsigned? Roel Kluin
@ 2009-10-16 20:56 ` Aguirre Rodriguez, Sergio Alberto
2009-10-21 15:43 ` Roel Kluin
0 siblings, 1 reply; 5+ messages in thread
From: Aguirre Rodriguez, Sergio Alberto @ 2009-10-16 20:56 UTC (permalink / raw)
To: Roel Kluin, Imre Deak, linux-fbdev-devel@lists.sourceforge.net,
linux-omap
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Roel Kluin
> Sent: Friday, October 16, 2009 1:24 PM
> To: Imre Deak; linux-fbdev-devel@lists.sourceforge.net;
> linux-omap@vger.kernel.org; Andrew Morton
> Subject: [PATCH] omapfb: Wrong test on unsigned?
>
> Only if the test is signed negative values can be spotted.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> Is this correct? please review.
>
> diff --git a/drivers/video/omap/omapfb_main.c
> b/drivers/video/omap/omapfb_main.c
> index 0d0c8c8..cc7dd93 100644
> --- a/drivers/video/omap/omapfb_main.c
> +++ b/drivers/video/omap/omapfb_main.c
> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info
> *info, u_int regno, u_int red, u_int green,
> if (r != 0)
> break;
>
> - if (regno < 0) {
> + if ((int)regno < 0) {
Hmm...
Isn't regno unsigned integer from the start?
2 things here:
- regno will never be negative.
- Casting won't make a difference in the meaning., it'll make a negative only when:
regno > ((2^32) / 2)
Which doesn't make any sense IMHO.
Isn't it?
Regards,
Sergio
> r = -EINVAL;
> break;
> }
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] omapfb: Wrong test on unsigned?
2009-10-21 15:43 ` Roel Kluin
@ 2009-10-21 15:39 ` Aguirre Rodriguez, Sergio Alberto
2009-10-22 18:22 ` Tony Lindgren
0 siblings, 1 reply; 5+ messages in thread
From: Aguirre Rodriguez, Sergio Alberto @ 2009-10-21 15:39 UTC (permalink / raw)
To: Roel Kluin
Cc: Imre Deak, linux-fbdev-devel@lists.sourceforge.net,
linux-omap@vger.kernel.org, Andrew Morton
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Roel Kluin
> Sent: Wednesday, October 21, 2009 10:44 AM
> To: Aguirre Rodriguez, Sergio Alberto
> Cc: Imre Deak; linux-fbdev-devel@lists.sourceforge.net;
> linux-omap@vger.kernel.org; Andrew Morton
> Subject: Re: [PATCH] omapfb: Wrong test on unsigned?
>
> regno is unsigned so the test didn't work. If regno
> can't be used return an error.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> >> Is this correct? please review.
> >>
> >> diff --git a/drivers/video/omap/omapfb_main.c
> >> b/drivers/video/omap/omapfb_main.c
> >> index 0d0c8c8..cc7dd93 100644
> >> --- a/drivers/video/omap/omapfb_main.c
> >> +++ b/drivers/video/omap/omapfb_main.c
> >> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info
> >> *info, u_int regno, u_int red, u_int green,
> >> if (r != 0)
> >> break;
> >>
> >> - if (regno < 0) {
> >> + if ((int)regno < 0) {
> >
> > Hmm...
> >
> > Isn't regno unsigned integer from the start?
>
> yes
>
> > 2 things here:
> >
> > - regno will never be negative.
> > - Casting won't make a difference in the meaning., it'll
> make a negative only when:
> >
> > regno > ((2^32) / 2)
> >
> > Which doesn't make any sense IMHO.
>
> I think it is strange that _setcolreg() accepts a regno that
> is invalid,
> ignores it and just returns as if all was OK. If you agree
> then you may
> like the patch below.
Yep. Looks nicer to me ;)
Acked-by: Sergio Aguirre <saaguirre@ti.com>
Regards,
Sergio
>
> > Regards,
> > Sergio
>
> Thanks,
>
> Roel
>
> diff --git a/drivers/video/omap/omapfb_main.c
> b/drivers/video/omap/omapfb_main.c
> index 0d0c8c8..4da94d0 100644
> --- a/drivers/video/omap/omapfb_main.c
> +++ b/drivers/video/omap/omapfb_main.c
> @@ -285,12 +285,6 @@ static int _setcolreg(struct fb_info
> *info, u_int regno, u_int red, u_int green,
> case OMAPFB_COLOR_RGB444:
> if (r != 0)
> break;
> -
> - if (regno < 0) {
> - r = -EINVAL;
> - break;
> - }
> -
> if (regno < 16) {
> u16 pal;
> pal = ((red >> (16 - var->red.length)) <<
> @@ -299,6 +293,8 @@ static int _setcolreg(struct fb_info
> *info, u_int regno, u_int red, u_int green,
> var->green.offset) |
> (blue >> (16 - var->blue.length));
> ((u32 *)(info->pseudo_palette))[regno] = pal;
> + } else {
> + r = -EINVAL;
> }
> break;
> default:
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] omapfb: Wrong test on unsigned?
2009-10-16 20:56 ` Aguirre Rodriguez, Sergio Alberto
@ 2009-10-21 15:43 ` Roel Kluin
2009-10-21 15:39 ` Aguirre Rodriguez, Sergio Alberto
0 siblings, 1 reply; 5+ messages in thread
From: Roel Kluin @ 2009-10-21 15:43 UTC (permalink / raw)
To: Aguirre Rodriguez, Sergio Alberto
Cc: Imre Deak, linux-fbdev-devel@lists.sourceforge.net,
linux-omap@vger.kernel.org, Andrew Morton
regno is unsigned so the test didn't work. If regno
can't be used return an error.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
>> Is this correct? please review.
>>
>> diff --git a/drivers/video/omap/omapfb_main.c
>> b/drivers/video/omap/omapfb_main.c
>> index 0d0c8c8..cc7dd93 100644
>> --- a/drivers/video/omap/omapfb_main.c
>> +++ b/drivers/video/omap/omapfb_main.c
>> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info
>> *info, u_int regno, u_int red, u_int green,
>> if (r != 0)
>> break;
>>
>> - if (regno < 0) {
>> + if ((int)regno < 0) {
>
> Hmm...
>
> Isn't regno unsigned integer from the start?
yes
> 2 things here:
>
> - regno will never be negative.
> - Casting won't make a difference in the meaning., it'll make a negative only when:
>
> regno > ((2^32) / 2)
>
> Which doesn't make any sense IMHO.
I think it is strange that _setcolreg() accepts a regno that is invalid,
ignores it and just returns as if all was OK. If you agree then you may
like the patch below.
> Regards,
> Sergio
Thanks,
Roel
diff --git a/drivers/video/omap/omapfb_main.c b/drivers/video/omap/omapfb_main.c
index 0d0c8c8..4da94d0 100644
--- a/drivers/video/omap/omapfb_main.c
+++ b/drivers/video/omap/omapfb_main.c
@@ -285,12 +285,6 @@ static int _setcolreg(struct fb_info *info, u_int regno, u_int red, u_int green,
case OMAPFB_COLOR_RGB444:
if (r != 0)
break;
-
- if (regno < 0) {
- r = -EINVAL;
- break;
- }
-
if (regno < 16) {
u16 pal;
pal = ((red >> (16 - var->red.length)) <<
@@ -299,6 +293,8 @@ static int _setcolreg(struct fb_info *info, u_int regno, u_int red, u_int green,
var->green.offset) |
(blue >> (16 - var->blue.length));
((u32 *)(info->pseudo_palette))[regno] = pal;
+ } else {
+ r = -EINVAL;
}
break;
default:
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] omapfb: Wrong test on unsigned?
2009-10-21 15:39 ` Aguirre Rodriguez, Sergio Alberto
@ 2009-10-22 18:22 ` Tony Lindgren
0 siblings, 0 replies; 5+ messages in thread
From: Tony Lindgren @ 2009-10-22 18:22 UTC (permalink / raw)
To: Aguirre Rodriguez, Sergio Alberto
Cc: Roel Kluin, Imre Deak, linux-fbdev-devel@lists.sourceforge.net,
linux-omap@vger.kernel.org, Andrew Morton
* Aguirre Rodriguez, Sergio Alberto <saaguirre@ti.com> [091021 08:41]:
>
>
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Roel Kluin
> > Sent: Wednesday, October 21, 2009 10:44 AM
> > To: Aguirre Rodriguez, Sergio Alberto
> > Cc: Imre Deak; linux-fbdev-devel@lists.sourceforge.net;
> > linux-omap@vger.kernel.org; Andrew Morton
> > Subject: Re: [PATCH] omapfb: Wrong test on unsigned?
> >
> > regno is unsigned so the test didn't work. If regno
> > can't be used return an error.
> >
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > >> Is this correct? please review.
> > >>
> > >> diff --git a/drivers/video/omap/omapfb_main.c
> > >> b/drivers/video/omap/omapfb_main.c
> > >> index 0d0c8c8..cc7dd93 100644
> > >> --- a/drivers/video/omap/omapfb_main.c
> > >> +++ b/drivers/video/omap/omapfb_main.c
> > >> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info
> > >> *info, u_int regno, u_int red, u_int green,
> > >> if (r != 0)
> > >> break;
> > >>
> > >> - if (regno < 0) {
> > >> + if ((int)regno < 0) {
> > >
> > > Hmm...
> > >
> > > Isn't regno unsigned integer from the start?
> >
> > yes
> >
> > > 2 things here:
> > >
> > > - regno will never be negative.
> > > - Casting won't make a difference in the meaning., it'll
> > make a negative only when:
> > >
> > > regno > ((2^32) / 2)
> > >
> > > Which doesn't make any sense IMHO.
> >
> > I think it is strange that _setcolreg() accepts a regno that
> > is invalid,
> > ignores it and just returns as if all was OK. If you agree
> > then you may
> > like the patch below.
>
> Yep. Looks nicer to me ;)
>
> Acked-by: Sergio Aguirre <saaguirre@ti.com>
Looks good to me too.
Acked-by: Tony Lindgren <tony@atomide.com>
>
> Regards,
> Sergio
> >
> > > Regards,
> > > Sergio
> >
> > Thanks,
> >
> > Roel
> >
> > diff --git a/drivers/video/omap/omapfb_main.c
> > b/drivers/video/omap/omapfb_main.c
> > index 0d0c8c8..4da94d0 100644
> > --- a/drivers/video/omap/omapfb_main.c
> > +++ b/drivers/video/omap/omapfb_main.c
> > @@ -285,12 +285,6 @@ static int _setcolreg(struct fb_info
> > *info, u_int regno, u_int red, u_int green,
> > case OMAPFB_COLOR_RGB444:
> > if (r != 0)
> > break;
> > -
> > - if (regno < 0) {
> > - r = -EINVAL;
> > - break;
> > - }
> > -
> > if (regno < 16) {
> > u16 pal;
> > pal = ((red >> (16 - var->red.length)) <<
> > @@ -299,6 +293,8 @@ static int _setcolreg(struct fb_info
> > *info, u_int regno, u_int red, u_int green,
> > var->green.offset) |
> > (blue >> (16 - var->blue.length));
> > ((u32 *)(info->pseudo_palette))[regno] = pal;
> > + } else {
> > + r = -EINVAL;
> > }
> > break;
> > default:
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-10-22 18:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-16 18:23 [PATCH] omapfb: Wrong test on unsigned? Roel Kluin
2009-10-16 20:56 ` Aguirre Rodriguez, Sergio Alberto
2009-10-21 15:43 ` Roel Kluin
2009-10-21 15:39 ` Aguirre Rodriguez, Sergio Alberto
2009-10-22 18:22 ` Tony Lindgren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).