* [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c @ 2008-01-23 22:37 Roel Kluin 2008-01-23 23:05 ` Joe Perches 2008-01-24 1:17 ` Josh Boyer 0 siblings, 2 replies; 9+ messages in thread From: Roel Kluin @ 2008-01-23 22:37 UTC (permalink / raw) To: jwboyer, lkml; +Cc: linuxppc-dev logical/bitand typo Signed-off-by: Roel Kluin <12o3l@tiscali.nl> --- diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c index ebf9e21..dcfb459 100644 --- a/arch/powerpc/boot/4xx.c +++ b/arch/powerpc/boot/4xx.c @@ -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; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c 2008-01-23 22:37 [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c Roel Kluin @ 2008-01-23 23:05 ` Joe Perches 2008-01-24 0:18 ` Roel Kluin 2008-01-24 1:17 ` Josh Boyer 1 sibling, 1 reply; 9+ messages in thread From: Joe Perches @ 2008-01-23 23:05 UTC (permalink / raw) To: Roel Kluin, Andy Whitcroft; +Cc: linuxppc-dev, lkml On Wed, 2008-01-23 at 23:37 +0100, Roel Kluin wrote: > Signed-off-by: Roel Kluin <12o3l@tiscali.nl> > --- > diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c > index ebf9e21..dcfb459 100644 > --- a/arch/powerpc/boot/4xx.c > +++ b/arch/powerpc/boot/4xx.c > @@ -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 <joe@perches.com> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 579f50f..147e573 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -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|{|\\|$)/) { ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c 2008-01-23 23:05 ` Joe Perches @ 2008-01-24 0:18 ` Roel Kluin 2008-01-24 0:39 ` Joe Perches 2008-01-29 10:55 ` Andy Whitcroft 0 siblings, 2 replies; 9+ messages in thread From: Roel Kluin @ 2008-01-24 0:18 UTC (permalink / raw) To: Joe Perches; +Cc: linuxppc-dev, lkml Joe Perches wrote: > On Wed, 2008-01-23 at 23:37 +0100, Roel Kluin wrote: >> Signed-off-by: Roel Kluin <12o3l@tiscali.nl> >> --- >> diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c >> index ebf9e21..dcfb459 100644 >> --- a/arch/powerpc/boot/4xx.c >> +++ b/arch/powerpc/boot/4xx.c >> @@ -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... > Signed-off-by: Joe Perches <joe@perches.com> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 579f50f..147e573 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -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 <12o3l@tiscali.nl> --- diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 579f50f..8ac7c7b 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -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|{|\\|$)/) { ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c 2008-01-24 0:18 ` Roel Kluin @ 2008-01-24 0:39 ` Joe Perches 2008-01-29 10:55 ` Andy Whitcroft 1 sibling, 0 replies; 9+ messages in thread From: Joe Perches @ 2008-01-24 0:39 UTC (permalink / raw) To: Roel Kluin; +Cc: linuxppc-dev, lkml On Thu, 2008-01-24 at 01:18 +0100, Roel Kluin wrote: > 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 > +# 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c 2008-01-24 0:18 ` Roel Kluin 2008-01-24 0:39 ` Joe Perches @ 2008-01-29 10:55 ` Andy Whitcroft 2008-01-29 13:34 ` Roel Kluin 1 sibling, 1 reply; 9+ messages in thread From: Andy Whitcroft @ 2008-01-29 10:55 UTC (permalink / raw) To: Roel Kluin; +Cc: Joe Perches, linuxppc-dev, lkml On Thu, Jan 24, 2008 at 01:18:48AM +0100, Roel Kluin wrote: > Joe Perches wrote: > > On Wed, 2008-01-23 at 23:37 +0100, Roel Kluin wrote: > >> Signed-off-by: Roel Kluin <12o3l@tiscali.nl> > >> --- > >> diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c > >> index ebf9e21..dcfb459 100644 > >> --- a/arch/powerpc/boot/4xx.c > >> +++ b/arch/powerpc/boot/4xx.c > >> @@ -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... > > > Signed-off-by: Joe Perches <joe@perches.com> > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 579f50f..147e573 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -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 <12o3l@tiscali.nl> > --- > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 579f50f..8ac7c7b 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -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|{|\\|$)/) { 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c 2008-01-29 10:55 ` Andy Whitcroft @ 2008-01-29 13:34 ` Roel Kluin 0 siblings, 0 replies; 9+ messages in thread From: Roel Kluin @ 2008-01-29 13:34 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Joe Perches, linuxppc-dev, lkml >>> On Wed, 2008-01-23 at 23:37 +0100, Roel Kluin wrote: >>>> - if (val && 0x1) >>>> + if (val & 0x1) >> Joe Perches wrote: >>> I think this pattern should be added to checkpatch >>> + if ($line =~ /\&\&\s*0[xX]/) { > On Thu, Jan 24, 2008 at 01:18:48AM +0100, Roel Kluin wrote: >> I agree but there will be false positives, i'd propose to change that to >> + if ($line =~ /(?:(?:\(|\&\&|\|\|)\s*0[xX]\s*(?:&&|\|\|)| >> + (?:\&\&|\|\|)\s*0[xX]\s*(?:\)|&&|\|\|))/) { Andy Whitcroft wrote: > 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:]]*(\)|&&|\|\|))" --- diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 579f50f..62276f7 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -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|{|\\|$)/) { ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c 2008-01-23 22:37 [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c Roel Kluin 2008-01-23 23:05 ` Joe Perches @ 2008-01-24 1:17 ` Josh Boyer 2008-01-26 13:37 ` Stefan Roese 2008-01-29 14:22 ` Valentine Barshak 1 sibling, 2 replies; 9+ messages in thread From: Josh Boyer @ 2008-01-24 1:17 UTC (permalink / raw) To: Roel Kluin; +Cc: linuxppc-dev, sr, lkml On Wed, 23 Jan 2008 23:37:33 +0100 Roel Kluin <12o3l@tiscali.nl> wrote: > logical/bitand typo > > Signed-off-by: Roel Kluin <12o3l@tiscali.nl> > --- > diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c > index ebf9e21..dcfb459 100644 > --- a/arch/powerpc/boot/4xx.c > +++ b/arch/powerpc/boot/4xx.c > @@ -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c 2008-01-24 1:17 ` Josh Boyer @ 2008-01-26 13:37 ` Stefan Roese 2008-01-29 14:22 ` Valentine Barshak 1 sibling, 0 replies; 9+ messages in thread From: Stefan Roese @ 2008-01-26 13:37 UTC (permalink / raw) To: Josh Boyer; +Cc: linuxppc-dev, Roel Kluin, lkml On Thursday 24 January 2008, Josh Boyer wrote: > On Wed, 23 Jan 2008 23:37:33 +0100 > > Roel Kluin <12o3l@tiscali.nl> wrote: > > logical/bitand typo > > > > Signed-off-by: Roel Kluin <12o3l@tiscali.nl> > > --- > > diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c > > index ebf9e21..dcfb459 100644 > > --- a/arch/powerpc/boot/4xx.c > > +++ b/arch/powerpc/boot/4xx.c > > @@ -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? I'm not using the bootwrapper on a 4xx with Denali core. Best regards, Stefan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c 2008-01-24 1:17 ` Josh Boyer 2008-01-26 13:37 ` Stefan Roese @ 2008-01-29 14:22 ` Valentine Barshak 1 sibling, 0 replies; 9+ messages in thread From: Valentine Barshak @ 2008-01-29 14:22 UTC (permalink / raw) To: Josh Boyer; +Cc: linuxppc-dev, sr, Roel Kluin, lkml Josh Boyer wrote: > On Wed, 23 Jan 2008 23:37:33 +0100 > Roel Kluin <12o3l@tiscali.nl> wrote: > >> logical/bitand typo >> >> Signed-off-by: Roel Kluin <12o3l@tiscali.nl> >> --- >> diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c >> index ebf9e21..dcfb459 100644 >> --- a/arch/powerpc/boot/4xx.c >> +++ b/arch/powerpc/boot/4xx.c >> @@ -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. 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. > > Stefan, have you had problems with this code at all? > > josh > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@ozlabs.org > https://ozlabs.org/mailman/listinfo/linuxppc-dev ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-01-29 14:24 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-01-23 22:37 [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c Roel Kluin 2008-01-23 23:05 ` Joe Perches 2008-01-24 0:18 ` Roel Kluin 2008-01-24 0:39 ` Joe Perches 2008-01-29 10:55 ` Andy Whitcroft 2008-01-29 13:34 ` Roel Kluin 2008-01-24 1:17 ` Josh Boyer 2008-01-26 13:37 ` Stefan Roese 2008-01-29 14:22 ` Valentine Barshak
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).