[PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Knut Omang]

This patch series implements features to make it easier to run checkers on the
entire kernel as part of automatic and developer testing.

This is done by replacing the sparse specific setup for the C={1,2} variable
in the makefiles with setup for running scripts/runchecks, a new program that
can run any number of different "checkers". The behaviour of runchecks is
defined by simple "global" configuration in scripts/runchecks.cfg which can be
extended by local configuration applying to individual files, directories or
subtrees in the source.

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

The runchecks.cfg files are parsed according to this minimal language:

       # comments
       # "Global configuration in scripts/runchecks.cfg:
       checker <name>
       typedef NAME regex
       run <list of checkers or "all"

       # "local" configuration:
       line_len <n>
       except checkpatch_type [files ...]
       pervasive checkpatch_type1 [checkpatch_type2 ...]

With "make C=2" runchecks first parse the file scripts/runchecks.cfg, then
look for a file named 'runchecks.cfg' in the same directory as the source file.
If that file exists, it will be parsed and it's local configuration applied to
allow suppression on a per checker, per check and per file basis.
If a "local" configuration does not exist, either in the source directory or
above, make will simply silently ignore the file.

The idea is that the community can work to add runchecks.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/subtrees without such files, automation can start right
away as it is trivially possible to run errorless with C=2 for the entire
kernel.

For the checker maintainers this should be a benefit as well: new
or improved checks would generate new errors/warnings. With automatic testing
for the checkers, these new checks would generate error reports and cause
builds to fail. Adding the new check a 'pervasive' option at the top level or
even for specific files, marked with a "new check - please consider fixing" comment
or similar would make those builds pass while documenting and making the new check
more visible.

The patches includes a documentation file with some more details.

This patch set has evolved from an earlier shell script implementation I made
as only 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 runchecks

I only include version 0 of runchecks.cfg in the two directories I
worked on here as the final two patches. These files both documents where
the issues are in those two directories, and can be used by the maintainer
to indicate to potential helpers what to focus on as I have tried to
illustrate by means of comments.

Changes from v1:
-----------------
Based on feedback, the implementation is completely rewritten and extended.
Instead of patches to checkpatch, and a sole checkpatch focus, it is now a
generic solution implemented in python, for any type of checkers, extendable
through some basic generic functionality, and for special needs by subclassing
the Checker class in the implementation.

This implementation fully supports checkpatch, sparse and
checkdoc == kernel-doc -none, and also has been tested with coccicheck.
To facilitate the same mechanism of using check types to filter what
checks to be suppressed, I introduced the concept of "typedefs" which allows
runchecks to effectively augment the check type space of the checker in cases
where types either are not available at all (checkdoc) or where only a few
can be filtered out (sparse)

With this in place it also became trivial to make the look and feel similar
for sparse and checkdoc as for checkpatch, with some optional color support
too, to make fixing issues in the code, the goal of this whole exercise,
much more pleasant IMHO :-)

Thanks,
Knut

Knut Omang (5):
  runchecks: Generalize make C={1,2} to support multiple checkers
  Documentation: Add doc for runchecks, a checker runner
  checkpatch: Improve --fix-inplace for TABSTOP
  rds: Add runchecks.cfg for net/rds
  RDMA/core: Add runchecks.cfg for drivers/infiniband/core

 Documentation/dev-tools/coccinelle.rst |  12 +-
 Documentation/dev-tools/index.rst      |   1 +-
 Documentation/dev-tools/runchecks.rst  | 215 ++++++++-
 Documentation/dev-tools/sparse.rst     |  30 +-
 Documentation/kbuild/kbuild.txt        |   9 +-
 Makefile                               |  23 +-
 drivers/infiniband/core/runchecks.cfg  |  83 +++-
 net/rds/runchecks.cfg                  |  76 +++-
 scripts/Makefile.build                 |   4 +-
 scripts/checkpatch.pl                  |   2 +-
 scripts/runchecks                      | 734 ++++++++++++++++++++++++++-
 scripts/runchecks.cfg                  |  63 ++-
 scripts/runchecks_help.txt             |  43 ++-
 13 files changed, 1274 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/dev-tools/runchecks.rst
 create mode 100644 drivers/infiniband/core/runchecks.cfg
 create mode 100644 net/rds/runchecks.cfg
 create mode 100755 scripts/runchecks
 create mode 100644 scripts/runchecks.cfg
 create mode 100644 scripts/runchecks_help.txt

base-commit: ae64f9bd1d3621b5e60d7363bc20afb46aede215
-- 
git-series 0.9.1

[PATCH v2 1/5] runchecks: Generalize make C={1,2} to support multiple checkers

[Knut Omang]

Add scripts/runchecks which has generic support for running
checker tools in a convenient and user friendly way that
the author hopes can contribute to rein in issues detected
by these tools in a manageable and convenient way.

scripts/runchecks provides the following basic functionality:

* Makes it possible to selectively suppress output from individual
  checks on a per file or per subsystem basis.
* Unifies output and suppression input from different tools
  by providing a single unified syntax for the underlying tools
  in the style of "scripts/checkpatch.pl --show-types".
* Allows selective run of one, or more (or all) configured tools
  for each file.

In the Makefile system, the sparse specific setup has been replaced
by setup for runchecks.

This version of runchecks together with a "global" configuration
file in "scripts/runchecks.cfg" supports sparse, checkpatch and checkdoc,
a trivial abstraction above a call to 'kernel-doc -none'.
It also supports forwarding calls to coccicheck for coccinelle support
but this is not quite as worked through as the three other checkers,
mainly because of lack of error data as all checks pass by default
right now.

The code is designed to be easily extensible to support more checkers
as they emerge, and some generic checker support is even available
just via simple additions to "scripts/runchecks.cfg".

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 Makefile                   |  23 +-
 scripts/Makefile.build     |   4 +-
 scripts/runchecks          | 734 ++++++++++++++++++++++++++++++++++++++-
 scripts/runchecks.cfg      |  63 +++-
 scripts/runchecks_help.txt |  43 ++-
 5 files changed, 857 insertions(+), 10 deletions(-)
 create mode 100755 scripts/runchecks
 create mode 100644 scripts/runchecks.cfg
 create mode 100644 scripts/runchecks_help.txt

diff --git a/Makefile b/Makefile
index c988e46..791e8df 100644
--- a/Makefile
+++ b/Makefile
@@ -159,14 +159,22 @@ ifeq ($(skip-makefile),)
 # so that IDEs/editors are able to understand relative filenames.
 MAKEFLAGS += --no-print-directory
 
-# Call a source code checker (by default, "sparse") as part of the
-# C compilation.
+# Do source code checking as part of the C compilation.
+#
 #
 # Use 'make C=1' to enable checking of only re-compiled files.
 # Use 'make C=2' to enable checking of *all* source files, regardless
 # of whether they are re-compiled or not.
 #
-# See the file "Documentation/dev-tools/sparse.rst" for more details,
+# Source code checking is done via the runchecks script, which
+# has knowledge of each individual cheker and how it wants to be called,
+# as well as options for rules as to which checks that are applicable
+# to different parts of the kernel, at source file granularity.
+#
+# Several types of checking is available, and custom checkers can also
+# be added.
+#
+# See the file "Documentation/dev-tools/runchecks.rst" for more details,
 # including where to get the "sparse" utility.
 
 ifeq ("$(origin C)", "command line")
@@ -383,10 +391,9 @@ INSTALLKERNEL  := installkernel
 DEPMOD		= /sbin/depmod
 PERL		= perl
 PYTHON		= python
-CHECK		= sparse
 
-CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
-		  -Wbitwise -Wno-return-void $(CF)
+CHECK		= $(srctree)/scripts/runchecks
+CHECKFLAGS      =
 NOSTDINC_FLAGS  =
 CFLAGS_MODULE   =
 AFLAGS_MODULE   =
@@ -429,7 +436,7 @@ GCC_PLUGINS_CFLAGS :=
 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 HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECK_CFLAGS CHECKFLAGS
 
 export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
 export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_KASAN CFLAGS_UBSAN
@@ -778,7 +785,7 @@ endif
 
 # arch Makefile may override CC so keep this after arch Makefile is included
 NOSTDINC_FLAGS += -nostdinc -isystem $(call shell-cached,$(CC) -print-file-name=include)
-CHECKFLAGS     += $(NOSTDINC_FLAGS)
+CHECK_CFLAGS     += $(NOSTDINC_FLAGS)
 
 # warn about C99 declaration after statement
 KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index cb8997e..13325b3 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -93,10 +93,10 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target) $(lib-target) $(extra-y)) \
 ifneq ($(KBUILD_CHECKSRC),0)
   ifeq ($(KBUILD_CHECKSRC),2)
     quiet_cmd_force_checksrc = CHECK   $<
-          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+          cmd_force_checksrc = $(CHECK) $(CF) $< -- $(CHECKFLAGS) $(c_flags);
   else
       quiet_cmd_checksrc     = CHECK   $<
-            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+            cmd_checksrc     = $(CHECK) $(CF) $< -- $(CHECKFLAGS) $(c_flags);
   endif
 endif
 
