[PATCH 0/7] Support for automatic checkpatch running in the kernel

[PATCH 0/7] Support for automatic checkpatch running in the kernel

[Knut Omang]

This patch series implements features to facilitate running checkpatch on the
entire kernel as part of automatic testing.

This is done by by adding a few small features to checkpatch and put these
features to use to implement support for a new Makefile environment variable
P={1,2} following the pattern of sparse and the C={1,2} variable.  The basic
functionality + docs are in patch #1-4.

It also fixes a minor issue with "checkpatch --fix-inplace" found during testing
(patch #5).

The most important checkpatch feature added is the --ignore-cfg feature, which
takes a file argument and parses that file according to this minimal language:

       # comments
       line_len <n>
       except checkpatch_type [files ...]
       pervasive checkpatch_type1 [checkpatch_type2 ...]

With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
same directory as the source file. If that file exists, checkpatch will be run
with an implicit --strict and with the @ignore list expanded with content from
the configuration file.  If it does not exist, make will simply silently ignore
the file.

Patches #6-7 enhances this behaviour to also scan the directories above a file
until a match for the --file parameter is found.

The idea is that the community can work to add checkpatch.cfg files to
directories, serving both as documentation and as a way for subsystem
maintainers to enforce policies and individual tastes as well as TODOs and/or
priorities, to make it easier for newcomers to contribute in this area. By
ignoring directories without such files, automation can start right away as it
is trivially possible to run errorless with P=2 for the entire kernel.

The patches includes a documentation file with some more details.

This patch set has evolved from an earlier implementation I made that was just a
wrapper script around checkpatch. That version have been used for a number of
years on a driver project I worked on where we had automatic checkin regression
testing. I extended that to also run checkpatch to avoid having to clean up
frequent unintended whitespace changes and style violations from others...

I have also tested this version on some directories I am familiar with.  The
result of that work is available in two patch sets of 10 and 11 patches, but we
agreed that it would be better to post them as separate patch sets later.

Those patch sets illustrates how I picture the "flow" from just "reining in" the
checkpatch detections to actually fixing classes of checkpatch issues one by
one, while updating the checkpatch.cfg file(s) to have 0 errors or warnings at
any commit boundary.

The combined set is available here:

   git://github.com/knuto/linux.git  branch checkpatch

Comments and suggestions appreciated!

Thanks,
Knut

Knut Omang (7):
  checkpatch: Implement new --ignore-cfg parameter
  kbuild: Add P= command line flag to run checkpatch
  checkpatch: Add a few convenience options to disable/modify features
  Documentation: Add documentation for the new P= Makefile option
  checkpatch: Improve --fix-inplace for TABSTOP
  checkpatch: Make --ignore-cfg look recursively for the file
  Documentation: Update checkpatch --ignore-cfg description

 Documentation/dev-tools/index.rst          |   1 +-
 Documentation/dev-tools/run-checkpatch.rst | 109 ++++++++++++++++++++++-
 Makefile                                   |  20 +++-
 scripts/Makefile.build                     |  13 +++-
 scripts/checkpatch.pl                      | 108 +++++++++++++++++++++-
 5 files changed, 249 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/dev-tools/run-checkpatch.rst

base-commit: bebc6082da0a9f5d47a1ea2edc099bf671058bd4
-- 
git-series 0.9.1

[PATCH 1/7] checkpatch: Implement new --ignore-cfg parameter

[Knut Omang]

This parameter is intended to be used in a subsequent commit to kbuild to allow
a convenient way to run checkpatch from make.

The parameter --ignore-cfg causes checkpatch.pl to look for the file given as
parameter, either directly from where checkpatch is executed, or in the
directory where the file to check is located. With this parameter checkpatch is
now limited to run on a single file for each invocation. This should not be a
problem for the Makefile scenario as it is the standard behaviour anyway.

This file accepts the following syntax:

	# comments
	line_len <n>
	except checkpatch_type [files ...]
	pervasive checkpatch_type1 [checkpatch_type2 ...]

The line_len command defines the upper bound of characters per line tolerated in
this directory.

The except command takes a checkpatch type such as for example MACRO_ARG_REUSE,
and a set of files that should not be subject to this particular check type.

The pervasive command disables the listed types of checks for all the files in
the directory. The except and pervasive command can be used cumulatively to add
more exceptions.

By accepting comments and multiple lines of commands, the idea is that the
maintainer or someone else with good knowledge of the code can maintain a file
per directory and group the different commands into commented sections that can
serve both as documentation of the current checkpatch status, a way to define
the line of tolerance (and gradually tighten it as fixes comes in) and as
documentation of TODOs and dont's if there are well justified exceptions.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
Acked-by: Håkon Bugge <haakon.bugge@oracle.com>
Acked-by: Åsmund Østvold <asmund.ostvold@oracle.com>
---
 scripts/checkpatch.pl | 57 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8b80bac..834a1d8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -48,6 +48,7 @@ my %ignore_type = ();
 my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
+my $ignore_cfg_file;
 my $max_line_length = 80;
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
@@ -90,6 +91,9 @@ Options:
   --list-types               list the possible message types
   --types TYPE(,TYPE2...)    show only these comma separated message types
   --ignore TYPE(,TYPE2...)   ignore various comma separated message types
+  --ignore-cfg FILE	     parse this file for a detailed file specific ignore list,
+			     silently exit without checking if an ignore config file
+			     is not found.
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
   --min-conf-desc-length=n   set the min description length, if shorter, warn
@@ -201,6 +205,7 @@ GetOptions(
 	'terse!'	=> \$terse,
 	'showfile!'	=> \$showfile,
 	'f|file!'	=> \$file,
+	'ignore-cfg=s'	=> \$ignore_cfg_file,
 	'g|git!'	=> \$git,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
@@ -233,6 +238,9 @@ help(0) if ($help);
 
 list_types(0) if ($list_types);
 
+# Enforce --strict if used with a ignore configuration file:
+$check = 1 if defined($ignore_cfg_file);
+
 $fix = 1 if ($fix_inplace);
 $check_orig = $check;
 
@@ -291,6 +299,7 @@ sub hash_show_words {
 	}
 }
 
+parse_ignore_cfg_file(@ARGV) || exit(0);
 hash_save_array_words(\%ignore_type, \@ignore);
 hash_save_array_words(\%use_type, \@use);
 
@@ -2160,6 +2169,54 @@ sub pos_last_openparen {
 	return length(expand_tabs(substr($line, 0, $last_openparen))) + 1;
 }
 
+# Checkpatch suppression list configuration file support
+#
+# See Documentation/dev-tools/run-checkpatch.rst
+#
+sub parse_ignore_cfg_file {
+	defined($ignore_cfg_file) || return 1;
+	my $path = shift(@_);
+	my $filename = basename($path);
+	my $dir = dirname($path);
+	my %IgnoreCfgKeywords = (
+		'except'	=> sub { my $type = shift(@_);
+					 grep( /^$filename$/, @_ ) && push(@ignore, $type);
+				       },
+		'pervasive'	=> sub { push(@ignore, @_); },
+		'line_len'	=> sub { $max_line_length = shift(@_); }
+	);
+	my $ignfile;
+
+	( -f $ignore_cfg_file ) || ( $ignore_cfg_file = "$dir/$ignore_cfg_file" );
+	( ! -f $ignore_cfg_file ) && return 0;
+	open($ignfile, '<', "$ignore_cfg_file") || return 0;
+
+	($#_ >= 0) &&
+		die "$P: The --ignore-cfg option is only supported with one source file at a time!\n";
+
+	while (<$ignfile>) {
+		my $line = $_;
+
+		$line =~ s/\s*\n?$//g;
+		$line =~ s/^\s*//g;
+		$line =~ s/\s+/ /g;
+		$line =~ s/#.*//g;
+		$line =~ m/^\s*$/ && next;
+
+		my @words = split(" ", $line);
+		my $kw = shift(@words);
+		defined($kw) or next;
+		my $cmd = $IgnoreCfgKeywords{$kw};
+		if ( !defined($cmd) ) {
+			print "$line\n";
+			warn("Unknown keyword \"$kw\" in file \"$ignore_cfg_file\"\n");
+			next;
+		}
+		&$cmd(@words);
+	}
+	return 1;
+}
+
 sub process {
 	my $filename = shift;
 
-- 
git-series 0.9.1

[PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Knut Omang]

Add interpretation of a new environment variable P={1,2} in spirit of the
C= option, but executing checkpatch instead of sparse.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
Acked-by: Åsmund Østvold <asmund.ostvold@oracle.com>
---
 Makefile               | 20 +++++++++++++++++++-
 scripts/Makefile.build | 13 +++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index ccd9818..eb4bca9 100644
--- a/Makefile
+++ b/Makefile
@@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC
   KBUILD_CHECKSRC = 0
 endif
 
+# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg
+#
+# Use 'make P=1' to enable checking of only re-compiled files.
+# Use 'make P=2' to enable checking of *all* source files, regardless
+#
+# See the file "Documentation/dev-tools/run-checkpatch.rst" for more details,
+#
+ifeq ("$(origin P)", "command line")
+  KBUILD_CHECKPATCH = $(P)
+endif
+ifndef KBUILD_CHECKPATCH
+  KBUILD_CHECKPATCH = 0
+endif
+
 # Use make M=dir to specify directory of external module to build
 # Old syntax make ... SUBDIRS=$PWD is still supported
 # Setting the environment variable KBUILD_EXTMOD take precedence
@@ -340,7 +354,7 @@ ifeq ($(MAKECMDGOALS),)
 endif
 
 export KBUILD_MODULES KBUILD_BUILTIN
-export KBUILD_CHECKSRC KBUILD_SRC KBUILD_EXTMOD
+export KBUILD_CHECKSRC KBUILD_CHECKPATCH KBUILD_SRC KBUILD_EXTMOD
 
 # We need some generic definitions (do not try to remake the file).
 scripts/Kbuild.include: ;
@@ -363,9 +377,12 @@ DEPMOD		= /sbin/depmod
 PERL		= perl
 PYTHON		= python
 CHECK		= sparse
+CHECKP		= scripts/checkpatch.pl
 
 CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
 		  -Wbitwise -Wno-return-void $(CF)
+CHECKPFLAGS    := --quiet --show-types --emacs \
+		  --ignore-cfg checkpatch.cfg --file $(PF)
 NOSTDINC_FLAGS  =
 CFLAGS_MODULE   =
 AFLAGS_MODULE   =
@@ -419,6 +436,7 @@ export ARCH SRCARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
 export CPP AR NM STRIP OBJCOPY OBJDUMP HOSTLDFLAGS HOST_LOADLIBES
 export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
+export CHECKP CHECKPFLAGS
 
 export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
 export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bb831d4..cfc540a 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -109,6 +109,17 @@ ifneq ($(KBUILD_CHECKSRC),0)
   endif
 endif
 
+# Run per-directory/per-file specific checkpatch testing:
+ifneq ($(KBUILD_CHECKPATCH),0)
+  ifeq ($(KBUILD_CHECKPATCH),2)
+    quiet_cmd_force_checkpatch = CHECKP   $<
+          cmd_force_checkpatch = $(srctree)/$(CHECKP) $(POPTS) $< $(CHECKPFLAGS) ;
+  else
+      quiet_cmd_checkpatch     = CHECKP   $<
+            cmd_checkpatch     = $(srctree)/$(CHECKP) $(POPTS) $< $(CHECKPFLAGS) ;
+  endif
+endif
+
 # Do section mismatch analysis for each module/built-in.o
 ifdef CONFIG_DEBUG_SECTION_MISMATCH
   cmd_secanalysis = ; scripts/mod/modpost $@
@@ -290,6 +301,7 @@ objtool_dep = $(objtool_obj)					\
 
 define rule_cc_o_c
 	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
+	$(call echo-cmd,checkpatch) $(cmd_checkpatch)			  \
 	$(call cmd_and_fixdep,cc_o_c)					  \
 	$(cmd_modversions_c)						  \
 	$(call echo-cmd,objtool) $(cmd_objtool)				  \
@@ -312,6 +324,7 @@ endif
 # Built-in and composite module parts
 $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
 	$(call cmd,force_checksrc)
+	$(call cmd,force_checkpatch)
 	$(call if_changed_rule,cc_o_c)
 
 # Single-part modules are special since we need to mark them in $(MODVERDIR)
-- 
git-series 0.9.1

[PATCH 3/7] checkpatch: Add a few convenience options to disable/modify features

[Knut Omang]

These are options that are useful when running from make, to allow
flags already supplied by make to be overridden (later on the command line)

Add these two options to cancel the effects of --quiet and --ignore-cfg:
--no-quiet
--no-ignore-cfg

Also added:
--req-ignore-cfg:  With --ignore-cfg - treat a nonexistent file as if it
    were an empty file, effectively enforcing a full checkpatch --strict run.
    The default behaviour with --ignore-cfg is not to perform any
    checkpatch checking if an ignore configuration file is not found.
    This was done to enable automated checkpatch runs for directories
    that are "clean" according to the configuration.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
Acked-by: Håkon Bugge <haakon.bugge@oracle.com>
Acked-by: Åsmund Østvold <asmund.ostvold@oracle.com>
---
 scripts/checkpatch.pl | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 834a1d8..387292f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -20,6 +20,7 @@ my $V = '0.32';
 use Getopt::Long qw(:config no_auto_abbrev);
 
 my $quiet = 0;
+my $no_quiet;
 my $tree = 1;
 my $chk_signoff = 1;
 my $chk_patch = 1;
@@ -49,6 +50,9 @@ my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
 my $ignore_cfg_file;
+my $no_ignore_cfg_file;
+my $req_ignore_cfg_file;
+my $not_defined;
 my $max_line_length = 80;
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
@@ -70,6 +74,7 @@ Version: $V
 
 Options:
   -q, --quiet                quiet
+  --no-quiet		     Undo the effect of a previous --quiet argument
   --no-tree                  run without a kernel tree
   --no-signoff               do not check for 'Signed-off-by' line
   --patch                    treat FILE as patchfile (default)
@@ -94,6 +99,10 @@ Options:
   --ignore-cfg FILE	     parse this file for a detailed file specific ignore list,
 			     silently exit without checking if an ignore config file
 			     is not found.
+  --req-ignore-cfg	     With --ignore-cfg - if the file is not found, behave as
+			     if the file existed and was empty, which is identical
+			     to a normal run with --strict
+  --no-ignore-cfg	     undo the effect of any previous --ignore-cfg argument
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
   --min-conf-desc-length=n   set the min description length, if shorter, warn
@@ -104,6 +113,8 @@ Options:
   --debug KEY=[0|1]          turn on/off debugging of KEY, where KEY is one of
                              'values', 'possible', 'type', and 'attr' (default
                              is all off)
+  --no-errors		     Let checkpatch return status 0 even in presence
+			     of errors.
   --test-only=WORD           report only warnings/errors containing WORD
                              literally
   --fix                      EXPERIMENTAL - may create horrible results
@@ -198,6 +209,7 @@ foreach (@ARGV) {
 
 GetOptions(
 	'q|quiet+'	=> \$quiet,
+	'no-quiet'	=> \$no_quiet,
 	'tree!'		=> \$tree,
 	'signoff!'	=> \$chk_signoff,
 	'patch!'	=> \$chk_patch,
@@ -206,6 +218,8 @@ GetOptions(
 	'showfile!'	=> \$showfile,
 	'f|file!'	=> \$file,
 	'ignore-cfg=s'	=> \$ignore_cfg_file,
+	'no-ignore-cfg!'	=> \$no_ignore_cfg_file,
+	'req-ignore-cfg!'	=> \$req_ignore_cfg_file,
 	'g|git!'	=> \$git,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
@@ -238,6 +252,9 @@ help(0) if ($help);
 
 list_types(0) if ($list_types);
 
+$quiet = 0 if defined($no_quiet);
+$ignore_cfg_file = $not_defined if defined($no_ignore_cfg_file);
+
 # Enforce --strict if used with a ignore configuration file:
 $check = 1 if defined($ignore_cfg_file);
 
@@ -299,7 +316,7 @@ sub hash_show_words {
 	}
 }
 
-parse_ignore_cfg_file(@ARGV) || exit(0);
+parse_ignore_cfg_file(@ARGV) || defined($req_ignore_cfg_file) || exit(0);
 hash_save_array_words(\%ignore_type, \@ignore);
 hash_save_array_words(\%use_type, \@use);
 
-- 
git-series 0.9.1

[PATCH 4/7] Documentation: Add documentation for the new P= Makefile option

[Knut Omang]

Signed-off-by: Knut Omang <knut.omang@oracle.com>
Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Åsmund Østvold <asmund.ostvold@oracle.com>
Reviewed-by: John Haxby <john.haxby@oracle.com>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 Documentation/dev-tools/index.rst          |   1 +-
 Documentation/dev-tools/run-checkpatch.rst | 105 ++++++++++++++++++++++-
 2 files changed, 106 insertions(+)
 create mode 100644 Documentation/dev-tools/run-checkpatch.rst

diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst
index a81787c..b678a3a 100644
--- a/Documentation/dev-tools/index.rst
+++ b/Documentation/dev-tools/index.rst
@@ -16,6 +16,7 @@ whole; patches welcome!
 
    coccinelle
    sparse
+   run-checkpatch
    kcov
    gcov
    kasan
diff --git a/Documentation/dev-tools/run-checkpatch.rst b/Documentation/dev-tools/run-checkpatch.rst
new file mode 100644
index 0000000..c72f818
--- /dev/null
+++ b/Documentation/dev-tools/run-checkpatch.rst
@@ -0,0 +1,105 @@
+.. Copyright 2017 Knut Omang <knut.omang@oracle.com>
+
+Makefile support for systematic checkpatch testing
+==================================================
+
+The scripts/checkpatch.pl script is able to detect a lot of syntactic and
+semantic issues with the code, and is also constantly evolving and detecting
+more. In an ideal world, all source files should adhere to whatever rules
+imposed by checkpatch.pl with all bells and whistles enabled, in a way that
+checkpatch can be run as a reflex by developers (and by bots) from the top level
+Makefile for every changing source file. In the real world however there's a
+number of challenges:
+
+* Sometimes there are valid reasons for accepting violations of a checkpatch
+  rule, even if that rule is a sensible one in the general case.
+* Some subsystems have different restrictions and requirements for checkpatch.
+  (Ideally, the number of subsystems with differing restrictions and
+  requirements will diminish over time.)
+* Similarly, the kernel contains a lot of code that predates checkpatch, or at
+  least some of the newer rules, and we would like checkpatch to evolve without
+  requiring the need to fix all issues detected with it in the same commit.
+* On the other hand, we want to make sure that files that are checkpatch clean
+  (to some well defined extent, such as passing checkpatch with checks only for
+  certain important types of issues) keep being so.
+
+This is the purpose of supplying the option ``--ignore-cfg checkpatch.cfg`` to
+``scripts/checkpatch.pl``. It will then look for a file named ``checkpatch.cfg``
+in the current directory or alternatively in the directory of the source
+file. If that file exists, checkpatch parses a set of rules from it, and use
+them to determine how to invoke checkpatch for a particular file. The kernel
+Makefile system supports using this feature as an integrated part of compiling
+the code.
+
+The ignore configuration file
+-----------------------------
+
+The ignore configuration file can be used to set policies and "rein in"
+checkpatch errors piece by piece for a particular subsystem or driver.
+The the following syntax is supported::
+
+	# comments
+	line_len <n>
+	except checkpatch_type [files ...]
+	pervasive checkpatch_type1 [checkpatch_type2 ...]
+
+The ``line_len`` directive defines the upper bound of characters per line
+tolerated in this directory. The ``except`` directive takes a checkpatch type
+such as for example ``MACRO_ARG_REUSE``, and a set of files that should not be
+subject to this particular check type.  You can run ``scripts/checkpatch.pl
+--list-types`` to see what types that are available. The ``pervasive`` directive
+disables the listed types of checks for all the files in the directory.  The
+``except`` and ``pervasive`` directives can be used cumulatively to add more
+exceptions.
+
+Running checkpatch from make
+----------------------------
+
+You can run checkpatch subject to rules defined in ``checkpatch.cfg`` in the
+directory of the source file by using "make P=1" to run checkpatch on all files
+that gets recompiled, or "make P=2" to run checkpatch on all source files.
+
+A make variable ``PF`` allows passing additional parameters to
+checkpatch.pl. You can for instance use::
+
+	make P=2 PF="--fix-inplace"
+
+to have checkpatch trying to fix issues it finds - *make sure you have a clean
+git tree and carefully review the output afterwards!* Combine this with
+selectively enabling of types of errors via changes to the local
+``checkpatch.cfg``, and you can focus on fixing up errors subsystem or driver by
+driver on a type by type basis.
+
+By default checkpatch will skip all files in directories without a
+checkpatch.cfg file when invoked with the --ignore-cfg parameter.  This is to
+allow builds with P=2 to pass even for subsystems that has not yet done anything
+to rein in checkpatch errors. At some point when all subsystems and drivers
+either have fixed all checkpatch errors or added proper checkpatch.cfg files,
+this can be changed.
+
+To force checkpatch to run a full run in directories without a checkpatch.cfg
+file as well, use::
+
+	make P=2 PF="--req-ignore-cfg"
+
+If you like to see all the warnings and errors produced by checkpatch, ignoring
+any checkpatch.cfg files, you can use::
+
+	make -k P=2 PF="--no-ignore-cfg"
+
+or for a specific module directory::
+
+	make -k P=2 M=drivers/infiniband/core PF="--no-ignore-cfg"
+
+with the -k option to ``make`` to let it continue upon errors.
+
+Ever tightening checkpatch rules
+--------------------------------
+
+Commit the changes to checkpatch.cfg together with the code changes that fixes a
+particular type of issue, this will allow automatic checkpatch testing. This way
+we can ensure that new errors of that particular type do not inadvertently sneak
+in again! This can be done at any subsystem or module maintainer's discretion
+and at the right time without having to do it all at the same time.
+
+Before submitting your changes, verify that "make P=2" passes with no errors.
-- 
git-series 0.9.1

[PATCH 5/7] checkpatch: Improve --fix-inplace for TABSTOP

[Knut Omang]

If the --fix-inplace option for TABSTOP encounters a sitation with several
spaces (but less than 8) at the end of an indentation, it will assume that there
are extra indentation and align back to the nearest tabstop instead of the next.

This might go undetected in a "full" checkpatch --fix-inplace run, as this
potential new error will be handled by SUSPECT_CODE_INDENT further down in the
script.  The code for TABSTOP have limited "knowledge" of the previous line.
Adding complexity when it is taken care of later anyway is maybe unnecessary/undesired.
As a simple heuristics just use a "natural" rounding algorithm and round
up for 4 spaces or more.

There's an example in line 108 in net/rds/ib_recv.c with indentation TAB + 7
spaces where the correct would have been an additional TAB.  With only TABSTOP
enabled, checkpatch will fix this to a single TAB, which is visually worse IMO.

Reported-by: Håkon Bugge <haakon.bugge@oracle.com>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 387292f..c17178e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3061,7 +3061,7 @@ sub process {
 				if (WARN("TABSTOP",
 					 "Statements should start on a tabstop\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
+					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x (($indent+4)/8)@e;
 				}
 			}
 		}
-- 
git-series 0.9.1

[PATCH 6/7] checkpatch: Make --ignore-cfg look recursively for the file

[Knut Omang]

The initial version of the logic for --ignore-cfg supported looking
for the file in the current directory, and if not found, look
in the directory of the source file.

With this change, in the case of a file name with no directory specification,
and for an in-kernel file, checkpatch will iterate upwards in the directories
above, looking for the same file until either hitting the top of the
tree or finding a match.

This should make the P={1,2} make target more useful for maintainers,
as a start of functionality for checkpatch checking that
can be globally enabled for a subsystem.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 scripts/checkpatch.pl | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c17178e..a276eca 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2186,15 +2186,47 @@ sub pos_last_openparen {
 	return length(expand_tabs(substr($line, 0, $last_openparen))) + 1;
 }
 
+
 # Checkpatch suppression list configuration file support
 #
 # See Documentation/dev-tools/run-checkpatch.rst
 #
-sub parse_ignore_cfg_file {
-	defined($ignore_cfg_file) || return 1;
+
+# Usage: find_ignore_cfg_file(filename,ignorefilename)
+# If filename contains a directory, use it directly with no further attempts,
+# If no directory is specified, whether relative or absolute,
+# look for 'filename' in the following order until a match is found:
+#   1) current dir,
+#   2) the directory of the source file
+#   3) if an in-source source-file (same source tree as this script)
+#      look in directories above for the first match.
+#
+sub find_ignore_cfg_file {
 	my $path = shift(@_);
+	my $ipath = shift(@_);
+	my $ifile = basename($ipath);
 	my $filename = basename($path);
 	my $dir = dirname($path);
+	my $root = dirname($D);
+	( -f $ipath ) && return ($filename, $ipath);
+	( $ipath =~ m/\// ) && return ($filename,"");
+
+	do {
+		$ipath = "$dir/$ifile";
+		#print "*** Trying $ipath ***\n";
+		( -f $ipath ) && return ($filename, $ipath);
+		$dir = dirname($dir);
+	} while ( $dir =~ m/^$root/ && ! -f $ifile );
+	return ($filename,"");
+}
+
+
+sub parse_ignore_cfg_file {
+	my $path = shift(@_);
+	my $filename; # The file to check
+	my $ifile;    # The ignore file name
+	my $ignfile;  # The ignore file handle
+	defined($ignore_cfg_file) || return 1;
 	my %IgnoreCfgKeywords = (
 		'except'	=> sub { my $type = shift(@_);
 					 grep( /^$filename$/, @_ ) && push(@ignore, $type);
@@ -2202,11 +2234,11 @@ sub parse_ignore_cfg_file {
 		'pervasive'	=> sub { push(@ignore, @_); },
 		'line_len'	=> sub { $max_line_length = shift(@_); }
 	);
-	my $ignfile;
 
-	( -f $ignore_cfg_file ) || ( $ignore_cfg_file = "$dir/$ignore_cfg_file" );
-	( ! -f $ignore_cfg_file ) && return 0;
-	open($ignfile, '<', "$ignore_cfg_file") || return 0;
+	($filename, $ifile) = find_ignore_cfg_file($path, $ignore_cfg_file);
+	( $ifile eq "" ) && return 0;
+	open($ignfile, '<', "$ifile") || return 0;
+	#print "*** Found $ifile ***\n";
 
 	($#_ >= 0) &&
 		die "$P: The --ignore-cfg option is only supported with one source file at a time!\n";
-- 
git-series 0.9.1

[PATCH 7/7] Documentation: Update checkpatch --ignore-cfg description

[Knut Omang]

When running checkpatch with --ignore-cfg it will now traverse the directories
above the source file until a match is found.

Reflect this enhanced behaviour in the documentation.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 Documentation/dev-tools/run-checkpatch.rst | 48 ++++++++++++-----------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/Documentation/dev-tools/run-checkpatch.rst b/Documentation/dev-tools/run-checkpatch.rst
index c72f818..566d8c6 100644
--- a/Documentation/dev-tools/run-checkpatch.rst
+++ b/Documentation/dev-tools/run-checkpatch.rst
@@ -26,17 +26,20 @@ number of challenges:
 This is the purpose of supplying the option ``--ignore-cfg checkpatch.cfg`` to
 ``scripts/checkpatch.pl``. It will then look for a file named ``checkpatch.cfg``
 in the current directory or alternatively in the directory of the source
-file. If that file exists, checkpatch parses a set of rules from it, and use
-them to determine how to invoke checkpatch for a particular file. The kernel
-Makefile system supports using this feature as an integrated part of compiling
-the code.
+file. If neither is found, and the file is within the kernel tree, checkpatch
+will recursively look for a file with the same name in the directories above
+until a file is found or the top of the tree is reached.
+
+If a match is found, checkpatch parses a set of rules from it, and use them to
+determine how to invoke checkpatch for a particular file.  The kernel Makefile
+system supports using this feature as an integrated part of compiling the code.
 
 The ignore configuration file
 -----------------------------
 
 The ignore configuration file can be used to set policies and "rein in"
-checkpatch errors piece by piece for a particular subsystem or driver.
-The the following syntax is supported::
+checkpatch errors piece by piece for a particular subsystem or driver.  The the
+following syntax is supported::
 
 	# comments
 	line_len <n>
@@ -55,9 +58,9 @@ exceptions.
 Running checkpatch from make
 ----------------------------
 
-You can run checkpatch subject to rules defined in ``checkpatch.cfg`` in the
-directory of the source file by using "make P=1" to run checkpatch on all files
-that gets recompiled, or "make P=2" to run checkpatch on all source files.
+You can run checkpatch subject to rules defined in the closest matching
+``checkpatch.cfg`` file in the tree by using "make P=1" to run checkpatch on all
+files that gets recompiled, or "make P=2" to run checkpatch on all source files.
 
 A make variable ``PF`` allows passing additional parameters to
 checkpatch.pl. You can for instance use::
@@ -70,15 +73,15 @@ selectively enabling of types of errors via changes to the local
 ``checkpatch.cfg``, and you can focus on fixing up errors subsystem or driver by
 driver on a type by type basis.
 
-By default checkpatch will skip all files in directories without a
-checkpatch.cfg file when invoked with the --ignore-cfg parameter.  This is to
-allow builds with P=2 to pass even for subsystems that has not yet done anything
-to rein in checkpatch errors. At some point when all subsystems and drivers
-either have fixed all checkpatch errors or added proper checkpatch.cfg files,
-this can be changed.
+When invoked with the --ignore-cfg parameter, by default checkpatch will skip
+all files in directories without a matching checkpatch.cfg according to the
+algorithm described above. This is to allow builds with P=2 to pass even for
+subsystems that has not yet done anything to rein in checkpatch errors. At some
+point when all subsystems and drivers either have fixed all checkpatch errors or
+added proper checkpatch.cfg files, this can be changed.
 
-To force checkpatch to run a full run in directories without a checkpatch.cfg
-file as well, use::
+To force checkpatch to run a full run in directories without a
+checkpatch.cfg file as well, use::
 
 	make P=2 PF="--req-ignore-cfg"
 
@@ -96,10 +99,11 @@ with the -k option to ``make`` to let it continue upon errors.
 Ever tightening checkpatch rules
 --------------------------------
 
-Commit the changes to checkpatch.cfg together with the code changes that fixes a
-particular type of issue, this will allow automatic checkpatch testing. This way
-we can ensure that new errors of that particular type do not inadvertently sneak
-in again! This can be done at any subsystem or module maintainer's discretion
-and at the right time without having to do it all at the same time.
+Commit the changes to the relevant checkpatch.cfg together with the code changes
+that fixes a particular type of issue, this will allow automatic checkpatch
+testing. This way we can ensure that new errors of that particular type do not
+inadvertently sneak in again! This can be done at any subsystem or module
+maintainer's discretion and at the right time without having to do it all at the
+same time.
 
 Before submitting your changes, verify that "make P=2" passes with no errors.
-- 
git-series 0.9.1

Re: [PATCH 1/7] checkpatch: Implement new --ignore-cfg parameter

[Joe Perches]

(adding Andrew Morton)

On Thu, 2017-11-16 at 18:01 +0100, Knut Omang wrote:
> This parameter is intended to be used in a subsequent commit to kbuild to allow
> a convenient way to run checkpatch from make.

_why_ is this useful?

> By accepting comments and multiple lines of commands, the idea is that the
> maintainer or someone else with good knowledge of the code can maintain a file
> per directory and group the different commands into commented sections that can
> serve both as documentation of the current checkpatch status, a way to define
> the line of tolerance (and gradually tighten it as fixes comes in) and as
> documentation of TODOs and dont's if there are well justified exceptions.

checkpatch can be run any time over individual files
so I don't find this compelling.

Does anyone else?

Re: [PATCH 1/7] checkpatch: Implement new --ignore-cfg parameter

[Knut Omang]

On Thu, 2017-11-16 at 09:09 -0800, Joe Perches wrote:
> (adding Andrew Morton)
> 
> On Thu, 2017-11-16 at 18:01 +0100, Knut Omang wrote:
> > This parameter is intended to be used in a subsequent commit to kbuild to allow
> > a convenient way to run checkpatch from make.
> 
> _why_ is this useful?

The cover letter and the following documentation patch hopefully makes it 
clearer. 

I realize the cc: list on the cover letter was a little too narrow, sorry about that!

> > By accepting comments and multiple lines of commands, the idea is that the
> > maintainer or someone else with good knowledge of the code can maintain a file
> > per directory and group the different commands into commented sections that can
> > serve both as documentation of the current checkpatch status, a way to define
> > the line of tolerance (and gradually tighten it as fixes comes in) and as
> > documentation of TODOs and dont's if there are well justified exceptions.
> 
> checkpatch can be run any time over individual files
> so I don't find this compelling.

The problem with that in general is the noise level.
What this patch set gives is in short:

* a way to filter out the noise to focus on one type of error at the time
* the means to automate prevention of reoccurrence of some types of checkpatch 
  errors that have been removed from a file, even when that file has
  100s of checkpatch issues of other types.

> Does anyone else?

I did subject the set to a couple of internal reviews with positive feedback. 
We have also had automated checkin regression testing going with 
a similar system for some time.  I was just going to improve upon that system
when I realized that we should really make it available for the broader community.

I hope this helps,

thanks,
Knut

Re: [PATCH 0/7] Support for automatic checkpatch running in the kernel

[Kees Cook]

On Thu, Nov 16, 2017 at 9:01 AM, Knut Omang <knut.omang@oracle.com> wrote:
> The most important checkpatch feature added is the --ignore-cfg feature, which
> takes a file argument and parses that file according to this minimal language:
>
>        # comments
>        line_len <n>
>        except checkpatch_type [files ...]
>        pervasive checkpatch_type1 [checkpatch_type2 ...]
>
> With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
> checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
> same directory as the source file. If that file exists, checkpatch will be run
> with an implicit --strict and with the @ignore list expanded with content from
> the configuration file.  If it does not exist, make will simply silently ignore
> the file.

Will these configurations be cascading? (For example, all of net/ uses
a different comment style, so having that recorded in a single file
would be nice.)


-- 
Kees Cook
Pixel Security

Re: [PATCH 0/7] Support for automatic checkpatch running in the kernel

[Knut Omang]

On Thu, 2017-11-16 at 14:57 -0800, Kees Cook wrote:
> On Thu, Nov 16, 2017 at 9:01 AM, Knut Omang <knut.omang@oracle.com> wrote:
> > The most important checkpatch feature added is the --ignore-cfg feature, which
> > takes a file argument and parses that file according to this minimal language:
> >
> >        # comments
> >        line_len <n>
> >        except checkpatch_type [files ...]
> >        pervasive checkpatch_type1 [checkpatch_type2 ...]
> >
> > With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
> > checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
> > same directory as the source file. If that file exists, checkpatch will be run
> > with an implicit --strict and with the @ignore list expanded with content from
> > the configuration file.  If it does not exist, make will simply silently ignore
> > the file.
> 
> Will these configurations be cascading? (For example, all of net/ uses
> a different comment style, so having that recorded in a single file
> would be nice.)

Good point, the net/ use case is certainly something I have been thinking about.
I didn't want to make it too complex in the first set, so I let patch 1-4
implement the basics with one file per directory and nothing across directories.

Patch 6 and 7 in this set extends this to "fallback", which (with the net/ case in mind)
should allow a single file under net/ to cover the whole subsystem for all subtrees that
don't have their own checkpatch.cfg, but will be overridden by files in subdirectories.

So from a documentation point of view this could be done with the set as it is,
with people just copying the "pervasive" and "line_len" commands from
a "global" net/checkpatch.cfg setup down into the subtrees as they start dealing with 
exceptions.

If we want something more automatic, it can certainly be extended, 
this would just require checkpatch to parse all the upwards checkpatch.cfg files for each
source file instead just taking the first found, which is what patch 6 implements.

Also the command language in the config files is rather limited, maybe other commands
maybe with more checkpatch additions would be useful to handle such scenarios..

Let me know what you think..

Thanks,
Knut

Re: [PATCH 0/7] Support for automatic checkpatch running in the kernel

[Knut Omang]

I realize the Cc: list for the cover letter probably should have included all 
the relevant maintainers for this set, sorry about that!

For convenience I also put up a more reader friendly copy of the doc after patch 7 here:

http://heim.ifi.uio.no/~knuto/kernel/4.14/dev-tools/run-checkpatch.html

Thanks,
Knut

On Thu, 2017-11-16 at 18:01 +0100, Knut Omang wrote:
> This patch series implements features to facilitate running checkpatch on the
> entire kernel as part of automatic testing.
> 
> This is done by by adding a few small features to checkpatch and put these
> features to use to implement support for a new Makefile environment variable
> P={1,2} following the pattern of sparse and the C={1,2} variable.  The basic
> functionality + docs are in patch #1-4.
> 
> It also fixes a minor issue with "checkpatch --fix-inplace" found during testing
> (patch #5).
> 
> The most important checkpatch feature added is the --ignore-cfg feature, which
> takes a file argument and parses that file according to this minimal language:
> 
>        # comments
>        line_len <n>
>        except checkpatch_type [files ...]
>        pervasive checkpatch_type1 [checkpatch_type2 ...]
> 
> With "make P=2" checkpatch is called with "--file" and "--ignore_cfg
> checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in the
> same directory as the source file. If that file exists, checkpatch will be run
> with an implicit --strict and with the @ignore list expanded with content from
> the configuration file.  If it does not exist, make will simply silently ignore
> the file.
> 
> Patches #6-7 enhances this behaviour to also scan the directories above a file
> until a match for the --file parameter is found.
> 
> The idea is that the community can work to add checkpatch.cfg files to
> directories, serving both as documentation and as a way for subsystem
> maintainers to enforce policies and individual tastes as well as TODOs and/or
> priorities, to make it easier for newcomers to contribute in this area. By
> ignoring directories without such files, automation can start right away as it
> is trivially possible to run errorless with P=2 for the entire kernel.
> 
> The patches includes a documentation file with some more details.
> 
> This patch set has evolved from an earlier implementation I made that was just a
> wrapper script around checkpatch. That version have been used for a number of
> years on a driver project I worked on where we had automatic checkin regression
> testing. I extended that to also run checkpatch to avoid having to clean up
> frequent unintended whitespace changes and style violations from others...
> 
> I have also tested this version on some directories I am familiar with.  The
> result of that work is available in two patch sets of 10 and 11 patches, but we
> agreed that it would be better to post them as separate patch sets later.
> 
> Those patch sets illustrates how I picture the "flow" from just "reining in" the
> checkpatch detections to actually fixing classes of checkpatch issues one by
> one, while updating the checkpatch.cfg file(s) to have 0 errors or warnings at
> any commit boundary.
> 
> The combined set is available here:
> 
>    git://github.com/knuto/linux.git  branch checkpatch
> 
> Comments and suggestions appreciated!
> 
> Thanks,
> Knut
> 
> Knut Omang (7):
>   checkpatch: Implement new --ignore-cfg parameter
>   kbuild: Add P= command line flag to run checkpatch
>   checkpatch: Add a few convenience options to disable/modify features
>   Documentation: Add documentation for the new P= Makefile option
>   checkpatch: Improve --fix-inplace for TABSTOP
>   checkpatch: Make --ignore-cfg look recursively for the file
>   Documentation: Update checkpatch --ignore-cfg description
> 
>  Documentation/dev-tools/index.rst          |   1 +-
>  Documentation/dev-tools/run-checkpatch.rst | 109 ++++++++++++++++++++++-
>  Makefile                                   |  20 +++-
>  scripts/Makefile.build                     |  13 +++-
>  scripts/checkpatch.pl                      | 108 +++++++++++++++++++++-
>  5 files changed, 249 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/dev-tools/run-checkpatch.rst
> 
> base-commit: bebc6082da0a9f5d47a1ea2edc099bf671058bd4

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Masahiro Yamada]

2017-11-17 2:01 GMT+09:00 Knut Omang <knut.omang@oracle.com>:
> Add interpretation of a new environment variable P={1,2} in spirit of the
> C= option, but executing checkpatch instead of sparse.
>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
> Acked-by: Åsmund Østvold <asmund.ostvold@oracle.com>
> ---
>  Makefile               | 20 +++++++++++++++++++-
>  scripts/Makefile.build | 13 +++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index ccd9818..eb4bca9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC
>    KBUILD_CHECKSRC = 0
>  endif
>
> +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg
> +#
> +# Use 'make P=1' to enable checking of only re-compiled files.
> +# Use 'make P=2' to enable checking of *all* source files, regardless
> +#
> +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more details,
> +#
> +ifeq ("$(origin P)", "command line")
> +  KBUILD_CHECKPATCH = $(P)
> +endif
> +ifndef KBUILD_CHECKPATCH
> +  KBUILD_CHECKPATCH = 0
> +endif


I am unhappy about adding a new interface
for each checker.

The default of CHECK is "sparse", but
users can override it to use another checker.



As Decumentation/dev-tools/coccinelle.rst says,
if you want to use coccinelle as a checker,

make C=1 CHECK="scripts/coccicheck"


Recently, I saw a patch to use scripts/kernel-doc as a checker.
https://patchwork.kernel.org/patch/10030521/



If I accept your patch,
we would end up with

KBUILD_CHECKPATCH,
KBUILD_CHECKCOCCI
KBUILD_CHECKDOC,
...


This is ugly.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Jim Davis]

On Mon, Nov 20, 2017 at 9:18 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:

>
> I am unhappy about adding a new interface
> for each checker.
>
> The default of CHECK is "sparse", but
> users can override it to use another checker.
>
>
>
> As Decumentation/dev-tools/coccinelle.rst says,
> if you want to use coccinelle as a checker,
>
> make C=1 CHECK="scripts/coccicheck"
>

I'd be nice if people could just specify CHECK and CHECKFLAGS to run
their favorite checker, but currently CHECKFLAGS seems hardwired for
running sparse.  So something liike

make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"

fails when checkpatch is passed lots of arguments like -D__linux__
-Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
in that long list is a workaround, but perhaps CHECKFLAGS should be
made less sparse-specific?

-- 
Jim

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Luc Van Oostenryck]

On Mon, Nov 20, 2017 at 12:48:35PM -0700, Jim Davis wrote:
> 
> I'd be nice if people could just specify CHECK and CHECKFLAGS to run
> their favorite checker, but currently CHECKFLAGS seems hardwired for
> running sparse.  So something liike
> 
> make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"
> 
> fails when checkpatch is passed lots of arguments like -D__linux__
> -Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
> in that long list is a workaround, but perhaps CHECKFLAGS should be
> made less sparse-specific?

It should be noted though that CHECKFLAGS contains very very few
sparse specific things. It's mainly flags for the compiler
coming from  KBUILD_CFLAGS (which of course, sparse needs to
do its job properly).

-- Luc Van Oostenryck

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Knut Omang]

On Tue, 2017-11-21 at 01:18 +0900, Masahiro Yamada wrote:
> 2017-11-17 2:01 GMT+09:00 Knut Omang <knut.omang@oracle.com>:
> > Add interpretation of a new environment variable P={1,2} in spirit of the
> > C= option, but executing checkpatch instead of sparse.
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
> > Acked-by: Åsmund Østvold <asmund.ostvold@oracle.com>
> > ---
> >  Makefile               | 20 +++++++++++++++++++-
> >  scripts/Makefile.build | 13 +++++++++++++
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index ccd9818..eb4bca9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -176,6 +176,20 @@ ifndef KBUILD_CHECKSRC
> >    KBUILD_CHECKSRC = 0
> >  endif
> > 
> > +# Run scripts/checkpatch.pl with --ignore-cfg checkpatch.cfg
> > +#
> > +# Use 'make P=1' to enable checking of only re-compiled files.
> > +# Use 'make P=2' to enable checking of *all* source files, regardless
> > +#
> > +# See the file "Documentation/dev-tools/run-checkpatch.rst" for more
> > details,
> > +#
> > +ifeq ("$(origin P)", "command line")
> > +  KBUILD_CHECKPATCH = $(P)
> > +endif
> > +ifndef KBUILD_CHECKPATCH
> > +  KBUILD_CHECKPATCH = 0
> > +endif
> 
> 
> I am unhappy about adding a new interface
> for each checker.

I see your point. I wanted to extend C= to handle checkpatch as well but see no
obvious good non-intrusive solution.

> The default of CHECK is "sparse", but
> users can override it to use another checker.
> 
> 
> 
> As Decumentation/dev-tools/coccinelle.rst says,
> if you want to use coccinelle as a checker,
> 
> make C=1 CHECK="scripts/coccicheck"

That works well with coccinelle since both sparse and coccinelle rely on getting
the same command line options as what's passed to the compiler, while checkpatch
is quite different:

  make C=2 CHECK="\$(srctree)/scripts/checkpatch.pl" 

fails, complaining about every single compiler flag.

Thanks,
Knut

> Recently, I saw a patch to use scripts/kernel-doc as a checker.
> https://patchwork.kernel.org/patch/10030521/
> 
> 
> 
> If I accept your patch,
> we would end up with
> 
> KBUILD_CHECKPATCH,
> KBUILD_CHECKCOCCI
> KBUILD_CHECKDOC,
> ...
> 
> This is ugly.

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Knut Omang]

On Mon, 2017-11-20 at 21:08 +0100, Luc Van Oostenryck wrote:
> On Mon, Nov 20, 2017 at 12:48:35PM -0700, Jim Davis wrote:
> > 
> > I'd be nice if people could just specify CHECK and CHECKFLAGS to run
> > their favorite checker, but currently CHECKFLAGS seems hardwired for
> > running sparse.  So something liike
> > 
> > make C=1 CHECK="scripts/checkpatch.pl" CHECKFLAGS="--quiet --file"
> > 
> > fails when checkpatch is passed lots of arguments like -D__linux__
> > -Dlinux -D__STDC__ .  A little shell wrapper to grab the last argument
> > in that long list is a workaround, but perhaps CHECKFLAGS should be
> > made less sparse-specific?
> 
> It should be noted though that CHECKFLAGS contains very very few
> sparse specific things. It's mainly flags for the compiler
> coming from  KBUILD_CFLAGS (which of course, sparse needs to
> do its job properly).

Yes, and we would want some arguments passed to checkpatch by default as well.

A wrapper script (which by the way was what I started this with..)
could of course solve this and other issues such as the ability 
to run multiple checkers, but I am not convinced that that would be 
less ugly?

Thanks,
Knut

> 
> -- Luc Van Oostenryck

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Luc Van Oostenryck]

On Mon, Nov 20, 2017 at 10:10:12PM +0100, Knut Omang wrote:
> On Mon, 2017-11-20 at 21:08 +0100, Luc Van Oostenryck wrote:
> > 
> > It should be noted though that CHECKFLAGS contains very very few
> > sparse specific things. It's mainly flags for the compiler
> > coming from  KBUILD_CFLAGS (which of course, sparse needs to
> > do its job properly).
> 
> Yes, and we would want some arguments passed to checkpatch by default as well.
> 
> A wrapper script (which by the way was what I started this with..)
> could of course solve this and other issues such as the ability 
> to run multiple checkers, but I am not convinced that that would be 
> less ugly?

A wrapper script is something else that need to be maintained
but of course, it's very flexible.
I don't have a strong opinion on this and prefer to let speak
the people who maintain kbuild.

Should it be possible to somehow keep the distinction between
the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS?


-- Luc Van Oostenryck

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Jim Davis]

On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:

> Should it be possible to somehow keep the distinction between
> the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS?

Well, the practical problem seems to be that $(CHECK) is called in
scripts/Makefile.build with both $(CHECKFLAGS) and $(c_flags) as
arguments, regardless of what $(CHECK) is.  That can be hacked around
with something inelegant like

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bb831d49bcfd..102194f9afb9 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -98,14 +98,20 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target)
$(lib-target) $(extra-y)) \
         $(subdir-ym) $(always)
        @:

-# Linus' kernel sanity checking tool
+# Linus' kernel sanity checking tool, or $CHECK program of choice
+ifneq ($(KBUILD_CHECKSRC),)
+  add_to_checkflags =
+  ifeq ($(CHECK),sparse)
+    add_to_checkflags = $(c_flags)
+  endif
+endif
 ifneq ($(KBUILD_CHECKSRC),0)
   ifeq ($(KBUILD_CHECKSRC),2)
     quiet_cmd_force_checksrc = CHECK   $<
-          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $< ;
   else
       quiet_cmd_checksrc     = CHECK   $<
-            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $< ;
   endif
 endif

and then if I run scripts/checkpatch.pl as $CHECK and pass --quiet
--file as before it works -- until checkpatch returns with a non-zero
exit code, which causes the Makefile to bail at that point.

So maybe a wrapper script, with an exit 0 fixup to keep on keeping on
in case of checkpatch warnings or errors, would be better after all.


-- 
Jim

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Knut Omang]

On Mon, 2017-11-20 at 17:00 -0700, Jim Davis wrote:
> On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> 
> > Should it be possible to somehow keep the distinction between
> > the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS?
> 
> Well, the practical problem seems to be that $(CHECK) is called in
> scripts/Makefile.build with both $(CHECKFLAGS) and $(c_flags) as
> arguments, regardless of what $(CHECK) is.  That can be hacked around
> with something inelegant like
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index bb831d49bcfd..102194f9afb9 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -98,14 +98,20 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target)
> $(lib-target) $(extra-y)) \
>          $(subdir-ym) $(always)
>         @:
> 
> -# Linus' kernel sanity checking tool
> +# Linus' kernel sanity checking tool, or $CHECK program of choice
> +ifneq ($(KBUILD_CHECKSRC),)
> +  add_to_checkflags =
> +  ifeq ($(CHECK),sparse)
> +    add_to_checkflags = $(c_flags)
> +  endif
> +endif
>  ifneq ($(KBUILD_CHECKSRC),0)
>    ifeq ($(KBUILD_CHECKSRC),2)
>      quiet_cmd_force_checksrc = CHECK   $<
> -          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
> +          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $<
> ;
>    else
>        quiet_cmd_checksrc     = CHECK   $<
> -            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
> +            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $<
> ;
>    endif
>  endif
> 
> and then if I run scripts/checkpatch.pl as $CHECK and pass --quiet
> --file as before it works -- until checkpatch returns with a non-zero
> exit code, which causes the Makefile to bail at that point.

