[PATCH v2 0/2] checkpatch: fixes for vendor compatible check

[PATCH v2 0/2] checkpatch: fixes for vendor compatible check

[Florian Vaussard]

Hi,

Since v1 [1]:
- Add check for vendors with '-', as suggested by Joe Perches

Regards,
Florian

[1] http://thread.gmane.org/gmane.linux.drivers.devicetree/63770


Florian Vaussard (2):
  checkpatch: check vendor compatible with dashes
  checkpatch: fix spurious vendor compatible warnings

 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
1.8.5.3

[PATCH v2 1/2] checkpatch: check vendor compatible with dashes

[Florian Vaussard]

The current vendor compatible check will not match vendors with
dashes, like:

compatible="asahi-kasei"

Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 464dcef..e304e77 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2058,7 +2058,7 @@ sub process {
 				my $vendor = $compat;
 				my $vendor_path = $dt_path . "vendor-prefixes.txt";
 				next if (! -f $vendor_path);
-				$vendor =~ s/^([a-zA-Z0-9]+)\,.*/$1/;
+				$vendor =~ s/^([a-zA-Z0-9\-]+)\,.*/$1/;
 				`grep -Eq "$vendor" $vendor_path`;
 				if ( $? >> 8 ) {
 					WARN("UNDOCUMENTED_DT_STRING",
-- 
1.8.5.3

[PATCH v2 2/2] checkpatch: fix spurious vendor compatible warnings

[Florian Vaussard]

With a compatible string like

compatible = "foo";

checkpatch will currently try to find "foo" in  vendor-prefixes.txt,
which is wrong since the vendor prefix is empty in this specific case.

Skip the vendor test if the compatible is not like

compatible = "vendor,something";

Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
---
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e304e77..7437505 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2058,6 +2058,7 @@ sub process {
 				my $vendor = $compat;
 				my $vendor_path = $dt_path . "vendor-prefixes.txt";
 				next if (! -f $vendor_path);
+				next if not $vendor =~ /^[a-zA-Z0-9\-]+\,.*/;
 				$vendor =~ s/^([a-zA-Z0-9\-]+)\,.*/$1/;
 				`grep -Eq "$vendor" $vendor_path`;
 				if ( $? >> 8 ) {
-- 
1.8.5.3

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

Re: [PATCH v2 2/2] checkpatch: fix spurious vendor compatible warnings

[Joe Perches]

Hi.

A couple of suggestions and a couple of questions.

I made the patch below against your patches to.

o Look for ".compatible = "foo" strings in .c and .h files too
o Improve the vendor name match in vendor-prefix.txt by only
  matching the exact vendor name at the beginning of lines.

I then produced a file of all the compatible uses in dts
and used checkpatch on it.  It's a long list and a longer
checkpatch warning list.

$ git ls-files | grep -E "\.dtsi?$" | \
  xargs grep -hE "^\s*compatible\s*="| \
  sed -r -e 's/^\s*//' -e 's/\s*=\s*/ = /'| sort | uniq > tmp.dts
$ .scripts/checkpatch.pl -f tmp.dts

A couple of questions:

How does the $compat2 variable actually work?
What is it supposed to do?

				my $compat2 = $compat;
				$compat2 =~ s/\,[a-z]*\-/\,<\.\*>\-/;

For instance:
WARNING: DT compatible string "marvell,tauros2-cache" appears un-documented -- check ./Documentation/devicetree/bindings/

The prefix "tauros2-" doesn't match '=~ s/[a-z]*-/

Should it?  Should the '[a-z]*' be '[a-zA-Z0-9-]+'
like the other test?

Also the grep used when $compat2 is different than $compat
has <.*>

There aren't any descriptions I see in binding/ that have
any <foo> like uses with the angle brackets.

Are the angle brackets "<" and ">" necessary?

I think the code block should look more like:

				my $compat2 = $compat;
				$compat2 =~ s/\,[a-zA-Z0-9]*\-/\,\.\*\-/g;
				my $grepfor = "\Q$compat\E";
				$grepfor .= "|\Q$compat2\E" if ($compat2 ne $compat);
				`grep -Erq "$grepfor" $dt_path`;

so that there's only 1 grep pattern when
$compat2 is the same as $compat and the
strings are escape quoted.

There are a _lot_ of missing entries:

o 164 vendor names
o 2408 device names (maybe due to bad compat2 greps?)

What, if anything, should be done about them?

---
 scripts/checkpatch.pl | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4b5e7b3..7a9eed9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2051,29 +2051,29 @@ sub process {
 		}
 
 # check for DT compatible documentation
-		if (defined $root && $realfile =~ /\.dts/ &&
-		    $rawline =~ /^\+\s*compatible\s*=/) {
+		if (defined $root &&
+		    (($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*compatible\s*=\s*\"/) ||
+		     ($realfile =~ /\.[ch]$/ && $line =~ /^\+.*\.compatible\s*=\s*\"/))) {
+
 			my @compats = $rawline =~ /\"([a-zA-Z0-9\-\,\.\+_]+)\"/g;
 
+			my $dt_path = $root . "/Documentation/devicetree/bindings/";
+			my $vp_file = $root . "/Documentation/devicetree/bindings/vendor-prefixes.txt";
+
 			foreach my $compat (@compats) {
 				my $compat2 = $compat;
-				my $dt_path =  $root . "/Documentation/devicetree/bindings/";
 				$compat2 =~ s/\,[a-z]*\-/\,<\.\*>\-/;
 				`grep -Erq "$compat|$compat2" $dt_path`;
 				if ( $? >> 8 ) {
 					WARN("UNDOCUMENTED_DT_STRING",
 					     "DT compatible string \"$compat\" appears un-documented -- check $dt_path\n" . $herecurr);
 				}
-
-				my $vendor = $compat;
-				my $vendor_path = $dt_path . "vendor-prefixes.txt";
-				next if (! -f $vendor_path);
-				next if not $vendor =~ /^[a-zA-Z0-9\-]+\,.*/;
-				$vendor =~ s/^([a-zA-Z0-9\-]+)\,.*/$1/;
-				`grep -Eq "$vendor" $vendor_path`;
+				next if $compat !~ /^([a-zA-Z0-9\-]+)\,/;
+				my $vendor = $1;
+				`grep -Eq "^$vendor\\b" $vp_file`;
 				if ( $? >> 8 ) {
 					WARN("UNDOCUMENTED_DT_STRING",
-					     "DT compatible string vendor \"$vendor\" appears un-documented -- check $vendor_path\n" . $herecurr);
+					     "DT compatible string vendor \"$vendor\" appears un-documented -- check $vp_file\n" . $herecurr);
 				}
 			}
 		}
---

Re: [PATCH v2 2/2] checkpatch: fix spurious vendor compatible warnings

[Florian Vaussard]

Hi,

On 02/28/2014 10:06 PM, Joe Perches wrote:
> Hi.
> 
> A couple of suggestions and a couple of questions.
> 
> I made the patch below against your patches to.
> 
> o Look for ".compatible = "foo" strings in .c and .h files too
> o Improve the vendor name match in vendor-prefix.txt by only
>   matching the exact vendor name at the beginning of lines.
> 
> I then produced a file of all the compatible uses in dts
> and used checkpatch on it.  It's a long list and a longer
> checkpatch warning list.
> 
> $ git ls-files | grep -E "\.dtsi?$" | \
>   xargs grep -hE "^\s*compatible\s*="| \
>   sed -r -e 's/^\s*//' -e 's/\s*=\s*/ = /'| sort | uniq > tmp.dts
> $ .scripts/checkpatch.pl -f tmp.dts
> 
> A couple of questions:
> 
> How does the $compat2 variable actually work?
> What is it supposed to do?
> 
> 				my $compat2 = $compat;
> 				$compat2 =~ s/\,[a-z]*\-/\,<\.\*>\-/;
> 
> For instance:
> WARNING: DT compatible string "marvell,tauros2-cache" appears un-documented -- check ./Documentation/devicetree/bindings/
> 
> The prefix "tauros2-" doesn't match '=~ s/[a-z]*-/
> 

This is my best guess, but correct me if I am wrong.
Some bindings are documented in a more generic way. For instance,

$ git grep ',<.*>-' Documentation/devicetree/bindings/

gives examples like

Documentation/devicetree/bindings/serial/fsl-imx-uart.txt:
	compatible : Should be "fsl,<soc>-uart"

Here <soc> is a generic placeholder for any Freescale SoC
(imx23, imx27, imx51, ...).

> Should it?  Should the '[a-z]*' be '[a-zA-Z0-9-]+'
> like the other test?
> 

Looking at the current documentation, the list of these generic
placeholders is pretty short:

$ git grep ',<.*>-' Documentation/devicetree/bindings/ | \
  grep -P -o ',<.*?>-' | grep -P -o '<.*>' | sort | uniq

<board>
<chip>
<chip name>
<mcu-chip>
<processor>
<soc>
<SOC>
<soc-family>

so '[a-zA-Z0-9-]+' seems more reasonable indeed.

> Also the grep used when $compat2 is different than $compat
> has <.*>
> 
> There aren't any descriptions I see in binding/ that have
> any <foo> like uses with the angle brackets.
> 
> Are the angle brackets "<" and ">" necessary?
> 

I guess that they are necessary, as we are dealing with placeholders.

> I think the code block should look more like:
> 
> 				my $compat2 = $compat;
> 				$compat2 =~ s/\,[a-zA-Z0-9]*\-/\,\.\*\-/g;

$compat2 =~ s/\,[a-zA-Z0-9]*\-/<\.\*>\-/g;

?

> 				my $grepfor = "\Q$compat\E";
> 				$grepfor .= "|\Q$compat2\E" if ($compat2 ne $compat);
> 				`grep -Erq "$grepfor" $dt_path`;
> 
> so that there's only 1 grep pattern when
> $compat2 is the same as $compat and the
> strings are escape quoted.
> 

Looks better.

> There are a _lot_ of missing entries:
> 
> o 164 vendor names
> o 2408 device names (maybe due to bad compat2 greps?)
> 

This is why Rob implemented this check. This should force people to
properly document bindings when submitting patches.

> What, if anything, should be done about them?

Ideally, this should be fixed over time. Fixing vendor names is pretty
easy. Fixing device bindings implies a deeper knowledge of the binding
and will take longer IMHO.

Regards,
Florian

> 
> ---
>  scripts/checkpatch.pl | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4b5e7b3..7a9eed9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2051,29 +2051,29 @@ sub process {
>  		}
>  
>  # check for DT compatible documentation
> -		if (defined $root && $realfile =~ /\.dts/ &&
> -		    $rawline =~ /^\+\s*compatible\s*=/) {
> +		if (defined $root &&
> +		    (($realfile =~ /\.dtsi?$/ && $line =~ /^\+\s*compatible\s*=\s*\"/) ||
> +		     ($realfile =~ /\.[ch]$/ && $line =~ /^\+.*\.compatible\s*=\s*\"/))) {
> +
>  			my @compats = $rawline =~ /\"([a-zA-Z0-9\-\,\.\+_]+)\"/g;
>  
> +			my $dt_path = $root . "/Documentation/devicetree/bindings/";
> +			my $vp_file = $root . "/Documentation/devicetree/bindings/vendor-prefixes.txt";
> +
>  			foreach my $compat (@compats) {
>  				my $compat2 = $compat;
> -				my $dt_path =  $root . "/Documentation/devicetree/bindings/";
>  				$compat2 =~ s/\,[a-z]*\-/\,<\.\*>\-/;
>  				`grep -Erq "$compat|$compat2" $dt_path`;
>  				if ( $? >> 8 ) {
>  					WARN("UNDOCUMENTED_DT_STRING",
>  					     "DT compatible string \"$compat\" appears un-documented -- check $dt_path\n" . $herecurr);
>  				}
> -
> -				my $vendor = $compat;
> -				my $vendor_path = $dt_path . "vendor-prefixes.txt";
> -				next if (! -f $vendor_path);
> -				next if not $vendor =~ /^[a-zA-Z0-9\-]+\,.*/;
> -				$vendor =~ s/^([a-zA-Z0-9\-]+)\,.*/$1/;
> -				`grep -Eq "$vendor" $vendor_path`;
> +				next if $compat !~ /^([a-zA-Z0-9\-]+)\,/;
> +				my $vendor = $1;
> +				`grep -Eq "^$vendor\\b" $vp_file`;
>  				if ( $? >> 8 ) {
>  					WARN("UNDOCUMENTED_DT_STRING",
> -					     "DT compatible string vendor \"$vendor\" appears un-documented -- check $vendor_path\n" . $herecurr);
> +					     "DT compatible string vendor \"$vendor\" appears un-documented -- check $vp_file\n" . $herecurr);
>  				}
>  			}
>  		}
> ---
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] checkpatch: fix spurious vendor compatible warnings

[Joe Perches]

On Mon, 2014-03-03 at 11:44 +0100, Florian Vaussard wrote:
> Looking at the current documentation, the list of these generic
> placeholders is pretty short:
> 
> $ git grep ',<.*>-' Documentation/devicetree/bindings/ | \
>   grep -P -o ',<.*?>-' | grep -P -o '<.*>' | sort | uniq
> 
> <board>
> <chip>
> <chip name>
> <mcu-chip>
> <processor>
> <soc>
> <SOC>
> <soc-family>
> 
> so '[a-zA-Z0-9-]+' seems more reasonable indeed.

The <mcu-chip> use seems as if it's 2 wildcards

	fsl,<mcu-chip>-<board>

I'm not sure that could work with the current
checkpatch code.

There's a space in "<chip name>" that should
probably be replaced by "<chip-name>" or just
"<chip>" for consistency.

> $compat2 =~ s/\,[a-zA-Z0-9]*\-/<\.\*>\-/g;

hand-escaped, not necessary if using "\Q$compat2\E"

Anyway, if either you or Rob think it appropriate to
submit a patch for either of the things I mentioned
in the first place:

> o Look for ".compatible = "foo" strings in .c and .h files
> o Improve the vendor name match in vendor-prefix.txt by only
>   matching the exact vendor name at the beginning of lines.

or any of the stuff above here, please do.

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

Re: [PATCH v2 2/2] checkpatch: fix spurious vendor compatible warnings

[Florian Vaussard]

Hi,

On 03/03/2014 06:51 PM, Joe Perches wrote:
> On Mon, 2014-03-03 at 11:44 +0100, Florian Vaussard wrote:
>> Looking at the current documentation, the list of these generic
>> placeholders is pretty short:
>>
>> $ git grep ',<.*>-' Documentation/devicetree/bindings/ | \
>>   grep -P -o ',<.*?>-' | grep -P -o '<.*>' | sort | uniq
>>
>> <board>
>> <chip>
>> <chip name>
>> <mcu-chip>
>> <processor>
>> <soc>
>> <SOC>
>> <soc-family>
>>
>> so '[a-zA-Z0-9-]+' seems more reasonable indeed.
> 
> The <mcu-chip> use seems as if it's 2 wildcards
> 
> 	fsl,<mcu-chip>-<board>
> 
> I'm not sure that could work with the current
> checkpatch code.
> 

I don't think it is desirable to make it work with the current
checkpatch code, as it is not enough constrained. For example, it allows
any compatible "fsl,foo-bar" to match, which opens the door for abuse.
We should have at least one fixed pattern.

> There's a space in "<chip name>" that should
> probably be replaced by "<chip-name>" or just
> "<chip>" for consistency.
> 

Sure, some cleaning could be necessary. I will see what I can do.

>> $compat2 =~ s/\,[a-zA-Z0-9]*\-/<\.\*>\-/g;
> 
> hand-escaped, not necessary if using "\Q$compat2\E"
> 

I wasn't aware of this, thanks.

> Anyway, if either you or Rob think it appropriate to
> submit a patch for either of the things I mentioned
> in the first place:
> 
>> o Look for ".compatible = "foo" strings in .c and .h files
>> o Improve the vendor name match in vendor-prefix.txt by only
>>   matching the exact vendor name at the beginning of lines.
> 
> or any of the stuff above here, please do.
> 

As you already sent a patch for these, I will include it in the v3 of
this series.

I see another problem: with the current regexp patterns, we have
enforced the use of a limited character set. But if one uses an exotic
character (even a simple space ' '), no warning will be produced at all.
We may check that we have at least one matching test, and throw a
warning otherwise. But this opens the question of enforcing the
characters used in compatible strings, where there is no strict
guideline AFAIK.

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

Re: [PATCH v2 2/2] checkpatch: fix spurious vendor compatible warnings

[Rob Herring]

On Fri, Feb 28, 2014 at 3:06 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi.
>
> A couple of suggestions and a couple of questions.
>
> I made the patch below against your patches to.
>
> o Look for ".compatible = "foo" strings in .c and .h files too
> o Improve the vendor name match in vendor-prefix.txt by only
>   matching the exact vendor name at the beginning of lines.
>
> I then produced a file of all the compatible uses in dts
> and used checkpatch on it.  It's a long list and a longer
> checkpatch warning list.
>
> $ git ls-files | grep -E "\.dtsi?$" | \
>   xargs grep -hE "^\s*compatible\s*="| \
>   sed -r -e 's/^\s*//' -e 's/\s*=\s*/ = /'| sort | uniq > tmp.dts
> $ .scripts/checkpatch.pl -f tmp.dts
>
> A couple of questions:
>
> How does the $compat2 variable actually work?
> What is it supposed to do?
>
>                                 my $compat2 = $compat;
>                                 $compat2 =~ s/\,[a-z]*\-/\,<\.\*>\-/;
>
> For instance:
> WARNING: DT compatible string "marvell,tauros2-cache" appears un-documented -- check ./Documentation/devicetree/bindings/
>
> The prefix "tauros2-" doesn't match '=~ s/[a-z]*-/
>
> Should it?  Should the '[a-z]*' be '[a-zA-Z0-9-]+'
> like the other test?

What Florian said is exactly right.

> Also the grep used when $compat2 is different than $compat
> has <.*>
>
> There aren't any descriptions I see in binding/ that have
> any <foo> like uses with the angle brackets.
>
> Are the angle brackets "<" and ">" necessary?
>
> I think the code block should look more like:
>
>                                 my $compat2 = $compat;
>                                 $compat2 =~ s/\,[a-zA-Z0-9]*\-/\,\.\*\-/g;
>                                 my $grepfor = "\Q$compat\E";
>                                 $grepfor .= "|\Q$compat2\E" if ($compat2 ne $compat);
>                                 `grep -Erq "$grepfor" $dt_path`;
>
> so that there's only 1 grep pattern when
> $compat2 is the same as $compat and the
> strings are escape quoted.
>
> There are a _lot_ of missing entries:
>
> o 164 vendor names
> o 2408 device names (maybe due to bad compat2 greps?)
>
> What, if anything, should be done about them?

Documenting DT bindings is somewhat new, so there are lots of older
sparc and powerpc bindings that don't have documentation and are
basically grandfathered in. I had thought about adding documentation
in an undocumented binding list, but that would only help making
checks on files pass. For any new bindings, documentation is required,
but we are only hitting like 50-60% in each kernel version. Those are
the ones I hope to catch.

I've written a script that finds the commit adding the undocumented
property and can email the authors, but I've been reluctant to start
spamming people with this. I want to see if checkpatch helps improve
the rate.

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