diff --git a/scripts/runchecks b/scripts/runchecks
new file mode 100755
index 0000000..4dd2969
--- /dev/null
+++ b/scripts/runchecks
@@ -0,0 +1,734 @@
+#!/usr/bin/python
+
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+#    Author: Knut Omang <knut.omang@oracle.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 2
+# as published by the Free Software Foundation.
+
+# The program implements a generic and extensible code checker runner
+# that supports running various checker tools from the kernel Makefile
+# or standalone, with options for selectively suppressing individual
+# checks on a per file or per check basis.
+#
+# The program has some generic support for checkers, but to implement
+# support for a new checker to the full extent, it might be necessary to
+#   1) subclass the Checker class in this file with checker specific processing.
+#   2) add typedef definitions in runchecks.cfg in this directory
+#
+# This version of runchecks has full support for the following tools:
+#   sparse:      installed separately
+#   checkpatch:  checkpatch.pl
+#   checkdoc:    kernel-doc -none
+#
+# See file "Documentation/dev-tools/runchecks.rst" for more details
+#
+
+import sys, os, subprocess, fcntl, select, re
+from os.path import dirname, basename
+
+class CheckError(Exception):
+    def __init__(self, value):
+        self.value = value
+    def __str__(self):
+        return self.value
+
+def usage():
+    manual = os.path.join(srctree, "scripts/runchecks_help.txt")
+    f = open(manual, "r")
+    for line in f:
+        sys.stdout.write(line)
+    f.close()
+    print ""
+    print "Configured checkers:"
+    for (c, v) in checker_types.iteritems():
+        enabled = "[default disabled]"
+        for c_en in config.checkers:
+            if c_en.name == c:
+                enabled = ""
+                break
+        print "  %-20s %s" % (c, enabled)
+    exit(1)
+
+
+# A small configuration file parser:
+#
+class Config:
+    def __init__(self, srctree, workdir, filename):
+        self.path = []
+        self.relpath = {}
+        relpath = ""
+
+        # Look for a global config file in the scripts directory:
+        file = os.path.join(srctree, "scripts/%s" % filename)
+        if os.path.exists(file):
+            self.path.append(file)
+            self.relpath[file] = relpath
+
+        while not ignore_config:
+            self.file = os.path.join(workdir,filename)
+            if os.path.exists(self.file):
+                self.path.append(self.file)
+                self.relpath[self.file] = relpath
+            if len(workdir) <= len(srctree):
+                break
+            relpath = "%s/%s" % (basename(workdir), relpath)
+            workdir = dirname(workdir)
+
+        self.checkers = []
+        self.cur_chk = None
+        self.color = False
+        self.list_only = False
+
+        self.command = {
+            "checker"	: self.checker,
+            "addflags"	: self.addflags,
+            "run"	: self.runlist,
+            "except"	: self.exception,
+            "pervasive" : self.pervasive,
+            "cflags"	: self.cflags,
+            "typedef"	: self.typedef
+        }
+
+        if verbose:
+            print "  ** runchecks: config path: %s" % self.path
+        for f in self.path:
+            self.ParseConfig(f)
+
+    def checker(self, argv):
+        try:
+            self.cur_chk = checker_types[argv[0]]
+        except KeyError:
+            if len(argv) < 2:
+                d1 = "generic checker configurations!"
+                raise CheckError("%s:%d: use 'checker %s command' for %s" % \
+                                 (self.file, self.lineno, argv[0], d1))
+
+            AddChecker(Checker(argv[0], argv[1], srctree, workdir))
+            self.cur_chk = checker_types[argv[0]]
+
+    def addflags(self, argv):
+        self.cur_chk.addflags(argv)
+
+    def exception(self, argv):
+        type = argv[0]
+        if self.cur_chk:
+            relpath = self.relpath[self.file]
+            self.cur_chk.exception(type, relpath, argv[1:])
+        else:
+            raise CheckError("%s:%d: checker has not been set" % (self.file, self.lineno))
+
+    def pervasive(self, argv):
+        self.cur_chk.pervasive(argv)
+
+    def runlist(self, argv):
+        try:
+            for c in argv:
+                self.checkers.append(checker_types[c])
+        except KeyError, k:
+            if str(k) == "'all'":
+                self.checkers = checker_types.values()
+            else:
+                available = "\n  -- avaliable checkers are: %s" % ",".join(checker_types.keys())
+                raise CheckError("Checker %s not found - not configured?%s" % (str(k), available))
+
+    def cflags(self, argv):
+        self.cur_chk.cflags = True
+
+    def typedef(self, argv):
+        self.cur_chk.typedef(argv)
+
+    # Parse one configuration file in the configuration file list:
+    #
+    def ParseConfig(self, file):
+        f = open(file, 'r')
+        self.file = file
+        self.lineno = 0
+        for line in f:
+            self.lineno = self.lineno + 1
+            token = line.split()
+            if len(token) < 1:
+                continue
+            if token[0][0] == '#':
+                continue
+            try:
+                self.command[token[0]](token[1:])
+            except KeyError:
+                if not self.cur_chk:
+                    raise CheckError("%s:%s: checker has not been set" % (self.file, self.lineno))
+                self.cur_chk.ParseOptional(token[0], token[1:])
+            except AttributeError:
+                if not self.cur_chk:
+                    raise CheckError("%s:%s: checker has not been set" % (self.file, self.lineno))
+
+        f.close()
+        self.cur_chk = None
+
+    # Option forwarding to checkers
+    # and optional selection of which checkers to run:
+    def ProcessOpts(self, opts):
+        for opt in opts:
+            if opt == "--color":
+                self.color = True
+                continue
+            elif opt == "--list":
+                self.list_only = True
+                continue
+            elif opt == "--help":
+                usage_only = True
+
+            fw = re.match("^--to-(\w+):(.*)$", opt)
+            if fw:
+                try:
+                    cname = fw.group(1)
+                    checker = checker_types[cname]
+                except:
+                    raise CheckError("Unknown checker '%s' specified in option '%s'" % (cname, opt))
+                newargs = fw.group(2).split(',')
+                checker.cmdvec += newargs
+                if verbose:
+                    print "Added extra args for %s: %s" % (cname, newargs)
+                continue
+
+            runopt = re.match("^--run:(.*)$", opt)
+            if runopt:
+                clist = runopt.group(1).split(",")
+                # Command line override: reset list of checkers
+                self.checkers = []
+                self.runlist(clist)
+                continue
+
+            if len(self.checkers) == 1:
+                # If only one checker enabled, just pass everything we don't know about through:
+                self.checkers[0].cmdvec.append(opt)
+            else:
+                raise CheckError("Unknown option '%s'" % opt)
+
+    # We always expect at least one config file that sets up the active checkers:
+    #
+    def HasPathConfig(self):
+        return len(self.path) > 1
+
+
+# The base class for checkers:
+# For specific support a particular checker, implement a subclass of this:
+#
+class Checker:
+    def __init__(self, name, cmd, srctree, workdir, ofilter = None, efilter = None):
+        self.name = name
+        self.srctree = srctree
+	self.workdir = workdir
+	self.efilter = efilter
+        if ofilter:
+            self.ofilter = ofilter
+        else:
+            self.ofilter = self.suppress
+        self.strout = ""
+        self.strerr = ""
+        self.cflags = False
+        if cmd[0:7] == "scripts":
+            cmd = os.path.join(self.srctree, cmd)
+        self.cmd = cmd
+        self.cmdvec = cmd.split()
+        self.pervasive_opts = [] # "global" ignore list
+        self.exceptions = []     # exception list for this file
+        self.file_except = []    # Aggregated list of check types to ignore for this file
+        self.re_except_def = {}  # check_type -> <regex to match it in stderr>
+        self.doc = {}		 # Used when parsing documentation: check type -> doc string
+        self.cont = []
+        self.last_ignore = False
+        self.unclassified = 0	 # With RegexFilter: Number of "red" lines not classified
+
+    def filter_env(self, dict):
+        return dict
+
+    def readline(self, is_stdout, fd):
+        tmp_str = ""
+        try:
+            s = os.read(fd, 1000)
+            while s != '':
+                tmp_str += s
+                s = os.read(fd, 1)
+        except OSError:
+            None
+
+        if is_stdout:
+            self.strout += tmp_str
+            tmp_str = self.strout
+        else:
+            self.strerr += tmp_str
+            tmp_str = self.strerr
+
+        inx = tmp_str.find('\n') + 1
+        if inx != 0:
+            t = tmp_str[:inx]
+            if is_stdout:
+                self.strout = tmp_str[inx:]
+            else:
+                self.strerr = tmp_str[inx:]
+        else:
+            return ''
+        return t
+
+    def SetNonblocking(self, fd):
+        fl = fcntl.fcntl(fd, fcntl.F_GETFL)
+        try:
+            fcntl.fcntl(fd, fcntl.F_SETFL, fl | os.O_NDELAY)
+        except AttributeError:
+            fcntl.fcntl(fd, fcntl.F_SETFL, fl | fcntl.FNDELAY)
+
+    def Run(self, file, verbose):
+        cmdvec = self.cmdvec
+        if self.cflags:
+            cmdvec += c_argv
+        if not force:
+            self.file_except = set(self.exceptions + self.pervasive_opts)
+        self.Postprocess()
+        if not file:
+            raise CheckError("error: missing file parameter")
+        cmdvec.append(file)
+        if debug:
+            print "  ** running %s: %s" % (self.name, " ".join(cmdvec))
+        elif verbose:
+            print "  -- checker %s --" % self.name
+        try:
+            ret = self.RunCommand(cmdvec, self.ofilter, self.efilter)
+        except OSError, e:
+            if re.match(".*No such file or directory", str(e)):
+                if len(config.checkers) == 1:
+                    raise CheckError("Failed to run checker %s: %s: %s" % (self.name, self.cmd, str(e)))
+                if verbose:
+                    print "  ** %s does not exist - ignoring %s **" % (self.name, self.cmd)
+                return 0
+        ret = self.PostRun(ret)
+        return ret
+
+    def RunCommand(self, cmdvec, ofilter, efilter):
+	my_env = self.filter_env(os.environ)
+        child = subprocess.Popen(cmdvec, shell = False, \
+                     stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=".", env=my_env)
+        sout = child.stdout
+        serr = child.stderr
+        ofd = sout.fileno()
+        efd = serr.fileno()
+        oeof = False
+        eeof = False
+        check_errors = []
+        self.SetNonblocking(ofd)
+        self.SetNonblocking(efd)
+        while True:
+            ready = select.select([ofd,efd],[],[],0.1)
+            if ofd in ready[0]:
+                if child.poll() != None:
+                    oeof = True
+                oline = self.readline(True, ofd)
+                while oline != '':
+                    if ofilter:
+                        ofilter(oline, verbose)
+                    else:
+                        sys.stdout.write(oline)
+                    oline = self.readline(True, ofd)
+            if efd in ready[0]:
+                if child.poll() != None:
+                    eeof = True
+                eline = self.readline(False, efd)
+                while eline != '':
+                    if efilter:
+                        check_err = efilter(eline, verbose)
+                        if check_err != None:
+                                check_errors.append(check_err)
+                    else:
+                        sys.stderr.write(eline)
+                    eline = self.readline(False, efd)
+            if oeof and eeof:
+                break
+        serr.close()
+        sout.close()
+        retcode = child.wait()
+        if check_errors != []:
+            estr = "".join(check_errors)
+            if estr != "":
+                sys.stderr.write(estr)
+                if not retcode:
+                    retcode = 131
+            else:
+                if verbose:
+                    print "%s  ** %d suppressed errors/warnings from %s%s" % (BLUE, len(check_errors), self.name, ENDCOLOR)
+                retcode = 0
+        return retcode
+
+    def ParseOptional(self, cmd, argv):
+        raise CheckError("Undefined command '%s' for checker '%s'" % (cmd, self.name))
+
+    # Called as final step before running the checker:
+    def Postprocess(self):
+        # Do nothing - just for redefinition in subclasses
+        return
+
+    # Called as a post processing step after running the checker:
+    # Input parameter is return value from Run()
+    def PostRun(self, retval):
+        # Do nothing - just for redefinition in subclasses
+        return retval
+
+    # Default standard output filter:
+    def suppress(self, line, verbose):
+        if verbose:
+            sys.stdout.write(line)
+
+    # A matching filter for stderr:
+    def RegexFilter(self, line, verbose):
+        if self.cont:
+            m = re.match(self.cont[0], line)
+            self.cont = self.cont[1:]
+            if m:
+                if self.last_ignore:
+                    return ""
+                else:
+                    return line
+
+        for t, regex in self.re_except_def.iteritems():
+            r = "^(.*:\d+:)\s(\w+:)\s(%s.*)$" % regex[0]
+            m = re.match(r, line)
+            if m:
+                if len(regex) > 1:
+                    self.cont = regex[1:]
+                if t in self.file_except:
+                    self.last_ignore = True
+                    return ""
+                else:
+                    self.last_ignore = False
+                    return "%s%s %s:%s%s%s: %s %s\n" % (BROWN, m.group(1), self.name.upper(), BLUE, t, ENDCOLOR, m.group(2), m.group(3))
+        self.unclassified = self.unclassified + 1
+        return RED + line + ENDCOLOR
+
+    def ListTypes(self):
+        if len(self.re_except_def) > 0:
+            print BLUE + BOLD + "  Check types declared for %s in runchecks configuration%s" % (self.name, ENDCOLOR)
+        for t, regex in self.re_except_def.iteritems():
+            print "\t%-22s %s" % (t, "\\n".join(regex))
+        if len(self.re_except_def) > 0:
+            print ""
+        return 0
+
+    def addflags(self, argv):
+        self.cmdvec += argv
+
+    def exception(self, type, relpath, argv):
+        for f in argv:
+            if f == ("%s%s" % (relpath, bfile)):
+                self.exceptions.append(type)
+
+    def pervasive(self, argv):
+        self.pervasive_opts += argv
+
+    def typedef(self, argv):
+        exp = " ".join(argv[1:])
+        elist = exp.split("\\n")
+        self.re_except_def[argv[0]] = elist
+
+
+# Individual checker implementations:
+#
+
+# checkpatch
+class CheckpatchRunner(Checker):
+    def __init__(self, srctree, workdir):
+        Checker.__init__(self, "checkpatch", "scripts/checkpatch.pl", srctree, workdir)
+        self.cmdvec.append("--file")
+        self.line_len = 0
+        # checkpatch sends all it's warning and error output to stdout,
+        # redirect and do limited filtering:
+        self.ofilter = self.out_filter
+
+    def ParseOptional(self, cmd, argv):
+        if cmd == "line_len":
+            self.line_len = int(argv[0])
+        else:
+            Checker.ParseOptional(self, cmd, argv)
+
+    def Postprocess(self):
+        if config.color:
+            self.cmdvec.append("--color=always")
+        if self.line_len:
+            self.cmdvec.append("--max-line-length=%d" % self.line_len)
+        if self.file_except:
+            self.cmdvec.append("--ignore=%s" % ",".join(self.file_except))
+
+    # Extracting a condensed doc of types to filter on:
+    def man_filter(self, line, verbose):
+        t = line.split()
+        if len(t) > 1 and t[1] != "Message":
+            sys.stdout.write("\t%s\n" % t[1])
+
+    def out_filter(self, line, verbose):
+        # --terse produces this message even with no errors,
+        #  suppress unless run with -v:
+        if not verbose and re.match("^total: 0 errors, 0 warnings, 0 checks,", line):
+            return
+        sys.write.stderr(line)
+
+    def ListTypes(self):
+        print BLUE + BOLD + "  Supported check types for checkpatch" + ENDCOLOR
+        # Parse help output:
+        cmdvec = ["%s/scripts/checkpatch.pl" % self.srctree, "--list-types"]
+        self.RunCommand(cmdvec, self.man_filter, None)
+        print ""
+        return 0
+
+# sparse
+class SparseRunner(Checker):
+    def __init__(self, srctree, workdir):
+        Checker.__init__(self, "sparse", "sparse", srctree, workdir)
+        self.efilter = self.RegexFilter
+
+    def sparse_name(self, rs_type):
+        l_name = rs_type.lower()
+        s_name = ""
+        for c in l_name:
+            if c == '_':
+                s_name += '-'
+            else:
+                s_name += c
+        return s_name
+
+    def runchecks_name(self, sparse_type):
+        u_name = sparse_type.upper()
+        rc_name =""
+        for c in u_name:
+            if c == '-':
+                rc_name += '_'
+            else:
+                rc_name += c
+        return rc_name
+
+    def Postprocess(self):
+        if self.file_except:
+            for e in self.file_except:
+                self.cmdvec.append("-Wno-%s" % self.sparse_name(e))
+
+    # Extracting a condensed doc of types to filter on:
+    def man_filter(self, line, verbose):
+        if self.doc_next:
+            doc = line.strip()
+            self.doc[self.doc_next] = doc
+            self.doc_next = False
+            return
+        match = re.search("^\s+-W([\w-]+)\s*$", line)
+        if match:
+            name = match.group(1)
+            if re.match("sparse-", name):
+                return
+            rs_type = self.runchecks_name(name)
+            self.doc_next = rs_type
+
+    def ListTypes(self):
+        # Parse manual output:
+        cmdvec = ["man", "sparse"]
+        self.doc_next = False
+        ret = self.RunCommand(cmdvec, self.man_filter, None)
+        if ret:
+            return ret
+        print BLUE + BOLD + "\n  Types derived from sparse from documentation in manpage" + ENDCOLOR
+        for t, doc in self.doc.iteritems():
+            print "\t%-22s %s" % (t, doc)
+            try:
+                regex = self.re_except_def[t]
+                print "\t%-22s %s" % ("", GREEN + "\\n".join(regex) + ENDCOLOR)
+            except:
+                print "\t%-22s %s" % ("", RED + "(regex match (typedef) missing)" + ENDCOLOR)
+        print BLUE + BOLD + "\n  Types for sparse only declared for runchecks or not documented in manpage" + ENDCOLOR
+        for t, regex in self.re_except_def.iteritems():
+            try:
+                self.doc[t]
+            except:
+                print "\t%-22s %s" % (t, GREEN + "\\n".join(regex) + ENDCOLOR)
+        print ""
+        return 0
+
+# checkdoc
+class CheckdocRunner(Checker):
+    def __init__(self, srctree, workdir):
+        Checker.__init__(self, "checkdoc", "scripts/kernel-doc", srctree, workdir)
+        self.cmdvec.append("-none")
+        self.efilter = self.RegexFilter
+
+# coccicheck (coccinelle) (WIP)
+class CoccicheckRunner(Checker):
+    def __init__(self, srctree, workdir):
+        Checker.__init__(self, "coccicheck", "scripts/coccicheck", srctree, workdir)
+        self.debug_file = None
+        self.efilter = self.CoccicheckFilter
+
+    def filter_env(self, dict):
+        newdict = os.environ
+        # If debug file is not set by the user, override it and present the output on stderr:
+        try:
+            df = newdict["DEBUG_FILE"]
+        except:
+            print "*** debug_file!"
+            self.debug_file = '/tmp/cocci_%s.log' % os.getpid()
+            newdict["DEBUG_FILE"] = self.debug_file
+        return newdict
+
+    def CoccicheckFilter(self, line, verbose):
+        self.unclassified = self.unclassified + 1
+        if re.match(".*spatch -D report", line):
+            if verbose:
+                sys.stdout.write(line)
+        else:
+            return RED + line + ENDCOLOR
+
+    def PostRun(self, retval):
+        if not self.debug_file:
+            return retval
+        f = open(self.debug_file)
+        for line in f:
+            line = self.CoccicheckFilter(line, verbose)
+            if line:
+                sys.stderr.write(line)
+        f.close()
+        if self.debug_file:
+            os.remove(self.debug_file)
+        if retval == 0:
+            reval = ret
+        return retval
+
+checker_types = {}
+
+def AddChecker(checker):
+    checker_types[checker.name] = checker
+
+#
+# Start main program:
+#
+program = os.path.realpath(sys.argv[0])
+progname = basename(program)
+scriptsdir = dirname(program)
+srctree = dirname(scriptsdir)
+force = False
+ignore_config = False
+verbose = False
+debug = False
+error = True
+error_on_red = False
+usage_only = False
+argv = []
+c_argv = []
+fw_opts = []
+workdir = os.getcwd()
+optarg = False
+argc = 0
+
+AddChecker(CheckpatchRunner(srctree, workdir))
+AddChecker(SparseRunner(srctree, workdir))
+AddChecker(CheckdocRunner(srctree, workdir))
+AddChecker(CoccicheckRunner(srctree, workdir))
+
+for arg in sys.argv[1:]:
+    argc = argc + 1
+
+    if arg == "--":
+        argc = argc + 1
+        c_argv = sys.argv[argc:]
+        break;
+    elif arg == "-f":
+        force = True
+    elif arg == "-n":
+        ignore_config = True
+        force = True
+    elif arg == "-w":
+        error = False
+    elif arg == "-t":
+        error_on_red = True
+        error = False
+    elif arg == "-d":
+        debug = True
+    elif arg == "-v":
+        verbose = True
+    elif arg == "-h":
+        usage_only = True
+    else:
+        opt = re.match("^-.*$", arg)
+        if opt:
+            # Delay processing of these until we know the configuration:
+            fw_opts.append(opt.group(0))
+        else:
+            argv.append(arg)
+
+if not verbose:
+    try:
+        verb = int(os.environ["V"])
+        if verb != 0:
+            verbose = True
+    except KeyError:
+        verbose = False
+
+if not os.path.exists(os.path.join(srctree, "MAINTAINERS")):
+    srctree = None
+
+try:
+    file = argv[0]
+    bfile = basename(file)
+    workdir = dirname(file)
+except:
+    bfile = None
+    file = None
+
+unclassified = 0
+
+if debug:
+    print "Kernel root:\t%s\nFile:\t\t%s\nWorkdir:\t%s" % \
+        (srctree, bfile, workdir)
+    print "C args:\t\t%s\nargv:\t\t%s\n" % (" ".join(c_argv), " ".join(argv))
+
+try:
+    config = Config(srctree, workdir, "runchecks.cfg")
+    config.ProcessOpts(fw_opts)
+
+    if usage_only:
+        usage()
+    if not config.HasPathConfig() and not config.list_only and not force:
+        if verbose:
+            print "  ** %s: No configuration found - skip checks for %s" % (progname, file)
+        exit(0)
+
+    if config.color:
+        GREEN = '\033[32m'
+        RED = '\033[91m'
+        BROWN = '\033[33m'
+        BLUE = '\033[34m'
+        BOLD = '\033[1m'
+        ENDCOLOR = '\033[0m'
+    else:
+        BOLD = ''
+        GREEN = ''
+        RED = ''
+        BROWN = ''
+        BLUE = ''
+        ENDCOLOR = ''
+
+    ret = 0
+    for checker in config.checkers:
+        if config.list_only:
+            ret = checker.ListTypes()
+        else:
+            ret = checker.Run(file, verbose)
+            unclassified += checker.unclassified
+        if ret and error:
+            break
+
+    if not error and not (error_on_red and unclassified > 0):
+        ret = 0
+except CheckError, e:
+    print "  ** %s: %s" % (progname, str(e))
+    ret = 22
+except KeyboardInterrupt:
+    if verbose:
+        print "  ** %s: Interrupted by user" % progname
+    ret = 4
+
+exit(ret)
diff --git a/scripts/runchecks.cfg b/scripts/runchecks.cfg
new file mode 100644
index 0000000..c0b12cf
--- /dev/null
+++ b/scripts/runchecks.cfg
@@ -0,0 +1,63 @@
+checker checkpatch
+addflags  --quiet --show-types --strict --emacs
+line_len 110
+
+checker sparse
+addflags -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ -Wsparse-all
+cflags
+
+#	Name		Regular expression for matching in checker output
+typedef DECL		symbol '.*' was not declared. Should it be static\?
+typedef SHADOW		symbol '\w+' shadows an earlier one\n.*originally declared here
+typedef TYPESIGN	incorrect type in argument \d+ \(different signedness\)\n.*expected\n.*got
+typedef RETURN_VOID	returning void-valued expression
+typedef SIZEOF_BOOL	expression using sizeof bool
+typedef CONTEXT		context imbalance in '.*'
+typedef MEMCPY_MAX_COUNT \w+ with byte count of
+typedef CAST_TO_AS	cast adds address space to expression
+typedef ADDRESS_SPACE	incorrect type in .* \(different address spaces\)\n.*expected\n.*got
+typedef PTR_INHERIT	incorrect type in .* \(different base types\)\n.*expected\n.*got
+typedef PTR_SUBTRACTION_BLOWS potentially expensive pointer subtraction
+typedef VLA		Variable length array is used
+typedef OVERFLOW	constant [x\dA-F]+ is so big it is \w+
+typedef TAUTOLOGICAL_COMPARE self-comparison always evaluates to (true|false)
+typedef NON_POINTER_NULL Using plain integer as NULL pointer
+typedef BOOL_CAST_RESTRICTED restricted \w+ degrades to integer
+typedef TYPESIGN	incorrect type in .* \(different signedness\)\n.*expected\n.*got
+typedef FUNCTION_REDECL symbol '.*' redeclared with different type \(originally declared at
+typedef COND_ADDRESS_ARRAY the address of an array will always evaluate as true
+typedef BITWISE 	cast (to|from) restricted
+
+# Type names invented here - not maskable from sparse?
+typedef NO_DEREF	dereference of noderef expression
+typedef ARG_TYPE_MOD	incorrect type in .* \(different modifiers\)\n.*expected\n.*got
+typedef ARG_TYPE_COMP	incorrect type in .* \(incompatible .*\(.*\)\)\n.*expected\n.*got
+typedef ARG_AS_COMP	incompatible types in comparison expression \(different address spaces\)
+typedef CMP_TYPE	incompatible types in comparison expression \(different base types\)
+typedef SPARSE_OFF	"Sparse checking disabled for this file"
+typedef CAST_TRUNC	cast truncates bits from constant value
+typedef CAST_FROM_AS	cast removes address space of expression
+typedef EXT_LINK_DEF	function '\w+' with external linkage has definition
+typedef FUNC_ARITH	arithmetics on pointers to functions
+typedef CALL_NO_TYPE	call with no type!
+typedef FUNC_SUB	subtraction of functions\? Share your drugs
+typedef STRING_CONCAT	trying to concatenate \d+-character string \(\d+ bytes max\)
+typedef INARG_DIRECTIVE directive in argument list
+typedef NONSCALAR_CAST  cast (to|from) non-scalar
+
+checker checkdoc
+typedef PARAM_DESC	No description found for parameter
+typedef X_PARAM    	Excess function parameter
+typedef X_STRUCT	Excess struct member
+typedef FUN_PROTO    	cannot understand function prototype
+typedef DOC_FORMAT   	Incorrect use of kernel-doc format
+typedef BAD_LINE	bad line
+typedef AMBIGUOUS	Cannot understand.*\n on line
+typedef BOGUS_STRUCT	Cannot parse struct or union
+typedef DUPL_SEC	duplicate section name
+
+checker coccicheck
+cflags
+
+run sparse checkpatch checkdoc
+#run all
diff --git a/scripts/runchecks_help.txt b/scripts/runchecks_help.txt
new file mode 100644
index 0000000..aa3b69a
--- /dev/null
+++ b/scripts/runchecks_help.txt
@@ -0,0 +1,43 @@
+Usage: runchecks  [<options>] c_file [-- <c_parameters>]
+  - run code checkers in a conformant way.
+
+Options:
+  -h|--help	List this text
+  --list	List the different configured checkers and the list of interpreted check
+  		types for each of them.
+  --		Separator between parameters to runchecks and compiler parameters to be
+		passed directly to the checkers.
+  --run:[checker1[,checker2..]|all]
+  		Override the default set of checkers to be run for each source file. By
+		default the checkers to run will be the intersection of the checkers
+		configured by ``run`` commands in the configuration file and the
+		checkers that is actually available on the machine. Use 'all'
+		to run all the configured checkers.
+  --color	Use coloring in the error and warning output. In this mode
+		output from checkers that are supported by typedefs but not
+		captured by any such will be highlighted in red to make it
+		easy to detect that a typedef rule is missing. See -t below.
+  -f		Force mode: force runchecks to run a full run in directories/trees
+		where runchecks does not find a	runchecks.cfg file. The default
+		behaviour is to skip running checkers in directories/trees
+		where no matching runchecks.cfg file is found either in the
+		source file directory or above.
+  -n		Ignore all runchecks.cfg files except the one in scripts,
+		which are used for basic runchecks configuration. This allows
+		an easy way to run a "bare" version of checking where all
+		issues are reported, even those intended to be suppressed.
+		Implicitly enables force mode.
+  -w		Behave as if 0 on exit from all checkers. Normally
+		runchecks will fail on the first checker to produce errors or
+		warnings, in fact anything that produces not suppressed
+		output on stderr. This is to make it easy to work interactively,
+		avoiding overlooking anything, but sometimes it is useful to
+		be able to produce a full report of status.
+  -t		Typedef setup mode: For checkers where runchecks enable typedefs:
+  		Behaves as -w except for stderr output that is not captured
+		by any typedefs. This is a convenience mode while
+		fixing/improving typedef setup. Use with --color to get red
+		output for the statements to capture with new typedefs.
+  -v		Verbose output. Also enabled if called from make with V=1,
+		but it is useful to be able to only enable verbose mode for runchecks.
+  -d		Debugging output - more verbose.
-- 
git-series 0.9.1

[PATCH v2 2/5] Documentation: Add doc for runchecks, a checker runner

[Knut Omang]

The runchecks program unifies configuration, processing
and output for multiple checker tools to make them
all run as part of the C=1 or C=2 option to make.

Currently with full support and unified behaviour for
sparse:     sparse
checkpatch: scripts/checkpatch.pl
checkdoc:   kernel-doc -none

In principle supported but not unified in output(yet):
coccinelle: scripts/coccicheck

Introduces a new documentation section titled
"Makefile support for running checkers"

Also updates documentation for the make C= option
in some other doc files, as the behaviour has
been changed to be less sparse specific and more
generic. The coccinelle documentation also had the
behaviour of C=1 and C=2 swapped.

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>
---
 Documentation/dev-tools/coccinelle.rst |  12 +-
 Documentation/dev-tools/index.rst      |   1 +-
 Documentation/dev-tools/runchecks.rst  | 215 ++++++++++++++++++++++++++-
 Documentation/dev-tools/sparse.rst     |  30 +++-
 Documentation/kbuild/kbuild.txt        |   9 +-
 5 files changed, 257 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/dev-tools/runchecks.rst

diff --git a/Documentation/dev-tools/coccinelle.rst b/Documentation/dev-tools/coccinelle.rst
index 94f41c2..c98cc44 100644
--- a/Documentation/dev-tools/coccinelle.rst
+++ b/Documentation/dev-tools/coccinelle.rst
@@ -157,17 +157,19 @@ For example, to check drivers/net/wireless/ one may write::
 
     make coccicheck M=drivers/net/wireless/
 
-To apply Coccinelle on a file basis, instead of a directory basis, the
-following command may be used::
+To apply Coccinelle as the only checker on a file basis,
+instead of a directory basis, the following command may be used::
 
-    make C=1 CHECK="scripts/coccicheck"
+    make C=2 CF="--run:coccicheck"
 
-To check only newly edited code, use the value 2 for the C flag, i.e.::
+To check only newly edited code, use the value 1 for the C flag, i.e.::
 
-    make C=2 CHECK="scripts/coccicheck"
+    make C=1 CF="--run:coccicheck"
 
 In these modes, which works on a file basis, there is no information
 about semantic patches displayed, and no commit message proposed.
+For more information about options in this calling mode, see
+Documentation/dev-tools/runchecks.rst .
 
 This runs every semantic patch in scripts/coccinelle by default. The
 COCCI variable may additionally be used to only apply a single
diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst
index e313925..cb4506d 100644
--- a/Documentation/dev-tools/index.rst
+++ b/Documentation/dev-tools/index.rst
@@ -16,6 +16,7 @@ whole; patches welcome!
 
    coccinelle
    sparse
+   runchecks
    kcov
    gcov
    kasan
diff --git a/Documentation/dev-tools/runchecks.rst b/Documentation/dev-tools/runchecks.rst
new file mode 100644
index 0000000..b25b3de
--- /dev/null
+++ b/Documentation/dev-tools/runchecks.rst
@@ -0,0 +1,215 @@
+.. Copyright 2017 Knut Omang <knut.omang@oracle.com>
+
+Makefile support for running checkers
+=====================================
+
+Tools like sparse, coccinelle and scripts/checkpatch.pl 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 and sparse etc. with all
+bells and whistles enabled, in a way that these checkers 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 checker
+  rule, even if that rule is a sensible one in the general case.
+* Some subsystems have different restrictions and requirements.
+  (Ideally, the number of subsystems with differing restrictions and
+  requirements will diminish over time.)
+* Similarly, the kernel contains a lot of code that predates the tools, or at
+  least some of the newer rules, and we would like these tools to evolve without
+  requiring the need to fix all issues detected with it in the same commit.
+  We also want to accommodate new tools, so that each new tool does not
+  have to reinvent it's own mechanism for running checks.
+* On the other hand, we want to make sure that files that are clean
+  (to some well defined extent, such as passing checkpatch or sparse
+  with checks only for certain important types of issues) keep being so.
+
+This is the purpose of ``scripts/runchecks``.
+
+The ``runchecks`` program looks for files named ``runchecks.cfg`` in the
+``scripts`` directory, then in the directory hierarchy of the source file,
+starting where that source file is located, searching upwards. If at least
+one such file exists in the source tree, ``runchecks`` parses a set of
+rules from it, and use them to determine how to invoke a set of individual
+checker tools for a particular file. The kernel Makefile system supports using
+this feature as an integrated part of compiling the code, using the
+``C={1,2}`` option. With::
+
+	make C=1
+
+runchecks will be invoked if the file needs to be recompiled. With ::
+
+	make C=2
+
+runchecks will be invoked for all source files, even if they do not need
+recompiling. Based on the configuration, ``runchecks`` will invoke one or
+more checkers. The number and types of checkers to run are configurable and
+can also be selected on the command line::
+
+	make C=2 CF="--run:sparse,checkpatch"
+
+If only one checker is run, any parameter that is not recognized by
+runchecks itself will be forwarded to the checker. If more than one checker
+is enabled, parameters can be forwarded to a specific checker by means of
+this syntax::
+
+	make C=2 CF="--to-checkpatch:--terse"
+
+A comma separated list of parameters can be supplied if necessary.
+
+Supported syntax of the runchecks.cfg configuration file
+--------------------------------------------------------
+
+The runchecks configuration file chain can be used to set policies and "rein in"
+checker errors piece by piece for a particular subsystem or driver. It can
+also be used to mitigate and extend checkers that does not support
+selective suppression of all it's checks.
+
+Two classes of configuration is available. The first class is configuration
+that defines what checkers are enabled, and also allow a limited form of
+pattern matching to extend checking, to mitigate for checks that cannot be
+selectively disabled by command line parameters to the underlying tool
+itself. This type of configuration should go into the first accessed
+configuration file, and has been preconfigured for the currently supported
+checkers in ``scripts/runchecks.cfg``. The second class is the features for
+configuring the output of the checkers by selectively suppressing checks on
+a per file or per check basis. These typically goes in the source tree in
+the directory of the source file or above. Some of the syntax is generic
+and some is only supported by some checkers.
+
+For the first class of configuration the following syntax is supported::
+
+	# comments
+	checker checkpatch [command]
+	addflags <list of extra flags and parameters>
+	cflags
+	typedef NAME <regular expression>
+	run [checker list|all]
+
+The ``checker`` command switches ``runchecks``'s attention to a particular
+checker. The following commands until the next ``checker`` statement
+applies to that particular checker. The first occurrence of ``checker``
+also serves as a potentially defining operation, if the checker name
+has not been preconfigured. In that case, a second parameter can be used
+to provide the name of the command used to run the checker.
+A full checker integration into runchecks will typically require some
+additions to runchecks, and will then have been preconfigured,
+but simple checkers might just be configured on the fly.
+
+The ``addflags`` command incrementally adds more flags and parameters to
+the command line used to invoke the checker. This applies to all
+invocations of the checker from runchecks.
+The ``cflags`` command tells to forward all the flags and options passed to
+the compiler invocation to the checker. The default is to suppress these
+parameters when invoking the checker.
+
+The ``typedef`` command adds ``NAME`` and associates it with the given
+regular expression. This expression is used to match against standard error
+output from the checker and ``NAME`` can be used as a new named check that
+runchecks understands and that can be used with checker supported names
+below to selectively suppress that particular set of warning or error
+messages. This is useful to handle output checks for which the underlying
+checker is does not provide any suppression. Check type namespaces are
+separate for the individual checkers. You can list the state of the built in and
+configured checker and check types with::
+
+	scripts/runchecks --list
+
+The checker implementations of the ``typedef`` command also allows
+runchecks to perform some unification of output by rewriting the output
+lines, adding optional color support, and use of the new type names in the
+error output, to ease the process of updating the runchecks.cfg files.
+Having a unified representation of the error output also makes it much
+easier to do statistics or other operations on top of an aggregated output
+from several checkers.
+
+For the second class of configuration the following syntax is supported::
+
+	# comments
+	checker checker_name
+	line_len <n>
+	except check_type [files ...]
+	pervasive check_type1 [check_type2 ...]
+
+The ``line_len`` directive defines the upper bound of characters per line
+tolerated in this directory. Currently only ``checkpatch`` supports this
+command.  The ``except`` directive takes a check 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 subtree.  The ``except`` and
+``pervasive`` directives can be used cumulatively to add more exceptions.
+
+Running checker programs from make
+----------------------------------
+
+You can run checkpatch subject to rules defined in ``runcheck.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 ``CF`` allows passing additional parameters to
+``runchecks``. You can for instance use::
+
+	make C=2 CF="--run:checkpatch --fix-inplace"
+
+to run only the ``checkpatch`` checker, and 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 under ``checker checkpatch`` to the local
+``runchecks.cfg``, and you can focus on fixing up errors subsystem or
+driver by driver on a type by type basis.
+
+By default runchecks will skip all files if a ``runchecks.cfg`` file cannot
+be found in the directory of the file or in the tree above.  This is to
+allow builds with ``C=2`` to pass even for subsystems that has not yet done
+anything to rein in checker errors. At some point when all subsystems and
+drivers either have fixed all checker errors or added proper
+``runchecks.cfg`` files, this can be changed.
+
+To force runchecks to run a full run in directories/trees where runchecks
+does not find  a ``runchecks.cfg`` file as well, use::
+
+	make C=2 CF="-f"
+
+If you like to see all the warnings and errors produced by the checkers, ignoring
+any runchecks.cfg files except the one under ``scripts``, you can use::
+
+	make C=2 CF="-n"
+
+or for a specific module directory::
+
+	make C=2 M=drivers/infiniband/core CF="--color -n -w"
+
+with the -w option to ``runchecks`` to suppress errors from any of the
+checkers and just continue on, and the ``--color`` option to present errors
+with colors where supported.
+
+Ever tightening checker rules
+-----------------------------
+
+Commit the changes to the relevant ``runchecks.cfg`` together with the code
+changes that fixes a particular type of issue, this will allow automatic
+checker running by default. 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 a full "make C=2" passes with no
+errors.
+
+Extending and improving checker support in ``runchecks``
+--------------------------------------------------------
+
+The runchecks program has been written with extensibility in mind.
+If the checker starts it's reporting lines with filename:lineno, there's a
+good chance that a new checker can simply be added by adding::
+
+	checker mychecker path_to_mychecker
+
+to ``scripts/runchecks.cfg`` and suitable ``typedef`` expressions to provide
+selective suppressions of output, however it is likely that some quirks are
+needed to make the new checker behave similar to the others, and to support
+the full set of features, such as the ``--list`` option. This is done by
+implementing a new subclass of the Checker class in ``runchecks``. This is the way
+all the available default supported checkers are implemented, and those
+relatively lean implementations could serve as examples.
diff --git a/Documentation/dev-tools/sparse.rst b/Documentation/dev-tools/sparse.rst
index 78aa00a..e3e8b27 100644
--- a/Documentation/dev-tools/sparse.rst
+++ b/Documentation/dev-tools/sparse.rst
@@ -101,5 +101,31 @@ recompiled, or use "make C=2" to run sparse on the files whether they need to
 be recompiled or not.  The latter is a fast way to check the whole tree if you
 have already built it.
 
-The optional make variable CF can be used to pass arguments to sparse.  The
-build system passes -Wbitwise to sparse automatically.
+The "make C={1,2}" form of kernel make indirectly calls sparse via "runchecks",
+which dependent on configuration and command line options may dispatch calls to
+other checkers in addition to sparse. Details on how this works is covered
+in Documentation/dev-tools/runchecks.rst .
+
+The optional make variable CF can be used to pass arguments to runchecks for dispatch
+to sparse. If sparse is the only tool enabled, any option not recognized by
+runchecks will be forwarded to sparse. If more than one tool is active, you must
+add the parameters you want sparse to get as a comma separated list prefixed by
+``--to-sparse:``. If you want sparse to be the only checker run, and you want
+some nice colored output, you can specify this as::
+
+	make C=2 CF="--run:sparse --color"
+
+This will cause sparse to be called for all files which are supported by a valid
+runchecks configuration (again see Documentation/dev-tools/runchecks.rst for
+details). If you want to run sparse on all files and ignore any missing
+configuration files(s), just add ``-n`` to the list of options passed to
+runchecks. This will cause runchecks to call sparse with all errors enabled for
+all files even if no valid configuration is found in the tree for the source files.
+
+By default "runchecks" is set to enable all sparse errors, but you can
+configure what checks to be applied by sparse on a per file or per subsystem
+basis. With the above invocation, make will fail and stop on the first file
+encountered with sparse errors or warnings in it. If you want to continue
+anyway, you can use::
+
+	make C=2 CF="--run:sparse --color -w"
diff --git a/Documentation/kbuild/kbuild.txt b/Documentation/kbuild/kbuild.txt
index ac2363e..260e688 100644
--- a/Documentation/kbuild/kbuild.txt
+++ b/Documentation/kbuild/kbuild.txt
@@ -103,10 +103,13 @@ CROSS_COMPILE is also used for ccache in some setups.
 
 CF
 --------------------------------------------------
-Additional options for sparse.
-CF is often used on the command-line like this:
+Additional options for runchecks, the generic checker runner.
+CF is often used on the command-line for instance like this:
 
-    make CF=-Wbitwise C=2
+    make C=2 CF="--run:sparse --color -w"
+
+to run the sparse tool only, and to use colored output and continue on warnings
+or errors.
 
 INSTALL_PATH
 --------------------------------------------------
-- 
git-series 0.9.1

[PATCH v2 3/5] 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 040aa79..febe010 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2989,7 +2989,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 v2 4/5] rds: Add runchecks.cfg for net/rds

