[PATCH 0/7] update checkpatch to v0.29

[PATCH 0/7] update checkpatch to v0.29

[Andy Whitcroft]

Here is a few update for checkpatch that seem to have gotten lost
somewhere along the line, I though you had everything I had queued.
These have passed testing here, I have some more stuff in testing and I
will get those out soon.

This update brings an a number of minor fixes.  Thanks for the
contributions.

Complete changelog below.

-apw

Andy Whitcroft (5):
  checkpatch: possible types -- else cannot start a type
  checkpatch: indent checks -- stop when we run out of continuation
    lines
  checkpatch: format strings should not have brackets in macros
  checkpatch: limit sN/uN matches to actual bit sizes
  checkpatch: version 0.29

Daniel Walker (1):
  checkpatch: handle C99 comments correctly (performance issue)

Hannes Eder (1):
  checkpatch: make -f alias --file, add --help, more verbose help
    message

 scripts/checkpatch.pl |   84 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 65 insertions(+), 19 deletions(-)

[PATCH 1/7] checkpatch: possible types -- else cannot start a type

[Andy Whitcroft]

An else cannot start a type, it would have to be within a block after
the else.  This can trigger false modifier matching.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2d5ece7..fd64816 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1372,6 +1372,8 @@ sub process {
 			# Ignore functions being called
 			} elsif ($s =~ /^.\s*$Ident\s*\(/s) {
 
+			} elsif ($s =~ /^.\s*else\b/s) {
+
 			# declarations always start with types
 			} elsif ($prev_values eq 'E' && $s =~ /^.\s*(?:$Storage\s+)?(?:$Inline\s+)?(?:const\s+)?((?:\s*$Ident)+?)\b(?:\s+$Sparse)?\s*\**\s*(?:$Ident|\(\*[^\)]*\))(?:\s*$Modifier)?\s*(?:;|=|,|\()/s) {
 				my $type = $1;
-- 
1.6.3.3

[PATCH 2/7] checkpatch: handle C99 comments correctly (performance issue)

[Andy Whitcroft]

From: Daniel Walker <dwalker@fifo99.com>

This fixes the sanitation process in checkpatch.pl so that it blocks out
the text after a C99 style comment the same way it does with block style
comments. This prevents the text from getting processed as regular code.

Signed-off-by: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fd64816..aa009a3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -356,6 +356,13 @@ sub sanitise_line {
 			$off++;
 			next;
 		}
+		if ($sanitise_quote eq '' && substr($line, $off, 2) eq '//') {
+			$sanitise_quote = '//';
+
+			substr($res, $off, 2, $sanitise_quote);
+			$off++;
+			next;
+		}
 
 		# A \ in a string means ignore the next character.
 		if (($sanitise_quote eq "'" || $sanitise_quote eq '"') &&
@@ -379,6 +386,8 @@ sub sanitise_line {
 		#print "c<$c> SQ<$sanitise_quote>\n";
 		if ($off != 0 && $sanitise_quote eq '*/' && $c ne "\t") {
 			substr($res, $off, 1, $;);
+		} elsif ($off != 0 && $sanitise_quote eq '//' && $c ne "\t") {
+			substr($res, $off, 1, $;);
 		} elsif ($off != 0 && $sanitise_quote && $c ne "\t") {
 			substr($res, $off, 1, 'X');
 		} else {
@@ -386,6 +395,10 @@ sub sanitise_line {
 		}
 	}
 
+	if ($sanitise_quote eq '//') {
+		$sanitise_quote = '';
+	}
+
 	# The pathname on a #include may be surrounded by '<' and '>'.
 	if ($res =~ /^.\s*\#\s*include\s+\<(.*)\>/) {
 		my $clean = 'X' x length($1);
-- 
1.6.3.3

[PATCH 3/7] checkpatch: indent checks -- stop when we run out of continuation lines

[Andy Whitcroft]

Ensure we terminate when there are no futher continuation lines when
trying to determine relative indent of conditionals and their blocks.

Reported-by: John Daiker <daikerjohn@gmail.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index aa009a3..b6f267b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1547,8 +1547,9 @@ sub process {
 				    $s =~ /^\s*#\s*?/ ||
 				    $s =~ /^\s*$Ident\s*:/) {
 					$continuation = ($s =~ /^.*?\\\n/) ? 1 : 0;
-					$s =~ s/^.*?\n//;
-					$cond_lines++;
+					if ($s =~ s/^.*?\n//) {
+						$cond_lines++;
+					}
 				}
 			}
 
-- 
1.6.3.3

[PATCH 4/7] checkpatch: make -f alias --file, add --help, more verbose help message

[Andy Whitcroft]

From: Hannes Eder <hannes@hanneseder.net>

Impact:
  - More verbose help/usage message.
  - Make the option -f an alias for --file.
  - On -h, --help, and --version display help message and exit(0).
  - With no FILE(s) given, exit(1) with "no input files".
  - On invalid options display help/usage and exit(1).

Based on a patch by Pavel Machek.

Signed-off-by: Hannes Eder <hannes@hanneseder.net>
Cc: Pavel Machek <pavel@suse.cz>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   55 +++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b6f267b..e3c6b49 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -28,6 +28,41 @@ my $mailback = 0;
 my $summary_file = 0;
 my $root;
 my %debug;
+my $help = 0;
+
+sub help {
+	my ($exitcode) = @_;
+
+	print << "EOM";
+Usage: $P [OPTION]... [FILE]...
+Version: $V
+
+Options:
+  -q, --quiet                quiet
+  --no-tree                  run without a kernel tree
+  --no-signoff               do not check for 'Signed-off-by' line
+  --patch                    treat FILE as patchfile (default)
+  --emacs                    emacs compile window format
+  --terse                    one line per report
+  -f, --file                 treat FILE as regular source file
+  --subjective, --strict     enable more subjective tests
+  --root=PATH                PATH to the kernel tree root
+  --no-summary               suppress the per-file summary
+  --mailback                 only produce a report in case of warnings/errors
+  --summary-file             include the filename in summary
+  --debug KEY=[0|1]          turn on/off debugging of KEY, where KEY is one of
+                             'values', 'possible', 'type', and 'attr' (default
+                             is all off)
+  --test-only=WORD           report only warnings/errors containing WORD
+                             literally
+  -h, --help, --version      display this help and exit
+
+When FILE is - read standard input.
+EOM
+
+	exit($exitcode);
+}
+
 GetOptions(
 	'q|quiet+'	=> \$quiet,
 	'tree!'		=> \$tree,
@@ -35,7 +70,7 @@ GetOptions(
 	'patch!'	=> \$chk_patch,
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
-	'file!'		=> \$file,
+	'f|file!'	=> \$file,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
 	'root=s'	=> \$root,
@@ -45,22 +80,16 @@ GetOptions(
 
 	'debug=s'	=> \%debug,
 	'test-only=s'	=> \$tst_only,
-) or exit;
+	'h|help'	=> \$help,
+	'version'	=> \$help
+) or help(1);
+
+help(0) if ($help);
 
 my $exit = 0;
 
 if ($#ARGV < 0) {
-	print "usage: $P [options] patchfile\n";
-	print "version: $V\n";
-	print "options: -q               => quiet\n";
-	print "         --no-tree        => run without a kernel tree\n";
-	print "         --terse          => one line per report\n";
-	print "         --emacs          => emacs compile window format\n";
-	print "         --file           => check a source file\n";
-	print "         --strict         => enable more subjective tests\n";
-	print "         --root           => path to the kernel tree root\n";
-	print "         --no-summary     => suppress the per-file summary\n";
-	print "         --summary-file   => include the filename in summary\n";
+	print "$P: no input files\n";
 	exit(1);
 }
 
-- 
1.6.3.3

Re: [PATCH 4/7] checkpatch: make -f alias --file, add --help, more verbose help message

[Pavel Machek]

On Fri 2009-08-21 22:46:07, Andy Whitcroft wrote:
> From: Hannes Eder <hannes@hanneseder.net>
> 
> Impact:
>   - More verbose help/usage message.
>   - Make the option -f an alias for --file.
>   - On -h, --help, and --version display help message and exit(0).
>   - With no FILE(s) given, exit(1) with "no input files".
>   - On invalid options display help/usage and exit(1).
> 
> Based on a patch by Pavel Machek.
> 
> Signed-off-by: Hannes Eder <hannes@hanneseder.net>

Acked-by: Pavel Machek <pavel@suse.cz>

> Signed-off-by: Andy Whitcroft <apw@canonical.com>

Thanks!


> ---
>  scripts/checkpatch.pl |   55 +++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b6f267b..e3c6b49 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -28,6 +28,41 @@ my $mailback = 0;
>  my $summary_file = 0;
>  my $root;
>  my %debug;
> +my $help = 0;
> +
> +sub help {
> +	my ($exitcode) = @_;
> +
> +	print << "EOM";
> +Usage: $P [OPTION]... [FILE]...
> +Version: $V
> +
> +Options:
> +  -q, --quiet                quiet
> +  --no-tree                  run without a kernel tree
> +  --no-signoff               do not check for 'Signed-off-by' line
> +  --patch                    treat FILE as patchfile (default)
> +  --emacs                    emacs compile window format
> +  --terse                    one line per report
> +  -f, --file                 treat FILE as regular source file
> +  --subjective, --strict     enable more subjective tests
> +  --root=PATH                PATH to the kernel tree root
> +  --no-summary               suppress the per-file summary
> +  --mailback                 only produce a report in case of warnings/errors
> +  --summary-file             include the filename in summary
> +  --debug KEY=[0|1]          turn on/off debugging of KEY, where KEY is one of
> +                             'values', 'possible', 'type', and 'attr' (default
> +                             is all off)
> +  --test-only=WORD           report only warnings/errors containing WORD
> +                             literally
> +  -h, --help, --version      display this help and exit
> +
> +When FILE is - read standard input.
> +EOM
> +
> +	exit($exitcode);
> +}
> +
>  GetOptions(
>  	'q|quiet+'	=> \$quiet,
>  	'tree!'		=> \$tree,
> @@ -35,7 +70,7 @@ GetOptions(
>  	'patch!'	=> \$chk_patch,
>  	'emacs!'	=> \$emacs,
>  	'terse!'	=> \$terse,
> -	'file!'		=> \$file,
> +	'f|file!'	=> \$file,
>  	'subjective!'	=> \$check,
>  	'strict!'	=> \$check,
>  	'root=s'	=> \$root,
> @@ -45,22 +80,16 @@ GetOptions(
>  
>  	'debug=s'	=> \%debug,
>  	'test-only=s'	=> \$tst_only,
> -) or exit;
> +	'h|help'	=> \$help,
> +	'version'	=> \$help
> +) or help(1);
> +
> +help(0) if ($help);
>  
>  my $exit = 0;
>  
>  if ($#ARGV < 0) {
> -	print "usage: $P [options] patchfile\n";
> -	print "version: $V\n";
> -	print "options: -q               => quiet\n";
> -	print "         --no-tree        => run without a kernel tree\n";
> -	print "         --terse          => one line per report\n";
> -	print "         --emacs          => emacs compile window format\n";
> -	print "         --file           => check a source file\n";
> -	print "         --strict         => enable more subjective tests\n";
> -	print "         --root           => path to the kernel tree root\n";
> -	print "         --no-summary     => suppress the per-file summary\n";
> -	print "         --summary-file   => include the filename in summary\n";
> +	print "$P: no input files\n";
>  	exit(1);
>  }
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 5/7] checkpatch: format strings should not have brackets in macros

[Andy Whitcroft]

We should not recommend braces for the following:

    #define pr_fmt(fmt)    "%s: " fmt, __func__

allow things with double quotes round them to avoid this check.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3c6b49..57df906 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2288,7 +2288,8 @@ sub process {
 				DECLARE_PER_CPU|
 				DEFINE_PER_CPU|
 				__typeof__\(|
-				\.$Ident\s*=\s*
+				\.$Ident\s*=\s*|
+				^\"|\"$
 			}x;
 			#print "REST<$rest> dstat<$dstat>\n";
 			if ($rest ne '') {
-- 
1.6.3.3

[PATCH 6/7] checkpatch: limit sN/uN matches to actual bit sizes

[Andy Whitcroft]

Limit our type matcher to the s/u/le/be etc sizes that actually exist
to prevent miss categorising s2 as a type.  Fix up the spelling of the
error also.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 57df906..7fee823 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -182,7 +182,7 @@ our $UTF8	= qr {
 }x;
 
 our $typeTypedefs = qr{(?x:
-	(?:__)?(?:u|s|be|le)(?:\d|\d\d)|
+	(?:__)?(?:u|s|be|le)(?:8|16|32|64)|
 	atomic_t
 )};
 
@@ -1936,7 +1936,7 @@ sub process {
 						# A unary '*' may be const
 
 					} elsif ($ctx =~ /.xW/) {
-						ERROR("Aspace prohibited after that '$op' $at\n" . $hereptr);
+						ERROR("space prohibited after that '$op' $at\n" . $hereptr);
 					}
 
 				# unary ++ and unary -- are allowed no space on one side.
-- 
1.6.3.3

[PATCH 7/7] checkpatch: version 0.29

[Andy Whitcroft]

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7fee823..8b1dfd5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -10,7 +10,7 @@ use strict;
 my $P = $0;
 $P =~ s@.*/@@g;
 
-my $V = '0.28';
+my $V = '0.29';
 
 use Getopt::Long qw(:config no_auto_abbrev);
 
-- 
1.6.3.3

Re: [PATCH 0/7] update checkpatch to v0.29

[Andrew Morton]

On Fri, 21 Aug 2009 22:46:03 +0100
Andy Whitcroft <apw@canonical.com> wrote:

> Here is a few update for checkpatch that seem to have gotten lost
> somewhere along the line, I though you had everything I had queued.
> These have passed testing here, I have some more stuff in testing and I
> will get those out soon.
> 
> This update brings an a number of minor fixes.  Thanks for the
> contributions.

Running checkpatch 0.29 against the below diff produces a warning spew:

Use of uninitialized value in length at scripts/checkpatch.pl line 1480.
Use of uninitialized value in substr at scripts/checkpatch.pl line 1480.
Use of uninitialized value in substr at scripts/checkpatch.pl line 1480.
Use of uninitialized value in pattern match (m//) at scripts/checkpatch.pl line 1488.
Use of uninitialized value in pattern match (m//) at scripts/checkpatch.pl line 1545.
Use of uninitialized value in length at scripts/checkpatch.pl line 1480.
Use of uninitialized value in substr at scripts/checkpatch.pl line 1480.
Use of uninitialized value in substr at scripts/checkpatch.pl line 1480.
Use of uninitialized value in pattern match (m//) at scripts/checkpatch.pl line 1488.
Use of uninitialized value in pattern match (m//) at scripts/checkpatch.pl line 1545.
total: 0 errors, 0 warnings, 72 lines checked




From: Wu Fengguang <fengguang.wu@intel.com>

For mem_cgroup, shrink_zone() may call shrink_list() with nr_to_scan=1, in
which case shrink_list() _still_ calls isolate_pages() with the much
larger SWAP_CLUSTER_MAX.  It effectively scales up the inactive list scan
rate by up to 32 times.

For example, with 16k inactive pages and DEF_PRIORITY=12, (16k >> 12)=4. 
So when shrink_zone() expects to scan 4 pages in the active/inactive list,
the active list will be scanned 4 pages, while the inactive list will be
(over) scanned SWAP_CLUSTER_MAX=32 pages in effect.  And that could break
the balance between the two lists.

It can further impact the scan of anon active list, due to the anon
active/inactive ratio rebalance logic in balance_pgdat()/shrink_zone():

inactive anon list over scanned => inactive_anon_is_low() == TRUE
                                => shrink_active_list()
                                => active anon list over scanned

So the end result may be

- anon inactive  => over scanned
- anon active    => over scanned (maybe not as much)
- file inactive  => over scanned
- file active    => under scanned (relatively)

The accesses to nr_saved_scan are not lock protected and so not 100%
accurate, however we can tolerate small errors and the resulted small
imbalanced scan rates between zones.

Cc: Rik van Riel <riel@redhat.com>
Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/mmzone.h |    6 +++++-
 mm/page_alloc.c        |    2 +-
 mm/vmscan.c            |   20 +++++++++++---------
 3 files changed, 17 insertions(+), 11 deletions(-)

diff -puN include/linux/mmzone.h~mm-do-batched-scans-for-mem_cgroup include/linux/mmzone.h
--- a/include/linux/mmzone.h~mm-do-batched-scans-for-mem_cgroup
+++ a/include/linux/mmzone.h
@@ -273,6 +273,11 @@ struct zone_reclaim_stat {
 	 */
 	unsigned long		recent_rotated[2];
 	unsigned long		recent_scanned[2];
+
+	/*
+	 * accumulated for batching
+	 */
+	unsigned long		nr_saved_scan[NR_LRU_LISTS];
 };
 
 struct zone {
@@ -327,7 +332,6 @@ struct zone {
 	spinlock_t		lru_lock;	
 	struct zone_lru {
 		struct list_head list;
-		unsigned long nr_saved_scan;	/* accumulated for batching */
 	} lru[NR_LRU_LISTS];
 
 	struct zone_reclaim_stat reclaim_stat;
diff -puN mm/page_alloc.c~mm-do-batched-scans-for-mem_cgroup mm/page_alloc.c
--- a/mm/page_alloc.c~mm-do-batched-scans-for-mem_cgroup
+++ a/mm/page_alloc.c
@@ -3830,7 +3830,7 @@ static void __paginginit free_area_init_
 		zone_pcp_init(zone);
 		for_each_lru(l) {
 			INIT_LIST_HEAD(&zone->lru[l].list);
-			zone->lru[l].nr_saved_scan = 0;
+			zone->reclaim_stat.nr_saved_scan[l] = 0;
 		}
 		zone->reclaim_stat.recent_rotated[0] = 0;
 		zone->reclaim_stat.recent_rotated[1] = 0;
diff -puN mm/vmscan.c~mm-do-batched-scans-for-mem_cgroup mm/vmscan.c
--- a/mm/vmscan.c~mm-do-batched-scans-for-mem_cgroup
+++ a/mm/vmscan.c
@@ -1586,6 +1586,7 @@ static void shrink_zone(int priority, st
 	enum lru_list l;
 	unsigned long nr_reclaimed = sc->nr_reclaimed;
 	unsigned long swap_cluster_max = sc->swap_cluster_max;
+	struct zone_reclaim_stat *reclaim_stat = get_reclaim_stat(zone, sc);
 	int noswap = 0;
 
 	/* If we have no swap space, do not bother scanning anon pages. */
@@ -1605,12 +1606,9 @@ static void shrink_zone(int priority, st
 			scan >>= priority;
 			scan = (scan * percent[file]) / 100;
 		}
-		if (scanning_global_lru(sc))
-			nr[l] = nr_scan_try_batch(scan,
-						  &zone->lru[l].nr_saved_scan,
-						  swap_cluster_max);
-		else
-			nr[l] = scan;
+		nr[l] = nr_scan_try_batch(scan,
+					  &reclaim_stat->nr_saved_scan[l],
+					  swap_cluster_max);
 	}
 
 	while (nr[LRU_INACTIVE_ANON] || nr[LRU_ACTIVE_FILE] ||
@@ -2220,6 +2218,7 @@ static void shrink_all_zones(unsigned lo
 {
 	struct zone *zone;
 	unsigned long nr_reclaimed = 0;
+	struct zone_reclaim_stat *reclaim_stat;
 
 	for_each_populated_zone(zone) {
 		enum lru_list l;
@@ -2236,11 +2235,14 @@ static void shrink_all_zones(unsigned lo
 						l == LRU_ACTIVE_FILE))
 				continue;
 
-			zone->lru[l].nr_saved_scan += (lru_pages >> prio) + 1;
-			if (zone->lru[l].nr_saved_scan >= nr_pages || pass > 3) {
+			reclaim_stat = get_reclaim_stat(zone, sc);
+			reclaim_stat->nr_saved_scan[l] +=
+						(lru_pages >> prio) + 1;
+			if (reclaim_stat->nr_saved_scan[l]
+						>= nr_pages || pass > 3) {
 				unsigned long nr_to_scan;
 
-				zone->lru[l].nr_saved_scan = 0;
+				reclaim_stat->nr_saved_scan[l] = 0;
 				nr_to_scan = min(nr_pages, lru_pages);
 				nr_reclaimed += shrink_list(l, nr_to_scan, zone,
 								sc, prio);
_

checkpatch infinite loop ?

[Eric Dumazet]

Hi Andy

Following checkpatch.pl invocation has a problem in current linux-2.6 tree :

scripts/checkpatch.pl --file include/linux/inetdevice.h

linux-2.6.27.10/scripts/checkpatch.pl is OK, but starting from 2.6.28, there is an infinite loop...

Thanks