Skip to content Skip to conversation

omapfb: Wrong test on unsigned?

PATCH In review Roel Kluin 3 participants 5 messages Oct 16, 2009 – Oct 22, 2009 Framebuffer layer

Share & tools

Roel Kluin

author

2009-10-16 18:14 UTC #0 lore raw

PATCH 1/1 omapfb: Wrong test on unsigned?

Only if the test is signed negative values can be spotted.

Signed-off-by Roel Kluin <[email protected]>

---
Is this correct? please review.

drivers/video/omap/omapfb_main.c +1 -1

index 0d0c8c8..cc7dd93 100644
@@ -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;
 		}

Aguirre Rodriguez, Sergio Alberto

2009-10-16 20:56 UTC #1 lore raw

Quoted 22 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Roel Kluin
Sent: Friday, October 16, 2009 1:24 PM
To: Imre Deak; [email protected]; [email protected]; 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 <[email protected]>
---
Is this correct? please review.
drivers/video/omap/omapfb_main.c +0 -0
index 0d0c8c8..cc7dd93 100644

drivers/video/omap/omapfb_main.c static int _setcolreg(struct fb_info · line 290

Quoted 6 lines

drivers/video/omap/omapfb_main.c +1 -1

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

Quoted 10 lines

drivers/video/omap/omapfb_main.c +0 -0
 			r = -EINVAL;
 			break;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Roel Kluin

author

2009-10-21 15:33 UTC #2 lore raw

regno is unsigned so the test didn't work. If regno can't be used return an error.

Signed-off-by Roel Kluin <[email protected]>

---

Quoted 12 lines

Quoted 12 lines

Is this correct? please review.

drivers/video/omap/omapfb_main.c +0 -0

index 0d0c8c8..cc7dd93 100644
@@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info 
 *info, u_int regno, u_int red, u_int green,
 		if (r != 0)
 			break;

Quoted 6 lines

drivers/video/omap/omapfb_main.c static int _setcolreg(struct fb_info · line 290
Quoted 2 lines
drivers/video/omap/omapfb_main.c +1 -1
-		if (regno < 0) {
+		if ((int)regno < 0) {
Hmm...
Isn't regno unsigned integer from the start?

yes

Quoted 8 lines

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.

Quoted 2 lines

Regards,
Sergio

Thanks,

Roel

drivers/video/omap/omapfb_main.c +2 -6

index 0d0c8c8..4da94d0 100644
@@ -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:

Aguirre Rodriguez, Sergio Alberto

Acked-by

2009-10-21 15:39 UTC #3 lore raw

Quoted 45 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Roel Kluin
Sent: Wednesday, October 21, 2009 10:44 AM
To: Aguirre Rodriguez, Sergio Alberto
Cc: Imre Deak; [email protected]; [email protected]; 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 <[email protected]>
---
Quoted 12 lines
Quoted 12 lines
Is this correct? please review.
drivers/video/omap/omapfb_main.c +0 -0
index 0d0c8c8..cc7dd93 100644
@@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info 
 *info, u_int regno, u_int red, u_int green,
 		if (r != 0)
 			break;
 
Quoted 6 lines
drivers/video/omap/omapfb_main.c static int _setcolreg(struct fb_info · line 290
Quoted 2 lines
drivers/video/omap/omapfb_main.c +1 -1
-		if (regno < 0) {
+		if ((int)regno < 0) {
Hmm...
Isn't regno unsigned integer from the start?
yes
Quoted 4 lines
2 things here:
- regno will never be negative.
- Casting won't make a difference in the meaning., it'll
make a negative only when:
Quoted 3 lines
	regno > ((2^32) / 2)

Quoted 7 lines

Quoted 1 line
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 <[email protected]>

Regards,
Sergio

Quoted 44 lines

Quoted 3 lines

Regards,
Sergio

Thanks,

Roel

drivers/video/omap/omapfb_main.c +0 -6

index 0d0c8c8..4da94d0 100644
@@ -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;

drivers/video/omap/omapfb_main.c +2 -0

 			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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Tony Lindgren

Acked-by

2009-10-22 18:22 UTC #4 lore raw

Quoted 53 lines · Aguirre Rodriguez, Sergio Alberto

Quoted 45 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Roel Kluin
Sent: Wednesday, October 21, 2009 10:44 AM
To: Aguirre Rodriguez, Sergio Alberto
Cc: Imre Deak; [email protected]; [email protected]; 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 <[email protected]>
---
Quoted 12 lines
Quoted 12 lines
Is this correct? please review.
drivers/video/omap/omapfb_main.c +0 -0
index 0d0c8c8..cc7dd93 100644
@@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info 
 *info, u_int regno, u_int red, u_int green,
 		if (r != 0)
 			break;
 
Quoted 6 lines
drivers/video/omap/omapfb_main.c static int _setcolreg(struct fb_info · line 290
Quoted 2 lines
drivers/video/omap/omapfb_main.c +1 -1
-		if (regno < 0) {
+		if ((int)regno < 0) {
Hmm...
Isn't regno unsigned integer from the start?
yes
Quoted 4 lines
2 things here:
- regno will never be negative.
- Casting won't make a difference in the meaning., it'll
make a negative only when:
Quoted 3 lines
	regno > ((2^32) / 2)
Quoted 7 lines
Quoted 1 line
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.

Quoted 3 lines

Yep. Looks nicer to me ;)
Acked-by Sergio Aguirre <[email protected]>

Looks good to me too.

Acked-by Tony Lindgren <[email protected]>

Quoted 50 lines

Regards,
Sergio
Quoted 43 lines
Quoted 3 lines
Regards,
Sergio
Thanks,
Roel
drivers/video/omap/omapfb_main.c +0 -6
index 0d0c8c8..4da94d0 100644
@@ -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;
drivers/video/omap/omapfb_main.c +2 -0
 			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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 1 line
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html