[x86] is checkpatch.pl broken

[x86] is checkpatch.pl broken

[Cyrill Gorcunov]

Hi list,

by doing cleanup of arch/x86/boot/*.[ch] i found that
checkpatch does ignore obvious things. For example,
run it over edd.c showed only one warning:

---
cyrill@cvg linux-2.6.git $ scripts/checkpatch.pl --file arch/x86/boot/edd.c 
WARNING: externs should be avoided in .c files
#45: FILE: x86/boot/edd.c:45:
+       extern char _end[];

total: 0 errors, 1 warnings, 167 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
---

but on line 53 we have:

mbr_base = (buf_base+sector_size-1) & ~(sector_size-1);

so checkpatch should at least worn me about missed space
btw math operators. Am I wrong?


--
BTW, is there someone who is already involved in a such
cleanup to eliminate double effort?

			- Cyrill -

Re: [x86] is checkpatch.pl broken

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> Hi list,
> 
> by doing cleanup of arch/x86/boot/*.[ch] i found that
> checkpatch does ignore obvious things. For example,
> run it over edd.c showed only one warning:

I'm generally skeptical to the kind of "cleanups" that you seem to be 
referring to.  More often then not they reduce legibility instead of the 
opposite.

> ---
> cyrill@cvg linux-2.6.git $ scripts/checkpatch.pl --file arch/x86/boot/edd.c 
> WARNING: externs should be avoided in .c files
> #45: FILE: x86/boot/edd.c:45:
> +       extern char _end[];
> 
> total: 0 errors, 1 warnings, 167 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> ---
> 
> but on line 53 we have:
> 
> mbr_base = (buf_base+sector_size-1) & ~(sector_size-1);
> 
> so checkpatch should at least worn me about missed space
> btw math operators. Am I wrong?

If checkpatch considered that line to be a problem, I would consider 
checkpatch to be utterly broken.  That line is perfectly legible, and 
padding in a bunch of spaces would make it LESS so, especially since it 
would have to be split between lines.

	-hpa

Re: [x86] is checkpatch.pl broken

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> Hi list,
> 
> by doing cleanup of arch/x86/boot/*.[ch] i found that
> checkpatch does ignore obvious things. For example,
> run it over edd.c showed only one warning:
> 
> BTW, is there someone who is already involved in a such
> cleanup to eliminate double effort?
> 

BTW, it's more than a wee bit rude of you to Cc: a bunch of people but 
not the listed maintainer of the piece of code you're claiming to be 
cleaning up.

	-hpa

Re: [x86] is checkpatch.pl broken

[Cyrill Gorcunov]

On Dec 26, 2007 2:48 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> Cyrill Gorcunov wrote:
> > Hi list,
> >
> > by doing cleanup of arch/x86/boot/*.[ch] i found that
> > checkpatch does ignore obvious things. For example,
> > run it over edd.c showed only one warning:
> >
> > BTW, is there someone who is already involved in a such
> > cleanup to eliminate double effort?
> >
>
> BTW, it's more than a wee bit rude of you to Cc: a bunch of people but
> not the listed maintainer of the piece of code you're claiming to be
> cleaning up.
>
>         -hpa
>
It's a quite true, sorry for this and thanks for the note. And Peter,
the line I marked
is not to be splitted even having additional spaces over  math operators. Look

orig:
mbr_base = (buf_base+sector_size-1) & ~(sector_size-1);
new (could be):
mbr_base = (buf_base + sector_size - 1) & ~(sector_size - 1);

Is a new version that bad?
Anyway, as only I've something really done, i'll send it to the list
so you/mainteiner could choose which version is to be in kernel ;)

Re: [x86] is checkpatch.pl broken

[H. Peter Anvin]

Cyrill Gorcunov wrote:
>
>>
> It's a quite true, sorry for this and thanks for the note. And Peter,
> the line I marked
> is not to be splitted even having additional spaces over  math operators. Look
> 
> orig:
> mbr_base = (buf_base+sector_size-1) & ~(sector_size-1);
> new (could be):
> mbr_base = (buf_base + sector_size - 1) & ~(sector_size - 1);
> 
> Is a new version that bad?

Certainly isn't any better.

	-hpa

Re: [x86] is checkpatch.pl broken

[Cyrill Gorcunov]

[H. Peter Anvin - Wed, Dec 26, 2007 at 09:44:18AM -0800]
> Cyrill Gorcunov wrote:
>>
>>>
>> It's a quite true, sorry for this and thanks for the note. And Peter,
>> the line I marked
>> is not to be splitted even having additional spaces over  math operators. Look
>>
>> orig:
>> mbr_base = (buf_base+sector_size-1) & ~(sector_size-1);
>> new (could be):
>> mbr_base = (buf_base + sector_size - 1) & ~(sector_size - 1);
>>
>> Is a new version that bad?
>
> Certainly isn't any better.
>
> 	-hpa
>

ok, i'm leaving this section then, thanks for comments Peter.

			- Cyrill -

Re: [x86] is checkpatch.pl broken

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> orig:
> mbr_base = (buf_base+sector_size-1) & ~(sector_size-1);
> new (could be):
> mbr_base = (buf_base + sector_size - 1) & ~(sector_size - 1);
> 
> Is a new version that bad?

it's certainly acceptable as newly introduced code but only borderline 
better than the original code. I'd suggest to stick to the problem areas 
that checkpatch.pl complains about at the moment - we have really 
obvious bad looking pieces of code that checkpatch.pl reports, and going 
after the borderline cases will only result in coding-style lawyering 
and flamewars, not any genuine increase in code quality ;-)

for example:

  arch/x86/kernel/bootflag.c:

  total: 19 errors, 2 warnings, 98 lines checked

or:

  arch/x86/kernel/apm_32.c:

  total: 56 errors, 31 warnings, 2402 lines checked

and once we have nothing but the borderline cases and if we get really 
bored we can start coding style flamewars ;-)

	Ingo

Re: [x86] is checkpatch.pl broken

[Cyrill Gorcunov]

[Ingo Molnar - Sun, Dec 30, 2007 at 06:22:50PM +0100]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > orig:
| > mbr_base = (buf_base+sector_size-1) & ~(sector_size-1);
| > new (could be):
| > mbr_base = (buf_base + sector_size - 1) & ~(sector_size - 1);
| > 
| > Is a new version that bad?
| 
| it's certainly acceptable as newly introduced code but only borderline 
| better than the original code. I'd suggest to stick to the problem areas 
| that checkpatch.pl complains about at the moment - we have really 
| obvious bad looking pieces of code that checkpatch.pl reports, and going 
| after the borderline cases will only result in coding-style lawyering 
| and flamewars, not any genuine increase in code quality ;-)
| 
| for example:
| 
|   arch/x86/kernel/bootflag.c:
| 
|   total: 19 errors, 2 warnings, 98 lines checked
| 
| or:
| 
|   arch/x86/kernel/apm_32.c:
| 
|   total: 56 errors, 31 warnings, 2402 lines checked
| 
| and once we have nothing but the borderline cases and if we get really 
| bored we can start coding style flamewars ;-)
| 
| 	Ingo
| 

Thanks Ingo, you're quite right! Next time i'll appear in list with real
(and hope usefull) patch ;)

		Cyrill

Re: [x86] is checkpatch.pl broken

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> 
> Thanks Ingo, you're quite right! Next time i'll appear in list with real
> (and hope usefull) patch ;)
> 

Wonderful!  I also *really* have to apologize for my short fuse earlier, 
it was uncalled for.

	-hpa

Re: [x86] is checkpatch.pl broken

[Cyrill Gorcunov]

[Ingo Molnar - Sun, Dec 30, 2007 at 06:22:50PM +0100]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > orig:
| > mbr_base = (buf_base+sector_size-1) & ~(sector_size-1);
| > new (could be):
| > mbr_base = (buf_base + sector_size - 1) & ~(sector_size - 1);
| > 
| > Is a new version that bad?
| 
| it's certainly acceptable as newly introduced code but only borderline 
| better than the original code. I'd suggest to stick to the problem areas 
| that checkpatch.pl complains about at the moment - we have really 
| obvious bad looking pieces of code that checkpatch.pl reports, and going 
| after the borderline cases will only result in coding-style lawyering 
| and flamewars, not any genuine increase in code quality ;-)
| 
| for example:
| 
|   arch/x86/kernel/bootflag.c:
| 
|   total: 19 errors, 2 warnings, 98 lines checked
| 
| or:
| 
|   arch/x86/kernel/apm_32.c:
| 
|   total: 56 errors, 31 warnings, 2402 lines checked
| 
| and once we have nothing but the borderline cases and if we get really 
| bored we can start coding style flamewars ;-)
| 
| 	Ingo
| 

Hi Ingo,
here is a first for x86 tree

		- Cyrill -
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [x86] coding style cleanup for kernel/bootflag.c

This patch eliminates checkpatch.pl complains
on bootflag.c

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

 arch/x86/kernel/bootflag.c |   40 +++++++++++++++++++++++-----------------
 1 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
index 0b98605..1697e49 100644
--- a/arch/x86/kernel/bootflag.c
+++ b/arch/x86/kernel/bootflag.c
@@ -24,30 +24,29 @@
 
 int sbf_port __initdata = -1;	/* set via acpi_boot_init() */
 