[Knut Omang]

Add a runchecks.cfg to net/rds to start "reining in"
future checker errors, and making it easier to
selectively clean up existing issues.

This runchecks.cfg lets make C=2 M=net/rds
pass with all errors/warnings suppressed

See Documentation/dev-tools/runchecks.rst for
motivation and details.

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>
---
 net/rds/runchecks.cfg | 76 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+)
 create mode 100644 net/rds/runchecks.cfg

diff --git a/net/rds/runchecks.cfg b/net/rds/runchecks.cfg
new file mode 100644
index 0000000..2a02701
--- /dev/null
+++ b/net/rds/runchecks.cfg
@@ -0,0 +1,76 @@
+#
+# checker suppression lists for net/rds
+#
+# see Documentation/dev-tools/runchecks.rst
+#
+
+checker checkpatch
+##################
+
+# Accept somewhat longer lines:
+line_len 110
+
+# Important to fix from a quality perspective:
+#
+except AVOID_BUG connection.c ib.c ib_cm.c ib_rdma.c ib_recv.c ib_ring.c ib_send.c info.c loop.c message.c
+except AVOID_BUG rdma.c recv.c send.c stats.c tcp_recv.c transport.c
+except MEMORY_BARRIER ib_recv.c send.c tcp_send.c
+except WAITQUEUE_ACTIVE cong.c ib_rdma.c ib_ring.c ib_send.c
+except UNNECESSARY_ELSE bind.c ib_cm.c
+except MACRO_ARG_PRECEDENCE connection.c ib.h rds.h
+except MACRO_ARG_REUSE rds.h
+except ALLOC_SIZEOF_STRUCT cong.c ib.c ib_cm.c loop.c message.c rdma.c
+except UNCOMMENTED_DEFINITION ib_cm.c
+
+# Code simplification:
+#
+except ALLOC_WITH_MULTIPLY ib.c
+except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c transport.c
+except UNNECESSARY_ELSE ib_fmr.c
+except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
+except PRINTK_RATELIMITED ib_frmr.c
+except EMBEDDED_FUNCTION_NAME ib_rdma.c
+
+# Style and readability:
+#
+except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
+except OOM_MESSAGE ib.c tcp.c
+except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
+except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
+except OPEN_ENDED_LINE recv.c ib_recv.c
+
+# Candidates to leave as exceptions (don't fix):
+except MULTIPLE_ASSIGNMENTS ib_send.c
+except LONG_LINE_STRING connection.c
+except OPEN_BRACE connection.c
+
+# These are in most of the source files, ignore for all files:
+#
+pervasive NETWORKING_BLOCK_COMMENT_STYLE BLOCK_COMMENT_STYLE
+
+# These are easily autocorrected by checkpatch with --fix-inplace:
+# Just ignore here - fixed in a separate commit:
+#
+pervasive PREFER_KERNEL_TYPES LINE_SPACING SPACING SPACE_BEFORE_TAB SPLIT_STRING
+pervasive BIT_MACRO SIZEOF_PARENTHESIS LOGICAL_CONTINUATIONS GLOBAL_INITIALISERS
+pervasive ALLOC_WITH_MULTIPLY TABSTOP
+
+# These were easy to fix manually while getting make C=2 to pass.
+# Fixed in a separate commit:
+#
+pervasive SUSPECT_CODE_INDENT PARENTHESIS_ALIGNMENT BRACES PRINTF_L COMPARISON_TO_NULL
+pervasive LOGICAL_CONTINUATIONS
+
+
+checker sparse
+##############
+
+except VLA connection.c
+except COND_ADDRESS_ARRAY connection.c
+except PTR_SUBTRACTION_BLOWS ib_recv.c
+except TYPESIGN ib_frmr.c
+
+# This is coming from linux/dma-mapping.h:
+except RETURN_VOID ib_cm.c
+
+pervasive BITWISE SHADOW
-- 
git-series 0.9.1

[PATCH v2 5/5] RDMA/core: Add runchecks.cfg for drivers/infiniband/core

[Knut Omang]

Add a runchecks.cfg to drivers/infiniband/core
to start "reining in" future checker errors,
and making it easier to selectively clean up existing
issues.

This runchecks.cfg lets make C=2 M=drivers/infiniband/core
pass with all errors/warnings suppressed

See Documentation/dev-tools/runchecks.rst for
motivation and details.

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>
---
 drivers/infiniband/core/runchecks.cfg | 83 ++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+)
 create mode 100644 drivers/infiniband/core/runchecks.cfg

diff --git a/drivers/infiniband/core/runchecks.cfg b/drivers/infiniband/core/runchecks.cfg
new file mode 100644
index 0000000..75ca57c
--- /dev/null
+++ b/drivers/infiniband/core/runchecks.cfg
@@ -0,0 +1,83 @@
+#
+# checker suppression lists for drivers/infiniband/core
+#
+# see Documentation/dev-tools/runchecks.rst
+#
+
+checker checkpatch
+##################
+
+# Accept somewhat longer lines:
+line_len 110
+
+# Uncategorized:
+#
+except TRAILING_STATEMENTS packer.c
+except NON_OCTAL_PERMISSIONS rw.c
+except STATIC_CONST_CHAR_ARRAY sysfs.c
+except SYMBOLIC_PERMS sysfs.c
+except NEEDLESS_IF sysfs.c
+except NAKED_SSCANF device.c
+except ALLOC_WITH_MULTIPLY fmr_pool.c iwpm_util.c cma.c
+except MULTIPLE_ASSIGNMENTS rw.c verbs.c
+except ALLOC_SIZEOF_STRUCT sysfs.c iwpm_util.c iwpm_msg.c cma.c cm.c iwcm.c
+except LOGICAL_CONTINUATIONS mad.c iwpm_util.c user_mad.c
+
+# Important to fix/go through from a quality perspective:
+#
+except AVOID_BUG rw.c mad.c cm.c iwcm.c cma.c
+except UNCOMMENTED_DEFINITION ucma.c fmr_pool.c multicast.c mad_rmpp.c
+except UNCOMMENTED_DEFINITION ucm.c umem.c cma.c user_mad.c cm.c
+except ASSIGN_IN_IF mad.c cma.c uverbs_ioctl_merge.c
+except SUSPECT_CODE_INDENT iwpm_util.c cma.c uverbs_ioctl.c user_mad.c uverbs_cmd.c
+except COMPLEX_MACRO uverbs_ioctl_merge.c
+except BOOL_COMPARISON roce_gid_mgmt.c
+
+# Code simplification:
+#
+except UNNECESSARY_ELSE cache.c mad.c mad_rmpp.c
+except UNNECESSARY_PARENTHESES cma.c sa_query.c mad.c ucma.c mad_rmpp.c umem.c
+except UNNECESSARY_PARENTHESES user_mad.c uverbs_cmd.c uverbs_marshall.c cm.c
+except EMBEDDED_FUNCTION_NAME mad.c umem.c cma.c ucma.c user_mad.c
+except RETURN_VOID sysfs.c sa_query.c mad.c cma.c ucm.c uverbs_main.c umem.c
+except CONSTANT_COMPARISON smi.c
+except OOM_MESSAGE iwpm_util.c
+
+# Style and readability:
+#
+except OPEN_BRACE cache.c mad.c umem_odp.c umem_rbtree.c cm.c
+except MULTILINE_DEREFERENCE mad.c cm.c cma.c uverbs_main.c
+except LONG_LINE cma.c
+except PARENTHESIS_ALIGNMENT umem.c cm.c
+except FUNCTION_ARGUMENTS verbs.c uverbs_cmd.c sa_query.c sysfs.c
+
+# Candidates to leave as exceptions (don't fix):
+except SPACING umem_rbtree.c
+except MACRO_ARG_REUSE ud_header.c sa_query.c uverbs_ioctl_merge.c
+
+# These are in most of the source files, ignore for all files:
+#
+pervasive BLOCK_COMMENT_STYLE ENOSYS BRACES OPEN_ENDED_LINE
+
+# These are easily autocorrected by checkpatch with --fix-inplace:
+#
+pervasive SIZEOF_PARENTHESIS SPACING LINE_SPACING SPACE_BEFORE_TAB TABSTOP
+pervasive TRAILING_WHITESPACE POINTER_LOCATION INITIALISED_STATIC
+pervasive CODE_INDENT ELSE_AFTER_BRACE SYMBOLIC_PERMS LEADING_SPACE
+pervasive PARENTHESIS_ALIGNMENT COMPARISON_TO_NULL TYPO_SPELLING
+
+checker sparse
+##############
+
+except SHADOW sysfs.c fmr_pool.c user_mad.c uverbs_main.c ucm.c ucma.c
+except RETURN_VOID roce_gid_mgmt.c
+except TYPESIGN ucm.c ucma.c
+
+# From linux/device.h:
+except RETURN_VOID ucm.c
+
+checker checkdoc
+################
+
+except PARAM_DESC verbs.c device.c fmr_pool.c cache.c sa_query.c
+except X_PARAM verbs.c fmr_pool.c cache.c
-- 
git-series 0.9.1

Re: [PATCH v2 2/5] Documentation: Add doc for runchecks, a checker runner

[Julia Lawall]

> diff --git a/Documentation/dev-tools/runchecks.rst b/Documentation/dev-tools/runchecks.rst
> new file mode 100644
> index 0000000..b25b3de
> --- /dev/null
> +++ b/Documentation/dev-tools/runchecks.rst
> @@ -0,0 +1,215 @@
> +.. Copyright 2017 Knut Omang <knut.omang@oracle.com>
> +
> +Makefile support for running checkers
> +=====================================
> +
> +Tools like sparse, coccinelle and scripts/checkpatch.pl is able to detect a

is -> are

> +lot of syntactic and semantic issues with the code, and is also constantly

is -> are

> +evolving and detecting more. In an ideal world, all source files should
> +adhere to whatever rules imposed by checkpatch.pl and sparse etc. with all
> +bells and whistles enabled, in a way that these checkers 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 checker
> +  rule, even if that rule is a sensible one in the general case.
> +* Some subsystems have different restrictions and requirements.
> +  (Ideally, the number of subsystems with differing restrictions and
> +  requirements will diminish over time.)
> +* Similarly, the kernel contains a lot of code that predates the tools, or at
> +  least some of the newer rules, and we would like these tools to evolve without
> +  requiring the need to fix all issues detected with it in the same commit.
> +  We also want to accommodate new tools, so that each new tool does not
> +  have to reinvent it's own mechanism for running checks.

it's -> its

> +* On the other hand, we want to make sure that files that are clean
> +  (to some well defined extent, such as passing checkpatch or sparse
> +  with checks only for certain important types of issues) keep being so.
> +
> +This is the purpose of ``scripts/runchecks``.
> +
> +The ``runchecks`` program looks for files named ``runchecks.cfg`` in the
> +``scripts`` directory, then in the directory hierarchy of the source file,
> +starting where that source file is located, searching upwards. If at least
> +one such file exists in the source tree, ``runchecks`` parses a set of
> +rules from it, and use them to determine how to invoke a set of individual
> +checker tools for a particular file. The kernel Makefile system supports using
> +this feature as an integrated part of compiling the code, using the
> +``C={1,2}`` option. With::
> +
> +	make C=1
> +
> +runchecks will be invoked if the file needs to be recompiled. With ::
> +
> +	make C=2
> +
> +runchecks will be invoked for all source files, even if they do not need
> +recompiling. Based on the configuration, ``runchecks`` will invoke one or
> +more checkers. The number and types of checkers to run are configurable and
> +can also be selected on the command line::
> +
> +	make C=2 CF="--run:sparse,checkpatch"
> +
> +If only one checker is run, any parameter that is not recognized by
> +runchecks itself will be forwarded to the checker. If more than one checker
> +is enabled, parameters can be forwarded to a specific checker by means of
> +this syntax::
> +
> +	make C=2 CF="--to-checkpatch:--terse"
> +
> +A comma separated list of parameters can be supplied if necessary.
> +
> +Supported syntax of the runchecks.cfg configuration file
> +--------------------------------------------------------
> +
> +The runchecks configuration file chain can be used to set policies and "rein in"
> +checker errors piece by piece for a particular subsystem or driver. It can
> +also be used to mitigate and extend checkers that does not support

does -> do

> +selective suppression of all it's checks.

it's -> its

> +Two classes of configuration is available. The first class is configuration
> +that defines what checkers are enabled, and also allow a limited form of
> +pattern matching to extend checking, to mitigate for checks that cannot be
> +selectively disabled by command line parameters to the underlying tool
> +itself. This type of configuration should go into the first accessed
> +configuration file, and has been preconfigured for the currently supported
> +checkers in ``scripts/runchecks.cfg``. The second class is the features for
> +configuring the output of the checkers by selectively suppressing checks on
> +a per file or per check basis. These typically goes in the source tree in

goes -> go

> +the directory of the source file or above. Some of the syntax is generic
> +and some is only supported by some checkers.
> +
> +For the first class of configuration the following syntax is supported::
> +
> +	# comments
> +	checker checkpatch [command]
> +	addflags <list of extra flags and parameters>
> +	cflags
> +	typedef NAME <regular expression>
> +	run [checker list|all]
> +
> +The ``checker`` command switches ``runchecks``'s attention to a particular
> +checker. The following commands until the next ``checker`` statement
> +applies to that particular checker. The first occurrence of ``checker``

applies -> apply

> +also serves as a potentially defining operation, if the checker name
> +has not been preconfigured. In that case, a second parameter can be used
> +to provide the name of the command used to run the checker.
> +A full checker integration into runchecks will typically require some
> +additions to runchecks, and will then have been preconfigured,
> +but simple checkers might just be configured on the fly.
> +
> +The ``addflags`` command incrementally adds more flags and parameters to
> +the command line used to invoke the checker. This applies to all
> +invocations of the checker from runchecks.
> +The ``cflags`` command tells to forward all the flags and options passed to
> +the compiler invocation to the checker. The default is to suppress these
> +parameters when invoking the checker.
> +
> +The ``typedef`` command adds ``NAME`` and associates it with the given
> +regular expression. This expression is used to match against standard error
> +output from the checker and ``NAME`` can be used as a new named check that
> +runchecks understands and that can be used with checker supported names
> +below to selectively suppress that particular set of warning or error
> +messages. This is useful to handle output checks for which the underlying
> +checker is does not provide any suppression. Check type namespaces are
> +separate for the individual checkers. You can list the state of the built in and
> +configured checker and check types with::
> +
> +	scripts/runchecks --list
> +
> +The checker implementations of the ``typedef`` command also allows
> +runchecks to perform some unification of output by rewriting the output
> +lines, adding optional color support, and use of the new type names in the
> +error output, to ease the process of updating the runchecks.cfg files.
> +Having a unified representation of the error output also makes it much
> +easier to do statistics or other operations on top of an aggregated output
> +from several checkers.
> +
> +For the second class of configuration the following syntax is supported::
> +
> +	# comments
> +	checker checker_name
> +	line_len <n>
> +	except check_type [files ...]
> +	pervasive check_type1 [check_type2 ...]
> +
> +The ``line_len`` directive defines the upper bound of characters per line
> +tolerated in this directory. Currently only ``checkpatch`` supports this
> +command.  The ``except`` directive takes a check 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 subtree.  The ``except`` and
> +``pervasive`` directives can be used cumulatively to add more exceptions.
> +
> +Running checker programs from make
> +----------------------------------
> +
> +You can run checkpatch subject to rules defined in ``runcheck.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 ``CF`` allows passing additional parameters to
> +``runchecks``. You can for instance use::
> +
> +	make C=2 CF="--run:checkpatch --fix-inplace"
> +
> +to run only the ``checkpatch`` checker, and to have checkpatch trying to fix


trying -> try

> +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 under ``checker checkpatch`` to the local
> +``runchecks.cfg``, and you can focus on fixing up errors subsystem or
> +driver by driver on a type by type basis.
> +
> +By default runchecks will skip all files if a ``runchecks.cfg`` file cannot
> +be found in the directory of the file or in the tree above.  This is to
> +allow builds with ``C=2`` to pass even for subsystems that has not yet done
> +anything to rein in checker errors. At some point when all subsystems and
> +drivers either have fixed all checker errors or added proper
> +``runchecks.cfg`` files, this can be changed.
> +
> +To force runchecks to run a full run in directories/trees where runchecks
> +does not find  a ``runchecks.cfg`` file as well, use::
> +
> +	make C=2 CF="-f"
> +
> +If you like to see all the warnings and errors produced by the checkers, ignoring
> +any runchecks.cfg files except the one under ``scripts``, you can use::
> +
> +	make C=2 CF="-n"
> +
> +or for a specific module directory::
> +
> +	make C=2 M=drivers/infiniband/core CF="--color -n -w"
> +
> +with the -w option to ``runchecks`` to suppress errors from any of the
> +checkers and just continue on, and the ``--color`` option to present errors
> +with colors where supported.
> +
> +Ever tightening checker rules
> +-----------------------------
> +
> +Commit the changes to the relevant ``runchecks.cfg`` together with the code
> +changes that fixes a particular type of issue, this will allow automatic
> +checker running by default. 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 a full "make C=2" passes with no
> +errors.
> +
> +Extending and improving checker support in ``runchecks``
> +--------------------------------------------------------
> +
> +The runchecks program has been written with extensibility in mind.
> +If the checker starts it's reporting lines with filename:lineno, there's a

it's -> its

> +good chance that a new checker can simply be added by adding::
> +
> +	checker mychecker path_to_mychecker
> +
> +to ``scripts/runchecks.cfg`` and suitable ``typedef`` expressions to provide
> +selective suppressions of output, however it is likely that some quirks are
> +needed to make the new checker behave similar to the others, and to support

similar -> similarly

julia

> +the full set of features, such as the ``--list`` option. This is done by
> +implementing a new subclass of the Checker class in ``runchecks``. This is the way
> +all the available default supported checkers are implemented, and those
> +relatively lean implementations could serve as examples.
> diff --git a/Documentation/dev-tools/sparse.rst b/Documentation/dev-tools/sparse.rst
> index 78aa00a..e3e8b27 100644
> --- a/Documentation/dev-tools/sparse.rst
> +++ b/Documentation/dev-tools/sparse.rst
> @@ -101,5 +101,31 @@ recompiled, or use "make C=2" to run sparse on the files whether they need to
>  be recompiled or not.  The latter is a fast way to check the whole tree if you
>  have already built it.
>
> -The optional make variable CF can be used to pass arguments to sparse.  The
> -build system passes -Wbitwise to sparse automatically.
> +The "make C={1,2}" form of kernel make indirectly calls sparse via "runchecks",
> +which dependent on configuration and command line options may dispatch calls to
> +other checkers in addition to sparse. Details on how this works is covered
> +in Documentation/dev-tools/runchecks.rst .
> +
> +The optional make variable CF can be used to pass arguments to runchecks for dispatch
> +to sparse. If sparse is the only tool enabled, any option not recognized by
> +runchecks will be forwarded to sparse. If more than one tool is active, you must
> +add the parameters you want sparse to get as a comma separated list prefixed by
> +``--to-sparse:``. If you want sparse to be the only checker run, and you want
> +some nice colored output, you can specify this as::
> +
> +	make C=2 CF="--run:sparse --color"
> +
> +This will cause sparse to be called for all files which are supported by a valid
> +runchecks configuration (again see Documentation/dev-tools/runchecks.rst for
> +details). If you want to run sparse on all files and ignore any missing
> +configuration files(s), just add ``-n`` to the list of options passed to
> +runchecks. This will cause runchecks to call sparse with all errors enabled for
> +all files even if no valid configuration is found in the tree for the source files.
> +
> +By default "runchecks" is set to enable all sparse errors, but you can
> +configure what checks to be applied by sparse on a per file or per subsystem
> +basis. With the above invocation, make will fail and stop on the first file
> +encountered with sparse errors or warnings in it. If you want to continue
> +anyway, you can use::
> +
> +	make C=2 CF="--run:sparse --color -w"
> diff --git a/Documentation/kbuild/kbuild.txt b/Documentation/kbuild/kbuild.txt
> index ac2363e..260e688 100644
> --- a/Documentation/kbuild/kbuild.txt
> +++ b/Documentation/kbuild/kbuild.txt
> @@ -103,10 +103,13 @@ CROSS_COMPILE is also used for ccache in some setups.
>
>  CF
>  --------------------------------------------------
> -Additional options for sparse.
> -CF is often used on the command-line like this:
> +Additional options for runchecks, the generic checker runner.
> +CF is often used on the command-line for instance like this:
>
> -    make CF=-Wbitwise C=2
> +    make C=2 CF="--run:sparse --color -w"
> +
> +to run the sparse tool only, and to use colored output and continue on warnings
> +or errors.
>
>  INSTALL_PATH
>  --------------------------------------------------
> --
> git-series 0.9.1
>

Re: [PATCH v2 3/5] checkpatch: Improve --fix-inplace for TABSTOP

[Joe Perches]

On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> 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>

Well written commit log description, thanks but
I'm not sure that's actually better overall.

It's not possible to know if the appropriate thing
to do is to add a tab or remove spaces.

This may get it wrong about as often as the current
code gets it right.

Last I checked 6 months or so ago, there were ~8000
TABSTOP warnings in the kernel tree.

Most of those still seem valid and moving additional
places to the right is sometimes incorrect too.

It seems most common when there are 3 or 4 spaces
used as indent level indentation instead of tabs.

Dunno.

Anyone else have an opinion?

> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 040aa79..febe010 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2989,7 +2989,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;
>  				}
>  			}
>  		}

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Joe Perches]

On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> This patch series implements features to make it easier to run checkers on the
> entire kernel as part of automatic and developer testing.

This seems like a useful and reasonable series, thanks.
Do please take Julia's grammar updates.

How is this series to be applied?

Re: [PATCH v2 2/5] Documentation: Add doc for runchecks, a checker runner

[Knut Omang]

