* [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-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: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-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).