[PATCH 0/9] Kconfig: cleanup s390 v2.

[PATCH 0/9] Kconfig: cleanup s390 v2.

[Martin Schwidefsky]

Greetings,
I've added the results of the review to the Kconfig cleanup patches
for s390. Patch #2 has been split, one half has all the HAS_IOMEM
depends lines the other the remaining !S390 depends lines.

Andrew: I plan to add patches 1-5 to the for-andrew branch of the
git390 repository if that is fine with you. The only thing that will
be missing in the tree is the patch that disables wireless for s390.
The code does compile but without hardware it is mute to have the
config options. I'll wait until the git-wireless.patch is upstream.
Patches 7-9 depend on patches found in -mm.

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Arnd Bergmann]

On Monday 23 April 2007, Martin Schwidefsky wrote:
> I've added the results of the review to the Kconfig cleanup patches
> for s390. Patch #2 has been split, one half has all the HAS_IOMEM
> depends lines the other the remaining !S390 depends lines.
> 

They all look good to me now

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Andrew Morton]

On Mon, 23 Apr 2007 16:11:23 +0200
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> Greetings,
> I've added the results of the review to the Kconfig cleanup patches
> for s390. Patch #2 has been split, one half has all the HAS_IOMEM
> depends lines the other the remaining !S390 depends lines.
> 
> Andrew: I plan to add patches 1-5 to the for-andrew branch of the
> git390 repository if that is fine with you. The only thing that will
> be missing in the tree is the patch that disables wireless for s390.
> The code does compile but without hardware it is mute to have the
> config options. I'll wait until the git-wireless.patch is upstream.
> Patches 7-9 depend on patches found in -mm.
> 

umm, OK.  If it's Ok I think I'll duck it for now: -mm is full.

Over-full, really: I've been working basically continuously since Friday
getting the current dungpile to compile and boot, and it's still miles away
from that.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Martin Schwidefsky]

On Mon, 2007-04-23 at 10:45 -0700, Andrew Morton wrote:
> > Andrew: I plan to add patches 1-5 to the for-andrew branch of the
> > git390 repository if that is fine with you. The only thing that will
> > be missing in the tree is the patch that disables wireless for s390.
> > The code does compile but without hardware it is mute to have the
> > config options. I'll wait until the git-wireless.patch is upstream.
> > Patches 7-9 depend on patches found in -mm.
> > 
> 
> umm, OK.  If it's Ok I think I'll duck it for now: -mm is full.
> 
> Over-full, really: I've been working basically continuously since Friday
> getting the current dungpile to compile and boot, and it's still miles away
> from that.

I understand. I'll wait until -mm is a little bit smaller again. It is
just that someday I want to finish with the Kconfig cleanup, it has been
sitting on my harddriver for ages now.

-- 
blue skies,              IBM Deutschland Entwicklung GmbH
   Martin                Vorsitzender des Aufsichtsrats: Johann Weihen
                         Geschäftsführung: Herbert Kircher
Martin Schwidefsky       Sitz der Gesellschaft: Böblingen
Linux on zSeries         Registergericht: Amtsgericht Stuttgart,
   Development           HRB 243294

"Reality continues to ruin my life." - Calvin.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Randy Dunlap]

On Mon, 23 Apr 2007 10:45:34 -0700 Andrew Morton wrote:

> On Mon, 23 Apr 2007 16:11:23 +0200
> Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
> 
> > Greetings,
> > I've added the results of the review to the Kconfig cleanup patches
> > for s390. Patch #2 has been split, one half has all the HAS_IOMEM
> > depends lines the other the remaining !S390 depends lines.
> > 
> > Andrew: I plan to add patches 1-5 to the for-andrew branch of the
> > git390 repository if that is fine with you. The only thing that will
> > be missing in the tree is the patch that disables wireless for s390.
> > The code does compile but without hardware it is mute to have the
> > config options. I'll wait until the git-wireless.patch is upstream.
> > Patches 7-9 depend on patches found in -mm.
> > 
> 
> umm, OK.  If it's Ok I think I'll duck it for now: -mm is full.
> 
> Over-full, really: I've been working basically continuously since Friday
> getting the current dungpile to compile and boot, and it's still miles away
> from that.

and I continue to be concerned about the amount of patch reviews
compared to new patch material overall (not just s390).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Andrew Morton]

On Wed, 25 Apr 2007 11:21:33 -0700
Randy Dunlap <randy.dunlap@oracle.com> wrote:

> On Mon, 23 Apr 2007 10:45:34 -0700 Andrew Morton wrote:
> 
> > On Mon, 23 Apr 2007 16:11:23 +0200
> > Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
> > 
> > > Greetings,
> > > I've added the results of the review to the Kconfig cleanup patches
> > > for s390. Patch #2 has been split, one half has all the HAS_IOMEM
> > > depends lines the other the remaining !S390 depends lines.
> > > 
> > > Andrew: I plan to add patches 1-5 to the for-andrew branch of the
> > > git390 repository if that is fine with you. The only thing that will
> > > be missing in the tree is the patch that disables wireless for s390.
> > > The code does compile but without hardware it is mute to have the
> > > config options. I'll wait until the git-wireless.patch is upstream.
> > > Patches 7-9 depend on patches found in -mm.
> > > 
> > 
> > umm, OK.  If it's Ok I think I'll duck it for now: -mm is full.
> > 
> > Over-full, really: I've been working basically continuously since Friday
> > getting the current dungpile to compile and boot, and it's still miles away
> > from that.
> 
> and I continue to be concerned about the amount of patch reviews
> compared to new patch material overall (not just s390).
> 

yes.  I'm increasingly reluctant to merge things which have had no visible
review from any third party.  Nowadays I'll shove such patches into a
pending folder and will wait a day or three to see if anyone has any
feedback.  If they don't I have to either ignore the patches or review them
myself.

I expect (and hope) that more formal processes will come about here.  Perhaps
up to it-won't-be-merged-without-a-Reviewed-by:.


But that only applies to things which I merge.  There's heaps of stuff
coming in via the git trees which is obviously inadequately reviewed - look
at all the instances of open-coded kernel_thread() which were merged after
the kthread() API was introduced, for example.


And other basic stuff like "use mutexes, not semaphores":

box:/usr/src/25> grep '^+.*[    ]down[  ]*[(]' patches/git-*.patch | wc -l
32



Ever wonder where all those whitespace bugs are coming from?

box:/usr/src/25> grep '^+.*[    ]if[(]' patches/git-*.patch | wc -l
265
box:/usr/src/25> grep '^+.*[    ]while[(]' patches/git-*.patch | wc -l  
35


Code which use spaces where it should be using tabs?

box:/usr/src/25> grep '^+        ' patches/git-*.patch | wc -l
1346


Heaven knows how many more serious problems are being snuck into the tree
via this route.

What do we do?

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Andrew Morton]

On Wed, 25 Apr 2007 14:30:11 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> But that only applies to things which I merge.  There's heaps of stuff
> coming in via the git trees which is obviously inadequately reviewed - look
> at all the instances of open-coded kernel_thread() which were merged after
> the kthread() API was introduced, for example.
> 
> 
> And other basic stuff like "use mutexes, not semaphores":
> 
> box:/usr/src/25> grep '^+.*[    ]down[  ]*[(]' patches/git-*.patch | wc -l
> 32
> 
> 
> 
> Ever wonder where all those whitespace bugs are coming from?
> 
> box:/usr/src/25> grep '^+.*[    ]if[(]' patches/git-*.patch | wc -l
> 265
> box:/usr/src/25> grep '^+.*[    ]while[(]' patches/git-*.patch | wc -l  
> 35
> 
> 
> Code which use spaces where it should be using tabs?
> 
> box:/usr/src/25> grep '^+        ' patches/git-*.patch | wc -l
> 1346
> 

It would be neat if someone could create and maintain a new
scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
about newly-added code (and only newly-added code) which has busted
whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.

It would need to be fairly simple and easily-extensible, as I can
imagine quite a few things getting added to it.

(Imagines a procmail rule which just bounces the email if
spot-common-mistakes failed)


> 
> Heaven knows how many more serious problems are being snuck into the tree
> via this route.

But it won't solve this problem.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Arnd Bergmann]

On Thursday 26 April 2007, Andrew Morton wrote:
> It would be neat if someone could create and maintain a new
> scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
> about newly-added code (and only newly-added code) which has busted
> whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.

http://patchstylecheck.googlecode.com/svn/trunk/patchstylecheckemail.pl
Might serve as a starting point for this. It doesn't have any semantic
checks right now, but I guess they can be added.

	Arnd <><

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Dave Jones]

On Wed, Apr 25, 2007 at 05:24:47PM -0700, Andrew Morton wrote:

 > It would be neat if someone could create and maintain a new
 > scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
 > about newly-added code (and only newly-added code) which has busted
 > whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.

years and years ago, when the dinosaurs roamed the land, I hacked up..
http://janitor.kernelnewbies.org/scripts/  and then left it by the wayside.
Some of the checks it did are actually bogus, but I'm happy to pick that
up again if there's interest in it being a useful tool.

In fact, I should probably munge it together with a similar thing
I wrote at http://www.codemonkey.org.uk/projects/findbugs/
(Warning: scary regexps)

 > It would need to be fairly simple and easily-extensible, as I can
 > imagine quite a few things getting added to it.
 > 
 > (Imagines a procmail rule which just bounces the email if
 > spot-common-mistakes failed)

or a git checkin rule that refuses to commit if it fails ;-)

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Andrew Morton]

On Thu, 26 Apr 2007 02:32:06 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> On Thursday 26 April 2007, Andrew Morton wrote:
> > It would be neat if someone could create and maintain a new
> > scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
> > about newly-added code (and only newly-added code) which has busted
> > whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.
> 
> http://patchstylecheck.googlecode.com/svn/trunk/patchstylecheckemail.pl
> Might serve as a starting point for this. It doesn't have any semantic
> checks right now, but I guess they can be added.
> 

	print "Your patch is now worthy to be reviewed by a real person\n";

heh.  Yes, that looks like an ideal starting point.