On Sat, 2017-12-16 at 16:08 +0100, Julia Lawall wrote:
> > diff --git a/Documentation/dev-tools/runchecks.rst b/Documentation/dev-
> tools/runchecks.rst
> > new file mode 100644
> > index 0000000..b25b3de
> > --- /dev/null
> > +++ b/Documentation/dev-tools/runchecks.rst
> > @@ -0,0 +1,215 @@
> > +.. Copyright 2017 Knut Omang <knut.omang@oracle.com>
> > +
> > +Makefile support for running checkers
> > +=====================================
> > +
> > +Tools like sparse, coccinelle and scripts/checkpatch.pl is able to detect a
> 
> is -> are
> 
> > +lot of syntactic and semantic issues with the code, and is also constantly
> 
> is -> are
> 
> > +evolving and detecting more. In an ideal world, all source files should
> > +adhere to whatever rules imposed by checkpatch.pl and sparse etc. with all
> > +bells and whistles enabled, in a way that these checkers 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 checker
> > +  rule, even if that rule is a sensible one in the general case.
> > +* Some subsystems have different restrictions and requirements.
> > +  (Ideally, the number of subsystems with differing restrictions and
> > +  requirements will diminish over time.)
> > +* Similarly, the kernel contains a lot of code that predates the tools, or at
> > +  least some of the newer rules, and we would like these tools to evolve without
> > +  requiring the need to fix all issues detected with it in the same commit.
> > +  We also want to accommodate new tools, so that each new tool does not
> > +  have to reinvent it's own mechanism for running checks.
> 
> it's -> its
> 
> > +* On the other hand, we want to make sure that files that are clean
> > +  (to some well defined extent, such as passing checkpatch or sparse
> > +  with checks only for certain important types of issues) keep being so.
> > +
> > +This is the purpose of ``scripts/runchecks``.
> > +
> > +The ``runchecks`` program looks for files named ``runchecks.cfg`` in the
> > +``scripts`` directory, then in the directory hierarchy of the source file,
> > +starting where that source file is located, searching upwards. If at least
> > +one such file exists in the source tree, ``runchecks`` parses a set of
> > +rules from it, and use them to determine how to invoke a set of individual
> > +checker tools for a particular file. The kernel Makefile system supports using
> > +this feature as an integrated part of compiling the code, using the
> > +``C={1,2}`` option. With::
> > +
> > +	make C=1
> > +
> > +runchecks will be invoked if the file needs to be recompiled. With ::
> > +
> > +	make C=2
> > +
> > +runchecks will be invoked for all source files, even if they do not need
> > +recompiling. Based on the configuration, ``runchecks`` will invoke one or
> > +more checkers. The number and types of checkers to run are configurable and
> > +can also be selected on the command line::
> > +
> > +	make C=2 CF="--run:sparse,checkpatch"
> > +
> > +If only one checker is run, any parameter that is not recognized by
> > +runchecks itself will be forwarded to the checker. If more than one checker
> > +is enabled, parameters can be forwarded to a specific checker by means of
> > +this syntax::
> > +
> > +	make C=2 CF="--to-checkpatch:--terse"
> > +
> > +A comma separated list of parameters can be supplied if necessary.
> > +
> > +Supported syntax of the runchecks.cfg configuration file
> > +--------------------------------------------------------
> > +
> > +The runchecks configuration file chain can be used to set policies and "rein in"
> > +checker errors piece by piece for a particular subsystem or driver. It can
> > +also be used to mitigate and extend checkers that does not support
> 
> does -> do
> 
> > +selective suppression of all it's checks.
> 
> it's -> its
> 
> > +Two classes of configuration is available. The first class is configuration
> > +that defines what checkers are enabled, and also allow a limited form of
> > +pattern matching to extend checking, to mitigate for checks that cannot be
> > +selectively disabled by command line parameters to the underlying tool
> > +itself. This type of configuration should go into the first accessed
> > +configuration file, and has been preconfigured for the currently supported
> > +checkers in ``scripts/runchecks.cfg``. The second class is the features for
> > +configuring the output of the checkers by selectively suppressing checks on
> > +a per file or per check basis. These typically goes in the source tree in
> 
> goes -> go
> 
> > +the directory of the source file or above. Some of the syntax is generic
> > +and some is only supported by some checkers.
> > +
> > +For the first class of configuration the following syntax is supported::
> > +
> > +	# comments
> > +	checker checkpatch [command]
> > +	addflags <list of extra flags and parameters>
> > +	cflags
> > +	typedef NAME <regular expression>
> > +	run [checker list|all]
> > +
> > +The ``checker`` command switches ``runchecks``'s attention to a particular
> > +checker. The following commands until the next ``checker`` statement
> > +applies to that particular checker. The first occurrence of ``checker``
> 
> applies -> apply
> 
> > +also serves as a potentially defining operation, if the checker name
> > +has not been preconfigured. In that case, a second parameter can be used
> > +to provide the name of the command used to run the checker.
> > +A full checker integration into runchecks will typically require some
> > +additions to runchecks, and will then have been preconfigured,
> > +but simple checkers might just be configured on the fly.
> > +
> > +The ``addflags`` command incrementally adds more flags and parameters to
> > +the command line used to invoke the checker. This applies to all
> > +invocations of the checker from runchecks.
> > +The ``cflags`` command tells to forward all the flags and options passed to
> > +the compiler invocation to the checker. The default is to suppress these
> > +parameters when invoking the checker.
> > +
> > +The ``typedef`` command adds ``NAME`` and associates it with the given
> > +regular expression. This expression is used to match against standard error
> > +output from the checker and ``NAME`` can be used as a new named check that
> > +runchecks understands and that can be used with checker supported names
> > +below to selectively suppress that particular set of warning or error
> > +messages. This is useful to handle output checks for which the underlying
> > +checker is does not provide any suppression. Check type namespaces are
> > +separate for the individual checkers. You can list the state of the built in and
> > +configured checker and check types with::
> > +
> > +	scripts/runchecks --list
> > +
> > +The checker implementations of the ``typedef`` command also allows
> > +runchecks to perform some unification of output by rewriting the output
> > +lines, adding optional color support, and use of the new type names in the
> > +error output, to ease the process of updating the runchecks.cfg files.
> > +Having a unified representation of the error output also makes it much
> > +easier to do statistics or other operations on top of an aggregated output
> > +from several checkers.
> > +
> > +For the second class of configuration the following syntax is supported::
> > +
> > +	# comments
> > +	checker checker_name
> > +	line_len <n>
> > +	except check_type [files ...]
> > +	pervasive check_type1 [check_type2 ...]
> > +
> > +The ``line_len`` directive defines the upper bound of characters per line
> > +tolerated in this directory. Currently only ``checkpatch`` supports this
> > +command.  The ``except`` directive takes a check 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 subtree.  The ``except`` and
> > +``pervasive`` directives can be used cumulatively to add more exceptions.
> > +
> > +Running checker programs from make
> > +----------------------------------
> > +
> > +You can run checkpatch subject to rules defined in ``runcheck.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 ``CF`` allows passing additional parameters to
> > +``runchecks``. You can for instance use::
> > +
> > +	make C=2 CF="--run:checkpatch --fix-inplace"
> > +
> > +to run only the ``checkpatch`` checker, and to have checkpatch trying to fix
> 
> 
> trying -> try
> 
> > +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 under ``checker checkpatch`` to the local
> > +``runchecks.cfg``, and you can focus on fixing up errors subsystem or
> > +driver by driver on a type by type basis.
> > +
> > +By default runchecks will skip all files if a ``runchecks.cfg`` file cannot
> > +be found in the directory of the file or in the tree above.  This is to
> > +allow builds with ``C=2`` to pass even for subsystems that has not yet done
> > +anything to rein in checker errors. At some point when all subsystems and
> > +drivers either have fixed all checker errors or added proper
> > +``runchecks.cfg`` files, this can be changed.
> > +
> > +To force runchecks to run a full run in directories/trees where runchecks
> > +does not find  a ``runchecks.cfg`` file as well, use::
> > +
> > +	make C=2 CF="-f"
> > +
> > +If you like to see all the warnings and errors produced by the checkers, ignoring
> > +any runchecks.cfg files except the one under ``scripts``, you can use::
> > +
> > +	make C=2 CF="-n"
> > +
> > +or for a specific module directory::
> > +
> > +	make C=2 M=drivers/infiniband/core CF="--color -n -w"
> > +
> > +with the -w option to ``runchecks`` to suppress errors from any of the
> > +checkers and just continue on, and the ``--color`` option to present errors
> > +with colors where supported.
> > +
> > +Ever tightening checker rules
> > +-----------------------------
> > +
> > +Commit the changes to the relevant ``runchecks.cfg`` together with the code
> > +changes that fixes a particular type of issue, this will allow automatic
> > +checker running by default. 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 a full "make C=2" passes with no
> > +errors.
> > +
> > +Extending and improving checker support in ``runchecks``
> > +--------------------------------------------------------
> > +
> > +The runchecks program has been written with extensibility in mind.
> > +If the checker starts it's reporting lines with filename:lineno, there's a
> 
> it's -> its
> 
> > +good chance that a new checker can simply be added by adding::
> > +
> > +	checker mychecker path_to_mychecker
> > +
> > +to ``scripts/runchecks.cfg`` and suitable ``typedef`` expressions to provide
> > +selective suppressions of output, however it is likely that some quirks are
> > +needed to make the new checker behave similar to the others, and to support
> 
> similar -> similarly
> 
> julia

Thanks for the review - I'll certainly fix these for v3.

Knut

> 
> > +the full set of features, such as the ``--list`` option. This is done by
> > +implementing a new subclass of the Checker class in ``runchecks``. This is the way
> > +all the available default supported checkers are implemented, and those
> > +relatively lean implementations could serve as examples.
> > diff --git a/Documentation/dev-tools/sparse.rst b/Documentation/dev-tools/sparse.rst
> > index 78aa00a..e3e8b27 100644
> > --- a/Documentation/dev-tools/sparse.rst
> > +++ b/Documentation/dev-tools/sparse.rst
> > @@ -101,5 +101,31 @@ recompiled, or use "make C=2" to run sparse on the files whether
> they need to
> >  be recompiled or not.  The latter is a fast way to check the whole tree if you
> >  have already built it.
> >
> > -The optional make variable CF can be used to pass arguments to sparse.  The
> > -build system passes -Wbitwise to sparse automatically.
> > +The "make C={1,2}" form of kernel make indirectly calls sparse via "runchecks",
> > +which dependent on configuration and command line options may dispatch calls to
> > +other checkers in addition to sparse. Details on how this works is covered
> > +in Documentation/dev-tools/runchecks.rst .
> > +
> > +The optional make variable CF can be used to pass arguments to runchecks for dispatch
> > +to sparse. If sparse is the only tool enabled, any option not recognized by
> > +runchecks will be forwarded to sparse. If more than one tool is active, you must
> > +add the parameters you want sparse to get as a comma separated list prefixed by
> > +``--to-sparse:``. If you want sparse to be the only checker run, and you want
> > +some nice colored output, you can specify this as::
> > +
> > +	make C=2 CF="--run:sparse --color"
> > +
> > +This will cause sparse to be called for all files which are supported by a valid
> > +runchecks configuration (again see Documentation/dev-tools/runchecks.rst for
> > +details). If you want to run sparse on all files and ignore any missing
> > +configuration files(s), just add ``-n`` to the list of options passed to
> > +runchecks. This will cause runchecks to call sparse with all errors enabled for
> > +all files even if no valid configuration is found in the tree for the source files.
> > +
> > +By default "runchecks" is set to enable all sparse errors, but you can
> > +configure what checks to be applied by sparse on a per file or per subsystem
> > +basis. With the above invocation, make will fail and stop on the first file
> > +encountered with sparse errors or warnings in it. If you want to continue
> > +anyway, you can use::
> > +
> > +	make C=2 CF="--run:sparse --color -w"
> > diff --git a/Documentation/kbuild/kbuild.txt b/Documentation/kbuild/kbuild.txt
> > index ac2363e..260e688 100644
> > --- a/Documentation/kbuild/kbuild.txt
> > +++ b/Documentation/kbuild/kbuild.txt
> > @@ -103,10 +103,13 @@ CROSS_COMPILE is also used for ccache in some setups.
> >
> >  CF
> >  --------------------------------------------------
> > -Additional options for sparse.
> > -CF is often used on the command-line like this:
> > +Additional options for runchecks, the generic checker runner.
> > +CF is often used on the command-line for instance like this:
> >
> > -    make CF=-Wbitwise C=2
> > +    make C=2 CF="--run:sparse --color -w"
> > +
> > +to run the sparse tool only, and to use colored output and continue on warnings
> > +or errors.
> >
> >  INSTALL_PATH
> >  --------------------------------------------------
> > --
> > git-series 0.9.1
> >

Re: [PATCH v2 3/5] checkpatch: Improve --fix-inplace for TABSTOP

[Knut Omang]

On Sat, 2017-12-16 at 07:13 -0800, Joe Perches wrote:
> On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> > 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>
> 
> Well written commit log description, thanks but
> I'm not sure that's actually better overall.
> 
> It's not possible to know if the appropriate thing
> to do is to add a tab or remove spaces.
> 
> This may get it wrong about as often as the current
> code gets it right.
> 
> Last I checked 6 months or so ago, there were ~8000
> TABSTOP warnings in the kernel tree.
> 
> Most of those still seem valid and moving additional
> places to the right is sometimes incorrect too.
> 
> It seems most common when there are 3 or 4 spaces
> used as indent level indentation instead of tabs.

I see your point here, definitely 
- more detailed statistics needed...

> Dunno.

Let me take this out of the set in the next version
as it is really a separate issue.

> Anyone else have an opinion?


Thanks,
Knut

>
> > ---
> >  scripts/checkpatch.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 040aa79..febe010 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2989,7 +2989,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;
> >  				}
> >  			}
> >  		}

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Knut Omang]

On Sat, 2017-12-16 at 07:21 -0800, Joe Perches wrote:
> On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> > This patch series implements features to make it easier to run checkers on the
> > entire kernel as part of automatic and developer testing.
> 
> This seems like a useful and reasonable series, thanks.

thanks, appreciated,

> Do please take Julia's grammar updates.

will do,

> How is this series to be applied?

I am open for suggestions.

Let's leave patch #3 out for now.

It's safe to apply #1 (and #2) alone.

Patches #4 and #5 only have value once patch #1 is accepted,
and I included them in the set for documentation purposes.

They don't break anything by getting into the RDMA or RDS subsystem 
without patch #1, alternatively they can go together with the set of cleanup patches I
have prepared for each of RDMA and RDS after #1 is in.

Thanks,
Knut

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Joe Perches]

On Sat, 2017-12-16 at 17:27 +0100, Knut Omang wrote:
> On Sat, 2017-12-16 at 07:21 -0800, Joe Perches wrote:
> > On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> > > This patch series implements features to make it easier to run checkers on the
> > > entire kernel as part of automatic and developer testing.
> > 
> > This seems like a useful and reasonable series, thanks.
> 
> thanks, appreciated,
> 
> > Do please take Julia's grammar updates.
> 
> will do,
> 
> > How is this series to be applied?
> 
> I am open for suggestions.
> 
> Let's leave patch #3 out for now.
> 
> It's safe to apply #1 (and #2) alone.

I'd just as soon combine patch #1 and an updated 
patch #2 into a single patch and get that applied
sooner than later.

> They don't break anything by getting into the RDMA or RDS subsystem 
> without patch #1, alternatively they can go together with the set of cleanup patches I
> have prepared for each of RDMA and RDS after #1 is in.

right.

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Knut Omang]

On Sat, 2017-12-16 at 09:00 -0800, Joe Perches wrote:
> On Sat, 2017-12-16 at 17:27 +0100, Knut Omang wrote:
> > On Sat, 2017-12-16 at 07:21 -0800, Joe Perches wrote:
> > > On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> > > > This patch series implements features to make it easier to run checkers on the
> > > > entire kernel as part of automatic and developer testing.
> > > 
> > > This seems like a useful and reasonable series, thanks.
> > 
> > thanks, appreciated,
> > 
> > > Do please take Julia's grammar updates.
> > 
> > will do,
> > 
> > > How is this series to be applied?
> > 
> > I am open for suggestions.
> > 
> > Let's leave patch #3 out for now.
> > 
> > It's safe to apply #1 (and #2) alone.
> 
> I'd just as soon combine patch #1 and an updated 
> patch #2 into a single patch and get that applied
> sooner than later.

Ok, will do that.

> > They don't break anything by getting into the RDMA or RDS subsystem 
> > without patch #1, alternatively they can go together with the set of cleanup patches I
> > have prepared for each of RDMA and RDS after #1 is in.
> 
> right.

Thanks,
Knut

Re: [PATCH v2 4/5] rds: Add runchecks.cfg for net/rds

[Stephen Hemminger]

On Sat, 16 Dec 2017 15:42:29 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> +
> +# Important to fix from a quality perspective:
> +#
> +except AVOID_BUG connection.c ib.c ib_cm.c ib_rdma.c ib_recv.c ib_ring.c ib_send.c info.c loop.c message.c
> +except AVOID_BUG rdma.c recv.c send.c stats.c tcp_recv.c transport.c
> +except MEMORY_BARRIER ib_recv.c send.c tcp_send.c
> +except WAITQUEUE_ACTIVE cong.c ib_rdma.c ib_ring.c ib_send.c
> +except UNNECESSARY_ELSE bind.c ib_cm.c
> +except MACRO_ARG_PRECEDENCE connection.c ib.h rds.h
> +except MACRO_ARG_REUSE rds.h
> +except ALLOC_SIZEOF_STRUCT cong.c ib.c ib_cm.c loop.c message.c rdma.c
> +except UNCOMMENTED_DEFINITION ib_cm.c
> +
> +# Code simplification:
> +#
> +except ALLOC_WITH_MULTIPLY ib.c
> +except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c transport.c
> +except UNNECESSARY_ELSE ib_fmr.c
> +except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
> +except PRINTK_RATELIMITED ib_frmr.c
> +except EMBEDDED_FUNCTION_NAME ib_rdma.c
> +
> +# Style and readability:
> +#
> +except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
> +except OOM_MESSAGE ib.c tcp.c
> +except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
> +except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
> +except OPEN_ENDED_LINE recv.c ib_recv.c
> +
> +# Candidates to leave as exceptions (don't fix):
> +except MULTIPLE_ASSIGNMENTS ib_send.c
> +except LONG_LINE_STRING connection.c
> +except OPEN_BRACE connection.c
> +

Why start letting subsystems have a free-pass?
Also this would mean that new patches to IB would continue the bad habits.

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Stephen Hemminger]

On Sat, 16 Dec 2017 15:42:25 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> This patch series implements features to make it easier to run checkers on the
> entire kernel as part of automatic and developer testing.
> 
> This is done by replacing the sparse specific setup for the C={1,2} variable
> in the makefiles with setup for running scripts/runchecks, a new program that
> can run any number of different "checkers". The behaviour of runchecks is
> defined by simple "global" configuration in scripts/runchecks.cfg which can be
> extended by local configuration applying to individual files, directories or
> subtrees in the source.
> 
> It also fixes a minor issue with "checkpatch --fix-inplace" found during testing
> (patch #3).
> 
> The runchecks.cfg files are parsed according to this minimal language:
> 
>        # comments
>        # "Global configuration in scripts/runchecks.cfg:
>        checker <name>
>        typedef NAME regex
>        run <list of checkers or "all"
> 
>        # "local" configuration:
>        line_len <n>
>        except checkpatch_type [files ...]
>        pervasive checkpatch_type1 [checkpatch_type2 ...]
> 
> With "make C=2" runchecks first parse the file scripts/runchecks.cfg, then
> look for a file named 'runchecks.cfg' in the same directory as the source file.
> If that file exists, it will be parsed and it's local configuration applied to
> allow suppression on a per checker, per check and per file basis.
> If a "local" configuration does not exist, either in the source directory or
> above, make will simply silently ignore the file.
> 
> The idea is that the community can work to add runchecks.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/subtrees without such files, automation can start right
> away as it is trivially possible to run errorless with C=2 for the entire
> kernel.
> 
> For the checker maintainers this should be a benefit as well: new
> or improved checks would generate new errors/warnings. With automatic testing
> for the checkers, these new checks would generate error reports and cause
> builds to fail. Adding the new check a 'pervasive' option at the top level or
> even for specific files, marked with a "new check - please consider fixing" comment
> or similar would make those builds pass while documenting and making the new check
> more visible.
> 
> The patches includes a documentation file with some more details.
> 
> This patch set has evolved from an earlier shell script implementation I made
> as only 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 runchecks
> 
> I only include version 0 of runchecks.cfg in the two directories I
> worked on here as the final two patches. These files both documents where
> the issues are in those two directories, and can be used by the maintainer
> to indicate to potential helpers what to focus on as I have tried to
> illustrate by means of comments.
> 
> Changes from v1:
> -----------------
> Based on feedback, the implementation is completely rewritten and extended.
> Instead of patches to checkpatch, and a sole checkpatch focus, it is now a
> generic solution implemented in python, for any type of checkers, extendable
> through some basic generic functionality, and for special needs by subclassing
> the Checker class in the implementation.
> 
> This implementation fully supports checkpatch, sparse and
> checkdoc == kernel-doc -none, and also has been tested with coccicheck.
> To facilitate the same mechanism of using check types to filter what
> checks to be suppressed, I introduced the concept of "typedefs" which allows
> runchecks to effectively augment the check type space of the checker in cases
> where types either are not available at all (checkdoc) or where only a few
> can be filtered out (sparse)
> 
> With this in place it also became trivial to make the look and feel similar
> for sparse and checkdoc as for checkpatch, with some optional color support
> too, to make fixing issues in the code, the goal of this whole exercise,
> much more pleasant IMHO :-)
> 
> Thanks,
> Knut
> 
> Knut Omang (5):
>   runchecks: Generalize make C={1,2} to support multiple checkers
>   Documentation: Add doc for runchecks, a checker runner
>   checkpatch: Improve --fix-inplace for TABSTOP
>   rds: Add runchecks.cfg for net/rds
>   RDMA/core: Add runchecks.cfg for drivers/infiniband/core
> 
>  Documentation/dev-tools/coccinelle.rst |  12 +-
>  Documentation/dev-tools/index.rst      |   1 +-
>  Documentation/dev-tools/runchecks.rst  | 215 ++++++++-
>  Documentation/dev-tools/sparse.rst     |  30 +-
>  Documentation/kbuild/kbuild.txt        |   9 +-
>  Makefile                               |  23 +-
>  drivers/infiniband/core/runchecks.cfg  |  83 +++-
>  net/rds/runchecks.cfg                  |  76 +++-
>  scripts/Makefile.build                 |   4 +-
>  scripts/checkpatch.pl                  |   2 +-
>  scripts/runchecks                      | 734 ++++++++++++++++++++++++++-
>  scripts/runchecks.cfg                  |  63 ++-
>  scripts/runchecks_help.txt             |  43 ++-
>  13 files changed, 1274 insertions(+), 21 deletions(-)
>  create mode 100644 Documentation/dev-tools/runchecks.rst
>  create mode 100644 drivers/infiniband/core/runchecks.cfg
>  create mode 100644 net/rds/runchecks.cfg
>  create mode 100755 scripts/runchecks
>  create mode 100644 scripts/runchecks.cfg
>  create mode 100644 scripts/runchecks_help.txt
> 
> base-commit: ae64f9bd1d3621b5e60d7363bc20afb46aede215

I like the ability to add more checkers and keep then in the main
upstream tree. But adding overrides for specific subsystems goes against
the policy that all subsystems should be treated equally.

There was discussion at Kernel Summit about how the different
subsystems already have different rules. This appears to be a
way to make that worse.

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Joe Perches]

On Sat, 2017-12-16 at 09:47 -0800, Stephen Hemminger wrote:
> On Sat, 16 Dec 2017 15:42:25 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > This patch series implements features to make it easier to run checkers on the
> > entire kernel as part of automatic and developer testing.
> > 
> > This is done by replacing the sparse specific setup for the C={1,2} variable
> > in the makefiles with setup for running scripts/runchecks, a new program that
> > can run any number of different "checkers". The behaviour of runchecks is
> > defined by simple "global" configuration in scripts/runchecks.cfg which can be
> > extended by local configuration applying to individual files, directories or
> > subtrees in the source.
[]
> I like the ability to add more checkers and keep then in the main
> upstream tree. But adding overrides for specific subsystems goes against
> the policy that all subsystems should be treated equally.
> 
> There was discussion at Kernel Summit about how the different
> subsystems already have different rules. This appears to be a
> way to make that worse.

I think that's OK and somewhat reasonable.

What is perhaps unreasonable is requiring subsystems with
a local specific style to change to some universal style.

see comments like:

https://lkml.org/lkml/2017/12/11/689

Re: [PATCH v2 4/5] rds: Add runchecks.cfg for net/rds

[Joe Perches]

On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:
> On Sat, 16 Dec 2017 15:42:29 +0100 Knut Omang <knut.omang@oracle.com> wrote:
> > +# Code simplification:
> > +#
> > +except ALLOC_WITH_MULTIPLY ib.c
> > +except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c transport.c
> > +except UNNECESSARY_ELSE ib_fmr.c
> > +except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
> > +except PRINTK_RATELIMITED ib_frmr.c
> > +except EMBEDDED_FUNCTION_NAME ib_rdma.c
> > +
> > +# Style and readability:
> > +#
> > +except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
> > +except OOM_MESSAGE ib.c tcp.c
> > +except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
> > +except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
> > +except OPEN_ENDED_LINE recv.c ib_recv.c
> > +
> > +# Candidates to leave as exceptions (don't fix):
> > +except MULTIPLE_ASSIGNMENTS ib_send.c
> > +except LONG_LINE_STRING connection.c
> > +except OPEN_BRACE connection.c
> > +
> 
> Why start letting subsystems have a free-pass?
> Also this would mean that new patches to IB would continue the bad habits.

I agree with this comment at least for net/rds.

Most of these existing messages from checkpatch should
probably be inspected and corrected where possible to
minimize the style differences between this subsystem
and the rest of the kernel.

For instance, here's a trivial patch to substitute
pr_<level> for printks and a couple braces next to
these substitutions.

btw:

in ib_cm, why is one call to ib_modify_qp emitted
with a -ret and the other with a positive err?

---
 net/rds/ib_cm.c          | 21 ++++++++++-----------
 net/rds/ib_recv.c        |  5 ++---
 net/rds/ib_send.c        | 23 ++++++++++++-----------
 net/rds/rdma_transport.c | 14 +++++++-------
 net/rds/send.c           |  8 ++++----
 net/rds/tcp_send.c       |  4 +---
 net/rds/threads.c        |  6 ++----
 net/rds/transport.c      | 12 ++++++------
 8 files changed, 44 insertions(+), 49 deletions(-)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 80fb6f63e768..92694c9cb7c9 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -86,7 +86,7 @@ rds_ib_tune_rnr(struct rds_ib_connection *ic, struct ib_qp_attr *attr)
 	attr->min_rnr_timer = IB_RNR_TIMER_000_32;
 	ret = ib_modify_qp(ic->i_cm_id->qp, attr, IB_QP_MIN_RNR_TIMER);
 	if (ret)
