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. Reviewed-by: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx> Tested-by: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx> Signed-off-by: Dwaipayan Ray <dwaipayanray1@xxxxxxxxx> --- Changes in v4: - Change some rst code blocks which were supposed to be simply enumerations. 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/eab0487d7b4e68badbbe0505b2a7903b9d8931c4.camel@xxxxxxxxxxx/T/#t - Use passive voice in the documentation Documentation/dev-tools/checkpatch.rst | 399 ++++++++++++++++++++----- 1 file changed, 328 insertions(+), 71 deletions(-) diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst index 87b859f321de..f0956e9ea2d8 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/20070930191054.GA15876@xxxxxxxxxxxxxxxxxxx/ + 2. https://lore.kernel.org/lkml/1313384834-24433-12-git-send-email-lacombar@xxxxxxxxx/ + 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/20190430180111.10688-1-mcroce@xxxxxxxxxx/ + 2. https://lore.kernel.org/lkml/20190531131422.14970-1-mcroce@xxxxxxxxxx/ + + **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@xxxxxxxxxxxxxx/ + + **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/20200510182252.GA411829@xxxxxxx/ + + **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/20200401101714.44781-1-elver@xxxxxxxxxx/ + + **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@xxxxxxxxxxxxxx/ + **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 @@ -791,7 +1054,7 @@ Spacing and Brackets See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces **UNNECESSARY_PARENTHESES** - Parentheses are not required in the following cases:: + Parentheses are not required in the following cases: 1. Function pointer uses:: @@ -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