Methinks it should do `exit 1' if anything was detected.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Anton Vorontsov]

On Thu, Apr 26, 2007 at 02:32:06AM +0200, Arnd Bergmann wrote:
> On Thursday 26 April 2007, Andrew Morton wrote:
> > It would be neat if someone could create and maintain a new
> > scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
> > about newly-added code (and only newly-added code) which has busted
> > whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.
> 
> http://patchstylecheck.googlecode.com/svn/trunk/patchstylecheckemail.pl
> Might serve as a starting point for this. It doesn't have any semantic
> checks right now, but I guess they can be added.

Had run this utility against my battery patches, and caught
bunch of false positives (I believe).


+#define BATTERY_PROP(bat, prop) ({                                 \
+	void *value = bat->get_property(bat, BATTERY_PROP_##prop); \
+	value ? *(int*)value : 0;                                  \
+})

Got: "Macros with multiple statements should be enclosed in a do - while
loop"

I believed ({}) is equivalent for "do - while", it's widely used in
kernel.


+	switch (bp) {
+	default: break;
+	};

Got "Gotos should not be indented", at "default: break;"


+static int bind_pst_to_psy(struct power_supplicant *pst,
+                           struct power_supply *psy)
+{

Got "use tabs not spaces". Here spaces intentionally used for
formatting purpose, not for the indenting.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Randy Dunlap]

Dave Jones wrote:
> On Wed, Apr 25, 2007 at 05:24:47PM -0700, Andrew Morton wrote:
> 
>  > It would be neat if someone could create and maintain a new
>  > scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
>  > about newly-added code (and only newly-added code) which has busted
>  > whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.
> 
> years and years ago, when the dinosaurs roamed the land, I hacked up..
> http://janitor.kernelnewbies.org/scripts/  and then left it by the wayside.
> Some of the checks it did are actually bogus, but I'm happy to pick that
> up again if there's interest in it being a useful tool.
> 
> In fact, I should probably munge it together with a similar thing
> I wrote at http://www.codemonkey.org.uk/projects/findbugs/
> (Warning: scary regexps)
> 
>  > It would need to be fairly simple and easily-extensible, as I can
>  > imagine quite a few things getting added to it.
>  > 
>  > (Imagines a procmail rule which just bounces the email if
>  > spot-common-mistakes failed)
> 
> or a git checkin rule that refuses to commit if it fails ;-)

Yep, I was going to mention your scripts but you beat me to it.

I'll be glad to help maintain such animals if wanted.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Andrew Morton]

On Wed, 25 Apr 2007 19:38:23 -0700 Randy Dunlap <randy.dunlap@oracle.com> wrote:

> Dave Jones wrote:
> > On Wed, Apr 25, 2007 at 05:24:47PM -0700, Andrew Morton wrote:
> > 
> >  > It would be neat if someone could create and maintain a new
> >  > scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
> >  > about newly-added code (and only newly-added code) which has busted
> >  > whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.
> > 
> > years and years ago, when the dinosaurs roamed the land, I hacked up..
> > http://janitor.kernelnewbies.org/scripts/  and then left it by the wayside.
> > Some of the checks it did are actually bogus, but I'm happy to pick that
> > up again if there's interest in it being a useful tool.
> > 
> > In fact, I should probably munge it together with a similar thing
> > I wrote at http://www.codemonkey.org.uk/projects/findbugs/
> > (Warning: scary regexps)
> > 
> >  > It would need to be fairly simple and easily-extensible, as I can
> >  > imagine quite a few things getting added to it.
> >  > 
> >  > (Imagines a procmail rule which just bounces the email if
> >  > spot-common-mistakes failed)
> > 
> > or a git checkin rule that refuses to commit if it fails ;-)
> 
> Yep, I was going to mention your scripts but you beat me to it.
> 
> I'll be glad to help maintain such animals if wanted.
> 

wanted ;)

At least, it would be interesting to investigate the usefulness.  I suspect
it will prove to be very useful for the little things.

Heck, someone could subscribe a robot to all the mailing lists which sends
nastygrams straight back at people who submit broken patches.  We already
need that for tab-replaced and word-wrapped patches.  (ok, we have it -
it's called akpm, but being robotic wearies one)

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Dave Jones]

On Wed, Apr 25, 2007 at 08:02:07PM -0700, Andrew Morton wrote:
 > On Wed, 25 Apr 2007 19:38:23 -0700 Randy Dunlap <randy.dunlap@oracle.com> wrote:
 > > > In fact, I should probably munge it together with a similar thing
 > > > I wrote at http://www.codemonkey.org.uk/projects/findbugs/
 > > > (Warning: scary regexps)
 > > I'll be glad to help maintain such animals if wanted.
 > 
 > wanted ;)
 > 
 > At least, it would be interesting to investigate the usefulness.  I suspect
 > it will prove to be very useful for the little things.

Yeah, the original script tried to do things like spinlock balancing checks,
(badly). This was long before had sparse, and it was partly a "lets learn some perl"
experience for myself. I'll toss that idea out now that we have better tools
for that, and keep it to simple checks.

 > Heck, someone could subscribe a robot to all the mailing lists which sends
 > nastygrams straight back at people who submit broken patches.  We already
 > need that for tab-replaced and word-wrapped patches.  (ok, we have it -
 > it's called akpm, but being robotic wearies one)

Ok, I've got a few different flavours of that script. I'll roll them
all into one tomorrow and throw out some of the noisy silly ones
(I don't think warning about strcpy->strncpy is really worthwhile for eg).

Additional regexps gratefully recieved.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Andrew Morton]

On Thu, 26 Apr 2007 02:32:06 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> On Thursday 26 April 2007, Andrew Morton wrote:
> > It would be neat if someone could create and maintain a new
> > scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
> > about newly-added code (and only newly-added code) which has busted
> > whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.
> 
> http://patchstylecheck.googlecode.com/svn/trunk/patchstylecheckemail.pl
> Might serve as a starting point for this. It doesn't have any semantic
> checks right now, but I guess they can be added.

oh man, every patch I review, every bug I fix, I dream of this.



Wishlist:

- wire it up to a robot which monitors all Linux mailing lists and sends
  machine-review comments back to originators.  This will be a huge win by
  eliminating so much stupid crap.  

- auto-detect wordwrapped and tab-replaced emails (oh glory)

- auto-detect code wider than 80-cols (swoon)

- auto-detect missing Signed-off-by:

- auto-check patch format and protocol, as per
  http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt and
  http://linux.yyz.us/patch-format.html

- teach it about semantics:

  - kthread instead of kernel_thread

  - mutexes instead of semaphores

  - the whole plethora of whitespace uckfuppednesses

  - needlessly-initialised-to-zero-static-variables

  - extern-decls-in-C

  - EXPORT_SYMBOL(foo) is placed immediately after foo()'s closing
    brace

    Hard to do?  Could just whine about all EXPORT_SYMBOL's which
    aren't immediately preceded by ^}$ or by ;$

  - new typedefs

  - use of uint32_t and friends

  - use of BUG_ON and BUG, frankly.  The thing's a damn pest.  Suggest
    WARN_ON+recover-from-it.

  - use of `if ((var = expr()))' and similar

  - large inlined functions?

  - braces around single statements (advanced topic ;))

  - StudlyCaps?

  - anything called "tmp" or "temp"

  - old-style struct initialisers

  - non-ascii characters(?)

  - lots more to come, I'm sure.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Andy Whitcroft]

Andrew Morton wrote:
> On Wed, 25 Apr 2007 11:21:33 -0700
> Randy Dunlap <randy.dunlap@oracle.com> wrote:
> 
>> On Mon, 23 Apr 2007 10:45:34 -0700 Andrew Morton wrote:
>>
>>> On Mon, 23 Apr 2007 16:11:23 +0200
>>> Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
>>>
>>>> Greetings,
>>>> I've added the results of the review to the Kconfig cleanup patches
>>>> for s390. Patch #2 has been split, one half has all the HAS_IOMEM
>>>> depends lines the other the remaining !S390 depends lines.
>>>>
>>>> Andrew: I plan to add patches 1-5 to the for-andrew branch of the
>>>> git390 repository if that is fine with you. The only thing that will
>>>> be missing in the tree is the patch that disables wireless for s390.
>>>> The code does compile but without hardware it is mute to have the
>>>> config options. I'll wait until the git-wireless.patch is upstream.
>>>> Patches 7-9 depend on patches found in -mm.
>>>>
>>> umm, OK.  If it's Ok I think I'll duck it for now: -mm is full.
>>>
>>> Over-full, really: I've been working basically continuously since Friday
>>> getting the current dungpile to compile and boot, and it's still miles away
>>> from that.
>> and I continue to be concerned about the amount of patch reviews
>> compared to new patch material overall (not just s390).
>>
> 
> yes.  I'm increasingly reluctant to merge things which have had no visible
> review from any third party.  Nowadays I'll shove such patches into a
> pending folder and will wait a day or three to see if anyone has any
> feedback.  If they don't I have to either ignore the patches or review them
> myself.
> 
> I expect (and hope) that more formal processes will come about here.  Perhaps
> up to it-won't-be-merged-without-a-Reviewed-by:.

Is this not the meaning of the Acked-by: ?

> Heaven knows how many more serious problems are being snuck into the tree
> via this route.
> 
> What do we do?

Perhaps its time for Linus to say he won't accept any patches which are
not Acked and place the onus on getting those on the tree maintainers.
In theory at least tree maintainers are supposed to be responsible for
the stuff coming through their tree.  They could be made responsible to
ensuring only Ack'd stuff is committed.  Automated checks could be made
for that at least.

As for the white space errors.  I think that we should perhaps run a
spectrum of commits from each tree through the checked proposed later in
the thread.  Where any significant non-compliance is detected that
should be sent to the tree maintainer and their 'upstream' and
corrections expected.

Public shaming.  A savaging from Linus' or any other respected community
member is something to be avoided at all costs.

-apw

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Randy Dunlap]

On Thu, 26 Apr 2007 01:30:59 -0700 Andrew Morton wrote:

> On Thu, 26 Apr 2007 02:32:06 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Thursday 26 April 2007, Andrew Morton wrote:
> > > It would be neat if someone could create and maintain a new
> > > scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
> > > about newly-added code (and only newly-added code) which has busted
> > > whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.
> > 
> > http://patchstylecheck.googlecode.com/svn/trunk/patchstylecheckemail.pl
> > Might serve as a starting point for this. It doesn't have any semantic
> > checks right now, but I guess they can be added.
> 
> oh man, every patch I review, every bug I fix, I dream of this.

singing?


> Wishlist:
> 
> - wire it up to a robot which monitors all Linux mailing lists and sends
>   machine-review comments back to originators.  This will be a huge win by
>   eliminating so much stupid crap.  
> 
> - auto-detect wordwrapped and tab-replaced emails (oh glory)
> 
> - auto-detect code wider than 80-cols (swoon)
> 
> - auto-detect missing Signed-off-by:
> 
> - auto-check patch format and protocol, as per
>   http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt and
>   http://linux.yyz.us/patch-format.html
> 
> - teach it about semantics:
> 
>   - kthread instead of kernel_thread
> 
>   - mutexes instead of semaphores
> 
>   - the whole plethora of whitespace uckfuppednesses
> 
>   - needlessly-initialised-to-zero-static-variables
> 
>   - extern-decls-in-C
> 
>   - EXPORT_SYMBOL(foo) is placed immediately after foo()'s closing
>     brace
> 
>     Hard to do?  Could just whine about all EXPORT_SYMBOL's which
>     aren't immediately preceded by ^}$ or by ;$
> 
>   - new typedefs
> 
>   - use of uint32_t and friends
> 
>   - use of BUG_ON and BUG, frankly.  The thing's a damn pest.  Suggest
>     WARN_ON+recover-from-it.
> 
>   - use of `if ((var = expr()))' and similar
> 
>   - large inlined functions?
> 
>   - braces around single statements (advanced topic ;))
> 
>   - StudlyCaps?
> 
>   - anything called "tmp" or "temp"
> 
>   - old-style struct initialisers
> 
>   - non-ascii characters(?)
> 
>   - lots more to come, I'm sure.

I made a short list last night, but yours is better/longer.
Here are a few more items:

- check for new docs and/or kernel-doc
- storage class should be first
- don't use deprecated APIs
- no floating point
- no MIN/MAX
- printk wants KERN_* levels
- limit on new /proc files


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

patch style checks

[Andy Whitcroft]

Andrew Morton wrote:
> On Thu, 26 Apr 2007 02:32:06 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> 
>> On Thursday 26 April 2007, Andrew Morton wrote:
>>> It would be neat if someone could create and maintain a new
>>> scripts/spot-common-mistakes.  Feed it a unified diff and it would complain
>>> about newly-added code (and only newly-added code) which has busted
>>> whitespace, adds new semaphores, adds new kernel_thread calls, etc, etc.
>> http://patchstylecheck.googlecode.com/svn/trunk/patchstylecheckemail.pl
>> Might serve as a starting point for this. It doesn't have any semantic
>> checks right now, but I guess they can be added.
>>
> 
> 	print "Your patch is now worthy to be reviewed by a real person\n";
> 
> heh.  Yes, that looks like an ideal starting point.
> 
> Methinks it should do `exit 1' if anything was detected.