-		printk(KERN_NOTICE "ib_modify_qp(IB_QP_MIN_RNR_TIMER): err=%d\n", -ret);
+		pr_notice("ib_modify_qp(IB_QP_MIN_RNR_TIMER): err=%d\n", -ret);
 }
 
 /*
@@ -146,13 +146,12 @@ void rds_ib_cm_connect_complete(struct rds_connection *conn, struct rdma_cm_even
 	qp_attr.qp_state = IB_QPS_RTS;
 	err = ib_modify_qp(ic->i_cm_id->qp, &qp_attr, IB_QP_STATE);
 	if (err)
-		printk(KERN_NOTICE "ib_modify_qp(IB_QP_STATE, RTS): err=%d\n", err);
+		pr_notice("ib_modify_qp(IB_QP_STATE, RTS): err=%d\n", err);
 
 	/* update ib_device with this local ipaddr */
 	err = rds_ib_update_ipaddr(ic->rds_ibdev, conn->c_laddr);
 	if (err)
-		printk(KERN_ERR "rds_ib_update_ipaddr failed (%d)\n",
-			err);
+		pr_err("rds_ib_update_ipaddr failed (%d)\n", err);
 
 	/* If the peer gave us the last packet it saw, process this as if
 	 * we had received a regular ACK. */
@@ -594,8 +593,7 @@ static u32 rds_ib_protocol_compatible(struct rdma_cm_event *event)
 
 	/* Be paranoid. RDS always has privdata */
 	if (!event->param.conn.private_data_len) {
-		printk(KERN_NOTICE "RDS incoming connection has no private data, "
-			"rejecting\n");
+		pr_notice("RDS incoming connection has no private data, rejecting\n");
 		return 0;
 	}
 
@@ -609,11 +607,12 @@ static u32 rds_ib_protocol_compatible(struct rdma_cm_event *event)
 		version = RDS_PROTOCOL_3_0;
 		while ((common >>= 1) != 0)
 			version++;
-	} else
-		printk_ratelimited(KERN_NOTICE "RDS: Connection from %pI4 using incompatible protocol version %u.%u\n",
-				&dp->dp_saddr,
-				dp->dp_protocol_major,
-				dp->dp_protocol_minor);
+	} else {
+		pr_notice_ratelimited("RDS: Connection from %pI4 using incompatible protocol version %u.%u\n",
+				      &dp->dp_saddr,
+				      dp->dp_protocol_major,
+				      dp->dp_protocol_minor);
+	}
 	return version;
 }
 
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index b4e421aa9727..9dfc8233c488 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -105,7 +105,7 @@ static int rds_ib_recv_alloc_cache(struct rds_ib_refill_cache *cache)
 
 	cache->percpu = alloc_percpu(struct rds_ib_cache_head);
 	if (!cache->percpu)
-	       return -ENOMEM;
+		return -ENOMEM;
 
 	for_each_possible_cpu(cpu) {
 		head = per_cpu_ptr(cache->percpu, cpu);
@@ -399,8 +399,7 @@ void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
 	while ((prefill || rds_conn_up(conn)) &&
 	       rds_ib_ring_alloc(&ic->i_recv_ring, 1, &pos)) {
 		if (pos >= ic->i_recv_ring.w_nr) {
-			printk(KERN_NOTICE "Argh - ring alloc returned pos=%u\n",
-					pos);
+			pr_notice("Argh - ring alloc returned pos=%u\n", pos);
 			break;
 		}
 
diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
index 8557a1cae041..cb1ce8d06582 100644
--- a/net/rds/ib_send.c
+++ b/net/rds/ib_send.c
@@ -180,9 +180,8 @@ static struct rds_message *rds_ib_send_unmap_op(struct rds_ib_connection *ic,
 		}
 		break;
 	default:
-		printk_ratelimited(KERN_NOTICE
-			       "RDS/IB: %s: unexpected opcode 0x%x in WR!\n",
-			       __func__, send->s_wr.opcode);
+		pr_notice_ratelimited("RDS/IB: %s: unexpected opcode 0x%x in WR!\n",
+				      __func__, send->s_wr.opcode);
 		break;
 	}
 
@@ -730,8 +729,8 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
 		 first, &first->s_wr, ret, failed_wr);
 	BUG_ON(failed_wr != &first->s_wr);
 	if (ret) {
-		printk(KERN_WARNING "RDS/IB: ib_post_send to %pI4 "
-		       "returned %d\n", &conn->c_faddr, ret);
+		pr_warn("RDS/IB: ib_post_send to %pI4 returned %d\n",
+			&conn->c_faddr, ret);
 		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
 		rds_ib_sub_signaled(ic, nr_sig);
 		if (prev->s_op) {
@@ -827,15 +826,16 @@ int rds_ib_xmit_atomic(struct rds_connection *conn, struct rm_atomic_op *op)
 		 send, &send->s_atomic_wr, ret, failed_wr);
 	BUG_ON(failed_wr != &send->s_atomic_wr.wr);
 	if (ret) {
-		printk(KERN_WARNING "RDS/IB: atomic ib_post_send to %pI4 "
-		       "returned %d\n", &conn->c_faddr, ret);
+		pr_warn("RDS/IB: atomic ib_post_send to %pI4 returned %d\n",
+			&conn->c_faddr, ret);
 		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
 		rds_ib_sub_signaled(ic, nr_sig);
 		goto out;
 	}
 
 	if (unlikely(failed_wr != &send->s_atomic_wr.wr)) {
-		printk(KERN_WARNING "RDS/IB: atomic ib_post_send() rc=%d, but failed_wqe updated!\n", ret);
+		pr_warn("RDS/IB: atomic ib_post_send() rc=%d, but failed_wqe updated!\n",
+			ret);
 		BUG_ON(failed_wr != &send->s_atomic_wr.wr);
 	}
 
@@ -967,15 +967,16 @@ int rds_ib_xmit_rdma(struct rds_connection *conn, struct rm_rdma_op *op)
 		 first, &first->s_rdma_wr.wr, ret, failed_wr);
 	BUG_ON(failed_wr != &first->s_rdma_wr.wr);
 	if (ret) {
-		printk(KERN_WARNING "RDS/IB: rdma ib_post_send to %pI4 "
-		       "returned %d\n", &conn->c_faddr, ret);
+		pr_warn("RDS/IB: rdma ib_post_send to %pI4 returned %d\n",
+			&conn->c_faddr, ret);
 		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
 		rds_ib_sub_signaled(ic, nr_sig);
 		goto out;
 	}
 
 	if (unlikely(failed_wr != &first->s_rdma_wr.wr)) {
-		printk(KERN_WARNING "RDS/IB: ib_post_send() rc=%d, but failed_wqe updated!\n", ret);
+		pr_warn("RDS/IB: ib_post_send() rc=%d, but failed_wqe updated!\n",
+			ret);
 		BUG_ON(failed_wr != &first->s_rdma_wr.wr);
 	}
 
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index fc59821f0a27..0ccb1cde4c52 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -131,7 +131,7 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
 
 	default:
 		/* things like device disconnect? */
-		printk(KERN_ERR "RDS: unknown event %u (%s)!\n",
+		pr_err("RDS: unknown event %u (%s)!\n",
 		       event->event, rdma_event_msg(event->event));
 		break;
 	}
@@ -156,8 +156,8 @@ static int rds_rdma_listen_init(void)
 			       RDMA_PS_TCP, IB_QPT_RC);
 	if (IS_ERR(cm_id)) {
 		ret = PTR_ERR(cm_id);
-		printk(KERN_ERR "RDS/RDMA: failed to setup listener, "
-		       "rdma_create_id() returned %d\n", ret);
+		pr_err("RDS/RDMA: failed to setup listener, rdma_create_id() returned %d\n",
+		       ret);
 		return ret;
 	}
 
@@ -171,15 +171,15 @@ static int rds_rdma_listen_init(void)
 	 */
 	ret = rdma_bind_addr(cm_id, (struct sockaddr *)&sin);
 	if (ret) {
-		printk(KERN_ERR "RDS/RDMA: failed to setup listener, "
-		       "rdma_bind_addr() returned %d\n", ret);
+		pr_err("RDS/RDMA: failed to setup listener, rdma_bind_addr() returned %d\n",
+		       ret);
 		goto out;
 	}
 
 	ret = rdma_listen(cm_id, 128);
 	if (ret) {
-		printk(KERN_ERR "RDS/RDMA: failed to setup listener, "
-		       "rdma_listen() returned %d\n", ret);
+		pr_err("RDS/RDMA: failed to setup listener, rdma_listen() returned %d\n",
+		       ret);
 		goto out;
 	}
 
diff --git a/net/rds/send.c b/net/rds/send.c
index b52cdc8ae428..f9bc3d499576 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1130,15 +1130,15 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 	}
 
 	if (rm->rdma.op_active && !conn->c_trans->xmit_rdma) {
-		printk_ratelimited(KERN_NOTICE "rdma_op %p conn xmit_rdma %p\n",
-			       &rm->rdma, conn->c_trans->xmit_rdma);
+		pr_notice_ratelimited("rdma_op %p conn xmit_rdma %p\n",
+				      &rm->rdma, conn->c_trans->xmit_rdma);
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
 
 	if (rm->atomic.op_active && !conn->c_trans->xmit_atomic) {
-		printk_ratelimited(KERN_NOTICE "atomic_op %p conn xmit_atomic %p\n",
-			       &rm->atomic, conn->c_trans->xmit_atomic);
+		pr_notice_ratelimited("atomic_op %p conn xmit_atomic %p\n",
+				      &rm->atomic, conn->c_trans->xmit_atomic);
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
diff --git a/net/rds/tcp_send.c b/net/rds/tcp_send.c
index dc860d1bb608..0e23e9d06c7e 100644
--- a/net/rds/tcp_send.c
+++ b/net/rds/tcp_send.c
@@ -153,9 +153,7 @@ int rds_tcp_xmit(struct rds_connection *conn, struct rds_message *rm,
 			 * an incoming RST.
 			 */
 			if (rds_conn_path_up(cp)) {
-				pr_warn("RDS/tcp: send to %pI4 on cp [%d]"
-					"returned %d, "
-					"disconnecting and reconnecting\n",
+				pr_warn("RDS/tcp: send to %pI4 on cp [%d]returned %d, disconnecting and reconnecting\n",
 					&conn->c_faddr, cp->cp_index, ret);
 				rds_conn_path_drop(cp, false);
 			}
diff --git a/net/rds/threads.c b/net/rds/threads.c
index f121daa402c8..499a0a8287cc 100644
--- a/net/rds/threads.c
+++ b/net/rds/threads.c
@@ -74,10 +74,8 @@ EXPORT_SYMBOL_GPL(rds_wq);
 void rds_connect_path_complete(struct rds_conn_path *cp, int curr)
 {
 	if (!rds_conn_path_transition(cp, curr, RDS_CONN_UP)) {
-		printk(KERN_WARNING "%s: Cannot transition to state UP, "
-				"current state is %d\n",
-				__func__,
-				atomic_read(&cp->cp_state));
+		pr_warn("%s: Cannot transition to state UP, current state is %d\n",
+			__func__, atomic_read(&cp->cp_state));
 		rds_conn_path_drop(cp, false);
 		return;
 	}
diff --git a/net/rds/transport.c b/net/rds/transport.c
index 0b188dd0a344..a0d7ccecdec3 100644
--- a/net/rds/transport.c
+++ b/net/rds/transport.c
@@ -46,12 +46,12 @@ void rds_trans_register(struct rds_transport *trans)
 
 	down_write(&rds_trans_sem);
 
-	if (transports[trans->t_type])
-		printk(KERN_ERR "RDS Transport type %d already registered\n",
-			trans->t_type);
-	else {
+	if (transports[trans->t_type]) {
+		pr_err("RDS Transport type %d already registered\n",
+		       trans->t_type);
+	} else {
 		transports[trans->t_type] = trans;
-		printk(KERN_INFO "Registered RDS/%s transport\n", trans->t_name);
+		pr_info("Registered RDS/%s transport\n", trans->t_name);
 	}
 
 	up_write(&rds_trans_sem);
@@ -63,7 +63,7 @@ void rds_trans_unregister(struct rds_transport *trans)
 	down_write(&rds_trans_sem);
 
 	transports[trans->t_type] = NULL;
-	printk(KERN_INFO "Unregistered RDS/%s transport\n", trans->t_name);
+	pr_info("Unregistered RDS/%s transport\n", trans->t_name);
 
 	up_write(&rds_trans_sem);
 }

Re: [PATCH v2 4/5] rds: Add runchecks.cfg for net/rds

[santosh.shilimkar]

On 12/16/17 10:24 AM, Joe Perches wrote:
> On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:
>> On Sat, 16 Dec 2017 15:42:29 +0100 Knut Omang <knut.omang@oracle.com> wrote:
>>> +# Code simplification:
>>> +#
>>> +except ALLOC_WITH_MULTIPLY ib.c
>>> +except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c transport.c
>>> +except UNNECESSARY_ELSE ib_fmr.c
>>> +except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
>>> +except PRINTK_RATELIMITED ib_frmr.c
>>> +except EMBEDDED_FUNCTION_NAME ib_rdma.c
>>> +
>>> +# Style and readability:
>>> +#
>>> +except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
>>> +except OOM_MESSAGE ib.c tcp.c
>>> +except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
>>> +except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
>>> +except OPEN_ENDED_LINE recv.c ib_recv.c
>>> +
>>> +# Candidates to leave as exceptions (don't fix):
>>> +except MULTIPLE_ASSIGNMENTS ib_send.c
>>> +except LONG_LINE_STRING connection.c
>>> +except OPEN_BRACE connection.c
>>> +
>>
>> Why start letting subsystems have a free-pass?
>> Also this would mean that new patches to IB would continue the bad habits.
And I don't need any free pass for RDS either.

I missed V1 of this series but Knut, please don't add
any exceptions for RDS and if there is something needs to
be fixed, we can address it. Once your infrastructure
gets merged, the subsequent fixes can be added.

> 
> I agree with this comment at least for net/rds.
> 
> Most of these existing messages from checkpatch should
> probably be inspected and corrected where possible to
> minimize the style differences between this subsystem
> and the rest of the kernel.
> 
> For instance, here's a trivial patch to substitute
> pr_<level> for printks and a couple braces next to
> these substitutions.
>
Thanks Joe. I actually had a similar patch a while back but
since it was lot of churn, and code was already merged,
never submitted it and then later forgot about it.

Will look into it.

> btw:
> 
> in ib_cm, why is one call to ib_modify_qp emitted
> with a -ret and the other with a positive err?
>
Its oversight and will fix that.

Regards,
Santosh

Re: [PATCH v2 4/5] rds: Add runchecks.cfg for net/rds

[Knut Omang]

On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:
> On Sat, 16 Dec 2017 15:42:29 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > +
> > +# Important to fix from a quality perspective:
> > +#
> > +except AVOID_BUG connection.c ib.c ib_cm.c ib_rdma.c ib_recv.c ib_ring.c ib_send.c
> info.c loop.c message.c
> > +except AVOID_BUG rdma.c recv.c send.c stats.c tcp_recv.c transport.c
> > +except MEMORY_BARRIER ib_recv.c send.c tcp_send.c
> > +except WAITQUEUE_ACTIVE cong.c ib_rdma.c ib_ring.c ib_send.c
> > +except UNNECESSARY_ELSE bind.c ib_cm.c
> > +except MACRO_ARG_PRECEDENCE connection.c ib.h rds.h
> > +except MACRO_ARG_REUSE rds.h
> > +except ALLOC_SIZEOF_STRUCT cong.c ib.c ib_cm.c loop.c message.c rdma.c
> > +except UNCOMMENTED_DEFINITION ib_cm.c
> > +
> > +# Code simplification:
> > +#
> > +except ALLOC_WITH_MULTIPLY ib.c
> > +except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c
> transport.c
> > +except UNNECESSARY_ELSE ib_fmr.c
> > +except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
> > +except PRINTK_RATELIMITED ib_frmr.c
> > +except EMBEDDED_FUNCTION_NAME ib_rdma.c
> > +
> > +# Style and readability:
> > +#
> > +except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
> > +except OOM_MESSAGE ib.c tcp.c
> > +except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
> > +except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
> > +except OPEN_ENDED_LINE recv.c ib_recv.c
> > +
> > +# Candidates to leave as exceptions (don't fix):
> > +except MULTIPLE_ASSIGNMENTS ib_send.c
> > +except LONG_LINE_STRING connection.c
> > +except OPEN_BRACE connection.c
> > +
> 
> Why start letting subsystems have a free-pass?

It's not a free pass, on the contrary - it's a way to enable the build bots/CI systems to
prevent regressions!

Right now, no automatic system can be set up to run checkpatch on almost any subsystem in
the kernel because there are so many warnings. That means that regressions happens all
over the place, even on source files and for types of checks that there currently are no
issues. Also reviewers have to spend time correcting whitespace issues which automation
can really handle much better!

Now, let's assume that we get the build bots to run their builds with make C=2 (which my
patches here allow, since it produces 0 warnings by design and by default)

Once this patch is in, errors of any kind of any of the types that are *not* excepted by
this file will break the build and generate a warning report, forcing the committer to fix
the errors right away. To me that's a big improvement from today.

> Also this would mean that new patches to IB would continue the bad habits

That's **only for the excepted types and files, and a temporary situation 
until we can fix the rest of the issues.

See my additional patch set here as an example of how I see us attack this piecemeal:

https://github.com/knuto/linux/tree/runchecks

I'll post that set as soon as patch #1/2 here is in.

I hope this clarifies!

Thanks,
Knut

Re: [PATCH v2 4/5] rds: Add runchecks.cfg for net/rds

[Knut Omang]

On Sat, 2017-12-16 at 10:24 -0800, Joe Perches wrote:
> On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:
> > On Sat, 16 Dec 2017 15:42:29 +0100 Knut Omang <knut.omang@oracle.com> wrote:
> > > +# Code simplification:
> > > +#
> > > +except ALLOC_WITH_MULTIPLY ib.c
> > > +except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c
> transport.c
> > > +except UNNECESSARY_ELSE ib_fmr.c
> > > +except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
> > > +except PRINTK_RATELIMITED ib_frmr.c
> > > +except EMBEDDED_FUNCTION_NAME ib_rdma.c
> > > +
> > > +# Style and readability:
> > > +#
> > > +except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
> > > +except OOM_MESSAGE ib.c tcp.c
> > > +except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
> > > +except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
> > > +except OPEN_ENDED_LINE recv.c ib_recv.c
> > > +
> > > +# Candidates to leave as exceptions (don't fix):
> > > +except MULTIPLE_ASSIGNMENTS ib_send.c
> > > +except LONG_LINE_STRING connection.c
> > > +except OPEN_BRACE connection.c
> > > +
> > 
> > Why start letting subsystems have a free-pass?
> > Also this would mean that new patches to IB would continue the bad habits.
> 
> I agree with this comment at least for net/rds.
> 
> Most of these existing messages from checkpatch should
> probably be inspected and corrected where possible to
> minimize the style differences between this subsystem
> and the rest of the kernel.

Please get me right here, I want us to fix all or at least most the issues, unless there
are very good reasons for keeping some. But to fix a problem, partitioning it into easy
manageable and distributable pieces can often be a good idea. It also allows us to focus
on the most important or easiest issues first - my comments here are intended as
examples/guidance/classification.

> For instance, here's a trivial patch to substitute
> pr_<level> for printks and a couple braces next to
> these substitutions.

yes - I have about 10 such patches for RDMA and 10 for RDS 
fixing various issues "queued up" here - done while I worked on the logic:

https://github.com/knuto/linux/tree/runchecks

I just felt that mixing them up with the runchecks functionality itself would be wrong,
but wanted to throw in these two configuration files to give you some example of how it
can be used.

Thanks,
Knut

> btw:
> 
> in ib_cm, why is one call to ib_modify_qp emitted
> with a -ret and the other with a positive err?


> 
> ---
>  net/rds/ib_cm.c          | 21 ++++++++++-----------
>  net/rds/ib_recv.c        |  5 ++---
>  net/rds/ib_send.c        | 23 ++++++++++++-----------
>  net/rds/rdma_transport.c | 14 +++++++-------
>  net/rds/send.c           |  8 ++++----
>  net/rds/tcp_send.c       |  4 +---
>  net/rds/threads.c        |  6 ++----
>  net/rds/transport.c      | 12 ++++++------
>  8 files changed, 44 insertions(+), 49 deletions(-)
> 
> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 80fb6f63e768..92694c9cb7c9 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -86,7 +86,7 @@ rds_ib_tune_rnr(struct rds_ib_connection *ic, struct ib_qp_attr *attr)
>  	attr->min_rnr_timer = IB_RNR_TIMER_000_32;
>  	ret = ib_modify_qp(ic->i_cm_id->qp, attr, IB_QP_MIN_RNR_TIMER);
>  	if (ret)
> -		printk(KERN_NOTICE "ib_modify_qp(IB_QP_MIN_RNR_TIMER): err=%d\n",
> -ret);
> +		pr_notice("ib_modify_qp(IB_QP_MIN_RNR_TIMER): err=%d\n", -ret);
>  }
>  
>  /*
> @@ -146,13 +146,12 @@ void rds_ib_cm_connect_complete(struct rds_connection *conn,
> struct rdma_cm_even
>  	qp_attr.qp_state = IB_QPS_RTS;
>  	err = ib_modify_qp(ic->i_cm_id->qp, &qp_attr, IB_QP_STATE);
>  	if (err)
> -		printk(KERN_NOTICE "ib_modify_qp(IB_QP_STATE, RTS): err=%d\n", err);
> +		pr_notice("ib_modify_qp(IB_QP_STATE, RTS): err=%d\n", err);
>  
>  	/* update ib_device with this local ipaddr */
>  	err = rds_ib_update_ipaddr(ic->rds_ibdev, conn->c_laddr);
>  	if (err)
> -		printk(KERN_ERR "rds_ib_update_ipaddr failed (%d)\n",
> -			err);
> +		pr_err("rds_ib_update_ipaddr failed (%d)\n", err);
>  
>  	/* If the peer gave us the last packet it saw, process this as if
>  	 * we had received a regular ACK. */
> @@ -594,8 +593,7 @@ static u32 rds_ib_protocol_compatible(struct rdma_cm_event *event)
>  
>  	/* Be paranoid. RDS always has privdata */
>  	if (!event->param.conn.private_data_len) {
> -		printk(KERN_NOTICE "RDS incoming connection has no private data, "
> -			"rejecting\n");
> +		pr_notice("RDS incoming connection has no private data, rejecting\n");
>  		return 0;
>  	}
>  
> @@ -609,11 +607,12 @@ static u32 rds_ib_protocol_compatible(struct rdma_cm_event *event)
>  		version = RDS_PROTOCOL_3_0;
>  		while ((common >>= 1) != 0)
>  			version++;
> -	} else
> -		printk_ratelimited(KERN_NOTICE "RDS: Connection from %pI4 using
> incompatible protocol version %u.%u\n",
> -				&dp->dp_saddr,
> -				dp->dp_protocol_major,
> -				dp->dp_protocol_minor);
> +	} else {
> +		pr_notice_ratelimited("RDS: Connection from %pI4 using incompatible
> protocol version %u.%u\n",
> +				      &dp->dp_saddr,
> +				      dp->dp_protocol_major,
> +				      dp->dp_protocol_minor);
> +	}
>  	return version;
>  }
>  
> diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
> index b4e421aa9727..9dfc8233c488 100644
> --- a/net/rds/ib_recv.c
> +++ b/net/rds/ib_recv.c
> @@ -105,7 +105,7 @@ static int rds_ib_recv_alloc_cache(struct rds_ib_refill_cache
> *cache)
>  
>  	cache->percpu = alloc_percpu(struct rds_ib_cache_head);
>  	if (!cache->percpu)
> -	       return -ENOMEM;
> +		return -ENOMEM;
>  
>  	for_each_possible_cpu(cpu) {
>  		head = per_cpu_ptr(cache->percpu, cpu);
> @@ -399,8 +399,7 @@ void rds_ib_recv_refill(struct rds_connection *conn, int prefill,
> gfp_t gfp)
>  	while ((prefill || rds_conn_up(conn)) &&
>  	       rds_ib_ring_alloc(&ic->i_recv_ring, 1, &pos)) {
>  		if (pos >= ic->i_recv_ring.w_nr) {
> -			printk(KERN_NOTICE "Argh - ring alloc returned pos=%u\n",
> -					pos);
> +			pr_notice("Argh - ring alloc returned pos=%u\n", pos);
>  			break;
>  		}
>  
> diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
> index 8557a1cae041..cb1ce8d06582 100644
> --- a/net/rds/ib_send.c
> +++ b/net/rds/ib_send.c
> @@ -180,9 +180,8 @@ static struct rds_message *rds_ib_send_unmap_op(struct
> rds_ib_connection *ic,
>  		}
>  		break;
>  	default:
> -		printk_ratelimited(KERN_NOTICE
> -			       "RDS/IB: %s: unexpected opcode 0x%x in WR!\n",
> -			       __func__, send->s_wr.opcode);
> +		pr_notice_ratelimited("RDS/IB: %s: unexpected opcode 0x%x in WR!\n",
> +				      __func__, send->s_wr.opcode);
>  		break;
>  	}
>  
> @@ -730,8 +729,8 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
>  		 first, &first->s_wr, ret, failed_wr);
>  	BUG_ON(failed_wr != &first->s_wr);
>  	if (ret) {
> -		printk(KERN_WARNING "RDS/IB: ib_post_send to %pI4 "
> -		       "returned %d\n", &conn->c_faddr, ret);
> +		pr_warn("RDS/IB: ib_post_send to %pI4 returned %d\n",
> +			&conn->c_faddr, ret);
>  		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
>  		rds_ib_sub_signaled(ic, nr_sig);
>  		if (prev->s_op) {
> @@ -827,15 +826,16 @@ int rds_ib_xmit_atomic(struct rds_connection *conn, struct
> rm_atomic_op *op)
>  		 send, &send->s_atomic_wr, ret, failed_wr);
>  	BUG_ON(failed_wr != &send->s_atomic_wr.wr);
>  	if (ret) {
> -		printk(KERN_WARNING "RDS/IB: atomic ib_post_send to %pI4 "
> -		       "returned %d\n", &conn->c_faddr, ret);
> +		pr_warn("RDS/IB: atomic ib_post_send to %pI4 returned %d\n",
> +			&conn->c_faddr, ret);
>  		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
>  		rds_ib_sub_signaled(ic, nr_sig);
>  		goto out;
>  	}
>  
>  	if (unlikely(failed_wr != &send->s_atomic_wr.wr)) {
> -		printk(KERN_WARNING "RDS/IB: atomic ib_post_send() rc=%d, but
> failed_wqe updated!\n", ret);
> +		pr_warn("RDS/IB: atomic ib_post_send() rc=%d, but failed_wqe
> updated!\n",
> +			ret);
>  		BUG_ON(failed_wr != &send->s_atomic_wr.wr);
>  	}
>  
> @@ -967,15 +967,16 @@ int rds_ib_xmit_rdma(struct rds_connection *conn, struct
> rm_rdma_op *op)
>  		 first, &first->s_rdma_wr.wr, ret, failed_wr);
>  	BUG_ON(failed_wr != &first->s_rdma_wr.wr);
>  	if (ret) {
> -		printk(KERN_WARNING "RDS/IB: rdma ib_post_send to %pI4 "
> -		       "returned %d\n", &conn->c_faddr, ret);
> +		pr_warn("RDS/IB: rdma ib_post_send to %pI4 returned %d\n",
> +			&conn->c_faddr, ret);
>  		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
>  		rds_ib_sub_signaled(ic, nr_sig);
>  		goto out;
>  	}
>  
>  	if (unlikely(failed_wr != &first->s_rdma_wr.wr)) {
> -		printk(KERN_WARNING "RDS/IB: ib_post_send() rc=%d, but failed_wqe
> updated!\n", ret);
> +		pr_warn("RDS/IB: ib_post_send() rc=%d, but failed_wqe updated!\n",
> +			ret);
>  		BUG_ON(failed_wr != &first->s_rdma_wr.wr);
>  	}
>  
> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index fc59821f0a27..0ccb1cde4c52 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c
> @@ -131,7 +131,7 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
>  
>  	default:
>  		/* things like device disconnect? */
> -		printk(KERN_ERR "RDS: unknown event %u (%s)!\n",
> +		pr_err("RDS: unknown event %u (%s)!\n",
>  		       event->event, rdma_event_msg(event->event));
>  		break;
>  	}
> @@ -156,8 +156,8 @@ static int rds_rdma_listen_init(void)
>  			       RDMA_PS_TCP, IB_QPT_RC);
>  	if (IS_ERR(cm_id)) {
>  		ret = PTR_ERR(cm_id);
> -		printk(KERN_ERR "RDS/RDMA: failed to setup listener, "
> -		       "rdma_create_id() returned %d\n", ret);
> +		pr_err("RDS/RDMA: failed to setup listener, rdma_create_id() returned
> %d\n",
> +		       ret);
>  		return ret;
>  	}
>  
> @@ -171,15 +171,15 @@ static int rds_rdma_listen_init(void)
>  	 */
>  	ret = rdma_bind_addr(cm_id, (struct sockaddr *)&sin);
>  	if (ret) {
> -		printk(KERN_ERR "RDS/RDMA: failed to setup listener, "
> -		       "rdma_bind_addr() returned %d\n", ret);
> +		pr_err("RDS/RDMA: failed to setup listener, rdma_bind_addr() returned
> %d\n",
> +		       ret);
>  		goto out;
>  	}
>  
>  	ret = rdma_listen(cm_id, 128);
>  	if (ret) {
> -		printk(KERN_ERR "RDS/RDMA: failed to setup listener, "
> -		       "rdma_listen() returned %d\n", ret);
> +		pr_err("RDS/RDMA: failed to setup listener, rdma_listen() returned
> %d\n",
> +		       ret);
>  		goto out;
>  	}
>  
> diff --git a/net/rds/send.c b/net/rds/send.c
> index b52cdc8ae428..f9bc3d499576 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -1130,15 +1130,15 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t
> payload_len)
>  	}
>  
>  	if (rm->rdma.op_active && !conn->c_trans->xmit_rdma) {
> -		printk_ratelimited(KERN_NOTICE "rdma_op %p conn xmit_rdma %p\n",
> -			       &rm->rdma, conn->c_trans->xmit_rdma);
> +		pr_notice_ratelimited("rdma_op %p conn xmit_rdma %p\n",
> +				      &rm->rdma, conn->c_trans->xmit_rdma);
>  		ret = -EOPNOTSUPP;
>  		goto out;
>  	}
>  
>  	if (rm->atomic.op_active && !conn->c_trans->xmit_atomic) {
> -		printk_ratelimited(KERN_NOTICE "atomic_op %p conn xmit_atomic %p\n",
> -			       &rm->atomic, conn->c_trans->xmit_atomic);
> +		pr_notice_ratelimited("atomic_op %p conn xmit_atomic %p\n",
> +				      &rm->atomic, conn->c_trans->xmit_atomic);
>  		ret = -EOPNOTSUPP;
>  		goto out;
>  	}
> diff --git a/net/rds/tcp_send.c b/net/rds/tcp_send.c
> index dc860d1bb608..0e23e9d06c7e 100644
> --- a/net/rds/tcp_send.c
> +++ b/net/rds/tcp_send.c
> @@ -153,9 +153,7 @@ int rds_tcp_xmit(struct rds_connection *conn, struct rds_message
> *rm,
>  			 * an incoming RST.
>  			 */
>  			if (rds_conn_path_up(cp)) {
> -				pr_warn("RDS/tcp: send to %pI4 on cp [%d]"
> -					"returned %d, "
> -					"disconnecting and reconnecting\n",
> +				pr_warn("RDS/tcp: send to %pI4 on cp [%d]returned %d,
> disconnecting and reconnecting\n",
>  					&conn->c_faddr, cp->cp_index, ret);
>  				rds_conn_path_drop(cp, false);
>  			}
> diff --git a/net/rds/threads.c b/net/rds/threads.c
> index f121daa402c8..499a0a8287cc 100644
> --- a/net/rds/threads.c
> +++ b/net/rds/threads.c
> @@ -74,10 +74,8 @@ EXPORT_SYMBOL_GPL(rds_wq);
>  void rds_connect_path_complete(struct rds_conn_path *cp, int curr)
>  {
>  	if (!rds_conn_path_transition(cp, curr, RDS_CONN_UP)) {
> -		printk(KERN_WARNING "%s: Cannot transition to state UP, "
> -				"current state is %d\n",
> -				__func__,
> -				atomic_read(&cp->cp_state));
> +		pr_warn("%s: Cannot transition to state UP, current state is %d\n",
> +			__func__, atomic_read(&cp->cp_state));
>  		rds_conn_path_drop(cp, false);
>  		return;
>  	}
> diff --git a/net/rds/transport.c b/net/rds/transport.c
> index 0b188dd0a344..a0d7ccecdec3 100644
> --- a/net/rds/transport.c
> +++ b/net/rds/transport.c
> @@ -46,12 +46,12 @@ void rds_trans_register(struct rds_transport *trans)
>  
>  	down_write(&rds_trans_sem);
>  
> -	if (transports[trans->t_type])
> -		printk(KERN_ERR "RDS Transport type %d already registered\n",
> -			trans->t_type);
> -	else {
> +	if (transports[trans->t_type]) {
> +		pr_err("RDS Transport type %d already registered\n",
> +		       trans->t_type);
> +	} else {
>  		transports[trans->t_type] = trans;
> -		printk(KERN_INFO "Registered RDS/%s transport\n", trans->t_name);
> +		pr_info("Registered RDS/%s transport\n", trans->t_name);
>  	}
>  
>  	up_write(&rds_trans_sem);
> @@ -63,7 +63,7 @@ void rds_trans_unregister(struct rds_transport *trans)
>  	down_write(&rds_trans_sem);
>  
>  	transports[trans->t_type] = NULL;
> -	printk(KERN_INFO "Unregistered RDS/%s transport\n", trans->t_name);
> +	pr_info("Unregistered RDS/%s transport\n", trans->t_name);
>  
>  	up_write(&rds_trans_sem);
>  }
> 

Re: [PATCH v2 4/5] rds: Add runchecks.cfg for net/rds

[Knut Omang]

On Sat, 2017-12-16 at 12:00 -0800, santosh.shilimkar@oracle.com wrote:
> On 12/16/17 10:24 AM, Joe Perches wrote:
> > On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:
> >> On Sat, 16 Dec 2017 15:42:29 +0100 Knut Omang <knut.omang@oracle.com> wrote:
> >>> +# Code simplification:
> >>> +#
> >>> +except ALLOC_WITH_MULTIPLY ib.c
> >>> +except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c
> transport.c
> >>> +except UNNECESSARY_ELSE ib_fmr.c
> >>> +except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
> >>> +except PRINTK_RATELIMITED ib_frmr.c
> >>> +except EMBEDDED_FUNCTION_NAME ib_rdma.c
> >>> +
> >>> +# Style and readability:
> >>> +#
> >>> +except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
> >>> +except OOM_MESSAGE ib.c tcp.c
> >>> +except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
> >>> +except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
> >>> +except OPEN_ENDED_LINE recv.c ib_recv.c
> >>> +
> >>> +# Candidates to leave as exceptions (don't fix):
> >>> +except MULTIPLE_ASSIGNMENTS ib_send.c
> >>> +except LONG_LINE_STRING connection.c
> >>> +except OPEN_BRACE connection.c
> >>> +
> >>
> >> Why start letting subsystems have a free-pass?
> >> Also this would mean that new patches to IB would continue the bad habits.
> And I don't need any free pass for RDS either.

It's not a free pass, it's an assessment of the current situation, to allow 
people to start working on it easily. I have already done some of that work
and will post that later.

> I missed V1 of this series but Knut, please don't add
> any exceptions for RDS and if there is something needs to
> be fixed, we can address it. Once your infrastructure
> gets merged, the subsequent fixes can be added.

This is about temporary masking some errors to allow automated testing
to prevent new regressions to occur in all the files and for all the 
types that are not excepted!

> > 
> > I agree with this comment at least for net/rds.
> > 
> > Most of these existing messages from checkpatch should
> > probably be inspected and corrected where possible to
> > minimize the style differences between this subsystem
> > and the rest of the kernel.
> > 
> > For instance, here's a trivial patch to substitute
> > pr_<level> for printks and a couple braces next to
> > these substitutions.
> >
> Thanks Joe. I actually had a similar patch a while back but
> since it was lot of churn, and code was already merged,
> never submitted it and then later forgot about it.
> 
> Will look into it.

Please look at my set here first - I have already spent considerable time cleaning up
stuff while working on this:

https://github.com/knuto/linux/tree/runchecks

Thanks,
Knut

> > btw:
> > 
> > in ib_cm, why is one call to ib_modify_qp emitted
> > with a -ret and the other with a positive err?
> >
> Its oversight and will fix that.
> 
> Regards,
> Santosh

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Knut Omang]

On Sat, 2017-12-16 at 09:47 -0800, Stephen Hemminger wrote:
> On Sat, 16 Dec 2017 15:42:25 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > This patch series implements features to make it easier to run checkers on the
> > entire kernel as part of automatic and developer testing.
> > 
> > This is done by replacing the sparse specific setup for the C={1,2} variable
> > in the makefiles with setup for running scripts/runchecks, a new program that
> > can run any number of different "checkers". The behaviour of runchecks is
> > defined by simple "global" configuration in scripts/runchecks.cfg which can be
> > extended by local configuration applying to individual files, directories or
> > subtrees in the source.
> > 
> > It also fixes a minor issue with "checkpatch --fix-inplace" found during testing
> > (patch #3).
> > 
> > The runchecks.cfg files are parsed according to this minimal language:
> > 
> >        # comments
> >        # "Global configuration in scripts/runchecks.cfg:
> >        checker <name>
> >        typedef NAME regex
> >        run <list of checkers or "all"
> > 
> >        # "local" configuration:
> >        line_len <n>
> >        except checkpatch_type [files ...]
> >        pervasive checkpatch_type1 [checkpatch_type2 ...]
> > 
> > With "make C=2" runchecks first parse the file scripts/runchecks.cfg, then
> > look for a file named 'runchecks.cfg' in the same directory as the source file.
> > If that file exists, it will be parsed and it's local configuration applied to
> > allow suppression on a per checker, per check and per file basis.
> > If a "local" configuration does not exist, either in the source directory or
> > above, make will simply silently ignore the file.
> > 
> > The idea is that the community can work to add runchecks.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/subtrees without such files, automation can start right
> > away as it is trivially possible to run errorless with C=2 for the entire
> > kernel.
> > 
> > For the checker maintainers this should be a benefit as well: new
> > or improved checks would generate new errors/warnings. With automatic testing
> > for the checkers, these new checks would generate error reports and cause
> > builds to fail. Adding the new check a 'pervasive' option at the top level or
> > even for specific files, marked with a "new check - please consider fixing" comment
> > or similar would make those builds pass while documenting and making the new check
> > more visible.
> > 
> > The patches includes a documentation file with some more details.
> > 
> > This patch set has evolved from an earlier shell script implementation I made
> > as only 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 runchecks
> > 
> > I only include version 0 of runchecks.cfg in the two directories I
> > worked on here as the final two patches. These files both documents where
> > the issues are in those two directories, and can be used by the maintainer
> > to indicate to potential helpers what to focus on as I have tried to
> > illustrate by means of comments.
> > 
> > Changes from v1:
> > -----------------
> > Based on feedback, the implementation is completely rewritten and extended.
> > Instead of patches to checkpatch, and a sole checkpatch focus, it is now a
> > generic solution implemented in python, for any type of checkers, extendable
> > through some basic generic functionality, and for special needs by subclassing
> > the Checker class in the implementation.
> > 
> > This implementation fully supports checkpatch, sparse and
> > checkdoc == kernel-doc -none, and also has been tested with coccicheck.
> > To facilitate the same mechanism of using check types to filter what
> > checks to be suppressed, I introduced the concept of "typedefs" which allows
> > runchecks to effectively augment the check type space of the checker in cases
> > where types either are not available at all (checkdoc) or where only a few
> > can be filtered out (sparse)
> > 
> > With this in place it also became trivial to make the look and feel similar
> > for sparse and checkdoc as for checkpatch, with some optional color support
> > too, to make fixing issues in the code, the goal of this whole exercise,
> > much more pleasant IMHO :-)
> > 
> > Thanks,
> > Knut
> > 
> > Knut Omang (5):
> >   runchecks: Generalize make C={1,2} to support multiple checkers
> >   Documentation: Add doc for runchecks, a checker runner
> >   checkpatch: Improve --fix-inplace for TABSTOP
> >   rds: Add runchecks.cfg for net/rds
> >   RDMA/core: Add runchecks.cfg for drivers/infiniband/core
> > 
> >  Documentation/dev-tools/coccinelle.rst |  12 +-
> >  Documentation/dev-tools/index.rst      |   1 +-
> >  Documentation/dev-tools/runchecks.rst  | 215 ++++++++-
> >  Documentation/dev-tools/sparse.rst     |  30 +-
> >  Documentation/kbuild/kbuild.txt        |   9 +-
> >  Makefile                               |  23 +-
> >  drivers/infiniband/core/runchecks.cfg  |  83 +++-
> >  net/rds/runchecks.cfg                  |  76 +++-
> >  scripts/Makefile.build                 |   4 +-
> >  scripts/checkpatch.pl                  |   2 +-
> >  scripts/runchecks                      | 734 ++++++++++++++++++++++++++-
> >  scripts/runchecks.cfg                  |  63 ++-
> >  scripts/runchecks_help.txt             |  43 ++-
> >  13 files changed, 1274 insertions(+), 21 deletions(-)
> >  create mode 100644 Documentation/dev-tools/runchecks.rst
> >  create mode 100644 drivers/infiniband/core/runchecks.cfg
> >  create mode 100644 net/rds/runchecks.cfg
> >  create mode 100755 scripts/runchecks
> >  create mode 100644 scripts/runchecks.cfg
> >  create mode 100644 scripts/runchecks_help.txt
> > 
> > base-commit: ae64f9bd1d3621b5e60d7363bc20afb46aede215
> 
> I like the ability to add more checkers and keep then in the main
> upstream tree. But adding overrides for specific subsystems goes against
> the policy that all subsystems should be treated equally.

This is a tool to enable automated testing for as many checks as possible, as soon as
possible. Like any tool, it can be misused, but that's IMHO an orthogonal problem that I
think the maintainers will be more than capable of preventing.

Think of this as a tightening screw: We eliminate errors class by class or file by file,
and in the same commit narrows in the list of exceptions. That way we can fix issues piece
by piece while avoiding a lot of regressions in already clean parts.

> There was discussion at Kernel Summit about how the different
> subsystems already have different rules. This appears to be a
> way to make that worse.

IMHO this is a tool that should help maintainers implement the policies they desire.
But the tool itself does not dictate any such.

Thanks,
Knut

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Jason Gunthorpe]

On Sun, Dec 17, 2017 at 03:14:10AM +0100, Knut Omang wrote:

> > I like the ability to add more checkers and keep then in the main
> > upstream tree. But adding overrides for specific subsystems goes against
> > the policy that all subsystems should be treated equally.
> 
> This is a tool to enable automated testing for as many checks as
> possible, as soon as possible. Like any tool, it can be misused, but
> that's IMHO an orthogonal problem that I think the maintainers will
> be more than capable of preventing.
> 
> Think of this as a tightening screw: We eliminate errors class by
> class or file by file, and in the same commit narrows in the list of
> exceptions. That way we can fix issues piece by piece while avoiding
> a lot of regressions in already clean parts.

Since you used drivers/infiniband as an example for this script..

I will say I agree with this idea.

It is not that we *want* infiniband to be different from the rest of
the kernel, it is that we have this historical situation where we
don't have a code base that already passes the various static checker
things.

I would like it very much if I could run 'make static checker' and see
no warnings. This helps me know that I when I accept patches I am not
introducing new problems to code that has already been cleaned up.

Today when we run checkers we get so many warnings it is too hard to
make any sense of it.

Being able to say File X is now clean for check XYZ seems very useful
and may motivate people to clean up the files they actualy care
about...

> > There was discussion at Kernel Summit about how the different
> > subsystems already have different rules. This appears to be a way
> > to make that worse.
> 
> IMHO this is a tool that should help maintainers implement the
> policies they desire.  But the tool itself does not dictate any
> such.

Yes, again, in infiniband we like to see checkpatch be good for new
submission, even if that clashes with surrounding code. For instance
we have a mixture of sizeof foo and sizeof(foo) styles in the same
file/function now.

I certainly don't want to tell people they need to follow some
different style from 10 years ago when they send patches.

Jason

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Joe Perches]

On Sun, 2017-12-17 at 22:00 -0700, Jason Gunthorpe wrote:
> On Sun, Dec 17, 2017 at 03:14:10AM +0100, Knut Omang wrote:
> 
> > > I like the ability to add more checkers and keep then in the main
> > > upstream tree. But adding overrides for specific subsystems goes against
> > > the policy that all subsystems should be treated equally.
> > 
> > This is a tool to enable automated testing for as many checks as
> > possible, as soon as possible. Like any tool, it can be misused, but
> > that's IMHO an orthogonal problem that I think the maintainers will
> > be more than capable of preventing.
> > 
> > Think of this as a tightening screw: We eliminate errors class by
> > class or file by file, and in the same commit narrows in the list of
> > exceptions. That way we can fix issues piece by piece while avoiding
> > a lot of regressions in already clean parts.
> 
> Since you used drivers/infiniband as an example for this script..
> 
> I will say I agree with this idea.
> 
> It is not that we *want* infiniband to be different from the rest of
> the kernel, it is that we have this historical situation where we
> don't have a code base that already passes the various static checker
> things.
> 
> I would like it very much if I could run 'make static checker' and see
> no warnings. This helps me know that I when I accept patches I am not
> introducing new problems to code that has already been cleaned up.
> 
> Today when we run checkers we get so many warnings it is too hard to
> make any sense of it.

Here is a list of the checkpatch messages for drivers/infiniband
sorted by type.

Many of these might be corrected by using

$ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
  $(git ls-files drivers/infiniband/)

   5243 CHECK:CAMELCASE
   4487 WARNING:LONG_LINE
   1755 CHECK:PARENTHESIS_ALIGNMENT
   1664 CHECK:SPACING
    910 WARNING:FUNCTION_ARGUMENTS
    742 CHECK:OPEN_ENDED_LINE
    685 CHECK:BRACES
    643 CHECK:UNNECESSARY_PARENTHESES
    478 WARNING:SIZEOF_PARENTHESIS
    361 WARNING:UNSPECIFIED_INT
    342 WARNING:LONG_LINE_COMMENT
    338 ERROR:SPACING
    338 CHECK:LINE_SPACING
    306 WARNING:SPLIT_STRING
    278 WARNING:SPACING
    242 WARNING:SYMBOLIC_PERMS
    194 WARNING:BLOCK_COMMENT_STYLE
    175 CHECK:BIT_MACRO
    158 WARNING:SPACE_BEFORE_TAB
    154 WARNING:LINE_SPACING
    139 CHECK:MACRO_ARG_REUSE
    133 CHECK:UNCOMMENTED_DEFINITION
    122 CHECK:AVOID_BUG
    103 CHECK:COMPARISON_TO_NULL
    101 WARNING:ENOSYS
     89 WARNING:BRACES
     78 WARNING:PREFER_PR_LEVEL
     74 WARNING:MULTILINE_DEREFERENCE
     59 CHECK:TYPO_SPELLING
     52 WARNING:EMBEDDED_FUNCTION_NAME
     52 CHECK:MULTIPLE_ASSIGNMENTS
     50 CHECK:PREFER_KERNEL_TYPES
     45 WARNING:RETURN_VOID
     39 WARNING:UNNECESSARY_ELSE
     38 ERROR:POINTER_LOCATION
     37 WARNING:ALLOC_WITH_MULTIPLY
     36 CHECK:ALLOC_SIZEOF_STRUCT
     35 CHECK:AVOID_EXTERNS
     34 WARNING:PRINTK_WITHOUT_KERN_LEVEL
     33 ERROR:CODE_INDENT
     32 WARNING:PREFER_PACKED
     32 CHECK:LOGICAL_CONTINUATIONS
     29 WARNING:MEMORY_BARRIER
     29 WARNING:LEADING_SPACE
     28 WARNING:DEEP_INDENTATION
     27 CHECK:USLEEP_RANGE
     23 WARNING:SUSPECT_CODE_INDENT
     23 ERROR:TRAILING_STATEMENTS
     21 WARNING:LONG_LINE_STRING
     20 WARNING:CONSIDER_KSTRTO
     18 WARNING:CONSTANT_COMPARISON
     18 ERROR:OPEN_BRACE
     15 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE
     14 WARNING:VOLATILE
     14 ERROR:SWITCH_CASE_INDENT_LEVEL
     11 WARNING:OOM_MESSAGE
     11 WARNING:INCLUDE_LINUX
     10 WARNING:SSCANF_TO_KSTRTO
     10 WARNING:INDENTED_LABEL
      9 ERROR:GLOBAL_INITIALISERS
      9 ERROR:COMPLEX_MACRO
      9 ERROR:ASSIGN_IN_IF
      8 WARNING:UNNECESSARY_BREAK
      6 WARNING:PRINTF_L
      6 WARNING:MISORDERED_TYPE
      6 ERROR:INITIALISED_STATIC
      5 WARNING:TABSTOP
      5 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO
      5 WARNING:NAKED_SSCANF
      4 WARNING:NEEDLESS_IF
      4 ERROR:RETURN_PARENTHESES
      4 CHECK:BOOL_COMPARISON
      3 WARNING:TRAILING_SEMICOLON
      3 WARNING:STATIC_CONST_CHAR_ARRAY
      3 ERROR:TRAILING_WHITESPACE
      2 WARNING:UNNECESSARY_PARENTHESES
      2 WARNING:MISSING_SPACE
      2 WARNING:LOGGING_CONTINUATION
      2 CHECK:ARCH_DEFINES
      1 WARNING:TYPECAST_INT_CONSTANT
      1 WARNING:PREFER_DEV_LEVEL
      1 WARNING:NR_CPUS
      1 WARNING:NEW_TYPEDEFS
      1 WARNING:MINMAX
      1 WARNING:MACRO_WITH_FLOW_CONTROL
      1 WARNING:LINE_CONTINUATIONS
      1 WARNING:DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON
      1 WARNING:DEFAULT_NO_BREAK
      1 WARNING:CONST_STRUCT
      1 WARNING:CONSIDER_COMPLETION
      1 ERROR:WHILE_AFTER_BRACE
      1 ERROR:ELSE_AFTER_BRACE
      1 CHECK:REDUNDANT_CODE