yes, in an earlier version, I added a --no-errors flag to checkpatch to handle
that, but based on feedback this version promotes make -k instead. It seems
however that that only holds to the next linker step. It is useful to be able to
select whether checkpatch should cause make to stop or not. While fixing issues,
failure is a better strategy while to assess the state or generate a report, a
return 0 behavior is better.

> So maybe a wrapper script, with an exit 0 fixup to keep on keeping on
> in case of checkpatch warnings or errors, would be better after all.

I can prepare a v2 based on the wrapper script I have already.

In that version, all added functionality was implemented in the wrapper
(including the configuration file parsing) 

Would you like to keep the checkpatch changes in some form, or would you rather
see everything happening in the wrapper?

A 3rd possibility that strikes me is that maybe what's needed is a general
checker runner that can also take care of running other checks, running multiple
checks, and maybe also handling temporary or decided suppression of sparse
errors in a similar fashion as I implemented for checkpatch errors. But maybe
that can be left to a later change set..

Thanks,
Knut

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Jim Davis]

On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang <knut.omang@oracle.com> wrote:

> Would you like to keep the checkpatch changes in some form, or would you rather
> see everything happening in the wrapper?

I don't have a strong preference one way or another, but keeping
everything in a wrapper script might be easier if only because you
wouldn't need to get signoffs from whoever maintains checkpatch too.

-- 
Jim

Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

[Joe Perches]

On Tue, 2017-11-21 at 12:48 -0700, Jim Davis wrote:
> On Tue, Nov 21, 2017 at 1:10 AM, Knut Omang <knut.omang@oracle.com> wrote:
> 
> > Would you like to keep the checkpatch changes in some form, or would you rather
> > see everything happening in the wrapper?
> 
> I don't have a strong preference one way or another, but keeping
> everything in a wrapper script might be easier if only because you
> wouldn't need to get signoffs from whoever maintains checkpatch too.

Keeping everything in a wrapper script would also allow
any arbitrary checker to be run with minimal changes to
the Makefile/build subsystem.