|
@@ -246,6 +246,7 @@ Allocation style
|
|
|
The first argument for kcalloc or kmalloc_array should be the
|
|
|
number of elements. sizeof() as the first argument is generally
|
|
|
wrong.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/core-api/memory-allocation.html
|
|
|
|
|
|
**ALLOC_SIZEOF_STRUCT**
|
|
@@ -264,6 +265,7 @@ Allocation style
|
|
|
**ALLOC_WITH_MULTIPLY**
|
|
|
Prefer kmalloc_array/kcalloc over kmalloc/kzalloc with a
|
|
|
sizeof multiply.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/core-api/memory-allocation.html
|
|
|
|
|
|
|
|
@@ -284,6 +286,7 @@ API usage
|
|
|
BUG() or BUG_ON() should be avoided totally.
|
|
|
Use WARN() and WARN_ON() instead, and handle the "impossible"
|
|
|
error condition as gracefully as possible.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on
|
|
|
|
|
|
**CONSIDER_KSTRTO**
|
|
@@ -292,12 +295,161 @@ API usage
|
|
|
may lead to unexpected results in callers. The respective kstrtol(),
|
|
|
kstrtoll(), kstrtoul(), and kstrtoull() functions tend to be the
|
|
|
correct replacements.
|
|
|
+
|
|
|
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@uranus.ravnborg.org/
|
|
|
+ 2. https://lore.kernel.org/lkml/1313384834-24433-12-git-send-email-lacombar@gmail.com/
|
|
|
+ 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@redhat.com/
|
|
|
+ 2. https://lore.kernel.org/lkml/20190531131422.14970-1-mcroce@redhat.com/
|
|
|
+
|
|
|
+ **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/20200510182252.GA411829@lunn.ch/
|
|
|
+
|
|
|
+ **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 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.
|
|
|
+
|
|
|
+ See: https://lore.kernel.org/lkml/20080320201723.b87b3732.akpm@linux-foundation.org/
|
|
|
+
|
|
|
**LOCKDEP**
|
|
|
The lockdep_no_validate class was added as a temporary measure to
|
|
|
prevent warnings on conversion of device->sem to device->mutex.
|
|
|
It should not be used for any other purpose.
|
|
|
+
|
|
|
See: https://lore.kernel.org/lkml/1268959062.9440.467.camel@laptop/
|
|
|
|
|
|
**MALFORMED_INCLUDE**
|
|
@@ -308,14 +460,21 @@ API usage
|
|
|
**USE_LOCKDEP**
|
|
|
lockdep_assert_held() annotations should be preferred over
|
|
|
assertions based on spin_is_locked()
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/locking/lockdep-design.html#annotations
|
|
|
|
|
|
**UAPI_INCLUDE**
|
|
|
No #include statements in include/uapi should use a uapi/ path.
|
|
|
|
|
|
+ **USLEEP_RANGE**
|
|
|
+ usleep_range() should be preferred over udelay(). The proper way of
|
|
|
+ using usleep_range() is mentioned in the kernel docs.
|
|
|
+
|
|
|
+ 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-
|
|
@@ -338,8 +497,24 @@ Comment style
|
|
|
**C99_COMMENTS**
|
|
|
C99 style single line comments (//) should not be used.
|
|
|
Prefer the block comment style instead.
|
|
|
+
|
|
|
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@google.com/
|
|
|
+
|
|
|
+ **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
|
|
|
--------------
|
|
@@ -347,6 +522,7 @@ Commit message
|
|
|
**BAD_SIGN_OFF**
|
|
|
The signed-off-by line does not fall in line with the standards
|
|
|
specified by the community.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1
|
|
|
|
|
|
**BAD_STABLE_ADDRESS_STYLE**
|
|
@@ -368,12 +544,33 @@ Commit message
|
|
|
**COMMIT_MESSAGE**
|
|
|
The patch is missing a commit description. A brief
|
|
|
description of the changes made by the patch should be added.
|
|
|
+
|
|
|
+ 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
|
|
|
+ email client.
|
|
|
+
|
|
|
+ This message is emitted due to any of the following reasons::
|
|
|
+
|
|
|
+ - The email names do not match.
|
|
|
+ - The email addresses do not match.
|
|
|
+ - The email subaddresses do not match.
|
|
|
+ - The email comments do not match.
|
|
|
+
|
|
|
**MISSING_SIGN_OFF**
|
|
|
The patch is missing a Signed-off-by line. A signed-off-by
|
|
|
line should be added according to Developer's certificate of
|
|
|
Origin.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
|
|
|
|
|
|
**NO_AUTHOR_SIGN_OFF**
|
|
@@ -382,6 +579,7 @@ Commit message
|
|
|
end of explanation of the patch to denote that the author has
|
|
|
written it or otherwise has the rights to pass it on as an open
|
|
|
source patch.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
|
|
|
|
|
|
**DIFF_IN_COMMIT_MSG**
|
|
@@ -389,6 +587,7 @@ Commit message
|
|
|
This causes problems when one tries to apply a file containing both
|
|
|
the changelog and the diff because patch(1) tries to apply the diff
|
|
|
which it found in the changelog.
|
|
|
+
|
|
|
See: https://lore.kernel.org/lkml/20150611134006.9df79a893e3636019ad2759e@linux-foundation.org/
|
|
|
|
|
|
**GERRIT_CHANGE_ID**
|
|
@@ -431,6 +630,7 @@ Comparison style
|
|
|
**BOOL_COMPARISON**
|
|
|
Comparisons of A to true and false are better written
|
|
|
as A and !A.
|
|
|
+
|
|
|
See: https://lore.kernel.org/lkml/1365563834.27174.12.camel@joe-AO722/
|
|
|
|
|
|
**COMPARISON_TO_NULL**
|
|
@@ -442,6 +642,120 @@ 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
|
|
|
+
|
|
|
+ **SPLIT_STRING**
|
|
|
+ Quoted strings that appear as messages in userspace and can be
|
|
|
+ grepped, should not be split across multiple lines.
|
|
|
+
|
|
|
+ See: https://lore.kernel.org/lkml/20120203052727.GA15035@leaf/
|
|
|
+
|
|
|
+ **MULTILINE_DEREFERENCE**
|
|
|
+ A single dereferencing identifier spanned on multiple lines like::
|
|
|
+
|
|
|
+ struct_identifier->member[index].
|
|
|
+ member = <foo>;
|
|
|
+
|
|
|
+ is generally hard to follow. It can easily lead to typos and so makes
|
|
|
+ the code vulnerable to bugs.
|
|
|
+
|
|
|
+ If fixing the multiple line dereferencing leads to an 80 column
|
|
|
+ violation, then either rewrite the code in a more simple way or if the
|
|
|
+ starting part of the dereferencing identifier is the same and used at
|
|
|
+ multiple places then store it in a temporary variable, and use that
|
|
|
+ temporary variable only at all the places. For example, if there are
|
|
|
+ two dereferencing identifiers::
|
|
|
+
|
|
|
+ member1->member2->member3.foo1;
|
|
|
+ member1->member2->member3.foo2;
|
|
|
+
|
|
|
+ then store the member1->member2->member3 part in a temporary variable.
|
|
|
+ It not only helps to avoid the 80 column violation but also reduces
|
|
|
+ the program size by removing the unnecessary dereferences.
|
|
|
+
|
|
|
+ But if none of the above methods work then ignore the 80 column
|
|
|
+ violation because it is much easier to read a dereferencing identifier
|
|
|
+ on a single line.
|
|
|
+
|
|
|
+ **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
|
|
|
------------------------------
|
|
|
|
|
@@ -472,7 +786,7 @@ Macros, Attributes and Symbols
|
|
|
|
|
|
**BIT_MACRO**
|
|
|
Defines like: 1 << <digit> could be BIT(digit).
|
|
|
- The BIT() macro is defined in include/linux/bitops.h::
|
|
|
+ The BIT() macro is defined via include/linux/bits.h::
|
|
|
|
|
|
#define BIT(nr) (1UL << (nr))
|
|
|
|
|
@@ -492,6 +806,7 @@ Macros, Attributes and Symbols
|
|
|
The kernel does *not* use the ``__DATE__`` and ``__TIME__`` macros,
|
|
|
and enables warnings if they are used as they can lead to
|
|
|
non-deterministic builds.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/kbuild/reproducible-builds.html#timestamps
|
|
|
|
|
|
**DEFINE_ARCH_HAS**
|
|
@@ -502,8 +817,12 @@ Macros, Attributes and Symbols
|
|
|
want architectures able to override them with optimized ones, we
|
|
|
should either use weak functions (appropriate for some cases), or
|
|
|
the symbol that protects them should be the same symbol we use.
|
|
|
+
|
|
|
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.
|
|
@@ -528,6 +847,20 @@ Macros, Attributes and Symbols
|
|
|
...
|
|
|
}
|
|
|
|
|
|
+ **MISPLACED_INIT**
|
|
|
+ It is possible to use section markers on variables in a way
|
|
|
+ which gcc doesn't understand (or at least not the way the
|
|
|
+ developer intended)::
|
|
|
+
|
|
|
+ static struct __initdata samsung_pll_clock exynos4_plls[nr_plls] = {
|
|
|
+
|
|
|
+ does not put exynos4_plls in the .initdata section. The __initdata
|
|
|
+ marker can be virtually anywhere on the line, except right after
|
|
|
+ "struct". The preferred location is before the "=" sign if there is
|
|
|
+ one, or before the trailing ";" otherwise.
|
|
|
+
|
|
|
+ See: https://lore.kernel.org/lkml/1377655732.3619.19.camel@joe-AO722/
|
|
|
+
|
|
|
**MULTISTATEMENT_MACRO_USE_DO_WHILE**
|
|
|
Macros with multiple statements should be enclosed in a
|
|
|
do - while block. Same should also be the case for macros
|
|
@@ -541,6 +874,42 @@ Macros, Attributes and Symbols
|
|
|
|
|
|
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
|
|
|
|
|
|
+ **PREFER_FALLTHROUGH**
|
|
|
+ Use the `fallthrough;` pseudo keyword instead of
|
|
|
+ `/* fallthrough */` like comments.
|
|
|
+
|
|
|
+ **TRAILING_SEMICOLON**
|
|
|
+ Macro definition should not end with a semicolon. The macro
|
|
|
+ invocation style should be consistent with function calls.
|
|
|
+ This can prevent any unexpected code paths::
|
|
|
+
|
|
|
+ #define MAC do_something;
|
|
|
+
|
|
|
+ If this macro is used within a if else statement, like::
|
|
|
+
|
|
|
+ if (some_condition)
|
|
|
+ MAC;
|
|
|
+
|
|
|
+ else
|
|
|
+ do_something;
|
|
|
+
|
|
|
+ Then there would be a compilation error, because when the macro is
|
|
|
+ expanded there are two trailing semicolons, so the else branch gets
|
|
|
+ orphaned.
|
|
|
+
|
|
|
+ See: https://lore.kernel.org/lkml/1399671106.2912.21.camel@joe-AO725/
|
|
|
+
|
|
|
+ **SINGLE_STATEMENT_DO_WHILE_MACRO**
|
|
|
+ For the multi-statement macros, it is necessary to use the do-while
|
|
|
+ loop to avoid unpredictable code paths. The do-while loop helps to
|
|
|
+ group the multiple statements into a single one so that a
|
|
|
+ function-like macro can be used as a function only.
|
|
|
+
|
|
|
+ But for the single statement macros, it is unnecessary to use the
|
|
|
+ do-while loop. Although the code is syntactically correct but using
|
|
|
+ the do-while loop is redundant. So remove the do-while loop for single
|
|
|
+ statement macros.
|
|
|
+
|
|
|
**WEAK_DECLARATION**
|
|
|
Using weak declarations like __attribute__((weak)) or __weak
|
|
|
can have unintended link defects. Avoid using them.
|
|
@@ -551,8 +920,51 @@ Functions and Variables
|
|
|
|
|
|
**CAMELCASE**
|
|
|
Avoid CamelCase Identifiers.
|
|
|
+
|
|
|
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::
|
|
|
|
|
@@ -573,6 +985,11 @@ Functions and Variables
|
|
|
Your compiler (or rather your loader) automatically does
|
|
|
it for you.
|
|
|
|
|
|
+ **MULTIPLE_ASSIGNMENTS**
|
|
|
+ Multiple assignments on a single line makes the code unnecessarily
|
|
|
+ complicated. So on a single line assign value to a single variable
|
|
|
+ only, this makes the code more readable and helps avoid typos.
|
|
|
+
|
|
|
**RETURN_PARENTHESES**
|
|
|
return is not a function and as such doesn't need parentheses::
|
|
|
|
|
@@ -583,6 +1000,45 @@ Functions and Variables
|
|
|
return bar;
|
|
|
|
|
|
|
|
|
+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.
|
|
|
+
|
|
|
+ **EXPORTED_WORLD_WRITABLE**
|
|
|
+ Exporting world writable sysfs/debugfs files is usually a bad thing.
|
|
|
+ When done arbitrarily they can introduce serious security bugs.
|
|
|
+ In the past, some of the debugfs vulnerabilities would seemingly allow
|
|
|
+ any local user to write arbitrary values into device registers - a
|
|
|
+ situation from which little good can be expected to emerge.
|
|
|
+
|
|
|
+ See: https://lore.kernel.org/linux-arm-kernel/cover.1296818921.git.segoon@openwall.com/
|
|
|
+
|
|
|
+ **NON_OCTAL_PERMISSIONS**
|
|
|
+ Permission bits should use 4 digit octal permissions (like 0700 or 0444).
|
|
|
+ Avoid using any other base like decimal.
|
|
|
+
|
|
|
+ **SYMBOLIC_PERMS**
|
|
|
+ Permission bits in the octal form are more readable and easier to
|
|
|
+ understand than their symbolic counterparts because many command-line
|
|
|
+ tools use this notation. Experienced kernel developers have been using
|
|
|
+ these traditional Unix permission bits for decades and so they find it
|
|
|
+ easier to understand the octal notation than the symbolic macros.
|
|
|
+ For example, it is harder to read S_IWUSR|S_IRUGO than 0644, which
|
|
|
+ obscures the developer's intent rather than clarifying it.
|
|
|
+
|
|
|
+ See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
|
|
|
+
|
|
|
+
|
|
|
Spacing and Brackets
|
|
|
--------------------
|
|
|
|
|
@@ -616,7 +1072,7 @@ Spacing and Brackets
|
|
|
|
|
|
1. With a type on the left::
|
|
|
|
|
|
- ;int [] a;
|
|
|
+ int [] a;
|
|
|
|
|
|
2. At the beginning of a line for slice initialisers::
|
|
|
|
|
@@ -626,12 +1082,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::
|
|
@@ -644,17 +1094,20 @@ Spacing and Brackets
|
|
|
|
|
|
**ELSE_AFTER_BRACE**
|
|
|
`else {` should follow the closing block `}` on the same line.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
|
|
|
|
|
|
**LINE_SPACING**
|
|
|
Vertical space is wasted given the limited number of lines an
|
|
|
editor window can display when multiple blank lines are used.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces
|
|
|
|
|
|
**OPEN_BRACE**
|
|
|
The opening brace should be following the function definitions on the
|
|
|
next line. For any non-functional block it should be on the same line
|
|
|
as the last construct.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
|
|
|
|
|
|
**POINTER_LOCATION**
|
|
@@ -671,37 +1124,47 @@ Spacing and Brackets
|
|
|
|
|
|
**SPACING**
|
|
|
Whitespace style used in the kernel sources is described in kernel docs.
|
|
|
- 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
|
|
|
+ See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces
|
|
|
|
|
|
**TRAILING_WHITESPACE**
|
|
|
Trailing whitespace should always be removed.
|
|
|
Some editors highlight the trailing whitespace and cause visual
|
|
|
distractions when editing files.
|
|
|
+
|
|
|
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces
|
|
|
|
|
|
+ **UNNECESSARY_PARENTHESES**
|
|
|
+ Parentheses are not required in the following cases:
|
|
|
+
|
|
|
+ 1. Function pointer uses::
|
|
|
+
|
|
|
+ (foo->bar)();
|
|
|
+
|
|
|
+ could be::
|
|
|
+
|
|
|
+ foo->bar();
|
|
|
+
|
|
|
+ 2. Comparisons in if::
|
|
|
+
|
|
|
+ if ((foo->bar) && (foo->baz))
|
|
|
+ if ((foo == bar))
|
|
|
+
|
|
|
+ could be::
|
|
|
+
|
|
|
+ if (foo->bar && foo->baz)
|
|
|
+ if (foo == bar)
|
|
|
+
|
|
|
+ 3. addressof/dereference single Lvalues::
|
|
|
+
|
|
|
+ &(foo->bar)
|
|
|
+ *(foo->bar)
|
|
|
+
|
|
|
+ could be::
|
|
|
+
|
|
|
+ &foo->bar
|
|
|
+ *foo->bar
|
|
|
+
|
|
|
**WHILE_AFTER_BRACE**
|
|
|
while should follow the closing bracket on the same line::
|
|
|
|
|
@@ -723,17 +1186,50 @@ 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.
|
|
|
|
|
|
- **EXECUTE_PERMISSIONS**
|
|
|
- There is no reason for source files to be executable. The executable
|
|
|
- bit can be removed safely.
|
|
|
+ **DT_SCHEMA_BINDING_PATCH**
|
|
|
+ DT bindings moved to a json-schema based format instead of
|
|
|
+ freeform text.
|
|
|
|
|
|
- **NON_OCTAL_PERMISSIONS**
|
|
|
- Permission bits should use 4 digit octal permissions (like 0700 or 0444).
|
|
|
- Avoid using any other base like decimal.
|
|
|
+ See: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-schema.html
|
|
|
+
|
|
|
+ **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/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
|
|
|
+
|
|
|
+ **EMBEDDED_FILENAME**
|
|
|
+ Embedding the complete filename path inside the file isn't particularly
|
|
|
+ useful as often the path is moved around and becomes incorrect.
|
|
|
+
|
|
|
+ **FILE_PATH_CHANGES**
|
|
|
+ Whenever files are added, moved, or deleted, the MAINTAINERS file
|
|
|
+ patterns can be out of sync or outdated.
|
|
|
+
|
|
|
+ So MAINTAINERS might need updating in these cases.
|
|
|
+
|
|
|
+ **MEMSET**
|
|
|
+ The memset use appears to be incorrect. This may be caused due to
|
|
|
+ badly ordered parameters. Please recheck the usage.
|
|
|
|
|
|
**NOT_UNIFIED_DIFF**
|
|
|
The patch file does not appear to be in unified-diff format. Please
|
|
@@ -742,14 +1238,12 @@ Others
|
|
|
**PRINTF_0XDECIMAL**
|
|
|
Prefixing 0x with decimal output is defective and should be corrected.
|
|
|
|
|
|
- **TRAILING_STATEMENTS**
|
|
|
- Trailing statements (for example after any conditional) should be
|
|
|
- on the next line.
|
|
|
- Like::
|
|
|
+ **SPDX_LICENSE_TAG**
|
|
|
+ The source file is missing or has an improper SPDX identifier tag.
|
|
|
+ The Linux kernel requires the precise SPDX identifier in all source files,
|
|
|
+ and it is thoroughly documented in the kernel docs.
|
|
|
|
|
|
- if (x == y) break;
|
|
|
+ See: https://www.kernel.org/doc/html/latest/process/license-rules.html
|
|
|
|
|
|
- should be::
|
|
|
-
|
|
|
- if (x == y)
|
|
|
- break;
|
|
|
+ **TYPO_SPELLING**
|
|
|
+ Some words may have been misspelled. Consider reviewing them.
|