Re: [PATCH v2 5/5] RDMA/core: Add runchecks.cfg for drivers/infiniband/core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 4568 bytes --]

On Sat, Dec 16, 2017 at 03:42:30PM +0100, Knut Omang wrote:
> Add a runchecks.cfg to drivers/infiniband/core
> to start "reining in" future checker errors,
> and making it easier to selectively clean up existing
> issues.
>
> This runchecks.cfg lets make C=2 M=drivers/infiniband/core
> pass with all errors/warnings suppressed
>
> See Documentation/dev-tools/runchecks.rst for
> motivation and details.
>
> 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>
> ---
>  drivers/infiniband/core/runchecks.cfg | 83 ++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+)
>  create mode 100644 drivers/infiniband/core/runchecks.cfg
>
> diff --git a/drivers/infiniband/core/runchecks.cfg b/drivers/infiniband/core/runchecks.cfg
> new file mode 100644
> index 0000000..75ca57c
> --- /dev/null
> +++ b/drivers/infiniband/core/runchecks.cfg
> @@ -0,0 +1,83 @@
> +#
> +# checker suppression lists for drivers/infiniband/core
> +#
> +# see Documentation/dev-tools/runchecks.rst
> +#
> +
> +checker checkpatch
> +##################
> +
> +# Accept somewhat longer lines:
> +line_len 110
> +
> +# Uncategorized:
> +#
> +except TRAILING_STATEMENTS packer.c
> +except NON_OCTAL_PERMISSIONS rw.c
> +except STATIC_CONST_CHAR_ARRAY sysfs.c
> +except SYMBOLIC_PERMS sysfs.c
> +except NEEDLESS_IF sysfs.c
> +except NAKED_SSCANF device.c
> +except ALLOC_WITH_MULTIPLY fmr_pool.c iwpm_util.c cma.c
> +except MULTIPLE_ASSIGNMENTS rw.c verbs.c
> +except ALLOC_SIZEOF_STRUCT sysfs.c iwpm_util.c iwpm_msg.c cma.c cm.c iwcm.c
> +except LOGICAL_CONTINUATIONS mad.c iwpm_util.c user_mad.c
> +
> +# Important to fix/go through from a quality perspective:
> +#
> +except AVOID_BUG rw.c mad.c cm.c iwcm.c cma.c
> +except UNCOMMENTED_DEFINITION ucma.c fmr_pool.c multicast.c mad_rmpp.c
> +except UNCOMMENTED_DEFINITION ucm.c umem.c cma.c user_mad.c cm.c
> +except ASSIGN_IN_IF mad.c cma.c uverbs_ioctl_merge.c
> +except SUSPECT_CODE_INDENT iwpm_util.c cma.c uverbs_ioctl.c user_mad.c uverbs_cmd.c
> +except COMPLEX_MACRO uverbs_ioctl_merge.c
> +except BOOL_COMPARISON roce_gid_mgmt.c
> +
> +# Code simplification:
> +#
> +except UNNECESSARY_ELSE cache.c mad.c mad_rmpp.c
> +except UNNECESSARY_PARENTHESES cma.c sa_query.c mad.c ucma.c mad_rmpp.c umem.c
> +except UNNECESSARY_PARENTHESES user_mad.c uverbs_cmd.c uverbs_marshall.c cm.c
> +except EMBEDDED_FUNCTION_NAME mad.c umem.c cma.c ucma.c user_mad.c
> +except RETURN_VOID sysfs.c sa_query.c mad.c cma.c ucm.c uverbs_main.c umem.c
> +except CONSTANT_COMPARISON smi.c
> +except OOM_MESSAGE iwpm_util.c
> +
> +# Style and readability:
> +#
> +except OPEN_BRACE cache.c mad.c umem_odp.c umem_rbtree.c cm.c
> +except MULTILINE_DEREFERENCE mad.c cm.c cma.c uverbs_main.c
> +except LONG_LINE cma.c
> +except PARENTHESIS_ALIGNMENT umem.c cm.c
> +except FUNCTION_ARGUMENTS verbs.c uverbs_cmd.c sa_query.c sysfs.c
> +
> +# Candidates to leave as exceptions (don't fix):
> +except SPACING umem_rbtree.c
> +except MACRO_ARG_REUSE ud_header.c sa_query.c uverbs_ioctl_merge.c
> +
> +# These are in most of the source files, ignore for all files:
> +#
> +pervasive BLOCK_COMMENT_STYLE ENOSYS BRACES OPEN_ENDED_LINE
> +
> +# These are easily autocorrected by checkpatch with --fix-inplace:
> +#
> +pervasive SIZEOF_PARENTHESIS SPACING LINE_SPACING SPACE_BEFORE_TAB TABSTOP
> +pervasive TRAILING_WHITESPACE POINTER_LOCATION INITIALISED_STATIC
> +pervasive CODE_INDENT ELSE_AFTER_BRACE SYMBOLIC_PERMS LEADING_SPACE
> +pervasive PARENTHESIS_ALIGNMENT COMPARISON_TO_NULL TYPO_SPELLING
> +
> +checker sparse
> +##############
> +
> +except SHADOW sysfs.c fmr_pool.c user_mad.c uverbs_main.c ucm.c ucma.c
> +except RETURN_VOID roce_gid_mgmt.c
> +except TYPESIGN ucm.c ucma.c
> +
> +# From linux/device.h:
> +except RETURN_VOID ucm.c
> +
> +checker checkdoc
> +################
> +
> +except PARAM_DESC verbs.c device.c fmr_pool.c cache.c sa_query.c
> +except X_PARAM verbs.c fmr_pool.c cache.c

I missed most of the patches from this series and previous version too,
but in regards to IB. I don't want to see static checkers exceptions for
core code. It does make a lot of sense for drivers which can be unmaintained.

Also, I agree with other reviewers, there is no excuse for adding
checkpatch specifics per-subsystem/folder, the differences are better
to be treated in checkpatch.pl itself.

Thanks

> --
> git-series 0.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/5] RDMA/core: Add runchecks.cfg for drivers/infiniband/core

[Knut Omang]

On Mon, 2017-12-18 at 10:02 +0200, Leon Romanovsky wrote:
> On Sat, Dec 16, 2017 at 03:42:30PM +0100, Knut Omang wrote:
> > Add a runchecks.cfg to drivers/infiniband/core
> > to start "reining in" future checker errors,
> > and making it easier to selectively clean up existing
> > issues.
> > 
> > This runchecks.cfg lets make C=2 M=drivers/infiniband/core
> > pass with all errors/warnings suppressed
> > 
> > See Documentation/dev-tools/runchecks.rst for
> > motivation and details.
> > 
> > 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>
> > ---
> >  drivers/infiniband/core/runchecks.cfg | 83 ++++++++++++++++++++++++++++-
> >  1 file changed, 83 insertions(+)
> >  create mode 100644 drivers/infiniband/core/runchecks.cfg
> > 
> > diff --git a/drivers/infiniband/core/runchecks.cfg b/drivers/infiniband/core/runchecks.cfg
> > new file mode 100644
> > index 0000000..75ca57c
> > --- /dev/null
> > +++ b/drivers/infiniband/core/runchecks.cfg
> > @@ -0,0 +1,83 @@
> > +#
> > +# checker suppression lists for drivers/infiniband/core
> > +#
> > +# see Documentation/dev-tools/runchecks.rst
> > +#
> > +
> > +checker checkpatch
> > +##################
> > +
> > +# Accept somewhat longer lines:
> > +line_len 110
> > +
> > +# Uncategorized:
> > +#
> > +except TRAILING_STATEMENTS packer.c
> > +except NON_OCTAL_PERMISSIONS rw.c
> > +except STATIC_CONST_CHAR_ARRAY sysfs.c
> > +except SYMBOLIC_PERMS sysfs.c
> > +except NEEDLESS_IF sysfs.c
> > +except NAKED_SSCANF device.c
> > +except ALLOC_WITH_MULTIPLY fmr_pool.c iwpm_util.c cma.c
> > +except MULTIPLE_ASSIGNMENTS rw.c verbs.c
> > +except ALLOC_SIZEOF_STRUCT sysfs.c iwpm_util.c iwpm_msg.c cma.c cm.c iwcm.c
> > +except LOGICAL_CONTINUATIONS mad.c iwpm_util.c user_mad.c
> > +
> > +# Important to fix/go through from a quality perspective:
> > +#
> > +except AVOID_BUG rw.c mad.c cm.c iwcm.c cma.c
> > +except UNCOMMENTED_DEFINITION ucma.c fmr_pool.c multicast.c mad_rmpp.c
> > +except UNCOMMENTED_DEFINITION ucm.c umem.c cma.c user_mad.c cm.c
> > +except ASSIGN_IN_IF mad.c cma.c uverbs_ioctl_merge.c
> > +except SUSPECT_CODE_INDENT iwpm_util.c cma.c uverbs_ioctl.c user_mad.c uverbs_cmd.c
> > +except COMPLEX_MACRO uverbs_ioctl_merge.c
> > +except BOOL_COMPARISON roce_gid_mgmt.c
> > +
> > +# Code simplification:
> > +#
> > +except UNNECESSARY_ELSE cache.c mad.c mad_rmpp.c
> > +except UNNECESSARY_PARENTHESES cma.c sa_query.c mad.c ucma.c mad_rmpp.c umem.c
> > +except UNNECESSARY_PARENTHESES user_mad.c uverbs_cmd.c uverbs_marshall.c cm.c
> > +except EMBEDDED_FUNCTION_NAME mad.c umem.c cma.c ucma.c user_mad.c
> > +except RETURN_VOID sysfs.c sa_query.c mad.c cma.c ucm.c uverbs_main.c umem.c
> > +except CONSTANT_COMPARISON smi.c
> > +except OOM_MESSAGE iwpm_util.c
> > +
> > +# Style and readability:
> > +#
> > +except OPEN_BRACE cache.c mad.c umem_odp.c umem_rbtree.c cm.c
> > +except MULTILINE_DEREFERENCE mad.c cm.c cma.c uverbs_main.c
> > +except LONG_LINE cma.c
> > +except PARENTHESIS_ALIGNMENT umem.c cm.c
> > +except FUNCTION_ARGUMENTS verbs.c uverbs_cmd.c sa_query.c sysfs.c
> > +
> > +# Candidates to leave as exceptions (don't fix):
> > +except SPACING umem_rbtree.c
> > +except MACRO_ARG_REUSE ud_header.c sa_query.c uverbs_ioctl_merge.c
> > +
> > +# These are in most of the source files, ignore for all files:
> > +#
> > +pervasive BLOCK_COMMENT_STYLE ENOSYS BRACES OPEN_ENDED_LINE
> > +
> > +# These are easily autocorrected by checkpatch with --fix-inplace:
> > +#
> > +pervasive SIZEOF_PARENTHESIS SPACING LINE_SPACING SPACE_BEFORE_TAB TABSTOP
> > +pervasive TRAILING_WHITESPACE POINTER_LOCATION INITIALISED_STATIC
> > +pervasive CODE_INDENT ELSE_AFTER_BRACE SYMBOLIC_PERMS LEADING_SPACE
> > +pervasive PARENTHESIS_ALIGNMENT COMPARISON_TO_NULL TYPO_SPELLING
> > +
> > +checker sparse
> > +##############
> > +
> > +except SHADOW sysfs.c fmr_pool.c user_mad.c uverbs_main.c ucm.c ucma.c
> > +except RETURN_VOID roce_gid_mgmt.c
> > +except TYPESIGN ucm.c ucma.c
> > +
> > +# From linux/device.h:
> > +except RETURN_VOID ucm.c
> > +
> > +checker checkdoc
> > +################
> > +
> > +except PARAM_DESC verbs.c device.c fmr_pool.c cache.c sa_query.c
> > +except X_PARAM verbs.c fmr_pool.c cache.c
> 
> I missed most of the patches from this series and previous version too,
> but in regards to IB. I don't want to see static checkers exceptions for
> core code. It does make a lot of sense for drivers which can be unmaintained.

And I couldn't agree more. But some aid in assessing the scope of the task 
and tracking progress/prevent regressions would be good, wouldn't it?

This file + enabling a build bot to run make C=2 allows you to prevent 
any checker error types from appearing anywhere in core apart from for 
the exception files and starting from this point of.

> Also, I agree with other reviewers, there is no excuse for adding
> checkpatch specifics per-subsystem/folder, the differences are better
> to be treated in checkpatch.pl itself.

Hopefully my intention should be clearer if you look at the documentation:

https://lkml.org/lkml/2017/12/16/135

The best is if no exceptions are needed, but until then let's at least get
automatic testing so that you and other reviewers can focus on the more important 
issues?

Thanks,
Knut

> 
> Thanks
> 
> > --
> > git-series 0.9.1

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Knut Omang]

On Sun, 2017-12-17 at 22:00 -0800, Joe Perches wrote:
> On Sun, 2017-12-17 at 22:00 -0700, Jason Gunthorpe wrote:
> > On Sun, Dec 17, 2017 at 03:14:10AM +0100, Knut Omang wrote:
> > 
> > > > I like the ability to add more checkers and keep then in the main
> > > > upstream tree. But adding overrides for specific subsystems goes against
> > > > the policy that all subsystems should be treated equally.
> > > 
> > > This is a tool to enable automated testing for as many checks as
> > > possible, as soon as possible. Like any tool, it can be misused, but
> > > that's IMHO an orthogonal problem that I think the maintainers will
> > > be more than capable of preventing.
> > > 
> > > Think of this as a tightening screw: We eliminate errors class by
> > > class or file by file, and in the same commit narrows in the list of
> > > exceptions. That way we can fix issues piece by piece while avoiding
> > > a lot of regressions in already clean parts.
> > 
> > Since you used drivers/infiniband as an example for this script..
> > 
> > I will say I agree with this idea.
> > 
> > It is not that we *want* infiniband to be different from the rest of
> > the kernel, it is that we have this historical situation where we
> > don't have a code base that already passes the various static checker
> > things.
> > 
> > I would like it very much if I could run 'make static checker' and see
> > no warnings. This helps me know that I when I accept patches I am not
> > introducing new problems to code that has already been cleaned up.
> > 
> > Today when we run checkers we get so many warnings it is too hard to
> > make any sense of it.
> 
> Here is a list of the checkpatch messages for drivers/infiniband
> sorted by type.
> 
> Many of these might be corrected by using
> 
> $ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
>   $(git ls-files drivers/infiniband/)

Yes, and I already did that work piece by piece for individual types,
just to test the runchecks tool, and want to post that set once the 
runchecks script and Makefile changes itself are in,

see:

https://github.com/torvalds/linux/compare/master...knuto:runchecks

What I personally like about this approach is that with the runchecks.cfg file
one can focus easily on one or two check types at a time, create a commit out of that
and at the same time "tighten" runchecks.cfg - changes in that file then serves as 
documentation for what got changed and use maintainers can use comments to indicate 
why the remaining exceptions are still there - and there's a one-stop for anyone 
wanting to contribute to checkpatch/sparse/doc fixes.

And it will be possible to run bisect with make C=2 and always have 0 errors.

Thanks,
Knut
> 
>    5243 CHECK:CAMELCASE
>    4487 WARNING:LONG_LINE
>    1755 CHECK:PARENTHESIS_ALIGNMENT
>    1664 CHECK:SPACING
>     910 WARNING:FUNCTION_ARGUMENTS
>     742 CHECK:OPEN_ENDED_LINE
>     685 CHECK:BRACES
>     643 CHECK:UNNECESSARY_PARENTHESES
>     478 WARNING:SIZEOF_PARENTHESIS
>     361 WARNING:UNSPECIFIED_INT
>     342 WARNING:LONG_LINE_COMMENT
>     338 ERROR:SPACING
>     338 CHECK:LINE_SPACING
>     306 WARNING:SPLIT_STRING
>     278 WARNING:SPACING
>     242 WARNING:SYMBOLIC_PERMS
>     194 WARNING:BLOCK_COMMENT_STYLE
>     175 CHECK:BIT_MACRO
>     158 WARNING:SPACE_BEFORE_TAB
>     154 WARNING:LINE_SPACING
>     139 CHECK:MACRO_ARG_REUSE
>     133 CHECK:UNCOMMENTED_DEFINITION
>     122 CHECK:AVOID_BUG
>     103 CHECK:COMPARISON_TO_NULL
>     101 WARNING:ENOSYS
>      89 WARNING:BRACES
>      78 WARNING:PREFER_PR_LEVEL
>      74 WARNING:MULTILINE_DEREFERENCE
>      59 CHECK:TYPO_SPELLING
>      52 WARNING:EMBEDDED_FUNCTION_NAME
>      52 CHECK:MULTIPLE_ASSIGNMENTS
>      50 CHECK:PREFER_KERNEL_TYPES
>      45 WARNING:RETURN_VOID
>      39 WARNING:UNNECESSARY_ELSE
>      38 ERROR:POINTER_LOCATION
>      37 WARNING:ALLOC_WITH_MULTIPLY
>      36 CHECK:ALLOC_SIZEOF_STRUCT
>      35 CHECK:AVOID_EXTERNS
>      34 WARNING:PRINTK_WITHOUT_KERN_LEVEL
>      33 ERROR:CODE_INDENT
>      32 WARNING:PREFER_PACKED
>      32 CHECK:LOGICAL_CONTINUATIONS
>      29 WARNING:MEMORY_BARRIER
>      29 WARNING:LEADING_SPACE
>      28 WARNING:DEEP_INDENTATION
>      27 CHECK:USLEEP_RANGE
>      23 WARNING:SUSPECT_CODE_INDENT
>      23 ERROR:TRAILING_STATEMENTS
>      21 WARNING:LONG_LINE_STRING
>      20 WARNING:CONSIDER_KSTRTO
>      18 WARNING:CONSTANT_COMPARISON
>      18 ERROR:OPEN_BRACE
>      15 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE
>      14 WARNING:VOLATILE
>      14 ERROR:SWITCH_CASE_INDENT_LEVEL
>      11 WARNING:OOM_MESSAGE
>      11 WARNING:INCLUDE_LINUX
>      10 WARNING:SSCANF_TO_KSTRTO
>      10 WARNING:INDENTED_LABEL
>       9 ERROR:GLOBAL_INITIALISERS
>       9 ERROR:COMPLEX_MACRO
>       9 ERROR:ASSIGN_IN_IF
>       8 WARNING:UNNECESSARY_BREAK
>       6 WARNING:PRINTF_L
>       6 WARNING:MISORDERED_TYPE
>       6 ERROR:INITIALISED_STATIC
>       5 WARNING:TABSTOP
>       5 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO
>       5 WARNING:NAKED_SSCANF
>       4 WARNING:NEEDLESS_IF
>       4 ERROR:RETURN_PARENTHESES
>       4 CHECK:BOOL_COMPARISON
>       3 WARNING:TRAILING_SEMICOLON
>       3 WARNING:STATIC_CONST_CHAR_ARRAY
>       3 ERROR:TRAILING_WHITESPACE
>       2 WARNING:UNNECESSARY_PARENTHESES
>       2 WARNING:MISSING_SPACE
>       2 WARNING:LOGGING_CONTINUATION
>       2 CHECK:ARCH_DEFINES
>       1 WARNING:TYPECAST_INT_CONSTANT
>       1 WARNING:PREFER_DEV_LEVEL
>       1 WARNING:NR_CPUS
>       1 WARNING:NEW_TYPEDEFS
>       1 WARNING:MINMAX
>       1 WARNING:MACRO_WITH_FLOW_CONTROL
>       1 WARNING:LINE_CONTINUATIONS
>       1 WARNING:DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON
>       1 WARNING:DEFAULT_NO_BREAK
>       1 WARNING:CONST_STRUCT
>       1 WARNING:CONSIDER_COMPLETION
>       1 ERROR:WHILE_AFTER_BRACE
>       1 ERROR:ELSE_AFTER_BRACE
>       1 CHECK:REDUNDANT_CODE
> 

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Knut Omang]

On Sun, 2017-12-17 at 22:00 -0700, Jason Gunthorpe wrote:
> On Sun, Dec 17, 2017 at 03:14:10AM +0100, Knut Omang wrote:
> 
> > > I like the ability to add more checkers and keep then in the main
> > > upstream tree. But adding overrides for specific subsystems goes against
> > > the policy that all subsystems should be treated equally.
> > 
> > This is a tool to enable automated testing for as many checks as
> > possible, as soon as possible. Like any tool, it can be misused, but
> > that's IMHO an orthogonal problem that I think the maintainers will
> > be more than capable of preventing.
> > 
> > Think of this as a tightening screw: We eliminate errors class by
> > class or file by file, and in the same commit narrows in the list of
> > exceptions. That way we can fix issues piece by piece while avoiding
> > a lot of regressions in already clean parts.
> 
> Since you used drivers/infiniband as an example for this script..
> 
> I will say I agree with this idea.

> It is not that we *want* infiniband to be different from the rest of
> the kernel, it is that we have this historical situation where we
> don't have a code base that already passes the various static checker
> things.

I poked around trying make M= in different parts of the kernel to get some 
"mileage" and to get as much examples as possible, and it appears most of 
the kernel has issues of sorts. Also Joe and others keep adding more checks 
as well, and we'd want that process to coexist with the need for automatic 
regression testing in this area.

> I would like it very much if I could run 'make static checker' and see
> no warnings. 

which is just what is the result with 'make C=2 M=drivers/infiniband/core' 
and the patches #1 and #5 in this set in case not everyone got the point,

> This helps me know that I when I accept patches I am not
> introducing new problems to code that has already been cleaned up.
> 
> Today when we run checkers we get so many warnings it is too hard to
> make any sense of it.
> 
> Being able to say File X is now clean for check XYZ seems very useful
> and may motivate people to clean up the files they actualy care
> about...
> 
> > > There was discussion at Kernel Summit about how the different
> > > subsystems already have different rules. This appears to be a way
> > > to make that worse.
> > 
> > IMHO this is a tool that should help maintainers implement the
> > policies they desire.  But the tool itself does not dictate any
> > such.
> 
> Yes, again, in infiniband we like to see checkpatch be good for new
> submission, even if that clashes with surrounding code. For instance
> we have a mixture of sizeof foo and sizeof(foo) styles in the same
> file/function now.

That's one of the fixes that the excellent --fix-inplace handles so 
one of my patches here

https://github.com/torvalds/linux/compare/master...knuto:runchecks

fixes those in drivers/infiniband/core.

> I certainly don't want to tell people they need to follow some
> different style from 10 years ago when they send patches.
> 

Thanks,
Knut

> Jason

Re: [PATCH v2 5/5] RDMA/core: Add runchecks.cfg for drivers/infiniband/core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 6363 bytes --]

On Mon, Dec 18, 2017 at 01:36:26PM +0100, Knut Omang wrote:
> On Mon, 2017-12-18 at 10:02 +0200, Leon Romanovsky wrote:
> > On Sat, Dec 16, 2017 at 03:42:30PM +0100, Knut Omang wrote:
> > > Add a runchecks.cfg to drivers/infiniband/core
> > > to start "reining in" future checker errors,
> > > and making it easier to selectively clean up existing
> > > issues.
> > >
> > > This runchecks.cfg lets make C=2 M=drivers/infiniband/core
> > > pass with all errors/warnings suppressed
> > >
> > > See Documentation/dev-tools/runchecks.rst for
> > > motivation and details.
> > >
> > > 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>
> > > ---
> > >  drivers/infiniband/core/runchecks.cfg | 83 ++++++++++++++++++++++++++++-
> > >  1 file changed, 83 insertions(+)
> > >  create mode 100644 drivers/infiniband/core/runchecks.cfg
> > >
> > > diff --git a/drivers/infiniband/core/runchecks.cfg b/drivers/infiniband/core/runchecks.cfg
> > > new file mode 100644
> > > index 0000000..75ca57c
> > > --- /dev/null
> > > +++ b/drivers/infiniband/core/runchecks.cfg
> > > @@ -0,0 +1,83 @@
> > > +#
> > > +# checker suppression lists for drivers/infiniband/core
> > > +#
> > > +# see Documentation/dev-tools/runchecks.rst
> > > +#
> > > +
> > > +checker checkpatch
> > > +##################
> > > +
> > > +# Accept somewhat longer lines:
> > > +line_len 110
> > > +
> > > +# Uncategorized:
> > > +#
> > > +except TRAILING_STATEMENTS packer.c
> > > +except NON_OCTAL_PERMISSIONS rw.c
> > > +except STATIC_CONST_CHAR_ARRAY sysfs.c
> > > +except SYMBOLIC_PERMS sysfs.c
> > > +except NEEDLESS_IF sysfs.c
> > > +except NAKED_SSCANF device.c
> > > +except ALLOC_WITH_MULTIPLY fmr_pool.c iwpm_util.c cma.c
> > > +except MULTIPLE_ASSIGNMENTS rw.c verbs.c
> > > +except ALLOC_SIZEOF_STRUCT sysfs.c iwpm_util.c iwpm_msg.c cma.c cm.c iwcm.c
> > > +except LOGICAL_CONTINUATIONS mad.c iwpm_util.c user_mad.c
> > > +
> > > +# Important to fix/go through from a quality perspective:
> > > +#
> > > +except AVOID_BUG rw.c mad.c cm.c iwcm.c cma.c
> > > +except UNCOMMENTED_DEFINITION ucma.c fmr_pool.c multicast.c mad_rmpp.c
> > > +except UNCOMMENTED_DEFINITION ucm.c umem.c cma.c user_mad.c cm.c
> > > +except ASSIGN_IN_IF mad.c cma.c uverbs_ioctl_merge.c
> > > +except SUSPECT_CODE_INDENT iwpm_util.c cma.c uverbs_ioctl.c user_mad.c uverbs_cmd.c
> > > +except COMPLEX_MACRO uverbs_ioctl_merge.c
> > > +except BOOL_COMPARISON roce_gid_mgmt.c
> > > +
> > > +# Code simplification:
> > > +#
> > > +except UNNECESSARY_ELSE cache.c mad.c mad_rmpp.c
> > > +except UNNECESSARY_PARENTHESES cma.c sa_query.c mad.c ucma.c mad_rmpp.c umem.c
> > > +except UNNECESSARY_PARENTHESES user_mad.c uverbs_cmd.c uverbs_marshall.c cm.c
> > > +except EMBEDDED_FUNCTION_NAME mad.c umem.c cma.c ucma.c user_mad.c
> > > +except RETURN_VOID sysfs.c sa_query.c mad.c cma.c ucm.c uverbs_main.c umem.c
> > > +except CONSTANT_COMPARISON smi.c
> > > +except OOM_MESSAGE iwpm_util.c
> > > +
> > > +# Style and readability:
> > > +#
> > > +except OPEN_BRACE cache.c mad.c umem_odp.c umem_rbtree.c cm.c
> > > +except MULTILINE_DEREFERENCE mad.c cm.c cma.c uverbs_main.c
> > > +except LONG_LINE cma.c
> > > +except PARENTHESIS_ALIGNMENT umem.c cm.c
> > > +except FUNCTION_ARGUMENTS verbs.c uverbs_cmd.c sa_query.c sysfs.c
> > > +
> > > +# Candidates to leave as exceptions (don't fix):
> > > +except SPACING umem_rbtree.c
> > > +except MACRO_ARG_REUSE ud_header.c sa_query.c uverbs_ioctl_merge.c
> > > +
> > > +# These are in most of the source files, ignore for all files:
> > > +#
> > > +pervasive BLOCK_COMMENT_STYLE ENOSYS BRACES OPEN_ENDED_LINE
> > > +
> > > +# These are easily autocorrected by checkpatch with --fix-inplace:
> > > +#
> > > +pervasive SIZEOF_PARENTHESIS SPACING LINE_SPACING SPACE_BEFORE_TAB TABSTOP
> > > +pervasive TRAILING_WHITESPACE POINTER_LOCATION INITIALISED_STATIC
> > > +pervasive CODE_INDENT ELSE_AFTER_BRACE SYMBOLIC_PERMS LEADING_SPACE
> > > +pervasive PARENTHESIS_ALIGNMENT COMPARISON_TO_NULL TYPO_SPELLING
> > > +
> > > +checker sparse
> > > +##############
> > > +
> > > +except SHADOW sysfs.c fmr_pool.c user_mad.c uverbs_main.c ucm.c ucma.c
> > > +except RETURN_VOID roce_gid_mgmt.c
> > > +except TYPESIGN ucm.c ucma.c
> > > +
> > > +# From linux/device.h:
> > > +except RETURN_VOID ucm.c
> > > +
> > > +checker checkdoc
> > > +################
> > > +
> > > +except PARAM_DESC verbs.c device.c fmr_pool.c cache.c sa_query.c
> > > +except X_PARAM verbs.c fmr_pool.c cache.c
> >
> > I missed most of the patches from this series and previous version too,
> > but in regards to IB. I don't want to see static checkers exceptions for
> > core code. It does make a lot of sense for drivers which can be unmaintained.
>
> And I couldn't agree more. But some aid in assessing the scope of the task
> and tracking progress/prevent regressions would be good, wouldn't it?

In current form, it doesn't fully prevent regressions, for example the
line "except TYPESIGN ucm.c ucma.c" means that new code with such errors
to the files ucm.c/ucma.c will be missed too.

>
> This file + enabling a build bot to run make C=2 allows you to prevent
> any checker error types from appearing anywhere in core apart from for
> the exception files and starting from this point of.

We are talking about the same and our goal is the same too - to see it is merged.
I'm just saying that for the IB part, cleanup of IB/core is needed prior
to merging IB related runcheck.cfg file, so it will include drivers only.

>
> > Also, I agree with other reviewers, there is no excuse for adding
> > checkpatch specifics per-subsystem/folder, the differences are better
> > to be treated in checkpatch.pl itself.
>
> Hopefully my intention should be clearer if you look at the documentation:
>
> https://lkml.org/lkml/2017/12/16/135
>
> The best is if no exceptions are needed, but until then let's at least get
> automatic testing so that you and other reviewers can focus on the more important
> issues?

Hiding simply mean that such errors will never be fixed

Thanks

>
> Thanks,
> Knut
>
> >
> > Thanks
> >
> > > --
> > > git-series 0.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/5] RDMA/core: Add runchecks.cfg for drivers/infiniband/core

[Knut Omang]

On Mon, 2017-12-18 at 16:04 +0200, Leon Romanovsky wrote:
> On Mon, Dec 18, 2017 at 01:36:26PM +0100, Knut Omang wrote:
> > On Mon, 2017-12-18 at 10:02 +0200, Leon Romanovsky wrote:
> > > On Sat, Dec 16, 2017 at 03:42:30PM +0100, Knut Omang wrote:
> > > > Add a runchecks.cfg to drivers/infiniband/core
> > > > to start "reining in" future checker errors,
> > > > and making it easier to selectively clean up existing
> > > > issues.
> > > >
> > > > This runchecks.cfg lets make C=2 M=drivers/infiniband/core
> > > > pass with all errors/warnings suppressed
> > > >
> > > > See Documentation/dev-tools/runchecks.rst for
> > > > motivation and details.
> > > >
> > > > 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>
> > > > ---
> > > >  drivers/infiniband/core/runchecks.cfg | 83 ++++++++++++++++++++++++++++-
> > > >  1 file changed, 83 insertions(+)
> > > >  create mode 100644 drivers/infiniband/core/runchecks.cfg
> > > >
> > > > diff --git a/drivers/infiniband/core/runchecks.cfg
> b/drivers/infiniband/core/runchecks.cfg
> > > > new file mode 100644
> > > > index 0000000..75ca57c
> > > > --- /dev/null
> > > > +++ b/drivers/infiniband/core/runchecks.cfg
> > > > @@ -0,0 +1,83 @@
> > > > +#
> > > > +# checker suppression lists for drivers/infiniband/core
> > > > +#
> > > > +# see Documentation/dev-tools/runchecks.rst
> > > > +#
> > > > +
> > > > +checker checkpatch
> > > > +##################
> > > > +
> > > > +# Accept somewhat longer lines:
> > > > +line_len 110
> > > > +
> > > > +# Uncategorized:
> > > > +#
> > > > +except TRAILING_STATEMENTS packer.c
> > > > +except NON_OCTAL_PERMISSIONS rw.c
> > > > +except STATIC_CONST_CHAR_ARRAY sysfs.c
> > > > +except SYMBOLIC_PERMS sysfs.c
> > > > +except NEEDLESS_IF sysfs.c
> > > > +except NAKED_SSCANF device.c
> > > > +except ALLOC_WITH_MULTIPLY fmr_pool.c iwpm_util.c cma.c
> > > > +except MULTIPLE_ASSIGNMENTS rw.c verbs.c
> > > > +except ALLOC_SIZEOF_STRUCT sysfs.c iwpm_util.c iwpm_msg.c cma.c cm.c iwcm.c
> > > > +except LOGICAL_CONTINUATIONS mad.c iwpm_util.c user_mad.c
> > > > +
> > > > +# Important to fix/go through from a quality perspective:
> > > > +#
> > > > +except AVOID_BUG rw.c mad.c cm.c iwcm.c cma.c
> > > > +except UNCOMMENTED_DEFINITION ucma.c fmr_pool.c multicast.c mad_rmpp.c
> > > > +except UNCOMMENTED_DEFINITION ucm.c umem.c cma.c user_mad.c cm.c
> > > > +except ASSIGN_IN_IF mad.c cma.c uverbs_ioctl_merge.c
> > > > +except SUSPECT_CODE_INDENT iwpm_util.c cma.c uverbs_ioctl.c user_mad.c
> uverbs_cmd.c
> > > > +except COMPLEX_MACRO uverbs_ioctl_merge.c
> > > > +except BOOL_COMPARISON roce_gid_mgmt.c
> > > > +
> > > > +# Code simplification:
> > > > +#
> > > > +except UNNECESSARY_ELSE cache.c mad.c mad_rmpp.c
> > > > +except UNNECESSARY_PARENTHESES cma.c sa_query.c mad.c ucma.c mad_rmpp.c umem.c
> > > > +except UNNECESSARY_PARENTHESES user_mad.c uverbs_cmd.c uverbs_marshall.c cm.c
> > > > +except EMBEDDED_FUNCTION_NAME mad.c umem.c cma.c ucma.c user_mad.c
> > > > +except RETURN_VOID sysfs.c sa_query.c mad.c cma.c ucm.c uverbs_main.c umem.c
> > > > +except CONSTANT_COMPARISON smi.c
> > > > +except OOM_MESSAGE iwpm_util.c
> > > > +
> > > > +# Style and readability:
> > > > +#
> > > > +except OPEN_BRACE cache.c mad.c umem_odp.c umem_rbtree.c cm.c
> > > > +except MULTILINE_DEREFERENCE mad.c cm.c cma.c uverbs_main.c
> > > > +except LONG_LINE cma.c
> > > > +except PARENTHESIS_ALIGNMENT umem.c cm.c
> > > > +except FUNCTION_ARGUMENTS verbs.c uverbs_cmd.c sa_query.c sysfs.c
> > > > +
> > > > +# Candidates to leave as exceptions (don't fix):
> > > > +except SPACING umem_rbtree.c
> > > > +except MACRO_ARG_REUSE ud_header.c sa_query.c uverbs_ioctl_merge.c
> > > > +
> > > > +# These are in most of the source files, ignore for all files:
> > > > +#
> > > > +pervasive BLOCK_COMMENT_STYLE ENOSYS BRACES OPEN_ENDED_LINE
> > > > +
> > > > +# These are easily autocorrected by checkpatch with --fix-inplace:
> > > > +#
> > > > +pervasive SIZEOF_PARENTHESIS SPACING LINE_SPACING SPACE_BEFORE_TAB TABSTOP
> > > > +pervasive TRAILING_WHITESPACE POINTER_LOCATION INITIALISED_STATIC
> > > > +pervasive CODE_INDENT ELSE_AFTER_BRACE SYMBOLIC_PERMS LEADING_SPACE
> > > > +pervasive PARENTHESIS_ALIGNMENT COMPARISON_TO_NULL TYPO_SPELLING
> > > > +
> > > > +checker sparse
> > > > +##############
> > > > +
> > > > +except SHADOW sysfs.c fmr_pool.c user_mad.c uverbs_main.c ucm.c ucma.c
> > > > +except RETURN_VOID roce_gid_mgmt.c
> > > > +except TYPESIGN ucm.c ucma.c
> > > > +
> > > > +# From linux/device.h:
> > > > +except RETURN_VOID ucm.c
> > > > +
> > > > +checker checkdoc
> > > > +################
> > > > +
> > > > +except PARAM_DESC verbs.c device.c fmr_pool.c cache.c sa_query.c
> > > > +except X_PARAM verbs.c fmr_pool.c cache.c
> > >
> > > I missed most of the patches from this series and previous version too,
> > > but in regards to IB. I don't want to see static checkers exceptions for
> > > core code. It does make a lot of sense for drivers which can be unmaintained.
> >
> > And I couldn't agree more. But some aid in assessing the scope of the task
> > and tracking progress/prevent regressions would be good, wouldn't it?
> 
> In current form, it doesn't fully prevent regressions, for example the
> line "except TYPESIGN ucm.c ucma.c" means that new code with such errors
> to the files ucm.c/ucma.c will be missed too.

Yes, that's true.

> > This file + enabling a build bot to run make C=2 allows you to prevent
> > any checker error types from appearing anywhere in core apart from for
> > the exception files and starting from this point of.
> 
> We are talking about the same and our goal is the same too - to see it is merged.
> I'm just saying that for the IB part, cleanup of IB/core is needed prior
> to merging IB related runcheck.cfg file, so it will include drivers only.

appreciated, 
my take on it is that this initial runchecks.cfg file is more 
of a plan for improvements than an exception list, and that the file can be used
to track changes and for you to indicate importance or state of different 
checks.

> > > Also, I agree with other reviewers, there is no excuse for adding
> > > checkpatch specifics per-subsystem/folder, the differences are better
> > > to be treated in checkpatch.pl itself.
> >
> > Hopefully my intention should be clearer if you look at the documentation:
> >
> > https://lkml.org/lkml/2017/12/16/135
> >
> > The best is if no exceptions are needed, but until then let's at least get
> > automatic testing so that you and other reviewers can focus on the more important
> > issues?
> 
> Hiding simply mean that such errors will never be fixed

I agree that's a potential way to misuse this tool.

It depends on how we view/use the runchecks.cfg file - after all it is a documentation of
issues that exists, and can be used to indicate why it has not been fixed yet or to point 
contributors to where the biggest pain points are.

Right now important detections that may hide potential bugs easily 
drown in whitespace or sizeof issues. I found it very useful just to 
do the exercise of creating these suppression lists. 

I first started out fixing issues while the checkers discovered them, 
but after a while I ended up taking a step back and starting over 
again by first creating the "todo list" in the form of a runchecks.cfg file that 
narrowly suppresses everything, then starting to fix issues type by type while
narrowing the exceptions list in the runchecks.cfg file 
this gives IMHO cleaner and more focused commits, and every step would 
pass the 'make C=2' test with no errors.

Thanks,
Knut

> Thanks
> 
> >
> > Thanks,
> > Knut
> >
> > >
> > > Thanks
> > >
> > > > --
> > > > git-series 0.9.1

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Joe Perches]

On Mon, 2017-12-18 at 14:05 +0100, Knut Omang wrote:
> > Here is a list of the checkpatch messages for drivers/infiniband
> > sorted by type.
> > 
> > Many of these might be corrected by using
> > 
> > $ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
> >   $(git ls-files drivers/infiniband/)
> 
> Yes, and I already did that work piece by piece for individual types,
> just to test the runchecks tool, and want to post that set once the 
> runchecks script and Makefile changes itself are in,

I think those are independent of any runcheck tool changes and
could be posted now.  In general, don't keep patches in a local
tree waiting on some other unrelated patch.

Just fyi:

There is a script that helps automate checkpatch "by-type" conversions
with compilation, .o difference checking, and git commit editing.

https://lkml.org/lkml/2014/7/11/794

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Knut Omang]

On Mon, 2017-12-18 at 07:30 -0800, Joe Perches wrote:
> On Mon, 2017-12-18 at 14:05 +0100, Knut Omang wrote:
> > > Here is a list of the checkpatch messages for drivers/infiniband
> > > sorted by type.
> > > 
> > > Many of these might be corrected by using
> > > 
> > > $ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
> > >   $(git ls-files drivers/infiniband/)
> > 
> > Yes, and I already did that work piece by piece for individual types,
> > just to test the runchecks tool, and want to post that set once the 
> > runchecks script and Makefile changes itself are in,
> 
> I think those are independent of any runcheck tool changes and
> could be posted now.  In general, don't keep patches in a local
> tree waiting on some other unrelated patch.

It becomes related in that the runchecks.cfg file is updated 
in all the patches to keep 'make C=2' run with 0 errors while 
enabling more checks. I think they serve well as examples of 
how a workflow with runchecks could be.

> Just fyi:
> 
> There is a script that helps automate checkpatch "by-type" conversions
> with compilation, .o difference checking, and git commit editing.
> 
> https://lkml.org/lkml/2014/7/11/794

oh - good to know - seems it would have been a good help
during my little exercise..

Thanks,
Knut

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Jason Gunthorpe]

On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:

> > Today when we run checkers we get so many warnings it is too hard to
> > make any sense of it.
> 
> Here is a list of the checkpatch messages for drivers/infiniband
> sorted by type.
> 
> Many of these might be corrected by using
> 
> $ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
>   $(git ls-files drivers/infiniband/)

How many of these do you think it is worth to fix?

We do get a steady trickle of changes in this topic every cycle.

Is it better to just do a big number of them all at once? Do you have
an idea how disruptive this kind of work is to the whole patch flow
eg new patches no longer applying to for-next, backports no longer
applying, merge conflicts?

Jason

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Jason Gunthorpe]

On Mon, Dec 18, 2017 at 02:05:15PM +0100, Knut Omang wrote:
 
> https://github.com/torvalds/linux/compare/master...knuto:runchecks

Several of these to rdma/core do not look so big, you should think
about sending them..

Jason

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Joe Perches]

On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> 
> > > Today when we run checkers we get so many warnings it is too hard to
> > > make any sense of it.
> > 
> > Here is a list of the checkpatch messages for drivers/infiniband
> > sorted by type.
> > 
> > Many of these might be corrected by using
> > 
> > $ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
> >   $(git ls-files drivers/infiniband/)
> 
> How many of these do you think it is worth to fix?
> 
> We do get a steady trickle of changes in this topic every cycle.
> 
> Is it better to just do a big number of them all at once?

I think so.

> Do you have
> an idea how disruptive this kind of work is to the whole patch flow
> eg new patches no longer applying to for-next, backports no longer
> applying, merge conflicts?

Some do complain about backport patch purity.

I think that difficulty is overstated, but then
again, I don't do backports very often.

I think the best time for any rather wholesale
change is immediately after an rc-1 so overall
in-flight patch conflict volume is minimized.

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Bart Van Assche]

On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> 
> > > Today when we run checkers we get so many warnings it is too hard to
> > > make any sense of it.
> > 
> > Here is a list of the checkpatch messages for drivers/infiniband
> > sorted by type.
> > 
> > Many of these might be corrected by using
> > 
> > $ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
> >   $(git ls-files drivers/infiniband/)
> 
> How many of these do you think it is worth to fix?
> 
> We do get a steady trickle of changes in this topic every cycle.
> 
> Is it better to just do a big number of them all at once? Do you have
> an idea how disruptive this kind of work is to the whole patch flow
> eg new patches no longer applying to for-next, backports no longer
> applying, merge conflicts?

In my opinion patches that only change the coding style and do not change any
functionality are annoying. Before posting a patch that fixes a bug the change
history (git log -p) has to be cheched to figure out which patch introduced
the bug. Patches that only change coding style pollute the change history.

Bart.

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Knut Omang]

On Mon, 2017-12-18 at 17:56 +0000, Bart Van Assche wrote:
> On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> > On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> > 
> > > > Today when we run checkers we get so many warnings it is too hard to
> > > > make any sense of it.
> > > 
> > > Here is a list of the checkpatch messages for drivers/infiniband
> > > sorted by type.
> > > 
> > > Many of these might be corrected by using
> > > 
> > > $ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
> > >   $(git ls-files drivers/infiniband/)
> > 
> > How many of these do you think it is worth to fix?
> > 
> > We do get a steady trickle of changes in this topic every cycle.
> > 
> > Is it better to just do a big number of them all at once? Do you have
> > an idea how disruptive this kind of work is to the whole patch flow
> > eg new patches no longer applying to for-next, backports no longer
> > applying, merge conflicts?
> 
> In my opinion patches that only change the coding style and do not change any
> functionality are annoying. Before posting a patch that fixes a bug the change
> history (git log -p) has to be cheched to figure out which patch introduced
> the bug. Patches that only change coding style pollute the change history.

I agree with you - the problem is that style issues should not have existed. 
But when they do it becomes a problem to remove them and a problem to 
keep them - for instance us who try to be compliant by having style helpers 
in our editor, we end up having to manually revert old style mistakes back in
to avoid making unrelated whitespace changes or similar.

I think what we get with this patch set is that there's a way to rein in the
issues and to enable some regression testing without enforcing a lot of work 
or annoying history issues *right away*. That way this problem can be tackled when
appropriate for the subsystem in question, and the reason for holding back on 
some changes can be documented in the local runchecks.cfg file, focusing
willing helping hands on the issues considered most important for that 
subsystem.

Thanks,
Knut

Re: [PATCH v2 5/5] RDMA/core: Add runchecks.cfg for drivers/infiniband/core

[Joe Perches]

On Mon, 2017-12-18 at 13:36 +0100, Knut Omang wrote:
> On Mon, 2017-12-18 at 10:02 +0200, Leon Romanovsky wrote:
[]
> > Also, I agree with other reviewers, there is no excuse for adding
> > checkpatch specifics per-subsystem/folder, the differences are better
> > to be treated in checkpatch.pl itself.

What other reviewers are those?

As a checkpatch maintainer, I don't believe it's appropriate
to add many per-subsystem specific rules to checkpatch.

Re: [PATCH v2 5/5] RDMA/core: Add runchecks.cfg for drivers/infiniband/core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Mon, Dec 18, 2017 at 11:03:51AM -0800, Joe Perches wrote:
> On Mon, 2017-12-18 at 13:36 +0100, Knut Omang wrote:
> > On Mon, 2017-12-18 at 10:02 +0200, Leon Romanovsky wrote:
> []
> > > Also, I agree with other reviewers, there is no excuse for adding
> > > checkpatch specifics per-subsystem/folder, the differences are better
> > > to be treated in checkpatch.pl itself.
>
> What other reviewers are those?

I saw responses from Stephen and Jason.

>
> As a checkpatch maintainer, I don't believe it's appropriate
> to add many per-subsystem specific rules to checkpatch.

There are not so much differences, I'm aware of only one specific subsystem - netdev.
The rest of the kernel more or less follows general CodingStyle and
don't need anything specific in checkpatch.pl

Thanks

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

On Mon, Dec 18, 2017 at 07:39:50PM +0100, Knut Omang wrote:
> On Mon, 2017-12-18 at 17:56 +0000, Bart Van Assche wrote:
> > On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> > > On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> > >
> > > > > Today when we run checkers we get so many warnings it is too hard to
> > > > > make any sense of it.
> > > >
> > > > Here is a list of the checkpatch messages for drivers/infiniband
> > > > sorted by type.
> > > >
> > > > Many of these might be corrected by using
> > > >
> > > > $ ./scripts/checkpatch.pl -f --fix-inplace --types=<TYPE> \
> > > >   $(git ls-files drivers/infiniband/)
> > >
> > > How many of these do you think it is worth to fix?
> > >
> > > We do get a steady trickle of changes in this topic every cycle.
> > >
> > > Is it better to just do a big number of them all at once? Do you have
> > > an idea how disruptive this kind of work is to the whole patch flow
> > > eg new patches no longer applying to for-next, backports no longer
> > > applying, merge conflicts?
> >
> > In my opinion patches that only change the coding style and do not change any
> > functionality are annoying. Before posting a patch that fixes a bug the change
> > history (git log -p) has to be cheched to figure out which patch introduced
> > the bug. Patches that only change coding style pollute the change history.
>
> I agree with you - the problem is that style issues should not have existed.
> But when they do it becomes a problem to remove them and a problem to
> keep them - for instance us who try to be compliant by having style helpers
> in our editor, we end up having to manually revert old style mistakes back in
> to avoid making unrelated whitespace changes or similar.

If the checkpatch.pl complains about coding style for the new patch in
newly added code, I'm asking from the author to prepare cleanup patch so
it will be applied before actual patch.

In case, complains are for code which patch are not touching, I'm
submitting it as is.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/5] rds: Add runchecks.cfg for net/rds

[Santosh Shilimkar]

On 12/16/2017 6:02 PM, Knut Omang wrote:
> On Sat, 2017-12-16 at 12:00 -0800, santosh.shilimkar@oracle.com wrote:
>> On 12/16/17 10:24 AM, Joe Perches wrote:

[...]
>>> Most of these existing messages from checkpatch should
>>> probably be inspected and corrected where possible to
>>> minimize the style differences between this subsystem
>>> and the rest of the kernel.
>>>
>>> For instance, here's a trivial patch to substitute
>>> pr_<level> for printks and a couple braces next to
>>> these substitutions.
>>>
>> Thanks Joe. I actually had a similar patch a while back but
>> since it was lot of churn, and code was already merged,
>> never submitted it and then later forgot about it.
>>
>> Will look into it.
> 
> Please look at my set here first - I have already spent considerable time cleaning up
> stuff while working on this:
> 

Just closing the loop. As discussed, I can use your patches without
any new tool dependency since existing checkpatch.pl already gives
those warnings. I started picking up Joes patch but since you have
changes, can use them instead once you untie them with runcheck.

Regarding the $subject, just re-iterating that I don't want any custom
script for RDS and want to just follow generic guidelines followed by
netdev for all net/* code.

Regards,
Santosh