-
 static int __init parity(u8 v)
 {
 	int x = 0;
 	int i;
-	
-	for(i=0;i<8;i++)
-	{
-		x^=(v&1);
-		v>>=1;
+
+	for (i = 0; i < 8; i++) {
+		x ^= (v & 1);
+		v >>= 1;
 	}
+
 	return x;
 }
 
 static void __init sbf_write(u8 v)
 {
 	unsigned long flags;
-	if(sbf_port != -1)
-	{
+	if (sbf_port != -1) {
 		v &= ~SBF_PARITY;
-		if(!parity(v))
-			v|=SBF_PARITY;
+		if (!parity(v))
+			v |= SBF_PARITY;
 
-		printk(KERN_INFO "Simple Boot Flag at 0x%x set to 0x%x\n", sbf_port, v);
+		printk(KERN_INFO "Simple Boot Flag at 0x%x set to 0x%x\n",
+			sbf_port, v);
 
 		spin_lock_irqsave(&rtc_lock, flags);
 		CMOS_WRITE(v, sbf_port);
@@ -59,31 +58,38 @@ static u8 __init sbf_read(void)
 {
 	u8 v;
 	unsigned long flags;
-	if(sbf_port == -1)
+
+	if (sbf_port == -1)
 		return 0;
+
 	spin_lock_irqsave(&rtc_lock, flags);
 	v = CMOS_READ(sbf_port);
 	spin_unlock_irqrestore(&rtc_lock, flags);
+
 	return v;
 }
 
 static int __init sbf_value_valid(u8 v)
 {
-	if(v&SBF_RESERVED)		/* Reserved bits */
+	if (v & SBF_RESERVED)		/* Reserved bits */
 		return 0;
-	if(!parity(v))
+	if (!parity(v))
 		return 0;
+
 	return 1;
 }
 
 static int __init sbf_init(void)
 {
 	u8 v;
-	if(sbf_port == -1)
+
+	if (sbf_port == -1)
 		return 0;
+
 	v = sbf_read();
-	if(!sbf_value_valid(v))
-		printk(KERN_WARNING "Simple Boot Flag value 0x%x read from CMOS RAM was invalid\n",v);
+	if (!sbf_value_valid(v))
+		printk(KERN_WARNING "Simple Boot Flag value 0x%x read from "
+			"CMOS RAM was invalid\n", v);
 
 	v &= ~SBF_RESERVED;
 	v &= ~SBF_BOOTING;

Re: [x86] is checkpatch.pl broken

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> This patch eliminates checkpatch.pl complains on bootflag.c

thanks, applied this to x86.git, to the v2.6.25 queue. See the finalized 
patch below. (I added two more small cleanups that checkpatch did not 
warn about but which were obvious)

	Ingo

----------->
Subject: x86: coding style cleanup for kernel/bootflag.c
From: Cyrill Gorcunov <gorcunov@gmail.com>

This patch eliminates checkpatch.pl complaints on bootflag.c

No code changed:

   text    data     bss     dec     hex filename
    321       8       0     329     149 bootflag.o.before
    321       8       0     329     149 bootflag.o.after

   md5:
      9c1b474bcf25ddc1724a29c19880043f  bootflag.o.before.asm
      9c1b474bcf25ddc1724a29c19880043f  bootflag.o.after.asm

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/bootflag.c |   50 ++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

Index: linux-x86.q/arch/x86/kernel/bootflag.c
===================================================================
--- linux-x86.q.orig/arch/x86/kernel/bootflag.c
+++ linux-x86.q/arch/x86/kernel/bootflag.c
@@ -1,8 +1,6 @@
 /*
  *	Implement 'Simple Boot Flag Specification 2.0'
  */
-
-
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -14,40 +12,38 @@
 
 #include <linux/mc146818rtc.h>
 
-
 #define SBF_RESERVED (0x78)
 #define SBF_PNPOS    (1<<0)
 #define SBF_BOOTING  (1<<1)
 #define SBF_DIAG     (1<<2)
 #define SBF_PARITY   (1<<7)
 
-
 int sbf_port __initdata = -1;	/* set via acpi_boot_init() */
 
-
 static int __init parity(u8 v)
 {
 	int x = 0;
 	int i;
-	
-	for(i=0;i<8;i++)
-	{
-		x^=(v&1);
-		v>>=1;
+
+	for (i = 0; i < 8; i++) {
+		x ^= (v & 1);
+		v >>= 1;
 	}
+
 	return x;
 }
 
 static void __init sbf_write(u8 v)
 {
 	unsigned long flags;
-	if(sbf_port != -1)
-	{
+
+	if (sbf_port != -1) {
 		v &= ~SBF_PARITY;
-		if(!parity(v))
-			v|=SBF_PARITY;
+		if (!parity(v))
+			v |= SBF_PARITY;
 
-		printk(KERN_INFO "Simple Boot Flag at 0x%x set to 0x%x\n", sbf_port, v);
+		printk(KERN_INFO "Simple Boot Flag at 0x%x set to 0x%x\n",
+			sbf_port, v);
 
 		spin_lock_irqsave(&rtc_lock, flags);
 		CMOS_WRITE(v, sbf_port);
@@ -57,33 +53,41 @@ static void __init sbf_write(u8 v)
 
 static u8 __init sbf_read(void)
 {
-	u8 v;
 	unsigned long flags;
-	if(sbf_port == -1)
+	u8 v;
+
+	if (sbf_port == -1)
 		return 0;
+
 	spin_lock_irqsave(&rtc_lock, flags);
 	v = CMOS_READ(sbf_port);
 	spin_unlock_irqrestore(&rtc_lock, flags);
+
 	return v;
 }
 
 static int __init sbf_value_valid(u8 v)
 {
-	if(v&SBF_RESERVED)		/* Reserved bits */
+	if (v & SBF_RESERVED)		/* Reserved bits */
 		return 0;
-	if(!parity(v))
+	if (!parity(v))
 		return 0;
+
 	return 1;
 }
 
 static int __init sbf_init(void)
 {
 	u8 v;
-	if(sbf_port == -1)
+
+	if (sbf_port == -1)
 		return 0;
+
 	v = sbf_read();
-	if(!sbf_value_valid(v))
-		printk(KERN_WARNING "Simple Boot Flag value 0x%x read from CMOS RAM was invalid\n",v);
+	if (!sbf_value_valid(v)) {
+		printk(KERN_WARNING "Simple Boot Flag value 0x%x read from "
+			"CMOS RAM was invalid\n", v);
+	}
 
 	v &= ~SBF_RESERVED;
 	v &= ~SBF_BOOTING;
@@ -92,7 +96,7 @@ static int __init sbf_init(void)
 	v |= SBF_PNPOS;
 #endif
 	sbf_write(v);
+
 	return 0;
 }
-
 module_init(sbf_init);

Re: [x86] is checkpatch.pl broken

[Cyrill Gorcunov]

[H. Peter Anvin - Sun, Dec 30, 2007 at 12:27:15PM -0800]
> Cyrill Gorcunov wrote:
>>
>> Thanks Ingo, you're quite right! Next time i'll appear in list with real
>> (and hope usefull) patch ;)
>>
>
> Wonderful!  I also *really* have to apologize for my short fuse earlier, it 
> was uncalled for.
>
> 	-hpa
>

ok ;) but any comment is *worth* for me.
Thanks.

		- Cyrill -