[PATCH 0/6] checkpatch.pl: Add features to help improve U-Boot code

[PATCH 0/6] checkpatch.pl: Add features to help improve U-Boot code

[Simon Glass]

U-Boot mostly follows linux for coding style, but it has some of its own
features. These come up again and again in code review. We should try to
reduce the load of reviewers.

This series adds a little U-Boot function to the checkpatch script and an
option to enable it. Hopefully it is easy enough to maintain this as the
script is updated, even if it is not accepted upstream.

Some initial checks are included as examples of what we can do.


Simon Glass (6):
  checkpatch.pl: Update to v5.7-rc6
  checkpatch.pl: Add a U-Boot option
  checkpatch.pl: Add a check for tests needed for uclasses
  checkpatch.pl: Warn if the flattree API is used
  checkpatch.pl: Request a test when a new command is added
  checkpatch.pl: Request if() instead #ifdef

 .checkpatch.conf      |   3 +
 doc/README.commands   |  50 +++++++++
 scripts/checkpatch.pl | 252 +++++++++++++++---------------------------
 3 files changed, 144 insertions(+), 161 deletions(-)

-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH 1/6] checkpatch.pl: Update to v5.7-rc6

[Simon Glass]

Bring in the newest script. This is close enough to v5.8 that it will
likely be final. Keep the U-Boot changes to $logFunctions

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 scripts/checkpatch.pl | 216 +++++++++++-------------------------------
 1 file changed, 55 insertions(+), 161 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c2641bc995e..cec09fbade8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -61,9 +61,7 @@ my $codespellfile = "/usr/share/codespell/dictionary.txt";
 my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
 my $color = "auto";
