Skip to contentSkip to conversation

logical/bitand typo in powerpc/boot/4xx.c

PATCH In review Roel Kluin 6 participants 9 messages Jan 23, 2008 – Jan 29, 2008

Subsystems (2)
PowerPC Checkpatch
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 logical/bitand typo in powerpc/boot/4xx.c

logical/bitand typo

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

---

arch/powerpc/boot/4xx.c +1 -1
index ebf9e21..dcfb459 100644
@@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
 	val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
 	cs = 0;
 	while (val) {
-		if (val && 0x1)
+		if (val & 0x1)
 			cs++;
 		val = val >> 1;
 	}
maintainer
#1 lore raw
Quoted 7 lines · Roel Kluin
Signed-off-by Roel Kluin <[email protected]>

---

arch/powerpc/boot/4xx.c +0 -0
index ebf9e21..dcfb459 100644

arch/powerpc/boot/4xx.c void ibm4xx_denali_fixup_memsize(void) · line 109

Quoted 7 lines
arch/powerpc/boot/4xx.c +1 -1
@@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
 	val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
 	cs = 0;
 	while (val) {
-		if (val && 0x1)
+		if (val & 0x1)
 			cs++;
 		val = val >> 1;

I think this pattern should be added to checkpatch

Signed-off-by Joe Perches <[email protected]>
scripts/checkpatch.pl +5 -0
index 579f50f..147e573 100755
@@ -1337,6 +1337,11 @@ sub process {
 			}
 		}
 