[Joel in case you'd not spotted this discussion, your
patchstylecheckemail script was found ...  Also this has produced a
little patch series improving the tool.  Where would you like that sent?]

As an experiment I took the -mm git repo and applied a modified versions
of the patchstylecheckemail.pl to all the of the commits reported
between 2.6.21-rc7 and 2.6.21-rc7-mm2.  The git version of -mm links
directly back to the real incoming git trees and so we get the
individual commits there also.

2.6.21-rc7-mm2 appears to contian some 4313 commits in total!!!  Of
these some 886 failed the style check; over 20%.  Obviously some of
these will be false positives, or actually better as they are than made
compliant.  I did have a quick look over a sample of the errors and bad
use of space at the start and end of line, plus overlength lines, and
the lack of spaces round operators seem to be the predominant errors
therein.

As these are coming from the git commits, we could consider sending an
email to those authors.  However, as we are only considering patches in
isolation we cannot see if they got fixed later for instance.

In an ideal world we would do the same sort of analysis on the files as
the appear in the HEAD of the tree.  Any lines in error then would be
attributed to an author and that author told to fix their mess.  Hmmm,
not so hard me thinks.

-apw

Re: patch style checks

[jschopp]

>>
>> Methinks it should do `exit 1' if anything was detected.
> 
> [Joel in case you'd not spotted this discussion, your
> patchstylecheckemail script was found ...  Also this has produced a
> little patch series improving the tool.  Where would you like that sent?]

Heh.  It's pretty crude but I've found it useful even as crude as it is.  I have been 
doing library work instead of kernel work for a little more than a year so I haven't been 
improving it.  That said, I'd be happy to see patches as I expect I'll do kernel work 
again in the future.  I had made a project page for it here: 
http://code.google.com/p/patchstylecheck/

I'll maintain it if people want to send me patches.  I also added Andy to the svn 
commiters list awhile back, so you could bug him as well.

> 2.6.21-rc7-mm2 appears to contian some 4313 commits in total!!!  Of
> these some 886 failed the style check; over 20%.  Obviously some of
> these will be false positives, or actually better as they are than made
> compliant.  I did have a quick look over a sample of the errors and bad
> use of space at the start and end of line, plus overlength lines, and
> the lack of spaces round operators seem to be the predominant errors
> therein.

Whitespace damage is the predominant one I've seen as well.  When I was developing it I 
fed it a number of lkml patches at random and fixed all the false positives I encountered.

checkpatch, a patch checking script.

[Dave Jones]

On Wed, Apr 25, 2007 at 08:02:07PM -0700, Andrew Morton wrote:

 > > Yep, I was going to mention your scripts but you beat me to it.
 > > 
 > > I'll be glad to help maintain such animals if wanted.
 > > 
 > wanted ;)
 > 
 > At least, it would be interesting to investigate the usefulness.  I suspect
 > it will prove to be very useful for the little things.

Randy and I got together and hashed out a first cut at this.
(Randy actually gutted quite a lot of what I originally wrote, so deserves
 much kudos for improving this beyond my initial crappy version).
You can find the script at http://www.codemonkey.org.uk/projects/checkpatch/
There's also a git clonable tree there (only http right now).

http://www.codemonkey.org.uk/projects/checkpatch/example.log shows
what fell out of running it on my mbox of lkml from the past month.
Some of them are kinda noisy, and perhaps should be moved under --pedantic

I'm all ears for additional regexps, bug reports or other suggestions.

Before wiring this up to a procmail rule to scan every patch, I think it's
probably a better idea to flesh it out a bit more.

	Dave

-- 
http://www.codemonkey.org.uk

Re: checkpatch, a patch checking script.

[Roland Dreier]

 > http://www.codemonkey.org.uk/projects/checkpatch/example.log shows
 > what fell out of running it on my mbox of lkml from the past month.
 > Some of them are kinda noisy, and perhaps should be moved under --pedantic
 > 
 > I'm all ears for additional regexps, bug reports or other suggestions.

Looks great... however I notice a few obvious false positives in the
example log:

 > Don't init statics to 0/NULL:
 > 94312:+static const struct in6_addr in6addr_v4mapped = { { { [10] = 0xff, [11] = 0xff } } };

ummm?

 > 137054:+static uint32_t drvr_ver  = 0x02200207;

that ain't zero...

 > 230079:+                path->static_rate = 0;

and that ain't a static variable.

I guess trying to parse C in a regexp is a little tricky.

Also, it would be nice to be able to do something like

    git diff v2.6.20..|perl ~/checkpatch.pl -

rather than having to create a temp file -- as it stands that command
produces

    unknown option: -
    usage: findbugs.pl [-options] file(s)
      -allsource : check entire source file, not just '+' patch lines
      -pedantic : TBD
      -style : TBD
      -v, --verbose : verbose
      -h, --help : this help text
    Version: 002

And even worse

    git diff v2.6.20..|perl ~/checkpatch.pl

just silently does nothing (maybe a "no input files" warning would be
a good clue for people).

 - R.

Re: checkpatch, a patch checking script.

[Adrian Bunk]

On Fri, Apr 27, 2007 at 08:36:17PM -0700, Roland Dreier wrote:
>...
> Also, it would be nice to be able to do something like
> 
>     git diff v2.6.20..|perl ~/checkpatch.pl -
>...

  perl ~/checkpatch.pl <(git diff v2.6.20..)

>  - R.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: checkpatch, a patch checking script.

[Andrew Morton]

On Fri, 27 Apr 2007 23:08:05 -0400 Dave Jones <davej@redhat.com> wrote:

> You can find the script at http://www.codemonkey.org.uk/projects/checkpatch/

hm.

box:/usr/src/25> ~/checkpatch.pl patches/slub-core.patch
Checking patches/slub-core.patch:  signoffs = 30        
Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
1588:+  VM_BUG_ON(!irqs_disabled());
1834:+  BUG_ON(flags & ~(GFP_DMA | GFP_LEVEL_MASK));
2538:+  BUG_ON(kmalloc_caches->size < sizeof(struct kmem_cache_node));
2544:+  BUG_ON(!page);
2546:+  BUG_ON(!n);
2736:+          BUG_ON(err);
2762:+  BUG_ON(flags & SLUB_UNIMPLEMENTED);
2777:+          BUG_ON(flags & (SLAB_RED_ZONE | SLAB_POISON |
2779:+          BUG_ON(ctor || dtor);
3054:+  BUG_ON(index < 0);
3118:+  BUG_ON(!page);
3120:+  BUG_ON(!s);
4062:+  BUG_ON(!name);
4083:+  BUG_ON(p > name + ID_STR_LENGTH - 1);
4188:+          BUG_ON(err);

15 warnings


surely we can do better than that ;)


box:/usr/src/25> ~/checkpatch.pl patches/git-ieee1394.patch 
Checking patches/git-ieee1394.patch:  signoffs = 291
Do not add new typedefs.
5239:+typedef int (*descriptor_callback_t)(struct context *ctx,
7254:+typedef void (*scsi_done_fn_t) (struct scsi_cmnd *);
8668:+typedef void (*fw_node_callback_t) (struct fw_card * card,
10077:+typedef void (*fw_packet_callback_t) (struct fw_packet *packet,
10080:+typedef void (*fw_transaction_callback_t)(struct fw_card *card, int rcode,
10085:+typedef void (*fw_address_callback_t)(struct fw_card *card,
10093:+typedef void (*fw_bus_reset_callback_t)(struct fw_card *handle,
10245:+typedef void (*fw_iso_callback_t) (struct fw_iso_context *context,

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
4342:+  BUG_ON(j >= ARRAY_SIZE(group->attrs));
9868:+  BUG_ON(retval < 0);
9872:+  BUG_ON(retval < 0);
9876:+  BUG_ON(retval < 0);
9878:+  BUG_ON(retval < 0);
10952:+ BUG_ON(!kv || !associate || kv->key.id == CSR1212_KV_ID_DESCRIPTOR ||
10983:+ BUG_ON(!kv || !dir || dir->key.type != CSR1212_KV_TYPE_DIRECTORY);
11396:+ BUG_ON(!csr || !csr->ops || !csr->ops->allocate_addr_range ||
11750:+ BUG_ON(!csr);
11802:+                 BUG_ON(csr->max_rom < 1);
12106:+ BUG_ON(!csr || !kv || csr->max_rom < 1);
12248:+ BUG_ON(!csr || !csr->ops || !csr->ops->bus_read);
14541:+         BUG_ON(max_payload < 512 - ETHER1394_GASP_OVERHEAD);
14567:+         BUG_ON(max_payload < 512 - ETHER1394_GASP_OVERHEAD);
15213:+         BUG_ON(!list_empty(&packet->driver_list) ||

23 warnings

ok.

box:/usr/src/25> ~/checkpatch.pl patches/git-net.patch     
Checking patches/git-net.patch:  signoffs = 831
Do not add new typedefs.
18871:+typedef unsigned int sk_buff_data_t;
18873:+typedef unsigned char *sk_buff_data_t;
20686:+typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *, void *);
20687:+typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);

Incorrect type usage for kernel code. Use __u32 etc.
21854:+uint32_t __attribute__((weak)) __div64_32(uint64_t *n, uint32_t base)
21865:+ uint32_t high, d;

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
11084:+         BUG_ON(ip_hdr(skb)->protocol != IPPROTO_TCP);
21600:+ BUG_ON(!wiphy);
21633:+ BUG_ON(!wdev);
25577:+                         BUG_ON(r->ctarget != NULL);
26832:+ BUG_ON(msgindex < 0 || msgindex >= RTM_NR_MSGTYPES);
26882:+ BUG_ON(protocol < 0 || protocol >= NPROTO);
26936:+ BUG_ON(protocol < 0 || protocol >= NPROTO);
26959:+ BUG_ON(protocol < 0 || protocol >= NPROTO);
27772:+ BUG_ON(len);
30411:+         BUG_ON(hctx->ccid3hctx_p && !hctx->ccid3hctx_x_calc);
30626:+ BUG_ON(hctx == NULL);
32199:+ BUG_ON(ptr == NULL);
32217:+ BUG_ON(ptr == NULL);
58250:+ BUILD_BUG_ON(sizeof(struct illinois) > ICSK_CA_PRIV_SIZE);
61747:+ BUG_ON(sizeof(struct yeah) > ICSK_CA_PRIV_SIZE);
63079:+ BUG_ON(pad < 0);
69953:+ BUG_ON(sk == NULL);
69962:+ BUG_ON(self == NULL);
70883:+ BUG_ON(destroy == NULL);
80348:+ BUG_ON(!wiphy);

Don't init statics to 0/NULL:
61061:+static int port __read_mostly = 0;
70417:+static int hashbin_lock_depth = 0;

28 warnings

Bad David.


git-ocfs2.patch: couple fo new typedefs, zillions of BUG_ONs

box:/usr/src/25> ~/checkpatch.pl patches/git-libata-all.patch 
Checking patches/git-libata-all.patch:  signoffs = 167
Do not add new typedefs.
14867:+typedef int (*ata_prereset_fn_t)(struct ata_port *ap, unsigned long deadline);
14868:+typedef int (*ata_reset_fn_t)(struct ata_port *ap, unsigned int *classes,

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
5426:+  BUG_ON(!legacy_dr);

Don't init statics to 0/NULL:
2861:+static int ata_ignore_hpa = 0;



box:/usr/src/25> ~/checkpatch.pl patches/git-ia64.patch      
Checking patches/git-ia64.patch:  signoffs = 38
Do not add new typedefs.
875:+typedef unsigned long u64;
876:+typedef unsigned int  u32;
878:+typedef union err_type_info_u {
890:+typedef union err_struct_info_u {
930:+typedef union err_data_buffer_u {
954:+typedef union capabilities_u {
1009:+typedef struct resources_s {
1443:+typedef struct {

box:/usr/src/25> ~/checkpatch.pl patches/git-scsi-misc.patch 
Checking patches/git-scsi-misc.patch:  signoffs = 165
Incorrect type usage for kernel code. Use __u32 etc.
7802:+                  uint32_t len;
7803:+                  uint32_t sense_len;

Don't init statics to 0/NULL:
4782:+static unsigned int ipr_transop_timeout = 0;

c'mon, this is scsi - it's full of low-hanging fruit.


box:/usr/src/25> ~/checkpatch.pl patches/git-mtd.patch      
Checking patches/git-mtd.patch:  signoffs = 89
kmalloc return shouldn't be cast.
2085:+  msp_flash = (struct mtd_info **)kmalloc(
2087:+  msp_parts = (struct mtd_partition **)kmalloc(
2089:+  msp_maps = (struct map_info *)kmalloc(
2107:+          msp_parts[i] = (struct mtd_partition *)kmalloc(

Incorrect type usage for kernel code. Use __u32 etc.
4690:+uint32_t jffs2_truncate_fragtree(struct jffs2_sb_info *c, struct rb_root *list, uint32_t size)
5255:+  uint32_t highest_version;
5256:+  uint32_t latest_mctime;
5257:+  uint32_t mctime_ver;
5285:+uint32_t jffs2_truncate_fragtree (struct jffs2_sb_info *c, struct rb_root *list, uint32_t size);
5491:+  uint32_t crc, ofs, len;
5632:+static struct jffs2_tmp_dnode_info *jffs2_lookup_tn(struct rb_root *tn_root, uint32_t offset)
5680:+  uint32_t fn_end = tn->fn->ofs + tn->fn->size;
5908:+  uint32_t high_ver = 0;
6420:+  uint32_t crc, new_size;
6616:+                  uint32_t empty_start, scan_end;
6620:+                  scan_end = min_t(uint32_t, EMPTY_SCAN_SIZE(c->sector_size)/8, buf_len);
6628:+                          if (unlikely(*(uint32_t *)(&buf[inbuf_ofs]) != 0xffffffff)) {
6680:+  uint32_t crc, ino = je32_to_cpu(ri->ino);
	
Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
5494:+  BUG_ON(tn->csize == 0);
5611:+  BUG_ON(ref_obsolete(tn->fn->raw));
5859:+  BUG_ON(node->rb_right);
	
Don't init statics to 0/NULL:
1309:+static int nr_devices = 0;
2769:+static char *badblocks = NULL;
2770:+static char *weakblocks = NULL;
2771:+static char *weakpages = NULL;
2772:+static unsigned int bitflips = 0;
2773:+static char *gravepages = NULL;
2774:+static unsigned int rptwear = 0;
2775:+static unsigned int overridesize = 0;
2877:+static unsigned long *erase_block_wear = NULL;
2878:+static unsigned int wear_eb_count = 0;
2879:+static unsigned long total_wear = 0;
2880:+static unsigned int rptwear_cnt = 0;
	
33 warnings

That's more like it.  MTD is a whole world of 120-column lines and type
innovations.


box:/usr/src/25> ~/checkpatch.pl patches/git-powerpc.patch
Checking patches/git-powerpc.patch:  signoffs = 427       
kmalloc return shouldn't be cast.
10969:+ char *out = (char*)kmalloc(count + 1, GFP_KERNEL);

Use 'inline' instead of '__inline__'26644:+static __inline__ void atomic_scrub(void *va, u32 size)

Do not add new typedefs.
7336:+typedef struct bd_info {

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
13629:+ BUG_ON(linear_map_hash_slots[lmi] & 0x80);
13641:+ BUG_ON(!(linear_map_hash_slots[lmi] & 0x80));
13880:+         BUG_ON(pte_val(*pg) & (_PAGE_PRESENT | _PAGE_HASHPTE));
13922:+ BUG_ON(PageHighMem(page));
15543:+ BUG_ON(mpic == NULL);
16671:+         BUG_ON(!tmp_np);
17745:+ BUG_ON(spu_coredump_calls != calls);
17887:+ BUG_ON(!list_empty(&ctx->rq));
19057:+         BUG_ON(list_empty(rq));
24237:+ BUG_ON(intsize != 2);
24271:+ BUG_ON(! device_is_compatible(node, "ibm,uic"));
24335:+ BUG_ON(!np); /* uic_init_tree() assumes there's a UIC as the
24383:+ BUG_ON(! primary_uic);

16 warnings

box:/usr/src/25> ~/checkpatch.pl patches/git-input.patch      
Checking patches/git-input.patch:  signoffs = 85
Incorrect type usage for kernel code. Use __u32 etc.
4164:+  uint32_t mask;
4185:+  uint32_t status;

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
3504:+  BUG_ON(kbd == NULL);
3540:+  BUG_ON(kbd == NULL);
5595:+  BUG_ON(ptr == NULL);
5632:+  BUG_ON(ptr == NULL);
7873:+  BUG_ON(mlc->ddi >= 6);
8106:+  BUG_ON(down_trylock(&mlc->isem));
8142:+          BUG_ON(node->object.func == NULL);
8345:+  BUG_ON(map == NULL);
8353:+  BUG_ON(mlc == NULL);
8373:+  BUG_ON(drv == NULL);
8408:+  BUG_ON(map == NULL);
8415:+  BUG_ON(mlc == NULL);
8431:+  BUG_ON(map == NULL);
8438:+  BUG_ON(mlc == NULL);
8463:+  BUG_ON(mlc == NULL);
8511:+  BUG_ON(mlc == NULL);
9700:+  BUG_ON(down_trylock(&mlc->isem));
9701:+  BUG_ON(down_trylock(&mlc->osem));
9762:+  BUG_ON(down_trylock(&mlc->osem));
9781:+  BUG_ON(down_trylock(&mlc->csem));
9829:+  BUG_ON(mlc->opacket & HIL_CTRL_APE);

Don't init statics to 0/NULL:
11053:+static unsigned int channel_mask = 0xFFFF;

24 warnings


box:/usr/src/25> ~/checkpatch.pl patches/git-e1000.patch        
Checking patches/git-e1000.patch:  signoffs = 44
Do not add new typedefs.
21237:+typedef enum {
33390:+typedef enum {
36694:+typedef enum {
36701:+typedef enum {

4 warnings


box:/usr/src/25> ~/checkpatch.pl patches/git-alsa.patch 
Checking patches/git-alsa.patch:  signoffs = 297
0 warnings

whoa.

box:/usr/src/25> ~/checkpatch.pl patches/git-infiniband.patch 
Checking patches/git-infiniband.patch:  signoffs = 113
Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
8143:+  BUG_ON(mlx4_ib_alloc_db_from_pgdir(pgdir, db, order));
12629:+ BUG_ON(cmd->free_head < 0);
16580:+         BUG_ON(index < dev->caps.num_mgms);
16665:+                 BUG_ON(amgm_index_to_free < dev->caps.num_mgms);
16681:+         BUG_ON(index < dev->caps.num_mgms);

Don't init statics to 0/NULL:
10333:+         path->static_rate = 0;
15461:+static int msi_x = 0;
16113:+ static int mlx4_version_printed = 0;

8 warnings



box:/usr/src/25> ~/checkpatch.pl patches/git-wireless.patch        
Checking patches/git-wireless.patch:  signoffs = 2009
kmalloc return shouldn't be cast.
62076:+ a16 = (zd_addr_t *)kmalloc(count16 * (sizeof(zd_addr_t) + sizeof(u16)),

Do not add new typedefs.
50317:+typedef void (debug_access_t)(struct rt2x00_dev *rt2x00dev,
64586:+typedef u16 __nocast zd_addr_t;
76135:+typedef enum { ALG_NONE, ALG_WEP, ALG_TKIP, ALG_CCMP, ALG_NULL }
76195:+typedef enum {
87265:+typedef enum {
87366:+typedef ieee80211_txrx_result (*ieee80211_tx_handler)
87369:+typedef ieee80211_txrx_result (*ieee80211_rx_handler)
87925:+typedef enum {
92694:+typedef enum { ParseOK = 0, ParseUnknown = 1, ParseFailed = -1 } ParseRes;
102286:+typedef int (*iw_compat_handler)(struct cfg80211_registered_device *drv,

Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
23286:+ BUILD_BUG_ON(BCM43xx_SEC_KEYSIZE < ETH_ALEN);
33026:+ BUILD_BUG_ON(BCM43xx_TAB_ROTOR_SIZE != ARRAY_SIZE(bcm43xx_tab_rotor));
33027:+ BUILD_BUG_ON(BCM43xx_TAB_RETARD_SIZE != ARRAY_SIZE(bcm43xx_tab_retard));
33028:+ BUILD_BUG_ON(BCM43xx_TAB_FINEFREQA_SIZE != ARRAY_SIZE(bcm43xx_tab_finefreqa));
33029:+ BUILD_BUG_ON(BCM43xx_TAB_FINEFREQG_SIZE != ARRAY_SIZE(bcm43xx_tab_finefreqg));
33030:+ BUILD_BUG_ON(BCM43xx_TAB_NOISEA2_SIZE != ARRAY_SIZE(bcm43xx_tab_noisea2));
33031:+ BUILD_BUG_ON(BCM43xx_TAB_NOISEA3_SIZE != ARRAY_SIZE(bcm43xx_tab_noisea3));
33032:+ BUILD_BUG_ON(BCM43xx_TAB_NOISEG1_SIZE != ARRAY_SIZE(bcm43xx_tab_noiseg1));
33033:+ BUILD_BUG_ON(BCM43xx_TAB_NOISEG2_SIZE != ARRAY_SIZE(bcm43xx_tab_noiseg2));
33034:+ BUILD_BUG_ON(BCM43xx_TAB_NOISESCALEG_SIZE != ARRAY_SIZE(bcm43xx_tab_noisescaleg1));
33035:+ BUILD_BUG_ON(BCM43xx_TAB_NOISESCALEG_SIZE != ARRAY_SIZE(bcm43xx_tab_noisescaleg2));
33036:+ BUILD_BUG_ON(BCM43xx_TAB_NOISESCALEG_SIZE != ARRAY_SIZE(bcm43xx_tab_noisescaleg3));
33037:+ BUILD_BUG_ON(BCM43xx_TAB_SIGMASQR_SIZE != ARRAY_SIZE(bcm43xx_tab_sigmasqr1));
33038:+ BUILD_BUG_ON(BCM43xx_TAB_SIGMASQR_SIZE != ARRAY_SIZE(bcm43xx_tab_sigmasqr2));
49080:+ BUILD_BUG_ON(!(__mask) ||               \
49090:+ BUILD_BUG_ON(!(__mask) ||               \
70106:+ BUILD_BUG_ON(SSB_PCICORE_BCAST_ADDR != SSB_CHIPCO_BCAST_ADDR);
70107:+ BUILD_BUG_ON(SSB_PCICORE_BCAST_DATA != SSB_CHIPCO_BCAST_DATA);
74967:+ BUILD_BUG_ON(index >= SSB_EXTIF_NR_GPIOOUT);    \
74971:+ BUILD_BUG_ON(index >= SSB_EXTIF_NR_GPIOOUT);    \
76919:+ BUG_ON(!wiphy);
76952:+ BUG_ON(!wdev);
86065:+ BUILD_BUG_ON(sizeof(struct ieee80211_rx_status) > sizeof(skb->cb));
86769:+ BUG_ON(local->reg_state != IEEE80211_DEV_REGISTERED);
86927:+ BUILD_BUG_ON(sizeof(struct ieee80211_tx_packet_data) > sizeof(skb->cb));
88357:+ BUG_ON(dev == local->apdev);
99612:+ BUG_ON(IS_ERR(drv));
99875:+ BUG_ON(!wiphy);

Don't init statics to 0/NULL:
48899:+static int rt2x00_debug_level = 0;
88421:+static int ieee80211_regdom = 0x10; /* FCC */
88431:+static int ieee80211_japan_5ghz /* = 0 */;
94013:+         static unsigned long last_tsf_debug = 0;

43 warnings



A good start, thanks.  The BUG_ON() thing might be unpopular, but I think it's
for the best.  It should skip BUILD_BUG_ON though.

Re: checkpatch, a patch checking script.

[Roland Dreier]

 > box:/usr/src/25> ~/checkpatch.pl patches/git-infiniband.patch 

Yup, I ran this too.

 > Checking patches/git-infiniband.patch:  signoffs = 113
 > Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
 > 8143:+  BUG_ON(mlx4_ib_alloc_db_from_pgdir(pgdir, db, order));
 > 12629:+ BUG_ON(cmd->free_head < 0);
 > 16580:+         BUG_ON(index < dev->caps.num_mgms);
 > 16665:+                 BUG_ON(amgm_index_to_free < dev->caps.num_mgms);
 > 16681:+         BUG_ON(index < dev->caps.num_mgms);

I agree -- killing the kernel for a driver bug is dump.  I'll remove
all these BUGs before merging.

 > Don't init statics to 0/NULL:
 > 10333:+         path->static_rate = 0;

This is a false positive/opportunity for script improvement, obviously.

 > 15461:+static int msi_x = 0;
 > 16113:+ static int mlx4_version_printed = 0;

Already zapped these.

 - R.

Re: checkpatch, a patch checking script.

[Roland Dreier]

 > Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
 > 23286:+ BUILD_BUG_ON(BCM43xx_SEC_KEYSIZE < ETH_ALEN);

BTW, I missed this before -- BUILD_BUG_ON() is actually far better
than WARN_ON(), I think.

Maybe something like this?  (Although someone who knows perl probably
has a better way)

---
Don't tell people to change BUILD_BUG_ON() to WARN_ON().

Signed-off-by: Roland Dreier <rolandd@cisco.com>

--- checkpatch.pl.orig	2007-04-27 20:30:34.000000000 -0700
+++ checkpatch.pl	2007-04-27 22:54:42.000000000 -0700
@@ -123,7 +123,7 @@
 	$warnings += search(qr/kernel_thread\(/, "Use kthread abstraction instead of kernel_thread()\n");
 	$warnings += search(qr/typedef/, "Do not add new typedefs.\n");
 	$warnings += search(qr/uint32_t/, "Incorrect type usage for kernel code. Use __u32 etc.\n");
-	$warnings += search(qr/BUG(_ON)\(/, "Use WARN_ON & Recovery code rather than BUG() and BUG_ON()\n");
+	$warnings += search(qr/(?<!BUILD_)BUG(_ON)\(/, "Use WARN_ON & Recovery code rather than BUG() and BUG_ON()\n");
 
 	# pedantic: Noisy regexps that aren't really fatal.
 	if ($opt_pedantic) {

Re: checkpatch, a patch checking script.

[Jan Engelhardt]

On Apr 27 2007 22:58, Roland Dreier wrote:
>
>--- checkpatch.pl.orig	2007-04-27 20:30:34.000000000 -0700
>+++ checkpatch.pl	2007-04-27 22:54:42.000000000 -0700
>@@ -123,7 +123,7 @@
> 	$warnings += search(qr/kernel_thread\(/, "Use kthread abstraction instead of kernel_thread()\n");
> 	$warnings += search(qr/typedef/, "Do not add new typedefs.\n");
> 	$warnings += search(qr/uint32_t/, "Incorrect type usage for kernel code. Use __u32 etc.\n");
>-	$warnings += search(qr/BUG(_ON)\(/, "Use WARN_ON & Recovery code rather than BUG() and BUG_ON()\n");
>+	$warnings += search(qr/(?<!BUILD_)BUG(_ON)\(/, "Use WARN_ON & Recovery code rather than BUG() and BUG_ON()\n");

I wonder what the capture is for?
 (?<!BUILD_)BUG(?:_ON) if you ask me :)
But you could also use...
 qr/\bBUG_ON\(/
which rules out a BUILD_BUG_ON, because _ does not constitute a word 
boundary, since _ is in \w.

And since when is uint32_t wrong? What makes u32 or __u32 better?
We have sprintf, (k)asprintf, abs(), etc. etc. etc. tons of functions
named similar to their ISO C counterparts, but when it comes to types,
we make an exception?


Jan
-- 

Re: checkpatch, a patch checking script.

[Andrew Morton]

On Sat, 28 Apr 2007 10:01:00 +0200 (MEST) Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:

> And since when is uint32_t wrong? What makes u32 or __u32 better?

There's not much to be said in favour of u32, really.  Except it's shorter
and I can never remember where the underscore goes in uint_32t.

If kernel used u_int32_t globally then the world would probably be a better
place.  But using just the one name has its advantages from a consistency
POV.

box:/usr/src/linux-2.6.21> grep -r '[   \(]u32' . | wc -l
39599
box:/usr/src/linux-2.6.21> grep -r '[   \(]uint32_t' . | wc -l 
5132

CodingStyle permits either variant, fwiw.

Re: checkpatch, a patch checking script.

[Andrew Morton]

On 28 Apr 2007 12:48:55 +0200 Andi Kleen <andi@firstfloor.org> wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > box:/usr/src/25> ~/checkpatch.pl patches/slub-core.patch
> > Checking patches/slub-core.patch:  signoffs = 30        
> > Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
> 
> The warning is bogus imho. How do you write recovery code for internal
> broken code logic? 

Yes, it is marginal.  But people do very often reach for BUG_ON() where
they could have at least partly recovered in some fashion - enough for the
info to hit the logs so we have a better chance of fixing it.

BUG_ON() is of course sometimes the right thing to do, but the idea here is
to suggest to the developers that they put a bit of thought into whether it
was really justified.

This little checking tool should have both "error" and "warning" levels -
AKA "fix this" and "think about this" levels.  BUG_ON would be a warning
thing.

Re: checkpatch, a patch checking script.

[Alan Cox]

> > The warning is bogus imho. How do you write recovery code for internal
> > broken code logic? 
> 
> Yes, it is marginal.  But people do very often reach for BUG_ON() where
> they could have at least partly recovered in some fashion - enough for the
> info to hit the logs so we have a better chance of fixing it.

At least one way to handle BUG_ON() type situations more cleanly (for
some anyway) is to fake a hot-unplug/plug event. Thats something that
would be nice to get into the PCI code as it'll let you recover from many
things (even in some cases hardware fails if you go via D3). Does need
some supporting logic (eg for networking to get back with the same config
and online)

Re: checkpatch, a patch checking script.

[Andi Kleen]

Andrew Morton <akpm@linux-foundation.org> writes:

> box:/usr/src/25> ~/checkpatch.pl patches/slub-core.patch
> Checking patches/slub-core.patch:  signoffs = 30        
> Use WARN_ON & Recovery code rather than BUG() and BUG_ON()

The warning is bogus imho. How do you write recovery code for internal
broken code logic? 

-Andi


> 1588:+  VM_BUG_ON(!irqs_disabled());
> 1834:+  BUG_ON(flags & ~(GFP_DMA | GFP_LEVEL_MASK));
> 2538:+  BUG_ON(kmalloc_caches->size < sizeof(struct kmem_cache_node));
> 2544:+  BUG_ON(!page);
> 2546:+  BUG_ON(!n);
> 2736:+          BUG_ON(err);
> 2762:+  BUG_ON(flags & SLUB_UNIMPLEMENTED);
> 2777:+          BUG_ON(flags & (SLAB_RED_ZONE | SLAB_POISON |
> 2779:+          BUG_ON(ctor || dtor);
> 3054:+  BUG_ON(index < 0);
> 3118:+  BUG_ON(!page);
> 3120:+  BUG_ON(!s);
> 4062:+  BUG_ON(!name);
> 4083:+  BUG_ON(p > name + ID_STR_LENGTH - 1);
> 4188:+          BUG_ON(err);
> 

Re: checkpatch, a patch checking script.

[Andi Kleen]

Roland Dreier <rdreier@cisco.com> writes:

>  > Checking patches/git-infiniband.patch:  signoffs = 113
>  > Use WARN_ON & Recovery code rather than BUG() and BUG_ON()
>  > 8143:+  BUG_ON(mlx4_ib_alloc_db_from_pgdir(pgdir, db, order));
>  > 12629:+ BUG_ON(cmd->free_head < 0);
>  > 16580:+         BUG_ON(index < dev->caps.num_mgms);
>  > 16665:+                 BUG_ON(amgm_index_to_free < dev->caps.num_mgms);
>  > 16681:+         BUG_ON(index < dev->caps.num_mgms);
> 
> I agree -- killing the kernel for a driver bug is dump.  I'll remove
> all these BUGs before merging.

So you prefer to corrupt data instead?  I don't think that's a good idea.
Don't do it. Silent failures are really bad.

BUG_ONs are only fatal when you're in interrupt context anyways because
it often ends up trying to kill the idle task. In process context
while there might be some lost locks in most cases the system continues
running happily. 

At some point I had  a hack to just restart idle to handle the 
interrupt case too. Perhaps that might be worth resurrecting if you
want to solve this properly. 

Would be imho far preferable over not having the BUGs imho.

-Andi

Re: checkpatch, a patch checking script.

[Jan Engelhardt]

On Apr 28 2007 01:16, Andrew Morton wrote:
>On Sat, 28 Apr 2007 10:01:00 +0200 (MEST) Jan Engelhardt wrote:
>
>> And since when is uint32_t wrong? What makes u32 or __u32 better?
>
>There's not much to be said in favour of u32, really.  Except it's
>shorter and I can never remember where the underscore goes in uint_32t.

The underscore always goes before the final t (short for type I assume).
The typedefs in the kernel do the same... kmem_cache_t (now replaced,
though) as an example.

>If kernel used u_int32_t globally then the world would probably be a
>better place.  But using just the one name has its advantages from a
>consistency POV.

#undef u32
#undef u_int32_t

>box:/usr/src/linux-2.6.21> grep -r '[   \(]u32' . | wc -l
>39599
>box:/usr/src/linux-2.6.21> grep -r '[   \(]uint32_t' . | wc -l 
>5132

Once again, \b comes to the rescue! :)
[And it also works with u32 at SOL and EOLs, and -o counts the number of
occurrences, not the number of lines]

grep -or '\b__uX\b' . | wc -l

.----..-------.------.---------.----------..------.------.--------.
|    ||    uX | __uX | uintX_t | u_intX_t ||   sX | __sX | intX_t |
>====**=======*======*=========*==========**======*======*========<
|  8 || 28004 | 6159 |    3883 |      422 ||  416 |   65 |     75 |
| 16 || 13734 | 3557 |    2613 |      426 ||  393 |  128 |     19 |
| 32 || 42971 | 6656 |    5416 |      879 || 1442 |  363 |    428 |
| 64 || 11219 | 1613 |     811 |       59 || 1061 |   81 |    111 |
`----^^-------^------^---------^----------^^------^------^--------'

