Menu
▾
▴
linux-fbdev-devel
|
From: Roel K. <roe...@gm...> - 2009-10-16 18:14:15
|
Only if the test is signed negative values can be spotted.
Signed-off-by: Roel Kluin <roe...@gm...>
---
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;
}
|
|
From: Aguirre R. S. A. <saa...@ti...> - 2009-10-16 20:57:17
|
> -----Original Message-----
> From: lin...@vg...
> [mailto:lin...@vg...] On Behalf Of Roel Kluin
> Sent: Friday, October 16, 2009 1:24 PM
> To: Imre Deak; lin...@li...;
> lin...@vg...; 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 <roe...@gm...>
> ---
> 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 maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
|
|
From: Roel K. <roe...@gm...> - 2009-10-21 15:33:52
|
regno is unsigned so the test didn't work. If regno
can't be used return an error.
Signed-off-by: Roel Kluin <roe...@gm...>
---
>> 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:
|
|
From: Aguirre R. S. A. <saa...@ti...> - 2009-10-21 15:40:11
|
> -----Original Message-----
> From: lin...@vg...
> [mailto:lin...@vg...] On Behalf Of Roel Kluin
> Sent: Wednesday, October 21, 2009 10:44 AM
> To: Aguirre Rodriguez, Sergio Alberto
> Cc: Imre Deak; lin...@li...;
> lin...@vg...; 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 <roe...@gm...>
> ---
> >> 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 <saa...@ti...>
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 maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
|
|
From: Tony L. <to...@at...> - 2009-10-22 18:22:54
|
* Aguirre Rodriguez, Sergio Alberto <saa...@ti...> [091021 08:41]:
>
>
> > -----Original Message-----
> > From: lin...@vg...
> > [mailto:lin...@vg...] On Behalf Of Roel Kluin
> > Sent: Wednesday, October 21, 2009 10:44 AM
> > To: Aguirre Rodriguez, Sergio Alberto
> > Cc: Imre Deak; lin...@li...;
> > lin...@vg...; 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 <roe...@gm...>
> > ---
> > >> 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 <saa...@ti...>
Looks good to me too.
Acked-by: Tony Lindgren <to...@at...>
>
> 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 maj...@vg...
> > 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 maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
|
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X