Add and document more checkpatch message types. About 50% of all
message types are documented now.
In addition to this:
- Create a new subsection 'Indentation and Line Breaks'.
- Rename subsection 'Comment style' to simply 'Comments'.
- Refactor some of the existing types to appropriate subsections.
Signed-off-by: Dwaipayan Ray <[email protected]>
---
Changes in v3:
- Update explanation for CONSTANT_CONVERSION
- Add more reference links
- Fix grammatical errors
Changes in v2:
- Correct DEVICE_ATTR message types as suggested by Joe Perches.
https://lore.kernel.org/lkml/[email protected]/T/#t
- Use passive voice in the documentation
Documentation/dev-tools/checkpatch.rst | 397 ++++++++++++++++++++-----
1 file changed, 327 insertions(+), 70 deletions(-)
diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index 87b859f321de..ad84e709aa25 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -298,10 +298,148 @@ API usage
See: https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull
+ **CONSTANT_CONVERSION**
+ Use of __constant_<foo> form is discouraged for the following functions::
+
+ __constant_cpu_to_be[x]
+ __constant_cpu_to_le[x]
+ __constant_be[x]_to_cpu
+ __constant_le[x]_to_cpu
+ __constant_htons
+ __constant_ntohs
+
+ Using any of these outside of include/uapi/ is not preferred as using the
+ function without __constant_ is identical when the argument is a
+ constant.
+
+ In big endian systems, the macros like __constant_cpu_to_be32(x) and
+ cpu_to_be32(x) expand to the same expression::
+
+ #define __constant_cpu_to_be32(x) ((__force __be32)(__u32)(x))
+ #define __cpu_to_be32(x) ((__force __be32)(__u32)(x))
+
+ In little endian systems, the macros __constant_cpu_to_be32(x) and
+ cpu_to_be32(x) expand to __constant_swab32 and __swab32. __swab32
+ has a __builtin_constant_p check::
+
+ #define __swab32(x) \
+ (__builtin_constant_p((__u32)(x)) ? \
+ ___constant_swab32(x) : \
+ __fswab32(x))
+
+ So ultimately they have a special case for constants.
+ Similar is the case with all of the macros in the list. Thus
+ using the __constant_... forms are unnecessarily verbose and
+ not preferred outside of include/uapi.
+
+ See: https://lore.kernel.org/lkml/1400106425.12666.6.camel@joe-AO725/
+
+ **DEPRECATED_API**
+ Usage of a deprecated RCU API is detected. It is recommended to replace
+ old flavourful RCU APIs by their new vanilla-RCU counterparts.
+
+ The full list of available RCU APIs can be viewed from the kernel docs.
+
+ See: https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#full-list-of-rcu-apis
+
+ **DEPRECATED_VARIABLE**
+ EXTRA_{A,C,CPP,LD}FLAGS are deprecated and should be replaced by the new
+ flags added via commit f77bf01425b1 ("kbuild: introduce ccflags-y,
+ asflags-y and ldflags-y").
+
+ The following conversion scheme maybe used::
+
+ EXTRA_AFLAGS -> asflags-y
+ EXTRA_CFLAGS -> ccflags-y
+ EXTRA_CPPFLAGS -> cppflags-y
+ EXTRA_LDFLAGS -> ldflags-y
+
+ See:
+
+ 1. https://lore.kernel.org/lkml/[email protected]/
+ 2. https://lore.kernel.org/lkml/[email protected]/
+ 3. https://www.kernel.org/doc/html/latest/kbuild/makefiles.html#compilation-flags
+
+ **DEVICE_ATTR_FUNCTIONS**
+ The function names used in DEVICE_ATTR is unusual.
+ Typically, the store and show functions are used with <attr>_store and
+ <attr>_show, where <attr> is a named attribute variable of the device.
+
+ Consider the following examples::
+
+ static DEVICE_ATTR(type, 0444, type_show, NULL);
+ static DEVICE_ATTR(power, 0644, power_show, power_store);
+
+ The function names should preferably follow the above pattern.
+
+ See: https://www.kernel.org/doc/html/latest/driver-api/driver-model/device.html#attributes
+
+ **DEVICE_ATTR_RO**
+ The DEVICE_ATTR_RO(name) helper macro can be used instead of
+ DEVICE_ATTR(name, 0444, name_show, NULL);
+
+ Note that the macro automatically appends _show to the named
+ attribute variable of the device for the show method.
+
+ See: https://www.kernel.org/doc/html/latest/driver-api/driver-model/device.html#attributes
+
+ **DEVICE_ATTR_RW**
+ The DEVICE_ATTR_RW(name) helper macro can be used instead of
+ DEVICE_ATTR(name, 0644, name_show, name_store);
+
+ Note that the macro automatically appends _show and _store to the
+ named attribute variable of the device for the show and store methods.
+
+ See: https://www.kernel.org/doc/html/latest/driver-api/driver-model/device.html#attributes
+
+ **DEVICE_ATTR_WO**
+ The DEVICE_AATR_WO(name) helper macro can be used instead of
+ DEVICE_ATTR(name, 0200, NULL, name_store);
+
+ Note that the macro automatically appends _store to the
+ named attribute variable of the device for the store method.
+
+ See: https://www.kernel.org/doc/html/latest/driver-api/driver-model/device.html#attributes
+
+ **DUPLICATED_SYSCTL_CONST**
+ Commit d91bff3011cf ("proc/sysctl: add shared variables for range
+ check") added some shared const variables to be used instead of a local
+ copy in each source file.
+
+ Consider replacing the sysctl range checking value with the shared
+ one in include/linux/sysctl.h. The following conversion scheme may
+ be used::
+
+ &zero -> SYSCTL_ZERO
+ &one -> SYSCTL_ONE
+ &int_max -> SYSCTL_INT_MAX
+
+ See:
+
+ 1. https://lore.kernel.org/lkml/[email protected]/
+ 2. https://lore.kernel.org/lkml/[email protected]/
+
+ **ENOSYS**
+ ENOSYS means that a nonexistent system call was called.
+ Earlier, it was wrongly used for things like invalid operations on
+ otherwise valid syscalls. This should be avoided in new code.
+
+ See: https://lore.kernel.org/lkml/5eb299021dec23c1a48fa7d9f2c8b794e967766d.1408730669.git.luto@amacapital.net/
+
+ **ENOTSUPP**
+ ENOTSUPP is not a standard error code and should be avoided in new patches.
+ EOPNOTSUPP should be used instead.
+
+ See: https://lore.kernel.org/netdev/[email protected]/
+
+ **EXPORT_SYMBOL**
+ EXPORT_SYMBOL should immediately follow the symbol to be exported.
+
**IN_ATOMIC**
in_atomic() is not for driver use so any such use is reported as an ERROR.
- Also in_atomic() is often used to determine if we may sleep, but it is not
- reliable in this use model therefore its use is strongly discouraged.
+ Also in_atomic() is often used to determine if sleeping is permitted,
+ but it is not reliable in this use model. Therefore its use is
+ strongly discouraged.
However, in_atomic() is ok for core kernel use.
@@ -335,8 +473,8 @@ API usage
See: https://www.kernel.org/doc/html/latest/timers/timers-howto.html#delays-information-on-the-various-kernel-delay-sleep-mechanisms
-Comment style
--------------
+Comments
+--------
**BLOCK_COMMENT_STYLE**
The comment style is incorrect. The preferred style for multi-
@@ -362,6 +500,21 @@ Comment style
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
+ **DATA_RACE**
+ Applications of data_race() should have a comment so as to document the
+ reasoning behind why it was deemed safe.
+
+ See: https://lore.kernel.org/lkml/[email protected]/
+
+ **FSF_MAILING_ADDRESS**
+ Kernel maintainers reject new instances of the GPL boilerplate paragraph
+ directing people to write to the FSF for a copy of the GPL, since the
+ FSF has moved in the past and may do so again.
+ So do not write paragraphs about writing to the Free Software Foundation's
+ mailing address.
+
+ See: https://lore.kernel.org/lkml/20131006222342.GT19510@leaf/
+
Commit message
--------------
@@ -394,6 +547,13 @@ Commit message
See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
+ **EMAIL_SUBJECT**
+ Naming the tool that found the issue is not very useful in the
+ subject line. A good subject line summarizes the change that
+ the patch brings.
+
+ See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
+
**FROM_SIGN_OFF_MISMATCH**
The author's email does not match with that in the Signed-off-by:
line(s). This can be sometimes caused due to an improperly configured
@@ -482,6 +642,87 @@ Comparison style
side of the test should be avoided.
+Indentation and Line Breaks
+---------------------------
+
+ **CODE_INDENT**
+ Code indent should use tabs instead of spaces.
+ Outside of comments, documentation and Kconfig,
+ spaces are never used for indentation.
+
+ See: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation
+
+ **DEEP_INDENTATION**
+ Indentation with 6 or more tabs usually indicate overly indented
+ code.
+
+ It is suggested to refactor excessive indentation of
+ if/else/for/do/while/switch statements.
+
+ See: https://lore.kernel.org/lkml/1328311239.21255.24.camel@joe2Laptop/
+
+ **SWITCH_CASE_INDENT_LEVEL**
+ switch should be at the same indent as case.
+ Example::
+
+ switch (suffix) {
+ case 'G':
+ case 'g':
+ mem <<= 30;
+ break;
+ case 'M':
+ case 'm':
+ mem <<= 20;
+ break;
+ case 'K':
+ case 'k':
+ mem <<= 10;
+ fallthrough;
+ default:
+ break;
+ }
+
+ See: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation
+
+ **LONG_LINE**
+ The line has exceeded the specified maximum length.
+ To use a different maximum line length, the --max-line-length=n option
+ may be added while invoking checkpatch.
+
+ Earlier, the default line length was 80 columns. Commit bdc48fa11e46
+ ("checkpatch/coding-style: deprecate 80-column warning") increased the
+ limit to 100 columns. This is not a hard limit either and it's
+ preferable to stay within 80 columns whenever possible.
+
+ See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
+
+ **LONG_LINE_STRING**
+ A string starts before but extends beyond the maximum line length.
+ To use a different maximum line length, the --max-line-length=n option
+ may be added while invoking checkpatch.
+
+ See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
+
+ **LONG_LINE_COMMENT**
+ A comment starts before but extends beyond the maximum line length.
+ To use a different maximum line length, the --max-line-length=n option
+ may be added while invoking checkpatch.
+
+ See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
+
+ **TRAILING_STATEMENTS**
+ Trailing statements (for example after any conditional) should be
+ on the next line.
+ Statements, such as::
+
+ if (x == y) break;
+
+ should be::
+
+ if (x == y)
+ break;
+
+
Macros, Attributes and Symbols
------------------------------
@@ -546,6 +787,9 @@ Macros, Attributes and Symbols
See: https://lore.kernel.org/lkml/CA+55aFycQ9XJvEOsiM3txHL5bjUc8CeKWJNR_H+MiicaddB42Q@mail.gmail.com/
+ **DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON**
+ do {} while(0) macros should not have a trailing semicolon.
+
**INIT_ATTRIBUTE**
Const init definitions should use __initconst instead of
__initdata.
@@ -614,6 +858,48 @@ Functions and Variables
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#naming
+ **CONST_CONST**
+ Using `const <type> const *` is generally meant to be
+ written `const <type> * const`.
+
+ **CONST_STRUCT**
+ Using const is generally a good idea. Checkpatch reads
+ a list of frequently used structs that are always or
+ almost always constant.
+
+ The existing structs list can be viewed from
+ `scripts/const_structs.checkpatch`.
+
+ See: https://lore.kernel.org/lkml/alpine.DEB.2.10.1608281509480.3321@hadrien/
+
+ **EMBEDDED_FUNCTION_NAME**
+ Embedded function names are less appropriate to use as
+ refactoring can cause function renaming. Prefer the use of
+ "%s", __func__ to embedded function names.
+
+ Note that this does not work with -f (--file) checkpatch option
+ as it depends on patch context providing the function name.
+
+ **FUNCTION_ARGUMENTS**
+ This warning is emitted due to any of the following reasons::
+
+ 1. Arguments for the function declaration do not follow
+ the identifier name. Example::
+
+ void foo
+ (int bar, int baz)
+
+ This should be corrected to::
+
+ void foo(int bar, int baz)
+
+ 2. Some arguments for the function definition do not
+ have an identifier name. Example::
+
+ void foo(int)
+
+ All arguments should have identifier names.
+
**FUNCTION_WITHOUT_ARGS**
Function declarations without arguments like::
@@ -647,6 +933,13 @@ Functions and Variables
Permissions
-----------
+ **DEVICE_ATTR_PERMS**
+ The permissions used in DEVICE_ATTR are unusual.
+ Typically only three permissions are used - 0644 (RW), 0444 (RO)
+ and 0200 (WO).
+
+ See: https://www.kernel.org/doc/html/latest/filesystems/sysfs.html#attributes
+
**EXECUTE_PERMISSIONS**
There is no reason for source files to be executable. The executable
bit can be removed safely.
@@ -708,13 +1001,6 @@ Spacing and Brackets
= { [0...10] = 5 }
- **CODE_INDENT**
- Code indent should use tabs instead of spaces.
- Outside of comments, documentation and Kconfig,
- spaces are never used for indentation.
-
- See: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation
-
**CONCATENATED_STRING**
Concatenated elements should have a space in between.
Example::
@@ -760,29 +1046,6 @@ Spacing and Brackets
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces
- **SWITCH_CASE_INDENT_LEVEL**
- switch should be at the same indent as case.
- Example::
-
- switch (suffix) {
- case 'G':
- case 'g':
- mem <<= 30;
- break;
- case 'M':
- case 'm':
- mem <<= 20;
- break;
- case 'K':
- case 'k':
- mem <<= 10;
- /* fall through */
- default:
- break;
- }
-
- See: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation
-
**TRAILING_WHITESPACE**
Trailing whitespace should always be removed.
Some editors highlight the trailing whitespace and cause visual
@@ -842,40 +1105,46 @@ Others
The patch seems to be corrupted or lines are wrapped.
Please regenerate the patch file before sending it to the maintainer.
+ **CVS_KEYWORD**
+ Since linux moved to git, the CVS markers are no longer used.
+ So, CVS style keywords ($Id$, $Revision$, $Log$) should not be
+ added.
+
+ **DEFAULT_NO_BREAK**
+ switch default case is sometimes written as "default:;". This can
+ cause new cases added below default to be defective.
+
+ A "break;" should be added after empty default statement to avoid
+ unwanted fallthrough.
+
**DOS_LINE_ENDINGS**
For DOS-formatted patches, there are extra ^M symbols at the end of
the line. These should be removed.
- **FSF_MAILING_ADDRESS**
- Kernel maintainers reject new instances of the GPL boilerplate paragraph
- directing people to write to the FSF for a copy of the GPL, since the
- FSF has moved in the past and may do so again.
- So do not write paragraphs about writing to the Free Software Foundation's
- mailing address.
+ **DT_SCHEMA_BINDING_PATCH**
+ DT bindings moved to a json-schema based format instead of
+ freeform text.
- See: https://lore.kernel.org/lkml/20131006222342.GT19510@leaf/
+ See: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-schema.html
- **LONG_LINE**
- The line has exceeded the specified maximum length. Consider refactoring
- it.
- To use a different maximum line length, the --max-line-length=n option
- may be added while invoking checkpatch.
+ **DT_SPLIT_BINDING_PATCH**
+ Devicetree bindings should be their own patch. This is because
+ bindings are logically independent from a driver implementation,
+ they have a different maintainer (even though they often
+ are applied via the same tree), and it makes for a cleaner history in the
+ DT only tree created with git-filter-branch.
- See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
+ See: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
- **LONG_LINE_STRING**
- A string starts before but extends beyond the maximum line length.
- To use a different maximum line length, the --max-line-length=n option
- may be added while invoking checkpatch.
+ **EMBEDDED_FILENAME**
+ Embedding the complete filename path inside the file isn't particularly
+ useful as often the path is moved around and becomes incorrect.
- See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
+ **FILE_PATH_CHANGES**
+ Whenever files are added, moved, or deleted, the MAINTAINERS file
+ patterns can be out of sync or outdated.
- **LONG_LINE_COMMENT**
- A comment starts before but extends beyond the maximum line length.
- To use a different maximum line length, the --max-line-length=n option
- may be added while invoking checkpatch.
-
- See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
+ So MAINTAINERS might need updating in these cases.
**MEMSET**
The memset use appears to be incorrect. This may be caused due to
@@ -895,17 +1164,5 @@ Others
See: https://www.kernel.org/doc/html/latest/process/license-rules.html
- **TRAILING_STATEMENTS**
- Trailing statements (for example after any conditional) should be
- on the next line.
- Like::
-
- if (x == y) break;
-
- should be::
-
- if (x == y)
- break;
-
**TYPO_SPELLING**
Some words may have been misspelled. Consider reviewing them.
--
2.28.0
On Fri, 11 Jun 2021, Dwaipayan Ray wrote:
> Add and document more checkpatch message types. About 50% of all
> message types are documented now.
>
> In addition to this:
>
> - Create a new subsection 'Indentation and Line Breaks'.
> - Rename subsection 'Comment style' to simply 'Comments'.
> - Refactor some of the existing types to appropriate subsections.
>
> Signed-off-by: Dwaipayan Ray <[email protected]>
Reviewed-by: Lukas Bulwahn <[email protected]>
Tested-by: Lukas Bulwahn <[email protected]>
'make htmldocs' produces no new warnings.
See one further comment on the html presentation below; other than that no
further comments.
Lukas
> ---
>
> Changes in v3:
> - Update explanation for CONSTANT_CONVERSION
> - Add more reference links
> - Fix grammatical errors
>
> Changes in v2:
> - Correct DEVICE_ATTR message types as suggested by Joe Perches.
> https://lore.kernel.org/lkml/[email protected]/T/#t
> - Use passive voice in the documentation
>
> Documentation/dev-tools/checkpatch.rst | 397 ++++++++++++++++++++-----
> 1 file changed, 327 insertions(+), 70 deletions(-)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index 87b859f321de..ad84e709aa25 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
<snip>
> +
> + **FUNCTION_ARGUMENTS**
> + This warning is emitted due to any of the following reasons::
I think here you wnt to have an enumeration, but the "::" makes it
a code block.
> +
> + 1. Arguments for the function declaration do not follow
> + the identifier name. Example::
> +
> + void foo
> + (int bar, int baz)
> +
> + This should be corrected to::
> +
> + void foo(int bar, int baz)
> +
> + 2. Some arguments for the function definition do not
> + have an identifier name. Example::
> +
> + void foo(int)
> +
> + All arguments should have identifier names.
> +
> **FUNCTION_WITHOUT_ARGS**
> Function declarations without arguments like::
>