The tendency towards __uX and uX is probably because these have been
existent since decades, even before stdint.h and uintX_t got introduced.
Which btw, is another point. Userspace programs that need to include
files from /usr/include/linux sometimes have a hard time compiling
because they want u8, which is defined (or not!) somewhere in
/usr/include/asm. Maybe it is not a problem anymore today, but I had
had it often enough to call it a problem. This is where uintX_t could come
a little more handy, because userspace has a higher chance of already
knowing it through stdint.h or some compiler foo.


Jan
-- 

Re: checkpatch, a patch checking script.

[Andi Kleen]

On Sat, Apr 28, 2007 at 11:15:04AM +0100, Alan Cox wrote:
> > > The warning is bogus imho. How do you write recovery code for internal
> > > broken code logic? 
> > 
> > Yes, it is marginal.  But people do very often reach for BUG_ON() where
> > they could have at least partly recovered in some fashion - enough for the
> > info to hit the logs so we have a better chance of fixing it.
> 
> At least one way to handle BUG_ON() type situations more cleanly (for
> some anyway) is to fake a hot-unplug/plug event. Thats something that

That would have a high risk of deadlock on some lost lock.

-Andi

Re: checkpatch, a patch checking script.

[Alan Cox]

> > At least one way to handle BUG_ON() type situations more cleanly (for
> > some anyway) is to fake a hot-unplug/plug event. Thats something that
> 
> That would have a high risk of deadlock on some lost lock.