+# Check for bitwise tests written as boolean
+		if ($line =~ /\&\&\s*0[xX]/) {
+			WARN("boolean test with hexadecimal, perhaps just 1 \&?\n" . $herecurr);
+		}
+
 # if and else should not have general statements after it
 		if ($line =~ /^.\s*(?:}\s*)?else\b(.*)/ &&
 		    $1 !~ /^\s*(?:\sif|{|\\|$)/) {
author
#2 lore raw
Quoted 17 lines · Joe Perches
Quoted 7 lines · Roel Kluin
Signed-off-by Roel Kluin <[email protected]>

---

arch/powerpc/boot/4xx.c +0 -0
index ebf9e21..dcfb459 100644

arch/powerpc/boot/4xx.c void ibm4xx_denali_fixup_memsize(void) · line 109

Quoted 7 lines
arch/powerpc/boot/4xx.c +1 -1
@@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
 	val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
 	cs = 0;
 	while (val) {
-		if (val && 0x1)
+		if (val & 0x1)
 			cs++;
 		val = val >> 1;

I think this pattern should be added to checkpatch

I agree but...

Quoted 7 lines
Signed-off-by Joe Perches <[email protected]>
scripts/checkpatch.pl +0 -0
index 579f50f..147e573 100755

scripts/checkpatch.pl sub process { · line 1344

Quoted 8 lines
scripts/checkpatch.pl +5 -0
@@ -1337,6 +1337,11 @@ sub process {
 			}
 		}
 
+# Check for bitwise tests written as boolean
+		if ($line =~ /\&\&\s*0[xX]/) {
+			WARN("boolean test with hexadecimal, perhaps just 1 \&?\n" . $herecurr);
+		}
+

when you use git-grep -n "\(&&\|||\)${s}0x\([A-Z0-9]*\|[a-z0-9]*\)", (with s="[[:space:]]*") there will be false positives like

if (relocation < -0x20000 || 0x1fffc < relocation)
if (ARCH_SUN4C_SUN4 && addr < 0xe0000000 && 0x20000000 - len < addr) { (and more)

However, if you search with
git-grep -n " \(&&\|||\)${s}0x\([A-Z0-9]*\|[a-z0-9]*\)$s\()\|&&\|||\)" you won't get these false positives, but you do get the one I fixed.

Also there is the same logic flipped (though it did not give matches at this time):
git-grep -n " \(&&\|||\|(\)${s}0x\([A-Z0-9]*\|[a-z0-9]*\)$s\(&&\|||\)"

so i'd propose to change that to
--
Catch boolean tests with hexadecimals, based on suggestion by Joe Perches

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

---

scripts/checkpatch.pl +6 -0
index 579f50f..8ac7c7b 100755
@@ -1337,6 +1337,12 @@ sub process {
 			}
 		}
 
+# Check for bitwise tests written as boolean
+		if ($line =~ /(?:(?:\(|\&\&|\|\|)\s*0[xX]\s*(?:&&|\|\|)|
+				(?:\&\&|\|\|)\s*0[xX]\s*(?:\)|&&|\|\|))/) {
+			WARN("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr);
+		}
+
 # if and else should not have general statements after it
 		if ($line =~ /^.\s*(?:}\s*)?else\b(.*)/ &&
 		    $1 !~ /^\s*(?:\sif|{|\\|$)/) {
maintainer
#3 lore raw

scripts/checkpatch.pl sub process { · line 1345

Quoted 9 lines · Roel Kluin

when you use git-grep -n "\(&&\|||\)${s}0x\([A-Z0-9]*\|[a-z0-9]*\)", (with s="[[:space:]]*") there will be false positives [] so i'd propose to change that to

scripts/checkpatch.pl +6 -0
+# Check for bitwise tests written as boolean
+		if ($line =~ /(?:(?:\(|\&\&|\|\|)\s*0[xX]\s*(?:&&|\|\|)|
+				(?:\&\&|\|\|)\s*0[xX]\s*(?:\)|&&|\|\|))/) {
+			WARN("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr);
+		}
+

All 13 false positives are "hex_constant logical_test variable". I think that Linus would say that they are all poor style.

Still, fine by me. cheers, Joe

#4 lore raw
In reply to Roel Kluin
Quoted 65 lines · Roel Kluin
Quoted 17 lines · Joe Perches
Quoted 7 lines · Roel Kluin
Signed-off-by Roel Kluin <[email protected]>

---

arch/powerpc/boot/4xx.c +0 -0
index ebf9e21..dcfb459 100644

arch/powerpc/boot/4xx.c void ibm4xx_denali_fixup_memsize(void) · line 109

Quoted 7 lines
arch/powerpc/boot/4xx.c +1 -1
@@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
 	val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
 	cs = 0;
 	while (val) {
-		if (val && 0x1)
+		if (val & 0x1)
 			cs++;
 		val = val >> 1;

I think this pattern should be added to checkpatch

I agree but...

Quoted 7 lines
Signed-off-by Joe Perches <[email protected]>
scripts/checkpatch.pl +0 -0
index 579f50f..147e573 100755

scripts/checkpatch.pl sub process { · line 1344

Quoted 8 lines
scripts/checkpatch.pl +5 -0
@@ -1337,6 +1337,11 @@ sub process {
 			}
 		}
 
+# Check for bitwise tests written as boolean
+		if ($line =~ /\&\&\s*0[xX]/) {
+			WARN("boolean test with hexadecimal, perhaps just 1 \&?\n" . $herecurr);
+		}
+

when you use git-grep -n "\(&&\|||\)${s}0x\([A-Z0-9]*\|[a-z0-9]*\)", (with s="[[:space:]]*") there will be false positives like

if (relocation < -0x20000 || 0x1fffc < relocation)
if (ARCH_SUN4C_SUN4 && addr < 0xe0000000 && 0x20000000 - len < addr) { (and more)

However, if you search with
git-grep -n " \(&&\|||\)${s}0x\([A-Z0-9]*\|[a-z0-9]*\)$s\()\|&&\|||\)" you won't get these false positives, but you do get the one I fixed.

Also there is the same logic flipped (though it did not give matches at this time):
git-grep -n " \(&&\|||\|(\)${s}0x\([A-Z0-9]*\|[a-z0-9]*\)$s\(&&\|||\)"

so i'd propose to change that to
--
Catch boolean tests with hexadecimals, based on suggestion by Joe Perches

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

---

scripts/checkpatch.pl +0 -0
index 579f50f..8ac7c7b 100755
@@ -1337,6 +1337,12 @@ sub process {
 			}
 		}

scripts/checkpatch.pl sub process { · line 1348

Quoted 10 lines
scripts/checkpatch.pl +6 -0
 
+# Check for bitwise tests written as boolean
+		if ($line =~ /(?:(?:\(|\&\&|\|\|)\s*0[xX]\s*(?:&&|\|\|)|
+				(?:\&\&|\|\|)\s*0[xX]\s*(?:\)|&&|\|\|))/) {
+			WARN("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr);
+		}
+
 # if and else should not have general statements after it
 		if ($line =~ /^.\s*(?:}\s*)?else\b(.*)/ &&
 		    $1 !~ /^\s*(?:\sif|{|\\|$)/) {

That one doesn't even match the original example. Seems to be missing some number matching. The concept seems sound though. Basically looking for numbers which are definatly adjacent to a boolean or a brace on both sides.

/me goes play with some examples to see if the falsies are better than the reports.

-apw

author
#5 lore raw
Quoted 12 lines
Quoted 7 lines
Quoted 3 lines · Roel Kluin
Quoted 2 lines
(quoted diff) +1 -1
-		if (val && 0x1)
+		if (val & 0x1)
Quoted 2 lines · Joe Perches

I think this pattern should be added to checkpatch

(quoted diff) +1 -0
+		if ($line =~ /\&\&\s*0[xX]/) {
Quoted 3 lines · Roel Kluin

I agree but there will be false positives, i'd propose to change that to

(quoted diff) +2 -0
+		if ($line =~ /(?:(?:\(|\&\&|\|\|)\s*0[xX]\s*(?:&&|\|\|)|
+				(?:\&\&|\|\|)\s*0[xX]\s*(?:\)|&&|\|\|))/) {
Quoted 4 lines · Andy Whitcroft

That one doesn't even match the original example. Seems to be missing some number matching. The concept seems sound though. Basically looking for numbers which are definatly adjacent to a boolean or a brace on both sides.

You are right. here's a version that does work. It works the same as
git-grep -E "((\(|&&|\|\|)[[:space:]]*(0[xX][0-9a-fA-F]+|[0-9]+)[[:space:]]*(&&|\|\|)|(&&|\|\|)[[:space:]]*(0[xX][0-9a-fA-F]+|[0-9]+)[[:space:]]*(\)|&&|\|\|))"

---

scripts/checkpatch.pl +5 -0
index 579f50f..62276f7 100755
@@ -1337,6 +1337,11 @@ sub process {
 			}
 		}
 
+# Check for bitwise tests written as boolean
+		if ($line =~ /((\(|&&|\|\|)\s*(0[xX][0-9a-fA-F]+|[0-9]+)\s*(&&|\|\|)|(&&|\|\|)\s*(0[xX][0-9a-fA-F]+|[0-9]+)\s*(\)|&&|\|\|))/) {
+			WARN("boolean test with hexadecimal, perhaps just \'&\' or \'|\'?\n" . $herecurr);
+		}
+
 # if and else should not have general statements after it
 		if ($line =~ /^.\s*(?:}\s*)?else\b(.*)/ &&
 		    $1 !~ /^\s*(?:\sif|{|\\|$)/) {
#6 lore raw
In reply to Roel Kluin
Quoted 9 lines · On Wed, 23 Jan 2008 23:37:33 +0100 Roel Kluin <[email protected]> wrote:

logical/bitand typo

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

---

arch/powerpc/boot/4xx.c +0 -0
index ebf9e21..dcfb459 100644

arch/powerpc/boot/4xx.c void ibm4xx_denali_fixup_memsize(void) · line 110

Quoted 8 lines
arch/powerpc/boot/4xx.c +1 -1
@@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
 	val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
 	cs = 0;
 	while (val) {
-		if (val && 0x1)
+		if (val & 0x1)
 			cs++;
 		val = val >> 1;
 	}

Hm, good catch.

Stefan, have you had problems with this code at all?

josh

#7 lore raw
Quoted 23 lines · Josh Boyer

On Wed, 23 Jan 2008 23:37:33 +0100

Quoted 9 lines · Roel Kluin

logical/bitand typo

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

---

arch/powerpc/boot/4xx.c +0 -0
index ebf9e21..dcfb459 100644

arch/powerpc/boot/4xx.c void ibm4xx_denali_fixup_memsize(void) · line 110

Quoted 8 lines
arch/powerpc/boot/4xx.c +1 -1
@@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
 	val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
 	cs = 0;
 	while (val) {
-		if (val && 0x1)
+		if (val & 0x1)
 			cs++;
 		val = val >> 1;
 	}

Hm, good catch.

Quoted 1 line

Stefan, have you had problems with this code at all?

I'm not using the bootwrapper on a 4xx with Denali core.

Best regards,
Stefan

#8 lore raw
In reply to Josh Boyer
Quoted 14 lines · Josh Boyer
Quoted 11 lines · On Wed, 23 Jan 2008 23:37:33 +0100 Roel Kluin <[email protected]> wrote:

logical/bitand typo

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

---

arch/powerpc/boot/4xx.c +0 -0
index ebf9e21..dcfb459 100644
@@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
 	val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
 	cs = 0;
Quoted 8 lines

arch/powerpc/boot/4xx.c void ibm4xx_denali_fixup_memsize(void) · line 110

Quoted 6 lines
arch/powerpc/boot/4xx.c +1 -1
 	while (val) {
-		if (val && 0x1)
+		if (val & 0x1)
 			cs++;
 		val = val >> 1;
 	}

Hm, good catch.

Yes, thanks!

Sequoia and Rainier are currently the only boards using denali controller and have both chipselects enabled (val = 0x3). In this case the problem doesn't show up and we still get the right cs value.
Thanks,
Valentine.

Quoted 8 lines

Stefan, have you had problems with this code at all?

josh
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev