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

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

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 <redacted>
Reviewed-by: Håkon Bugge <redacted>
Reviewed-by: Åsmund Østvold <redacted>
---
 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

On Sat, 16 Dec 2017 15:42:29 +0100
Knut Omang [off-list ref] wrote:

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

On Sat, 16 Dec 2017 15:42:25 +0100
Knut Omang [off-list ref] 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, 2017-12-16 at 09:47 -0800, Stephen Hemminger wrote:

On Sat, 16 Dec 2017 15:42:25 +0100
Knut Omang [off-list ref] wrote:

quoted

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

On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:

On Sat, 16 Dec 2017 15:42:29 +0100 Knut Omang [off-list ref] wrote:

quoted

+# 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);
 }

On 12/16/17 10:24 AM, Joe Perches wrote:

On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:

quoted

On Sat, 16 Dec 2017 15:42:29 +0100 Knut Omang [off-list ref] wrote:

quoted

+# 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

On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:

On Sat, 16 Dec 2017 15:42:29 +0100
Knut Omang [off-list ref] wrote:

quoted

+
+# 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

quoted

+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

quoted

+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

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

On Sat, 2017-12-16 at 12:00 -0800, santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org wrote:

On 12/16/17 10:24 AM, Joe Perches wrote:

quoted

On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:

quoted

On Sat, 16 Dec 2017 15:42:29 +0100 Knut Omang [off-list ref] wrote:

quoted

+# 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

quoted

quoted

quoted

+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!

quoted

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

quoted

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

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

On Sat, 2017-12-16 at 10:24 -0800, Joe Perches wrote:

On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:

quoted

On Sat, 16 Dec 2017 15:42:29 +0100 Knut Omang [off-list ref] wrote:

quoted

+# 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

quoted

quoted

+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?

On Sat, 2017-12-16 at 09:47 -0800, Stephen Hemminger wrote:

On Sat, 16 Dec 2017 15:42:25 +0100
Knut Omang [off-list ref] wrote:

quoted

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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

quoted

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...

quoted

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

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:

quoted

quoted

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

On Sun, 2017-12-17 at 22:00 -0800, Joe Perches wrote:

On Sun, 2017-12-17 at 22:00 -0700, Jason Gunthorpe wrote:

quoted

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

quoted

quoted

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

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:

quoted

quoted

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...

quoted

quoted

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

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

On Mon, 2017-12-18 at 14:05 +0100, Knut Omang wrote:

quoted

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

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

On Mon, 2017-12-18 at 07:30 -0800, Joe Perches wrote:

On Mon, 2017-12-18 at 14:05 +0100, Knut Omang wrote:

quoted

quoted

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

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

quoted

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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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:

quoted

quoted

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.

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:

quoted

quoted

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.

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:

quoted

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

quoted

quoted

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

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:

quoted

On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:

quoted

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

quoted

quoted

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

On 12/16/2017 6:02 PM, Knut Omang wrote:

On Sat, 2017-12-16 at 12:00 -0800, santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org wrote:

quoted

On 12/16/17 10:24 AM, Joe Perches wrote:

[...]

quoted

quoted

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

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