Well I was assuming you'd code this up in the driver not arbitarily - and
you need to do that for IRQ anyway. So something like


	writel(0xFFFFFFFF, &mdev->irq_mask);
	pci_mark_failed(pdev, PCI_TRY_REPLUG|PCI_TRY_D3);
	spin_unlock(&mylock);
	return -EXPLODED;

with pci_mark_failed firing off any replug via a work queue

Alan

Re: checkpatch, a patch checking script.

[Matt Mackall]

On Fri, Apr 27, 2007 at 11:08:05PM -0400, Dave Jones wrote:
> On Wed, Apr 25, 2007 at 08:02:07PM -0700, Andrew Morton wrote:
> 
>  > > Yep, I was going to mention your scripts but you beat me to it.
>  > > 
>  > > I'll be glad to help maintain such animals if wanted.
>  > > 
>  > wanted ;)
>  > 
>  > At least, it would be interesting to investigate the usefulness.  I suspect
>  > it will prove to be very useful for the little things.
> 
> Randy and I got together and hashed out a first cut at this.
> (Randy actually gutted quite a lot of what I originally wrote, so deserves
>  much kudos for improving this beyond my initial crappy version).
> You can find the script at http://www.codemonkey.org.uk/projects/checkpatch/
> There's also a git clonable tree there (only http right now).
> 
> http://www.codemonkey.org.uk/projects/checkpatch/example.log shows
> what fell out of running it on my mbox of lkml from the past month.
> Some of them are kinda noisy, and perhaps should be moved under --pedantic
> 
> I'm all ears for additional regexps, bug reports or other suggestions.