-my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
-# git output parsing needs US English output, so first set backtick child process LANGUAGE
-my $git_command ='export LANGUAGE=en_US.UTF-8; git';
+my $allow_c99_comments = 1;
 
 sub help {
 	my ($exitcode) = @_;
@@ -470,19 +468,8 @@ our $logFunctions = qr{(?x:
 	seq_vprintf|seq_printf|seq_puts
 )};
 
-our $allocFunctions = qr{(?x:
-	(?:(?:devm_)?
-		(?:kv|k|v)[czm]alloc(?:_node|_array)? |
-		kstrdup(?:_const)? |
-		kmemdup(?:_nul)?) |
-	(?:\w+)?alloc_skb(?:ip_align)? |
-				# dev_alloc_skb/netdev_alloc_skb, et al
-	dma_alloc_coherent
-)};
-
 our $signature_tags = qr{(?xi:
 	Signed-off-by:|
-	Co-developed-by:|
 	Acked-by:|
 	Tested-by:|
 	Reviewed-by:|
@@ -588,27 +575,6 @@ foreach my $entry (@mode_permission_funcs) {
 }
 $mode_perms_search = "(?:${mode_perms_search})";
 
-our %deprecated_apis = (
-	"synchronize_rcu_bh"			=> "synchronize_rcu",
-	"synchronize_rcu_bh_expedited"		=> "synchronize_rcu_expedited",
-	"call_rcu_bh"				=> "call_rcu",
-	"rcu_barrier_bh"			=> "rcu_barrier",
-	"synchronize_sched"			=> "synchronize_rcu",
-	"synchronize_sched_expedited"		=> "synchronize_rcu_expedited",
-	"call_rcu_sched"			=> "call_rcu",
-	"rcu_barrier_sched"			=> "rcu_barrier",
-	"get_state_synchronize_sched"		=> "get_state_synchronize_rcu",
-	"cond_synchronize_sched"		=> "cond_synchronize_rcu",
-);
-
-#Create a search pattern for all these strings to speed up a loop below
-our $deprecated_apis_search = "";
-foreach my $entry (keys %deprecated_apis) {
-	$deprecated_apis_search .= '|' if ($deprecated_apis_search ne "");
-	$deprecated_apis_search .= $entry;
-}
-$deprecated_apis_search = "(?:${deprecated_apis_search})";
-
 our $mode_perms_world_writable = qr{
 	S_IWUGO		|
 	S_IWOTH		|
@@ -908,7 +874,7 @@ sub seed_camelcase_includes {
 	$camelcase_seeded = 1;
 
 	if (-e ".git") {
-		my $git_last_include_commit = `${git_command} log --no-merges --pretty=format:"%h%n" -1 -- include`;
+		my $git_last_include_commit = `git log --no-merges --pretty=format:"%h%n" -1 -- include`;
 		chomp $git_last_include_commit;
 		$camelcase_cache = ".checkpatch-camelcase.git.$git_last_include_commit";
 	} else {
@@ -936,7 +902,7 @@ sub seed_camelcase_includes {
 	}
 
 	if (-e ".git") {
-		$files = `${git_command} ls-files "include/*.h"`;
+		$files = `git ls-files "include/*.h"`;
 		@include_files = split('\n', $files);
 	}
 
@@ -960,13 +926,13 @@ sub git_commit_info {
 
 	return ($id, $desc) if ((which("git") eq "") || !(-e ".git"));
 
-	my $output = `${git_command} log --no-color --format='%H %s' -1 $commit 2>&1`;
+	my $output = `git log --no-color --format='%H %s' -1 $commit 2>&1`;
 	$output =~ s/^\s*//gm;
 	my @lines = split("\n", $output);
 
 	return ($id, $desc) if ($#lines < 0);
 
-	if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous/) {
+	if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous\./) {
 # Maybe one day convert this block of bash into something that returns
 # all matching commit ids, but it's very slow...
 #
@@ -1010,7 +976,7 @@ if ($git) {
 		} else {
 			$git_range = "-1 $commit_expr";
 		}
-		my $lines = `${git_command} log --no-color --no-merges --pretty=format:'%H %s' $git_range`;
+		my $lines = `git log --no-color --no-merges --pretty=format:'%H %s' $git_range`;
 		foreach my $line (split(/\n/, $lines)) {
 			$line =~ /^([0-9a-fA-F]{40,40}) (.*)$/;
 			next if (!defined($1) || !defined($2));
@@ -1025,7 +991,6 @@ if ($git) {
 }
 
 my $vname;
-$allow_c99_comments = !defined $ignore_type{"C99_COMMENT_TOLERANCE"};
 for my $filename (@ARGV) {
 	my $FILE;
 	if ($git) {
@@ -2691,24 +2656,6 @@ sub process {
 			} else {
 				$signatures{$sig_nospace} = 1;
 			}
-
-# Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
-			if ($sign_off =~ /^co-developed-by:$/i) {
-				if ($email eq $author) {
-					WARN("BAD_SIGN_OFF",
-					      "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
-				}
-				if (!defined $lines[$linenr]) {
-					WARN("BAD_SIGN_OFF",
-                                             "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
-				} elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
-					WARN("BAD_SIGN_OFF",
-					     "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
-				} elsif ($1 ne $email) {
-					WARN("BAD_SIGN_OFF",
-					     "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
-				}
-			}
 		}
 
 # Check email subject for common tools that don't need to be mentioned
@@ -2729,10 +2676,8 @@ sub process {
 		    ($line =~ /^\s*(?:WARNING:|BUG:)/ ||
 		     $line =~ /^\s*\[\s*\d+\.\d{6,6}\s*\]/ ||
 					# timestamp
-		     $line =~ /^\s*\[\<[0-9a-fA-F]{8,}\>\]/) ||
-		     $line =~ /^(?:\s+\w+:\s+[0-9a-fA-F]+){3,3}/ ||
-		     $line =~ /^\s*\#\d+\s*\[[0-9a-fA-F]+\]\s*\w+ at [0-9a-fA-F]+/) {
-					# stack dump address styles
+		     $line =~ /^\s*\[\<[0-9a-fA-F]{8,}\>\]/)) {
+					# stack dump address
 			$commit_log_possible_stack_dump = 1;
 		}
 
@@ -2904,17 +2849,6 @@ sub process {
 			}
 		}
 
-# check for invalid commit id
-		if ($in_commit_log && $line =~ /(^fixes:|\bcommit)\s+([0-9a-f]{6,40})\b/i) {
-			my $id;
-			my $description;
-			($id, $description) = git_commit_info($2, undef, undef);
-			if (!defined($id)) {
-				WARN("UNKNOWN_COMMIT_ID",
-				     "Unknown commit id '$2', maybe rebased or not pulled?\n" . $herecurr);
-			}
-		}
-
 # ignore non-hunk lines and lines being removed
 		next if (!$hunk_line || $line =~ /^-/);
 
@@ -3044,7 +2978,7 @@ sub process {
 			my @compats = $rawline =~ /\"([a-zA-Z0-9\-\,\.\+_]+)\"/g;
 
 			my $dt_path = $root . "/Documentation/devicetree/bindings/";
-			my $vp_file = $dt_path . "vendor-prefixes.yaml";
+			my $vp_file = $dt_path . "vendor-prefixes.txt";
 
 			foreach my $compat (@compats) {
 				my $compat2 = $compat;
@@ -3059,7 +2993,7 @@ sub process {
 
 				next if $compat !~ /^([a-zA-Z0-9\-]+)\,/;
 				my $vendor = $1;
-				`grep -Eq "\\"\\^\Q$vendor\E,\\.\\*\\":" $vp_file`;
+				`grep -Eq "^$vendor\\b" $vp_file`;
 				if ( $? >> 8 ) {
 					WARN("UNDOCUMENTED_DT_STRING",
 					     "DT compatible string vendor \"$vendor\" appears un-documented -- check $vp_file\n" . $herecurr);
@@ -3083,24 +3017,16 @@ sub process {
 					$comment = '..';
 				}
 
-# check SPDX comment style for .[chsS] files
-				if ($realfile =~ /\.[chsS]$/ &&
-				    $rawline =~ /SPDX-License-Identifier:/ &&
-				    $rawline !~ m@^\+\s*\Q$comment\E\s*@) {
-					WARN("SPDX_LICENSE_TAG",
-					     "Improper SPDX comment style for '$realfile', please use '$comment' instead\n" . $herecurr);
-				}
-
 				if ($comment !~ /^$/ &&
-				    $rawline !~ m@^\+\Q$comment\E SPDX-License-Identifier: @) {
-					WARN("SPDX_LICENSE_TAG",
-					     "Missing or malformed SPDX-License-Identifier tag in line $checklicenseline\n" . $herecurr);
+				    $rawline !~ /^\+\Q$comment\E SPDX-License-Identifier: /) {
+					 WARN("SPDX_LICENSE_TAG",
+					      "Missing or malformed SPDX-License-Identifier tag in line $checklicenseline\n" . $herecurr);
 				} elsif ($rawline =~ /(SPDX-License-Identifier: .*)/) {
-					my $spdx_license = $1;
-					if (!is_SPDX_License_valid($spdx_license)) {
-						WARN("SPDX_LICENSE_TAG",
-						     "'$spdx_license' is not supported in LICENSES/...\n" . $herecurr);
-					}
+					 my $spdx_license = $1;
+					 if (!is_SPDX_License_valid($spdx_license)) {
+						  WARN("SPDX_LICENSE_TAG",
+						       "'$spdx_license' is not supported in LICENSES/...\n" . $herecurr);
+					 }
 				}
 			}
 		}
@@ -3108,14 +3034,6 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
 
-# check for using SPDX-License-Identifier on the wrong line number
-		if ($realline != $checklicenseline &&
-		    $rawline =~ /\bSPDX-License-Identifier:/ &&
-		    substr($line, @-, @+ - @-) eq "$;" x (@+ - @-)) {
-			WARN("SPDX_LICENSE_TAG",
-			     "Misplaced SPDX-License-Identifier tag - use line $checklicenseline instead\n" . $herecurr);
-		}
-
 # line length limit (with some exclusions)
 #
 # There are a few types of lines that may extend beyond $max_line_length:
@@ -3953,23 +3871,14 @@ sub process {
 			WARN("STATIC_CONST_CHAR_ARRAY",
 			     "static const char * array should probably be static const char * const\n" .
 				$herecurr);
-		}
-
-# check for initialized const char arrays that should be static const
-		if ($line =~ /^\+\s*const\s+(char|unsigned\s+char|_*u8|(?:[us]_)?int8_t)\s+\w+\s*\[\s*(?:\w+\s*)?\]\s*=\s*"/) {
-			if (WARN("STATIC_CONST_CHAR_ARRAY",
-				 "const array should probably be static const\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/(^.\s*)const\b/${1}static const/;
-			}
-		}
+               }
 
 # check for static char foo[] = "bar" declarations.
 		if ($line =~ /\bstatic\s+char\s+(\w+)\s*\[\s*\]\s*=\s*"/) {
 			WARN("STATIC_CONST_CHAR_ARRAY",
 			     "static char array declaration should probably be static const char\n" .
 				$herecurr);
-		}
+               }
 
 # check for const <foo> const where <foo> is not a pointer or array type
 		if ($sline =~ /\bconst\s+($BasicType)\s+const\b/) {
@@ -4677,7 +4586,7 @@ sub process {
 
 # closing brace should have a space following it when it has anything
 # on the line
-		if ($line =~ /}(?!(?:,|;|\)|\}))\S/) {
+		if ($line =~ /}(?!(?:,|;|\)))\S/) {
 			if (ERROR("SPACING",
 				  "space required after that close brace '}'\n" . $herecurr) &&
 			    $fix) {
@@ -5027,6 +4936,17 @@ sub process {
 		while ($line =~ m{($Constant|$Lval)}g) {
 			my $var = $1;
 
+#gcc binary extension
+			if ($var =~ /^$Binary$/) {
+				if (WARN("GCC_BINARY_CONSTANT",
+					 "Avoid gcc v4.3+ binary constant extension: <$var>\n" . $herecurr) &&
+				    $fix) {
+					my $hexval = sprintf("0x%x", oct($var));
+					$fixed[$fixlinenr] =~
+					    s/\b$var\b/$hexval/;
+				}
+			}
+
 #CamelCase
 			if ($var !~ /^$Constant$/ &&
 			    $var =~ /[A-Z][a-z]|[a-z][A-Z]/ &&
@@ -5208,7 +5128,7 @@ sub process {
 			        next if ($arg =~ /\.\.\./);
 			        next if ($arg =~ /^type$/i);
 				my $tmp_stmt = $define_stmt;
-				$tmp_stmt =~ s/\b(sizeof|typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
+				$tmp_stmt =~ s/\b(typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
 				$tmp_stmt =~ s/\#+\s*$arg\b//g;
 				$tmp_stmt =~ s/\b$arg\s*\#\#//g;
 				my $use_cnt = () = $tmp_stmt =~ /\b$arg\b/g;
@@ -5607,8 +5527,7 @@ sub process {
 			my ($s, $c) = ctx_statement_block($linenr - 3, $realcnt, 0);
 #			print("line: <$line>\nprevline: <$prevline>\ns: <$s>\nc: <$c>\n\n\n");
 
-			if ($s =~ /(?:^|\n)[ \+]\s*(?:$Type\s*)?\Q$testval\E\s*=\s*(?:\([^\)]*\)\s*)?\s*$allocFunctions\s*\(/ &&
-			    $s !~ /\b__GFP_NOWARN\b/ ) {
+			if ($s =~ /(?:^|\n)[ \+]\s*(?:$Type\s*)?\Q$testval\E\s*=\s*(?:\([^\)]*\)\s*)?\s*(?:devm_)?(?:[kv][czm]alloc(?:_node|_array)?\b|kstrdup|kmemdup|(?:dev_)?alloc_skb)/) {
 				WARN("OOM_MESSAGE",
 				     "Possible unnecessary 'out of memory' message\n" . $hereprev);
 			}
@@ -5729,7 +5648,7 @@ sub process {
 			# ignore udelay's < 10, however
 			if (! ($delay < 10) ) {
 				CHK("USLEEP_RANGE",
-				    "usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst\n" . $herecurr);
+				    "usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt\n" . $herecurr);
 			}
 			if ($delay > 2000) {
 				WARN("LONG_UDELAY",
@@ -5741,7 +5660,7 @@ sub process {
 		if ($line =~ /\bmsleep\s*\((\d+)\);/) {
 			if ($1 < 20) {
 				WARN("MSLEEP",
-				     "msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst\n" . $herecurr);
+				     "msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt\n" . $herecurr);
 			}
 		}
 
@@ -5890,18 +5809,6 @@ sub process {
 			     "__aligned(size) is preferred over __attribute__((aligned(size)))\n" . $herecurr);
 		}
 
-# Check for __attribute__ section, prefer __section
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(.*_*section_*\s*\(\s*("[^"]*")/) {
-			my $old = substr($rawline, $-[1], $+[1] - $-[1]);
-			my $new = substr($old, 1, -1);
-			if (WARN("PREFER_SECTION",
-				 "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/;
-			}
-		}
-
 # Check for __attribute__ format(printf, prefer __printf
 		if ($realfile !~ m@\binclude/uapi/@ &&
 		    $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf/) {
@@ -6024,7 +5931,7 @@ sub process {
 				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
 					$specifier = $1;
 					$extension = $2;
-					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
+					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOx]/) {
 						$bad_specifier = $specifier;
 						last;
 					}
@@ -6144,11 +6051,11 @@ sub process {
 			my $max = $7;
 			if ($min eq $max) {
 				WARN("USLEEP_RANGE",
-				     "usleep_range should not use min == max args; see Documentation/timers/timers-howto.rst\n" . "$here\n$stat\n");
+				     "usleep_range should not use min == max args; see Documentation/timers/timers-howto.txt\n" . "$here\n$stat\n");
 			} elsif ($min =~ /^\d+$/ && $max =~ /^\d+$/ &&
 				 $min > $max) {
 				WARN("USLEEP_RANGE",
-				     "usleep_range args reversed, use min then max; see Documentation/timers/timers-howto.rst\n" . "$here\n$stat\n");
+				     "usleep_range args reversed, use min then max; see Documentation/timers/timers-howto.txt\n" . "$here\n$stat\n");
 			}
 		}
 
@@ -6271,8 +6178,8 @@ sub process {
 			}
 		}
 
-# check for pointless casting of alloc functions
-		if ($line =~ /\*\s*\)\s*$allocFunctions\b/) {
+# check for pointless casting of kmalloc return
+		if ($line =~ /\*\s*\)\s*[kv][czm]alloc(_node){0,1}\b/) {
 			WARN("UNNECESSARY_CASTS",
 			     "unnecessary cast may hide bugs, see http://c-faq.com/malloc/mallocnocast.html\n" . $herecurr);
 		}
@@ -6280,7 +6187,7 @@ sub process {
 # alloc style
 # p = alloc(sizeof(struct foo), ...) should be p = alloc(sizeof(*p), ...)
 		if ($perl_version_ok &&
-		    $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc(?:_node)?)\s*\(\s*(sizeof\s*\(\s*struct\s+$Lval\s*\))/) {
+		    $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*([kv][mz]alloc(?:_node)?)\s*\(\s*(sizeof\s*\(\s*struct\s+$Lval\s*\))/) {
 			CHK("ALLOC_SIZEOF_STRUCT",
 			    "Prefer $3(sizeof(*$1)...) over $3($4...)\n" . $herecurr);
 		}
@@ -6443,6 +6350,19 @@ sub process {
 			}
 		}
 
+# check for bool bitfields
+		if ($sline =~ /^.\s+bool\s*$Ident\s*:\s*\d+\s*;/) {
+			WARN("BOOL_BITFIELD",
+			     "Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>\n" . $herecurr);
+		}
+
+# check for bool use in .h files
+		if ($realfile =~ /\.h$/ &&
+		    $sline =~ /^.\s+bool\s*$Ident\s*(?::\s*d+\s*)?;/) {
+			CHK("BOOL_MEMBER",
+			    "Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384\n" . $herecurr);
+		}
+
 # check for semaphores initialized locked
 		if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
 			WARN("CONSIDER_COMPLETION",
@@ -6461,20 +6381,6 @@ sub process {
 			     "please use device_initcall() or more appropriate function instead of __initcall() (see include/linux/init.h)\n" . $herecurr);
 		}
 
-# check for spin_is_locked(), suggest lockdep instead
-		if ($line =~ /\bspin_is_locked\(/) {
-			WARN("USE_LOCKDEP",
-			     "Where possible, use lockdep_assert_held instead of assertions based on spin_is_locked\n" . $herecurr);
-		}
-
-# check for deprecated apis
-		if ($line =~ /\b($deprecated_apis_search)\b\s*\(/) {
-			my $deprecated_api = $1;
-			my $new_api = $deprecated_apis{$deprecated_api};
-			WARN("DEPRECATED_API",
-			     "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr);
-		}
-
 # check for various structs that are normally const (ops, kgdb, device_tree)
 # and avoid what seem like struct definitions 'struct foo {'
 		if ($line !~ /\bconst\b/ &&
@@ -6509,12 +6415,6 @@ sub process {
 			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
 		}
 
-# nested likely/unlikely calls
-		if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
-			WARN("LIKELY_MISUSE",
-			     "nested (un)?likely() calls, $1 already uses unlikely() internally\n" . $herecurr);
-		}
-
 # whine mightly about in_atomic
 		if ($line =~ /\bin_atomic\s*\(/) {
 			if ($realfile =~ m@^drivers/@) {
@@ -6674,12 +6574,6 @@ sub process {
 				     "unknown module license " . $extracted_string . "\n" . $herecurr);
 			}
 		}
-
-# check for sysctl duplicate constants
-		if ($line =~ /\.extra[12]\s*=\s*&(zero|one|int_max)\b/) {
-			WARN("DUPLICATED_SYSCTL_CONST",
-				"duplicated sysctl range checking value '$1', consider using the shared one in include/linux/sysctl.h\n" . $herecurr);
-		}
 	}
 
 	# If we have no input at all, then there is nothing to report on
-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH 1/6] checkpatch.pl: Update to v5.7-rc6

[Tom Rini]

On Fri, May 22, 2020 at 04:32:35PM -0600, Simon Glass wrote:

> Bring in the newest script. This is close enough to v5.8 that it will
> likely be final. Keep the U-Boot changes to $logFunctions
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

I've moved this to v5.7 itself and reworded to match.

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200604/d5da8072/attachment.sig>

[PATCH 2/6] checkpatch.pl: Add a U-Boot option

[Simon Glass]

Add an option to indicate that U-Boot-specific checks should be enabled.
Add a function to house the code that will be added.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 .checkpatch.conf      |  3 +++
 scripts/checkpatch.pl | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/.checkpatch.conf b/.checkpatch.conf
index 95f19635d35..ed0c2150ba8 100644
--- a/.checkpatch.conf
+++ b/.checkpatch.conf
@@ -28,3 +28,6 @@
 
 # A bit shorter of a description is OK with us.
 --min-conf-desc-length=2
+
+# Extra checks for U-Boot
+--u-boot
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cec09fbade8..93863c8f086 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -60,6 +60,7 @@ my $codespell = 0;
 my $codespellfile = "/usr/share/codespell/dictionary.txt";
 my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
+my $u_boot = 0;
 my $color = "auto";
 my $allow_c99_comments = 1;
 
@@ -121,6 +122,7 @@ Options:
   --typedefsfile             Read additional types from this file
   --color[=WHEN]             Use colors 'always', 'never', or only when output
                              is a terminal ('auto'). Default is 'auto'.
+  --u-boot                   Run additional checks for U-Boot
   -h, --help, --version      display this help and exit
 
 When FILE is - read standard input.
@@ -225,6 +227,7 @@ GetOptions(
 	'codespell!'	=> \$codespell,
 	'codespellfile=s'	=> \$codespellfile,
 	'typedefsfile=s'	=> \$typedefsfile,
+	'u-boot'	=> \$u_boot,
 	'color=s'	=> \$color,
 	'no-color'	=> \$color,	#keep old behaviors of -nocolor
 	'nocolor'	=> \$color,	#keep old behaviors of -nocolor
@@ -2235,6 +2238,11 @@ sub pos_last_openparen {
 	return length(expand_tabs(substr($line, 0, $last_openparen))) + 1;
 }
 
+# Checks specific to U-Boot
+sub u_boot_line {
+	my ($realfile, $line,  $herecurr) = @_;
+}
+
 sub process {
 	my $filename = shift;
 
@@ -3102,6 +3110,10 @@ sub process {
 			     "adding a line without newline at end of file\n" . $herecurr);
 		}
 
+		if ($u_boot) {
+			u_boot_line($realfile, $line,  $herecurr);
+		}
+
 # check we are in a valid source file C or perl if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 
-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH 2/6] checkpatch.pl: Add a U-Boot option

[Tom Rini]

On Fri, May 22, 2020 at 04:32:36PM -0600, Simon Glass wrote:

> Add an option to indicate that U-Boot-specific checks should be enabled.
> Add a function to house the code that will be added.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200604/fa7988eb/attachment.sig>

[PATCH 3/6] checkpatch.pl: Add a check for tests needed for uclasses

[Simon Glass]

A common problem when submitting a new uclass is to forget to add sandbox
tests. Add a warning for this.

Of course tests should always be added for new code, but this one seems to
be missed by nearly every new contributor.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93863c8f086..9dc42520e2d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2241,6 +2241,12 @@ sub pos_last_openparen {
 # Checks specific to U-Boot
 sub u_boot_line {
 	my ($realfile, $line,  $herecurr) = @_;
+
+	# ask for a test if a new uclass ID is added
+	if ($realfile =~ /uclass-id.h/ && $line =~ /^\+/) {
+		WARN("NEW_UCLASS",
+		     "Possible new uclass - make sure to add a sandbox driver, plus a test in test/dm/<name>.c\n" . $herecurr);
+	}
 }
 
 sub process {
-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH 3/6] checkpatch.pl: Add a check for tests needed for uclasses

[Tom Rini]

On Fri, May 22, 2020 at 04:32:37PM -0600, Simon Glass wrote:

> A common problem when submitting a new uclass is to forget to add sandbox
> tests. Add a warning for this.
> 
> Of course tests should always be added for new code, but this one seems to
> be missed by nearly every new contributor.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200604/261c3b00/attachment.sig>

[PATCH 4/6] checkpatch.pl: Warn if the flattree API is used

[Simon Glass]

We want people to use the livetree API, so request it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9dc42520e2d..e3a2a289aea 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2247,6 +2247,12 @@ sub u_boot_line {
 		WARN("NEW_UCLASS",
 		     "Possible new uclass - make sure to add a sandbox driver, plus a test in test/dm/<name>.c\n" . $herecurr);
 	}
+
+	# try to get people to use the livetree API
+	if ($line =~ /^\+.*fdtdec_/) {
+		WARN("LIVETREE",
+		     "Use the livetree API (dev_read_...)\n" . $herecurr);
+	}
 }
 
 sub process {
-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH 4/6] checkpatch.pl: Warn if the flattree API is used

[Tom Rini]

On Fri, May 22, 2020 at 04:32:38PM -0600, Simon Glass wrote:

> We want people to use the livetree API, so request it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200604/d8bf9097/attachment.sig>

[PATCH 5/6] checkpatch.pl: Request a test when a new command is added

[Simon Glass]

This request is made with nearly every new command. Point to some docs
on how to do it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 doc/README.commands   | 50 +++++++++++++++++++++++++++++++++++++++++++
 scripts/checkpatch.pl |  6 ++++++
 2 files changed, 56 insertions(+)

diff --git a/doc/README.commands b/doc/README.commands
index 716ad227aa1..229f86d8fb2 100644
--- a/doc/README.commands
+++ b/doc/README.commands
@@ -134,3 +134,53 @@ by writing in u-boot.lds ($(srctree)/board/boardname/u-boot.lds) these
 	.u_boot_list : {
 		KEEP(*(SORT(.u_boot_list*)));
 	}
+
+Writing tests
+-------------
+
+All new commands should have tests. Tests for existing commands are very
+welcome.
+
+It is fairly easy to write a test for a command. Enable it in sandbox, and
+then add code that runs the command and checks the output.
+
+Here is an example:
+
+/* Test 'acpi items' command */
+static int dm_test_acpi_cmd_items(struct unit_test_state *uts)
+{
+	struct acpi_ctx ctx;
+	void *buf;
+
+	buf = malloc(BUF_SIZE);
+	ut_assertnonnull(buf);
+
+	ctx.current = buf;
+	ut_assertok(acpi_fill_ssdt(&ctx));
+	console_record_reset();
+	run_command("acpi items", 0);
+	ut_assert_nextline("dev 'acpi-test', type 1, size 2");
+	ut_assert_nextline("dev 'acpi-test2', type 1, size 2");
+	ut_assert_console_end();
+
+	ctx.current = buf;
+	ut_assertok(acpi_inject_dsdt(&ctx));
+	console_record_reset();
+	run_command("acpi items", 0);
+	ut_assert_nextline("dev 'acpi-test', type 2, size 2");
+	ut_assert_nextline("dev 'acpi-test2', type 2, size 2");
+	ut_assert_console_end();
+
+	console_record_reset();
+	run_command("acpi items -d", 0);
+	ut_assert_nextline("dev 'acpi-test', type 2, size 2");
+	ut_assert_nextlines_are_dump(2);
+	ut_assert_nextline("%s", "");
+	ut_assert_nextline("dev 'acpi-test2', type 2, size 2");
+	ut_assert_nextlines_are_dump(2);
+	ut_assert_nextline("%s", "");
+	ut_assert_console_end();
+
+	return 0;
+}
+DM_TEST(dm_test_acpi_cmd_items, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3a2a289aea..22869992e90 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2253,6 +2253,12 @@ sub u_boot_line {
 		WARN("LIVETREE",
 		     "Use the livetree API (dev_read_...)\n" . $herecurr);
 	}
+
+	# add tests for new commands
+	if ($line =~ /^\+.*do_($Ident)\(struct cmd_tbl.*/) {
+		WARN("CMD_TEST",
+		     "Possible new command - make sure you add a test\n" . $herecurr);
+	}
 }
 
 sub process {
-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH 5/6] checkpatch.pl: Request a test when a new command is added

[Tom Rini]

On Fri, May 22, 2020 at 04:32:39PM -0600, Simon Glass wrote:

> This request is made with nearly every new command. Point to some docs
> on how to do it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200604/33d14039/attachment.sig>

[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[Simon Glass]

There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
suggest using the compile-time construct.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 22869992e90..33ec4e2bfd4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2259,6 +2259,12 @@ sub u_boot_line {
 		WARN("CMD_TEST",
 		     "Possible new command - make sure you add a test\n" . $herecurr);
 	}
+
+	# use if instead of #if
+	if ($line =~ /^\+#if.*CONFIG.*/) {
+		WARN("PREFER_IF",
+		     "Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible\n" . $herecurr);
+	}
 }
 
 sub process {
-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[Tom Rini]

On Fri, May 22, 2020 at 04:32:40PM -0600, Simon Glass wrote:

> There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
> suggest using the compile-time construct.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200604/39110e11/attachment.sig>

[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[AKASHI Takahiro]

On Thu, Jun 04, 2020 at 07:39:35PM -0400, Tom Rini wrote:
> On Fri, May 22, 2020 at 04:32:40PM -0600, Simon Glass wrote:
> 
> > There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
> > suggest using the compile-time construct.
> > 
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> 
> Applied to u-boot/master, thanks!

This check is simple, but IMHO, too simple.
It will generate false-positive, or pointless, warnings
for some common use cases. Say,

In an include header,
#ifdef CONFIG_xxx
extern int foo_data;
int foo(void);
#endif

Or in a C file (foo_common.c),
#ifdef CONFIG_xxx_a
int foo_a(void)
...
#endif
#ifdef CONFIG_xxx_b
int foo_b(void)
...
#endif

Or,

struct baa baa_list[] = {
#ifdef CONFIG_xxx
        data_xxx,
#endif
...

They are harmless and can be ignored, but also annoying.
Can you sophisticate this check?

In addition, if I want to stick to this rule, there can co-exist
an "old" style and "new" style of code in a single file.
(particularly tons of examples in UEFI subsystem)

How should we deal with this?

Thanks,
-Takahiro Akashi

> -- 
> Tom

[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[Simon Glass]

Hi Akashi,

On Sun, 14 Jun 2020 at 20:59, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Thu, Jun 04, 2020 at 07:39:35PM -0400, Tom Rini wrote:
> > On Fri, May 22, 2020 at 04:32:40PM -0600, Simon Glass wrote:
> >
> > > There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
> > > suggest using the compile-time construct.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Applied to u-boot/master, thanks!
>
> This check is simple, but IMHO, too simple.
> It will generate false-positive, or pointless, warnings
> for some common use cases. Say,
>
> In an include header,
> #ifdef CONFIG_xxx
> extern int foo_data;
> int foo(void);
> #endif

We should try to avoid this in header files. But I sent a patch
earlier today to turn off the check for header files and device tree.

>
> Or in a C file (foo_common.c),
> #ifdef CONFIG_xxx_a
> int foo_a(void)
> ...
> #endif
> #ifdef CONFIG_xxx_b
> int foo_b(void)
> ...
> #endif
>

Perhaps the if() could be inside those functions in some cases?

> Or,
>
> struct baa baa_list[] = {
> #ifdef CONFIG_xxx
>         data_xxx,
> #endif

I'm not sure how to handle this one.

> ...
>
> They are harmless and can be ignored, but also annoying.
> Can you sophisticate this check?

Yes I agree we should avoid false negatives. It is better not to have
a check than have one that is unreliable.

>
> In addition, if I want to stick to this rule, there can co-exist
> an "old" style and "new" style of code in a single file.
> (particularly tons of examples in UEFI subsystem)
>
> How should we deal with this?

Convert it?

>
> Thanks,
> -Takahiro Akashi

Regards,
Simon

[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[Tom Rini]

On Sun, Jun 14, 2020 at 09:58:48PM -0600, Simon Glass wrote:
> Hi Akashi,
> 
> On Sun, 14 Jun 2020 at 20:59, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > On Thu, Jun 04, 2020 at 07:39:35PM -0400, Tom Rini wrote:
> > > On Fri, May 22, 2020 at 04:32:40PM -0600, Simon Glass wrote:
> > >
> > > > There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
> > > > suggest using the compile-time construct.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > >
> > > Applied to u-boot/master, thanks!
> >
> > This check is simple, but IMHO, too simple.
> > It will generate false-positive, or pointless, warnings
> > for some common use cases. Say,
> >
> > In an include header,
> > #ifdef CONFIG_xxx
> > extern int foo_data;
> > int foo(void);
> > #endif
> 
> We should try to avoid this in header files. But I sent a patch
> earlier today to turn off the check for header files and device tree.

Right, in a header that's a bad idea unless it's:
...
#else
static inline foo(void) {}
#endif

> > Or in a C file (foo_common.c),
> > #ifdef CONFIG_xxx_a
> > int foo_a(void)
> > ...
> > #endif
> > #ifdef CONFIG_xxx_b
> > int foo_b(void)
> > ...
> > #endif
> >
> 
> Perhaps the if() could be inside those functions in some cases?

Yeah, that's less clearly an example of something bad.

> > Or,
> >
> > struct baa baa_list[] = {
> > #ifdef CONFIG_xxx
> >         data_xxx,
> > #endif
> 
> I'm not sure how to handle this one.

Rasmus' series to add CONFIG_IS_ENABLED(SYM, true, false) stuff would be
handy here.

> > ...
> >
> > They are harmless and can be ignored, but also annoying.
> > Can you sophisticate this check?
> 
> Yes I agree we should avoid false negatives. It is better not to have
> a check than have one that is unreliable.
> 
> >
> > In addition, if I want to stick to this rule, there can co-exist
> > an "old" style and "new" style of code in a single file.
> > (particularly tons of examples in UEFI subsystem)
> >
> > How should we deal with this?
> 
> Convert it?

Yes, code should be cleaned up and converted to using if (...) when
possible.  That we have new code that doesn't make use of this is
because we didn't have tooling warning about when it wasn't used.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200615/d7a1d3dc/attachment.sig>

[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[AKASHI Takahiro]

On Mon, Jun 15, 2020 at 10:34:56AM -0400, Tom Rini wrote:
> On Sun, Jun 14, 2020 at 09:58:48PM -0600, Simon Glass wrote:
> > Hi Akashi,
> > 
> > On Sun, 14 Jun 2020 at 20:59, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Thu, Jun 04, 2020 at 07:39:35PM -0400, Tom Rini wrote:
> > > > On Fri, May 22, 2020 at 04:32:40PM -0600, Simon Glass wrote:
> > > >
> > > > > There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
> > > > > suggest using the compile-time construct.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > Applied to u-boot/master, thanks!
> > >
> > > This check is simple, but IMHO, too simple.
> > > It will generate false-positive, or pointless, warnings
> > > for some common use cases. Say,
> > >
> > > In an include header,
> > > #ifdef CONFIG_xxx
> > > extern int foo_data;
> > > int foo(void);
> > > #endif
> > 
> > We should try to avoid this in header files. But I sent a patch
> > earlier today to turn off the check for header files and device tree.
> 
> Right, in a header that's a bad idea unless it's:

I'm not sure that it is a so bad idea; I think that it will
detect some configuration error immediately rather than
at the link time.

> ...
> #else
> static inline foo(void) {}
> #endif

Well, in this case, a corresponding C file often has a definition like:
#ifdef CONFIG_xxx
int foo(void) {
    ...
}
#endif

> > > Or in a C file (foo_common.c),
> > > #ifdef CONFIG_xxx_a
> > > int foo_a(void)
> > > ...
> > > #endif
> > > #ifdef CONFIG_xxx_b
> > > int foo_b(void)
> > > ...
> > > #endif
> > >
> > 
> > Perhaps the if() could be inside those functions in some cases?
> 
> Yeah, that's less clearly an example of something bad.

Again, I'm not sure that it is a bad idea. Such a use can be
seen quite often in library code where there are many configurable
options.
The only way to avoid such a style of coding is that we would
put each function into a separate C file even if it can be
very small. It also requires more (common/helper) functions, which are
essentially local to that library, to be declared as global.

Is this what you want?

> > > Or,
> > >
> > > struct baa baa_list[] = {
> > > #ifdef CONFIG_xxx
> > >         data_xxx,
> > > #endif
> > 
> > I'm not sure how to handle this one.
> 
> Rasmus' series to add CONFIG_IS_ENABLED(SYM, true, false) stuff would be
> handy here.

Ah, I didn't notice that. We can now have the code like:
struct baa baa_list[] = {
    ...
    CONFIG_IS_ENABLED(xxx, (data_xxx,))
    ...
}

## I think the comma after 'data_xxx' is required, isn't it?

But what is the merit?

And, data_xxx itself has to be declared anyway like:
#ifdef CONFIG_xxx
struct baa data_xxx = {
    ...
};
#endif

> > > ...
> > >
> > > They are harmless and can be ignored, but also annoying.
> > > Can you sophisticate this check?
> > 
> > Yes I agree we should avoid false negatives. It is better not to have
> > a check than have one that is unreliable.
> > 
> > >
> > > In addition, if I want to stick to this rule, there can co-exist
> > > an "old" style and "new" style of code in a single file.
> > > (particularly tons of examples in UEFI subsystem)
> > >
> > > How should we deal with this?
> > 
> > Convert it?
> 
> Yes, code should be cleaned up and converted to using if (...) when
> possible.  That we have new code that doesn't make use of this is
> because we didn't have tooling warning about when it wasn't used.

So if we want to add a new commit that complies with this rule while
the file to which it will be applied has an old style of code,
do you *require* that this existing file should be converted first
in any case?

-Takahiro Akashi


> -- 
> Tom

[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[Tom Rini]

On Tue, Jun 16, 2020 at 09:34:30AM +0900, AKASHI Takahiro wrote:
> On Mon, Jun 15, 2020 at 10:34:56AM -0400, Tom Rini wrote:
> > On Sun, Jun 14, 2020 at 09:58:48PM -0600, Simon Glass wrote:
> > > Hi Akashi,
> > > 
> > > On Sun, 14 Jun 2020 at 20:59, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Thu, Jun 04, 2020 at 07:39:35PM -0400, Tom Rini wrote:
> > > > > On Fri, May 22, 2020 at 04:32:40PM -0600, Simon Glass wrote:
> > > > >
> > > > > > There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
> > > > > > suggest using the compile-time construct.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > Applied to u-boot/master, thanks!
> > > >
> > > > This check is simple, but IMHO, too simple.
> > > > It will generate false-positive, or pointless, warnings
> > > > for some common use cases. Say,
> > > >
> > > > In an include header,
> > > > #ifdef CONFIG_xxx
> > > > extern int foo_data;
> > > > int foo(void);
> > > > #endif
> > > 
> > > We should try to avoid this in header files. But I sent a patch
> > > earlier today to turn off the check for header files and device tree.
> > 
> > Right, in a header that's a bad idea unless it's:
> 
> I'm not sure that it is a so bad idea; I think that it will
> detect some configuration error immediately rather than
> at the link time.

We prefer link time failures as -Werror is not the default in a regular
build.

> > ...
> > #else
> > static inline foo(void) {}
> > #endif
> 
> Well, in this case, a corresponding C file often has a definition like:
> #ifdef CONFIG_xxx
> int foo(void) {
>     ...
> }
> #endif

Right, and that's fine.  But headers do not get guards around functions
unless it's else-inline-to-nop-it-out.

> > > > Or in a C file (foo_common.c),
> > > > #ifdef CONFIG_xxx_a
> > > > int foo_a(void)
> > > > ...
> > > > #endif
> > > > #ifdef CONFIG_xxx_b
> > > > int foo_b(void)
> > > > ...
> > > > #endif
> > > >
> > > 
> > > Perhaps the if() could be inside those functions in some cases?
> > 
> > Yeah, that's less clearly an example of something bad.
> 
> Again, I'm not sure that it is a bad idea. Such a use can be
> seen quite often in library code where there are many configurable
> options.
> The only way to avoid such a style of coding is that we would
> put each function into a separate C file even if it can be
> very small. It also requires more (common/helper) functions, which are
> essentially local to that library, to be declared as global.
> 
> Is this what you want?

It comes down to what the code reads best as, yes.  A checkpatch error
isn't a fatal you must fix it error.  But you must be able to explain
why it's wrong.  And I think we're getting away from the main point
here.  Generally, #ifdef CONFIG_FOO .... #endif, in a function is ugly
and we can do better.  It also means better code analysis as I believe
some tools will still evaluate if (0) { ... } but will not evaluate #if
0 ... #endif.

> > > > Or,
> > > >
> > > > struct baa baa_list[] = {
> > > > #ifdef CONFIG_xxx
> > > >         data_xxx,
> > > > #endif
> > > 
> > > I'm not sure how to handle this one.
> > 
> > Rasmus' series to add CONFIG_IS_ENABLED(SYM, true, false) stuff would be
> > handy here.
> 
> Ah, I didn't notice that. We can now have the code like:
> struct baa baa_list[] = {
>     ...
>     CONFIG_IS_ENABLED(xxx, (data_xxx,))
>     ...
> }
> 
> ## I think the comma after 'data_xxx' is required, isn't it?
> 
> But what is the merit?
> 
> And, data_xxx itself has to be declared anyway like:
> #ifdef CONFIG_xxx
> struct baa data_xxx = {
>     ...
> };
> #endif

We _could_ have that yes, he's posted an RFC I need to reply to
directly.  As you would probably also need a __maybe_unused on the
struct itself.  Is that better?

> > > > ...
> > > >
> > > > They are harmless and can be ignored, but also annoying.
> > > > Can you sophisticate this check?
> > > 
> > > Yes I agree we should avoid false negatives. It is better not to have
> > > a check than have one that is unreliable.
> > > 
> > > >
> > > > In addition, if I want to stick to this rule, there can co-exist
> > > > an "old" style and "new" style of code in a single file.
> > > > (particularly tons of examples in UEFI subsystem)
> > > >
> > > > How should we deal with this?
> > > 
> > > Convert it?
> > 
> > Yes, code should be cleaned up and converted to using if (...) when
> > possible.  That we have new code that doesn't make use of this is
> > because we didn't have tooling warning about when it wasn't used.
> 
> So if we want to add a new commit that complies with this rule while
> the file to which it will be applied has an old style of code,
> do you *require* that this existing file should be converted first
> in any case?

I honestly don't know.  Is it a problem to look over the code and make
use of if (IS_ENABLED(...)) { ... } when it would make the code read
better and get better analysis?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200615/4232080d/attachment.sig>

[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[Simon Glass]

Hi Akashi,

On Mon, 15 Jun 2020 at 18:34, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Mon, Jun 15, 2020 at 10:34:56AM -0400, Tom Rini wrote:
> > On Sun, Jun 14, 2020 at 09:58:48PM -0600, Simon Glass wrote:
> > > Hi Akashi,
> > >
> > > On Sun, 14 Jun 2020 at 20:59, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Thu, Jun 04, 2020 at 07:39:35PM -0400, Tom Rini wrote:
> > > > > On Fri, May 22, 2020 at 04:32:40PM -0600, Simon Glass wrote:
> > > > >
> > > > > > There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
> > > > > > suggest using the compile-time construct.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > Applied to u-boot/master, thanks!
> > > >
> > > > This check is simple, but IMHO, too simple.
> > > > It will generate false-positive, or pointless, warnings
> > > > for some common use cases. Say,
> > > >
> > > > In an include header,
> > > > #ifdef CONFIG_xxx
> > > > extern int foo_data;
> > > > int foo(void);
> > > > #endif
> > >
> > > We should try to avoid this in header files. But I sent a patch
> > > earlier today to turn off the check for header files and device tree.
> >
> > Right, in a header that's a bad idea unless it's:
>
> I'm not sure that it is a so bad idea; I think that it will
> detect some configuration error immediately rather than
> at the link time.

We have to give up something here. The link error normally reports
where the function was called from. By moving to relying on the
toolchain to sort out what is in the image and what is not, we should
try to do it fully, in my view. A halfway house where we still use
lots of #ifdefs seems seal-defeating.

>
> > ...
> > #else
> > static inline foo(void) {}
> > #endif
>
> Well, in this case, a corresponding C file often has a definition like:
> #ifdef CONFIG_xxx
> int foo(void) {
>     ...
> }
> #endif

Yes indeed. Tricky.

>
> > > > Or in a C file (foo_common.c),
> > > > #ifdef CONFIG_xxx_a
> > > > int foo_a(void)
> > > > ...
> > > > #endif
> > > > #ifdef CONFIG_xxx_b
> > > > int foo_b(void)
> > > > ...
> > > > #endif
> > > >
> > >
> > > Perhaps the if() could be inside those functions in some cases?
> >
> > Yeah, that's less clearly an example of something bad.
>
> Again, I'm not sure that it is a bad idea. Such a use can be
> seen quite often in library code where there are many configurable
> options.
> The only way to avoid such a style of coding is that we would
> put each function into a separate C file even if it can be
> very small. It also requires more (common/helper) functions, which are
> essentially local to that library, to be declared as global.
>
> Is this what you want?

The compiler puts each function into a separate section and the linker
throws away what is not used. So I don't think adding an #ifdef around
an exported function server a purpose except in a small proportion of
cases.

>
> > > > Or,
> > > >
> > > > struct baa baa_list[] = {
> > > > #ifdef CONFIG_xxx
> > > >         data_xxx,
> > > > #endif
> > >
> > > I'm not sure how to handle this one.
> >
> > Rasmus' series to add CONFIG_IS_ENABLED(SYM, true, false) stuff would be
> > handy here.
>
> Ah, I didn't notice that. We can now have the code like:
> struct baa baa_list[] = {
>     ...
>     CONFIG_IS_ENABLED(xxx, (data_xxx,))
>     ...
> }
>
> ## I think the comma after 'data_xxx' is required, isn't it?
>
> But what is the merit?
>
> And, data_xxx itself has to be declared anyway like:
> #ifdef CONFIG_xxx
> struct baa data_xxx = {
>     ...
> };
> #endif
>
> > > > ...
> > > >
> > > > They are harmless and can be ignored, but also annoying.
> > > > Can you sophisticate this check?
> > >
> > > Yes I agree we should avoid false negatives. It is better not to have
> > > a check than have one that is unreliable.
> > >
> > > >
> > > > In addition, if I want to stick to this rule, there can co-exist
> > > > an "old" style and "new" style of code in a single file.
> > > > (particularly tons of examples in UEFI subsystem)
> > > >
> > > > How should we deal with this?
> > >
> > > Convert it?
> >
> > Yes, code should be cleaned up and converted to using if (...) when
> > possible.  That we have new code that doesn't make use of this is
> > because we didn't have tooling warning about when it wasn't used.
>
> So if we want to add a new commit that complies with this rule while
> the file to which it will be applied has an old style of code,
> do you *require* that this existing file should be converted first
> in any case?

We don't require people to fix up old code style before submitting a
patch to a file, although checkpatch makes you do it for nearby code.
But I think we should ask contributors to help.

Regards,
Simon

[PATCH 0/6] checkpatch.pl: Add features to help improve U-Boot code

[Tom Rini]

On Fri, May 22, 2020 at 04:32:34PM -0600, Simon Glass wrote:

> U-Boot mostly follows linux for coding style, but it has some of its own
> features. These come up again and again in code review. We should try to
> reduce the load of reviewers.
> 
> This series adds a little U-Boot function to the checkpatch script and an
> option to enable it. Hopefully it is easy enough to maintain this as the
> script is updated, even if it is not accepted upstream.
> 
> Some initial checks are included as examples of what we can do.
> 
> 
> Simon Glass (6):
>   checkpatch.pl: Update to v5.7-rc6
>   checkpatch.pl: Add a U-Boot option
>   checkpatch.pl: Add a check for tests needed for uclasses
>   checkpatch.pl: Warn if the flattree API is used
>   checkpatch.pl: Request a test when a new command is added
>   checkpatch.pl: Request if() instead #ifdef
> 
>  .checkpatch.conf      |   3 +
>  doc/README.commands   |  50 +++++++++
>  scripts/checkpatch.pl | 252 +++++++++++++++---------------------------
>  3 files changed, 144 insertions(+), 161 deletions(-)

I'm not reviewing the changes as I am not a pearl person but I think
this is a great idea and will help keep us from dropping our changes in
the future too.  I'll rebase my CONFIG_CMD_xxx check on top of your
series before applying.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200524/98584393/attachment.sig>

[PATCHv2] checkpatch.pl: Add check for defining CONFIG_CMD_xxx via config files

[Tom Rini]

All of our cmds have a Kconfig entry.  Making enabling a CMD via the
config file an error to checkpatch.pl.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Changes in v2:
- Rebase on Simon's update that adds a u-boot section
- Catch undef as well
- Have a more generic message
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 33ec4e2bfd44..cabb072a0de9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2265,6 +2265,12 @@ sub u_boot_line {
 		WARN("PREFER_IF",
 		     "Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible\n" . $herecurr);
 	}
+
+	# use defconfig to manage CONFIG_CMD options
+	if ($line =~ /\+\s*#\s*(define|undef)\s+(CONFIG_CMD\w*)\b/) {
+		ERROR("DEFINE_CONFIG_CMD",
+		      "All commands are managed by Kconfig\n" . $herecurr);
+	}
 }
 
 sub process {
-- 
2.17.1

[PATCHv2] checkpatch.pl: Add check for defining CONFIG_CMD_xxx via config files

[Tom Rini]

On Tue, May 26, 2020 at 02:29:02PM -0400, Tom Rini wrote:

> All of our cmds have a Kconfig entry.  Making enabling a CMD via the
> config file an error to checkpatch.pl.
> 
> Signed-off-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200604/a5dfb92b/attachment.sig>