Neat.

Does it check for:

functions marked extern?
pulling in external functions or variables without a header file?
return used as a function, eg return(foo);?

-- 
Mathematics is the supreme nostalgia of our time.

Re: checkpatch, a patch checking script.

[Dave Jones]

On Sat, Apr 28, 2007 at 03:02:13AM -0700, Andrew Morton wrote:

 > This little checking tool should have both "error" and "warning" levels -
 > AKA "fix this" and "think about this" levels.  BUG_ON would be a warning
 > thing.

There's a -pedantic option there just for this.  I'll move BUG_ON under it.
What's the consensus on the u32 thing, move that too, or drop completely?

	Dave

-- 
http://www.codemonkey.org.uk

Re: checkpatch, a patch checking script.

[Dave Jones]

On Sat, Apr 28, 2007 at 11:11:36AM -0500, Matt Mackall wrote:
 > > I'm all ears for additional regexps, bug reports or other suggestions.
 > 
 > Neat.
 > 
 > Does it check for:
 > 
 > functions marked extern?
 > pulling in external functions or variables without a header file?
 > return used as a function, eg return(foo);?

These sound a little more tricky than just dumb regexps.
I don't want to expand this to a fullblown C parser (given
that we have sparse which can do a better job), but I don't
object to adding some extra code to give the searches more
context.

	Dave

-- 
http://www.codemonkey.org.uk

Re: checkpatch, a patch checking script.

[Matt Mackall]

On Sat, Apr 28, 2007 at 01:11:01PM -0400, Dave Jones wrote:
> On Sat, Apr 28, 2007 at 11:11:36AM -0500, Matt Mackall wrote:
>  > > I'm all ears for additional regexps, bug reports or other suggestions.
>  > 
>  > Neat.
>  > 
>  > Does it check for:
>  > 
>  > functions marked extern?
>  > pulling in external functions or variables without a header file?
>  > return used as a function, eg return(foo);?
> 
> These sound a little more tricky than just dumb regexps.
> I don't want to expand this to a fullblown C parser (given
> that we have sparse which can do a better job), but I don't
> object to adding some extra code to give the searches more
> context.

The first is a straightforward one-line regexp, as is the last:

^extern \w.*\w\(
return\s?\(

The middle one is a bit trickier. Basically, we don't want people
doing either:

extern int capital_of_assyria;
int sir_not_appearing_in_this_module();

in .c files. The first of those two is easy, provided you can figure
out the file type. And both are possible with multiline regexps.

This whole thing would be quite a bit more powerful if your search
function joined all the lines together, did a potentially multiline
search, then calculated the line numbers from the search results. Then
you could search for things like:

if (...)
{

if (...) {
  single_statement;
}

Looking forward to fully-automated pedantry. This should probably live
in scripts/.

-- 
Mathematics is the supreme nostalgia of our time.

Re: checkpatch, a patch checking script.

[Jeff Garzik]

Dave Jones wrote:
> On Sat, Apr 28, 2007 at 03:02:13AM -0700, Andrew Morton wrote:
> 
>  > This little checking tool should have both "error" and "warning" levels -
>  > AKA "fix this" and "think about this" levels.  BUG_ON would be a warning
>  > thing.
> 
> There's a -pedantic option there just for this.  I'll move BUG_ON under it.
> What's the consensus on the u32 thing, move that too, or drop completely?

Not answering your question directly, but FWIW:  In day to day coding 
and email, I strongly advocate the use of u8/u16/u32 over the C99 
size-based types.

Simple, direct (read: easiest to review), and less typing.

	Jeff

Re: checkpatch, a patch checking script.

[Randy Dunlap]

On Sat, 28 Apr 2007 10:01:00 +0200 (MEST) Jan Engelhardt wrote:

> 
> On Apr 27 2007 22:58, Roland Dreier wrote:
> >
> >--- checkpatch.pl.orig	2007-04-27 20:30:34.000000000 -0700
> >+++ checkpatch.pl	2007-04-27 22:54:42.000000000 -0700
> >@@ -123,7 +123,7 @@
> > 	$warnings += search(qr/kernel_thread\(/, "Use kthread abstraction instead of kernel_thread()\n");
> > 	$warnings += search(qr/typedef/, "Do not add new typedefs.\n");
> > 	$warnings += search(qr/uint32_t/, "Incorrect type usage for kernel code. Use __u32 etc.\n");
> >-	$warnings += search(qr/BUG(_ON)\(/, "Use WARN_ON & Recovery code rather than BUG() and BUG_ON()\n");
> >+	$warnings += search(qr/(?<!BUILD_)BUG(_ON)\(/, "Use WARN_ON & Recovery code rather than BUG() and BUG_ON()\n");
> 
> I wonder what the capture is for?
>  (?<!BUILD_)BUG(?:_ON) if you ask me :)
> But you could also use...
>  qr/\bBUG_ON\(/
> which rules out a BUILD_BUG_ON, because _ does not constitute a word 
> boundary, since _ is in \w.

Ack, I added \b.  Thanks.

> And since when is uint32_t wrong? What makes u32 or __u32 better?
> We have sprintf, (k)asprintf, abs(), etc. etc. etc. tons of functions
> named similar to their ISO C counterparts, but when it comes to types,
> we make an exception?


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: checkpatch, a patch checking script.

[Randy Dunlap]

On Sat, 28 Apr 2007 12:21:54 -0500 Matt Mackall wrote:

> On Sat, Apr 28, 2007 at 01:11:01PM -0400, Dave Jones wrote:
> > On Sat, Apr 28, 2007 at 11:11:36AM -0500, Matt Mackall wrote:
> >  > > I'm all ears for additional regexps, bug reports or other suggestions.
> >  > 
> >  > Neat.
> >  > 
> >  > Does it check for:
> >  > 
> >  > functions marked extern?

	data marked extern?

> >  > pulling in external functions or variables without a header file?
> >  > return used as a function, eg return(foo);?
> > 
> > These sound a little more tricky than just dumb regexps.
> > I don't want to expand this to a fullblown C parser (given
> > that we have sparse which can do a better job), but I don't
> > object to adding some extra code to give the searches more
> > context.
> 
> The first is a straightforward one-line regexp, as is the last:
> 
> ^extern \w.*\w\(

Just omit the ending \( to catch functions or data.
Added.

> return\s?\(

Added.  Thanks.

> The middle one is a bit trickier. Basically, we don't want people
> doing either:
> 
> extern int capital_of_assyria;
> int sir_not_appearing_in_this_module();
> 
> in .c files. The first of those two is easy, provided you can figure
> out the file type. And both are possible with multiline regexps.
> 
> This whole thing would be quite a bit more powerful if your search
> function joined all the lines together, did a potentially multiline
> search, then calculated the line numbers from the search results. Then
> you could search for things like:
> 
> if (...)
> {
> 
> if (...) {
>   single_statement;
> }
> 
> Looking forward to fully-automated pedantry. This should probably live
> in scripts/.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: checkpatch, a patch checking script.

[Matt Mackall]

On Sun, Apr 29, 2007 at 04:37:01PM -0700, Randy Dunlap wrote:
> On Sat, 28 Apr 2007 12:21:54 -0500 Matt Mackall wrote:
> 
> > On Sat, Apr 28, 2007 at 01:11:01PM -0400, Dave Jones wrote:
> > > On Sat, Apr 28, 2007 at 11:11:36AM -0500, Matt Mackall wrote:
> > >  > > I'm all ears for additional regexps, bug reports or other suggestions.
> > >  > 
> > >  > Neat.
> > >  > 
> > >  > Does it check for:
> > >  > 
> > >  > functions marked extern?
> 
> 	data marked extern?

It's perfectly reasonable to have a data extern declaration in a header file.

-- 
Mathematics is the supreme nostalgia of our time.

Re: checkpatch, a patch checking script.

[Randy Dunlap]

Matt Mackall wrote:
> On Sun, Apr 29, 2007 at 04:37:01PM -0700, Randy Dunlap wrote:
>> On Sat, 28 Apr 2007 12:21:54 -0500 Matt Mackall wrote:
>>
>>> On Sat, Apr 28, 2007 at 01:11:01PM -0400, Dave Jones wrote:
>>>> On Sat, Apr 28, 2007 at 11:11:36AM -0500, Matt Mackall wrote:
>>>>  > > I'm all ears for additional regexps, bug reports or other suggestions.
>>>>  > 
>>>>  > Neat.
>>>>  > 
>>>>  > Does it check for:
>>>>  > 
>>>>  > functions marked extern?
>> 	data marked extern?
> 
> It's perfectly reasonable to have a data extern declaration in a header file.

but it's not perfectly acceptable to have

extern unsigned long volatile jiffies;

in a .c file.

The biggest problem I'm seeing ATM is that this script is a bit too
simplistic.  It doesn't know what it's looking at.  We'll have to
address that, I think.


-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: checkpatch, a patch checking script.

[Randy Dunlap]

On Fri, 27 Apr 2007 20:36:17 -0700 Roland Dreier wrote:

>  > http://www.codemonkey.org.uk/projects/checkpatch/example.log shows
>  > what fell out of running it on my mbox of lkml from the past month.
>  > Some of them are kinda noisy, and perhaps should be moved under --pedantic
>  > 
>  > I'm all ears for additional regexps, bug reports or other suggestions.
> 
> Looks great... however I notice a few obvious false positives in the
> example log:
> 
>  > Don't init statics to 0/NULL:
>  > 94312:+static const struct in6_addr in6addr_v4mapped = { { { [10] = 0xff, [11] = 0xff } } };
> 
> ummm?

fixed

>  > 137054:+static uint32_t drvr_ver  = 0x02200207;
> 
> that ain't zero...

not?  fixed.

>  > 230079:+                path->static_rate = 0;
> 
> and that ain't a static variable.

ack, fixed.

> I guess trying to parse C in a regexp is a little tricky.
> 
> Also, it would be nice to be able to do something like
> 
>     git diff v2.6.20..|perl ~/checkpatch.pl -

I'll check into how to treat "-" as STDIN.

> rather than having to create a temp file -- as it stands that command
> produces
> 
>     unknown option: -
>     usage: findbugs.pl [-options] file(s)
>       -allsource : check entire source file, not just '+' patch lines
>       -pedantic : TBD
>       -style : TBD
>       -v, --verbose : verbose
>       -h, --help : this help text
>     Version: 002
> 
> And even worse
> 
>     git diff v2.6.20..|perl ~/checkpatch.pl
> 
> just silently does nothing (maybe a "no input files" warning would be
> a good clue for people).

or print usage info?


Thanks.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: checkpatch, a patch checking script.

[Randy Dunlap]

On Fri, 27 Apr 2007 22:18:03 -0700 Andrew Morton wrote:

> On Fri, 27 Apr 2007 23:08:05 -0400 Dave Jones <davej@redhat.com> wrote:
> 
> > You can find the script at http://www.codemonkey.org.uk/projects/checkpatch/
> 
> hm.
> 
...

> box:/usr/src/25> ~/checkpatch.pl patches/git-powerpc.patch
> Checking patches/git-powerpc.patch:  signoffs = 427       
> kmalloc return shouldn't be cast.
> 10969:+ char *out = (char*)kmalloc(count + 1, GFP_KERNEL);
> 
> Use 'inline' instead of '__inline__'26644:+static __inline__ void atomic_scrub(void *va, u32 size)

Fixed missing \n.

...

> A good start, thanks.  The BUG_ON() thing might be unpopular, but I think it's
> for the best.  It should skip BUILD_BUG_ON though.

BUILD_BUG_ON is now skipped (in my script, not sent to DaveJ yet).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: checkpatch, a patch checking script.

[Matt Mackall]

On Sun, Apr 29, 2007 at 05:18:00PM -0700, Randy Dunlap wrote:
> Matt Mackall wrote:
> >On Sun, Apr 29, 2007 at 04:37:01PM -0700, Randy Dunlap wrote:
> >>On Sat, 28 Apr 2007 12:21:54 -0500 Matt Mackall wrote:
> >>
> >>>On Sat, Apr 28, 2007 at 01:11:01PM -0400, Dave Jones wrote:
> >>>>On Sat, Apr 28, 2007 at 11:11:36AM -0500, Matt Mackall wrote:
> >>>> > > I'm all ears for additional regexps, bug reports or other 
> >>>> suggestions.
> >>>> > 
> >>>> > Neat.
> >>>> > 
> >>>> > Does it check for:
> >>>> > 
> >>>> > functions marked extern?
> >>	data marked extern?
> >
> >It's perfectly reasonable to have a data extern declaration in a header 
> >file.
> 
> but it's not perfectly acceptable to have
> 
> extern unsigned long volatile jiffies;
> 
> in a .c file.
> 
> The biggest problem I'm seeing ATM is that this script is a bit too
> simplistic.  It doesn't know what it's looking at.  We'll have to
> address that, I think.

If you can make it run a regexp over the whole file at once rather
than a line at a time, you can deal with this with multi-line regexps.
Roughly, match:

 a +++ patch header followed by
 any number of lines not starting with +++ followed by
 the actual target expression

So, approximately:

 /+++ [\w\/]+.c(.*)\\n((([^+])|(\+[^+]))\n)*extern.*/

Or you could just make search remember what file it's in as it walks
the list of lines. But as I mentioned, multiline regexps are useful
for things other than just remembering what file we're in.

-- 
Mathematics is the supreme nostalgia of our time.

Re: checkpatch, a patch checking script.

[Randy Dunlap]

On Fri, 27 Apr 2007 23:08:05 -0400 Dave Jones wrote:

> On Wed, Apr 25, 2007 at 08:02:07PM -0700, Andrew Morton wrote:
> 
>  > > Yep, I was going to mention your scripts but you beat me to it.
>  > > 
>  > > I'll be glad to help maintain such animals if wanted.
>  > > 
>  > wanted ;)
>  > 
>  > At least, it would be interesting to investigate the usefulness.  I suspect
>  > it will prove to be very useful for the little things.
> 
> Randy and I got together and hashed out a first cut at this.
> (Randy actually gutted quite a lot of what I originally wrote, so deserves
>  much kudos for improving this beyond my initial crappy version).
> You can find the script at http://www.codemonkey.org.uk/projects/checkpatch/
> There's also a git clonable tree there (only http right now).

FYI, checkpatch.pl Version 003 is now available at the URL above.


> http://www.codemonkey.org.uk/projects/checkpatch/example.log shows
> what fell out of running it on my mbox of lkml from the past month.
> Some of them are kinda noisy, and perhaps should be moved under --pedantic
> 
> I'm all ears for additional regexps, bug reports or other suggestions.
> 
> Before wiring this up to a procmail rule to scan every patch, I think it's
> probably a better idea to flesh it out a bit more.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: checkpatch, a patch checking script.

[Geert Uytterhoeven]

On Fri, 27 Apr 2007, Dave Jones wrote:
> On Wed, Apr 25, 2007 at 08:02:07PM -0700, Andrew Morton wrote:
>  > > Yep, I was going to mention your scripts but you beat me to it.
>  > > 
>  > > I'll be glad to help maintain such animals if wanted.
>  > > 
>  > wanted ;)
>  > 
>  > At least, it would be interesting to investigate the usefulness.  I suspect
>  > it will prove to be very useful for the little things.
> 
> Randy and I got together and hashed out a first cut at this.
> (Randy actually gutted quite a lot of what I originally wrote, so deserves
>  much kudos for improving this beyond my initial crappy version).
> You can find the script at http://www.codemonkey.org.uk/projects/checkpatch/
> There's also a git clonable tree there (only http right now).
> 
> http://www.codemonkey.org.uk/projects/checkpatch/example.log shows
> what fell out of running it on my mbox of lkml from the past month.
> Some of them are kinda noisy, and perhaps should be moved under --pedantic
> 
> I'm all ears for additional regexps, bug reports or other suggestions.

Nice!

Here are a few more:
  - Check for all of (u)int{8,16,32,64}_t
  - Check for GNU extension __FUNCTION__
  - Don't use space before tab
  - Don't use trailing white space

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>

diff --git a/checkpatch.pl b/checkpatch.pl
index cbda29a..b44a3f3 100755
--- a/checkpatch.pl
+++ b/checkpatch.pl
@@ -143,10 +143,26 @@ sub process {
 			"Bad variable name: tmp. Please use something more descriptive.\n");
 		$warnings += search(qr/temp(,|;)/,
 			"Bad variable name: temp. Please use something more descriptive.\n");
+		$warnings += search(qr/uint8_t/,
+			"Incorrect type usage for kernel code. Use __u8/u8\n");
+		$warnings += search(qr/uint16_t/,
+			"Incorrect type usage for kernel code. Use __u16/u16\n");
 		$warnings += search(qr/uint32_t/,
-			"Incorrect type usage for kernel code. Use __u32 etc.\n");
+			"Incorrect type usage for kernel code. Use __u32/u32\n");
+		$warnings += search(qr/uint64_t/,
+			"Incorrect type usage for kernel code. Use __u64/u64\n");
+		$warnings += search(qr/int8_t/,
+			"Incorrect type usage for kernel code. Use __s8/s8\n");
+		$warnings += search(qr/int16_t/,
+			"Incorrect type usage for kernel code. Use __s16/s16\n");
+		$warnings += search(qr/int32_t/,
+			"Incorrect type usage for kernel code. Use __s32/s32\n");
+		$warnings += search(qr/int64_t/,
+			"Incorrect type usage for kernel code. Use __s64/s64\n");
 		$warnings += search(qr/\b(BUG|BUG_ON)\b/,
 			"Try to use WARN_ON & Recovery code rather than BUG() or BUG_ON()\n");
+		$warnings += search(qr/__FUNCION__/,
+			"Should use C99 __func__ instead of GNU __FUNCTION__\n");
 	}
 
 	# coding style:
@@ -154,6 +170,8 @@ sub process {
 		$warnings += search(qr/,[^[:space:]]/, "Need space after comma:\n");
 		$warnings += search(qr/\([[:space:]]/, "Don't use space after '(':\n");
 		$warnings += search(qr/[[:space:]]\)/, "Don't use space before ')':\n");
+		$warnings += search(qr/ \t/, "Don't use space before tab:\n");
+		$warnings += search(qr/[[:space:]]$/, "Don't use trailing white space:\n");
 		$warnings += search(qr/if\(|for\(|while\(|switch\(/,
 				"Need space after if/for/while/switch:\n");
 		$warnings += search(qr/sizeof[[:space:]]/,

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: checkpatch, a patch checking script.

[Christoph Hellwig]

On Wed, May 02, 2007 at 04:28:27PM +0200, Geert Uytterhoeven wrote:
>   - Check for GNU extension __FUNCTION__

__FUNCTION__ is prefered  over __func__

Re: checkpatch, a patch checking script.

[Geert Uytterhoeven]

On Wed, 2 May 2007, Christoph Hellwig wrote:
> On Wed, May 02, 2007 at 04:28:27PM +0200, Geert Uytterhoeven wrote:
> >   - Check for GNU extension __FUNCTION__
> 
> __FUNCTION__ is prefered  over __func__

Is there a reason for that?
  - __FUNCTION__ is a GNU extension
  - __func__ is C99
  - __func__ is shorter to type ;-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: checkpatch, a patch checking script.

[Jan Engelhardt]

On May 2 2007 16:28, Geert Uytterhoeven wrote:

>  - Check for all of (u)int{8,16,32,64}_t

I strongly disagree. These should be allowed, for they are (I think) C99.


Jan
-- 

Re: checkpatch, a patch checking script.

[Jan Engelhardt]

On May 2 2007 16:29, Christoph Hellwig wrote:
>On Wed, May 02, 2007 at 04:28:27PM +0200, Geert Uytterhoeven wrote:
>>   - Check for GNU extension __FUNCTION__
>
>__FUNCTION__ is prefered  over __func__


`info gcc` tells:

 `__FUNCTION__' is another name for `__func__'.  Older versions of GCC
recognize only this name.  However, it is not standardized.  For
maximum portability, we recommend you use `__func__', but provide a
fallback definition with the preprocessor:[...]



Jan
-- 

Re: checkpatch, a patch checking script.

[Andrew Morton]

On Wed, 2 May 2007 17:32:49 +0200 (CEST)
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> On Wed, 2 May 2007, Christoph Hellwig wrote:
> > On Wed, May 02, 2007 at 04:28:27PM +0200, Geert Uytterhoeven wrote:
> > >   - Check for GNU extension __FUNCTION__
> > 
> > __FUNCTION__ is prefered  over __func__
> 
> Is there a reason for that?
>   - __FUNCTION__ is a GNU extension
>   - __func__ is C99
>   - __func__ is shorter to type ;-)
> 

In that case we should use __func__.

But we discussed this at some length 3-4 years ago and decided to use
__FUNCTION__.  I don't remember why.  Perhaps problems with gcc support for
__func__?


(It could have been that compile-time string concatenation was involved:


        printf("xxx" __FILE__);			/* works */
        printf("xxx" __FUNCTION__);		/* doesn't */

Or not.)

Re: checkpatch, a patch checking script.

[Geert Uytterhoeven]

On Wed, 2 May 2007, Andrew Morton wrote:
> On Wed, 2 May 2007 17:32:49 +0200 (CEST)
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > On Wed, 2 May 2007, Christoph Hellwig wrote:
> > > On Wed, May 02, 2007 at 04:28:27PM +0200, Geert Uytterhoeven wrote:
> > > >   - Check for GNU extension __FUNCTION__
> > > 
> > > __FUNCTION__ is prefered  over __func__
> > 
> > Is there a reason for that?
> >   - __FUNCTION__ is a GNU extension
> >   - __func__ is C99
> >   - __func__ is shorter to type ;-)
> > 
> 
> In that case we should use __func__.
> 
> But we discussed this at some length 3-4 years ago and decided to use
> __FUNCTION__.  I don't remember why.  Perhaps problems with gcc support for
> __func__?

I tried gcc 2.95/3.2/3.3/3.4/4.0/4.1, they all recognize __func__ and
__FUNCTION__, like in e.g. printf("%s", __func__);

> (It could have been that compile-time string concatenation was involved:
> 
> 
>         printf("xxx" __FILE__);			/* works */
>         printf("xxx" __FUNCTION__);		/* doesn't */
> 
> Or not.)

Yep, when trying concatenation, I got:
  - 2.95: works fine
  - 3.2:
      syntax error before "__func__"
      warning: concatenation of string literals with __FUNCTION__ is deprecated
  - 3.3:
      error: syntax error before "__func__"
      warning: concatenation of string literals with __FUNCTION__ is deprecated
  - 3.4/4.0:
      error: syntax error before "__func__"
      error: syntax error before "__FUNCTION__"
  - 4.1:
      error: expected ')' before '__func__'
      error: expected ')' before '__FUNCTION__'

Hence gcc 3.2 and up treat __func__ like the a variable, as per C99, while
__FUNCTION__ has been moving from a virtual preprocessor definition in 2.95 to
a variable, like __func__.

So in the end it doesn't matter, as concatenation has been fixed in the Linux
source tree anyway.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: checkpatch, a patch checking script.

[Andrew Morton]

On Wed, 2 May 2007 21:55:16 +0200 (CEST)
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> On Wed, 2 May 2007, Andrew Morton wrote:
> > On Wed, 2 May 2007 17:32:49 +0200 (CEST)
> > Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> > 
> > > On Wed, 2 May 2007, Christoph Hellwig wrote:
> > > > On Wed, May 02, 2007 at 04:28:27PM +0200, Geert Uytterhoeven wrote:
> > > > >   - Check for GNU extension __FUNCTION__
> > > > 
> > > > __FUNCTION__ is prefered  over __func__
> > > 
> > > Is there a reason for that?
> > >   - __FUNCTION__ is a GNU extension
> > >   - __func__ is C99
> > >   - __func__ is shorter to type ;-)
> > > 
> > 
> > In that case we should use __func__.
> > 
> > But we discussed this at some length 3-4 years ago and decided to use
> > __FUNCTION__.  I don't remember why.  Perhaps problems with gcc support for
> > __func__?
> 
> I tried gcc 2.95/3.2/3.3/3.4/4.0/4.1, they all recognize __func__ and
> __FUNCTION__, like in e.g. printf("%s", __func__);
> 
> > (It could have been that compile-time string concatenation was involved:
> > 
> > 
> >         printf("xxx" __FILE__);			/* works */
> >         printf("xxx" __FUNCTION__);		/* doesn't */
> > 
> > Or not.)
> 
> Yep, when trying concatenation, I got:
>   - 2.95: works fine
>   - 3.2:
>       syntax error before "__func__"
>       warning: concatenation of string literals with __FUNCTION__ is deprecated
>   - 3.3:
>       error: syntax error before "__func__"
>       warning: concatenation of string literals with __FUNCTION__ is deprecated
>   - 3.4/4.0:
>       error: syntax error before "__func__"
>       error: syntax error before "__FUNCTION__"
>   - 4.1:
>       error: expected ')' before '__func__'
>       error: expected ')' before '__FUNCTION__'
> 
> Hence gcc 3.2 and up treat __func__ like the a variable, as per C99, while
> __FUNCTION__ has been moving from a virtual preprocessor definition in 2.95 to
> a variable, like __func__.
> 
> So in the end it doesn't matter, as concatenation has been fixed in the Linux
> source tree anyway.
> 

Great, thanks for working all that out.

So __func__ it is.  In new code.  However "convert __FUNCTION__ to
__func__" patches will be cheerfully ignored - life is too short.


err, kernel.h has

/* Trap pasters of __FUNCTION__ at compile-time */
#define __FUNCTION__ (__func__)

Re: checkpatch, a patch checking script.

[Sébastien Dugué]

On Wed, 2 May 2007 16:28:27 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:


> +		$warnings += search(qr/__FUNCION__/,

                                       ^__FUNCTION__ maybe?

> +			"Should use C99 __func__ instead of GNU __FUNCTION__\n");


  Sébastien.

Re: checkpatch, a patch checking script.

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 646 bytes --]

On Thu, 3 May 2007, [ISO-8859-1] Sébastien Dugué wrote:
> On Wed, 2 May 2007 16:28:27 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> 
> > +		$warnings += search(qr/__FUNCION__/,
> 
>                                        ^__FUNCTION__ maybe?
> 
> > +			"Should use C99 __func__ instead of GNU __FUNCTION__\n");

Bummer... Of course!

Thx!

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Martin Schwidefsky]

On Mon, 2007-04-23 at 10:45 -0700, Andrew Morton wrote: 
> > Andrew: I plan to add patches 1-5 to the for-andrew branch of the
> > git390 repository if that is fine with you. The only thing that will
> > be missing in the tree is the patch that disables wireless for s390.
> > The code does compile but without hardware it is mute to have the
> > config options. I'll wait until the git-wireless.patch is upstream.
> > Patches 7-9 depend on patches found in -mm.
> > 
> 
> umm, OK.  If it's Ok I think I'll duck it for now: -mm is full.
> 
> Over-full, really: I've been working basically continuously since Friday
> getting the current dungpile to compile and boot, and it's still miles away
> from that.

Is -mm less full now? I've seen you dropped your third patch bomb, so
maybe there is room now ..
I regenerated the kconfig patches. They fit on top of todays git but
they do have some rejects on 2.6.21-mm1. The rejects are trivial to fix,
but there are about 10 of them.

So when will be a good time to add the Kconfig patches to git390?
And do you prefer to let them rot^H^H^Hmature in -mm for a while before
they can go upstream?

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Andrew Morton]

On Wed, 09 May 2007 13:21:50 +0200 Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> On Mon, 2007-04-23 at 10:45 -0700, Andrew Morton wrote: 
> > > Andrew: I plan to add patches 1-5 to the for-andrew branch of the
> > > git390 repository if that is fine with you. The only thing that will
> > > be missing in the tree is the patch that disables wireless for s390.
> > > The code does compile but without hardware it is mute to have the
> > > config options. I'll wait until the git-wireless.patch is upstream.
> > > Patches 7-9 depend on patches found in -mm.
> > > 
> > 
> > umm, OK.  If it's Ok I think I'll duck it for now: -mm is full.
> > 
> > Over-full, really: I've been working basically continuously since Friday
> > getting the current dungpile to compile and boot, and it's still miles away
> > from that.
> 
> Is -mm less full now? I've seen you dropped your third patch bomb, so
> maybe there is room now ..

Near 500 patches, which is the lowest it's been in years.  I do appear to
have a few hundred patch-bearing emails saved away though.

> I regenerated the kconfig patches. They fit on top of todays git but
> they do have some rejects on 2.6.21-mm1. The rejects are trivial to fix,
> but there are about 10 of them.
> 
> So when will be a good time to add the Kconfig patches to git390?

Go wild.

> And do you prefer to let them rot^H^H^Hmature in -mm for a while before
> they can go upstream?

Your call.  I'd be inclined to push them early personally - Kconfig
problems tend to be easy to spot and easy to fix and there's ample time to
fine tune it.

Re: [PATCH 0/9] Kconfig: cleanup s390 v2.

[Martin Schwidefsky]

On Wed, 2007-05-09 at 09:35 -0700, Andrew Morton wrote:
> > So when will be a good time to add the Kconfig patches to git390?
> 
> Go wild.
> 
> > And do you prefer to let them rot^H^H^Hmature in -mm for a while before
> > they can go upstream?
> 
> Your call.  I'd be inclined to push them early personally - Kconfig
> problems tend to be easy to spot and easy to fix and there's ample time to
> fine tune it.

Ok, that is my feeling as well. The patches won't get better by waiting.
I will add them to the for-linus/for-andrew branches and include them in
the next please-pull for Linus. Be prepared for some breakage in -mm..

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.