Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1 | .. SPDX-License-Identifier: GPL-2.0-only |
| 2 | |
| 3 | ========== |
| 4 | Checkpatch |
| 5 | ========== |
| 6 | |
| 7 | Checkpatch (scripts/checkpatch.pl) is a perl script which checks for trivial |
| 8 | style violations in patches and optionally corrects them. Checkpatch can |
| 9 | also be run on file contexts and without the kernel tree. |
| 10 | |
| 11 | Checkpatch is not always right. Your judgement takes precedence over checkpatch |
| 12 | messages. If your code looks better with the violations, then its probably |
| 13 | best left alone. |
| 14 | |
| 15 | |
| 16 | Options |
| 17 | ======= |
| 18 | |
| 19 | This section will describe the options checkpatch can be run with. |
| 20 | |
| 21 | Usage:: |
| 22 | |
| 23 | ./scripts/checkpatch.pl [OPTION]... [FILE]... |
| 24 | |
| 25 | Available options: |
| 26 | |
| 27 | - -q, --quiet |
| 28 | |
| 29 | Enable quiet mode. |
| 30 | |
| 31 | - -v, --verbose |
| 32 | Enable verbose mode. Additional verbose test descriptions are output |
| 33 | so as to provide information on why that particular message is shown. |
| 34 | |
| 35 | - --no-tree |
| 36 | |
| 37 | Run checkpatch without the kernel tree. |
| 38 | |
| 39 | - --no-signoff |
| 40 | |
| 41 | Disable the 'Signed-off-by' line check. The sign-off is a simple line at |
| 42 | the end of the explanation for the patch, which certifies that you wrote it |
| 43 | or otherwise have the right to pass it on as an open-source patch. |
| 44 | |
| 45 | Example:: |
| 46 | |
| 47 | Signed-off-by: Random J Developer <random@developer.example.org> |
| 48 | |
| 49 | Setting this flag effectively stops a message for a missing signed-off-by |
| 50 | line in a patch context. |
| 51 | |
| 52 | - --patch |
| 53 | |
| 54 | Treat FILE as a patch. This is the default option and need not be |
| 55 | explicitly specified. |
| 56 | |
| 57 | - --emacs |
| 58 | |
| 59 | Set output to emacs compile window format. This allows emacs users to jump |
| 60 | from the error in the compile window directly to the offending line in the |
| 61 | patch. |
| 62 | |
| 63 | - --terse |
| 64 | |
| 65 | Output only one line per report. |
| 66 | |
| 67 | - --showfile |
| 68 | |
| 69 | Show the diffed file position instead of the input file position. |
| 70 | |
| 71 | - -g, --git |
| 72 | |
| 73 | Treat FILE as a single commit or a git revision range. |
| 74 | |
| 75 | Single commit with: |
| 76 | |
| 77 | - <rev> |
| 78 | - <rev>^ |
| 79 | - <rev>~n |
| 80 | |
| 81 | Multiple commits with: |
| 82 | |
| 83 | - <rev1>..<rev2> |
| 84 | - <rev1>...<rev2> |
| 85 | - <rev>-<count> |
| 86 | |
| 87 | - -f, --file |
| 88 | |
| 89 | Treat FILE as a regular source file. This option must be used when running |
| 90 | checkpatch on source files in the kernel. |
| 91 | |
| 92 | - --subjective, --strict |
| 93 | |
| 94 | Enable stricter tests in checkpatch. By default the tests emitted as CHECK |
| 95 | do not activate by default. Use this flag to activate the CHECK tests. |
| 96 | |
| 97 | - --list-types |
| 98 | |
| 99 | Every message emitted by checkpatch has an associated TYPE. Add this flag |
| 100 | to display all the types in checkpatch. |
| 101 | |
| 102 | Note that when this flag is active, checkpatch does not read the input FILE, |
| 103 | and no message is emitted. Only a list of types in checkpatch is output. |
| 104 | |
| 105 | - --types TYPE(,TYPE2...) |
| 106 | |
| 107 | Only display messages with the given types. |
| 108 | |
| 109 | Example:: |
| 110 | |
| 111 | ./scripts/checkpatch.pl mypatch.patch --types EMAIL_SUBJECT,BRACES |
| 112 | |
| 113 | - --ignore TYPE(,TYPE2...) |
| 114 | |
| 115 | Checkpatch will not emit messages for the specified types. |
| 116 | |
| 117 | Example:: |
| 118 | |
| 119 | ./scripts/checkpatch.pl mypatch.patch --ignore EMAIL_SUBJECT,BRACES |
| 120 | |
| 121 | - --show-types |
| 122 | |
| 123 | By default checkpatch doesn't display the type associated with the messages. |
| 124 | Set this flag to show the message type in the output. |
| 125 | |
| 126 | - --max-line-length=n |
| 127 | |
| 128 | Set the max line length (default 100). If a line exceeds the specified |
| 129 | length, a LONG_LINE message is emitted. |
| 130 | |
| 131 | |
| 132 | The message level is different for patch and file contexts. For patches, |
| 133 | a WARNING is emitted. While a milder CHECK is emitted for files. So for |
| 134 | file contexts, the --strict flag must also be enabled. |
| 135 | |
| 136 | - --min-conf-desc-length=n |
| 137 | |
| 138 | Set the Kconfig entry minimum description length, if shorter, warn. |
| 139 | |
| 140 | - --tab-size=n |
| 141 | |
| 142 | Set the number of spaces for tab (default 8). |
| 143 | |
| 144 | - --root=PATH |
| 145 | |
| 146 | PATH to the kernel tree root. |
| 147 | |
| 148 | This option must be specified when invoking checkpatch from outside |
| 149 | the kernel root. |
| 150 | |
| 151 | - --no-summary |
| 152 | |
| 153 | Suppress the per file summary. |
| 154 | |
| 155 | - --mailback |
| 156 | |
| 157 | Only produce a report in case of Warnings or Errors. Milder Checks are |
| 158 | excluded from this. |
| 159 | |
| 160 | - --summary-file |
| 161 | |
| 162 | Include the filename in summary. |
| 163 | |
| 164 | - --debug KEY=[0|1] |
| 165 | |
| 166 | Turn on/off debugging of KEY, where KEY is one of 'values', 'possible', |
| 167 | 'type', and 'attr' (default is all off). |
| 168 | |
| 169 | - --fix |
| 170 | |
| 171 | This is an EXPERIMENTAL feature. If correctable errors exists, a file |
| 172 | <inputfile>.EXPERIMENTAL-checkpatch-fixes is created which has the |
| 173 | automatically fixable errors corrected. |
| 174 | |
| 175 | - --fix-inplace |
| 176 | |
| 177 | EXPERIMENTAL - Similar to --fix but input file is overwritten with fixes. |
| 178 | |
| 179 | DO NOT USE this flag unless you are absolutely sure and you have a backup |
| 180 | in place. |
| 181 | |
| 182 | - --ignore-perl-version |
| 183 | |
| 184 | Override checking of perl version. Runtime errors maybe encountered after |
| 185 | enabling this flag if the perl version does not meet the minimum specified. |
| 186 | |
| 187 | - --codespell |
| 188 | |
| 189 | Use the codespell dictionary for checking spelling errors. |
| 190 | |
| 191 | - --codespellfile |
| 192 | |
| 193 | Use the specified codespell file. |
| 194 | Default is '/usr/share/codespell/dictionary.txt'. |
| 195 | |
| 196 | - --typedefsfile |
| 197 | |
| 198 | Read additional types from this file. |
| 199 | |
| 200 | - --color[=WHEN] |
| 201 | |
| 202 | Use colors 'always', 'never', or only when output is a terminal ('auto'). |
| 203 | Default is 'auto'. |
| 204 | |
| 205 | - --kconfig-prefix=WORD |
| 206 | |
| 207 | Use WORD as a prefix for Kconfig symbols (default is `CONFIG_`). |
| 208 | |
| 209 | - -h, --help, --version |
| 210 | |
| 211 | Display the help text. |
| 212 | |
| 213 | Message Levels |
| 214 | ============== |
| 215 | |
| 216 | Messages in checkpatch are divided into three levels. The levels of messages |
| 217 | in checkpatch denote the severity of the error. They are: |
| 218 | |
| 219 | - ERROR |
| 220 | |
| 221 | This is the most strict level. Messages of type ERROR must be taken |
| 222 | seriously as they denote things that are very likely to be wrong. |
| 223 | |
| 224 | - WARNING |
| 225 | |
| 226 | This is the next stricter level. Messages of type WARNING requires a |
| 227 | more careful review. But it is milder than an ERROR. |
| 228 | |
| 229 | - CHECK |
| 230 | |
| 231 | This is the mildest level. These are things which may require some thought. |
| 232 | |
| 233 | Type Descriptions |
| 234 | ================= |
| 235 | |
| 236 | This section contains a description of all the message types in checkpatch. |
| 237 | |
| 238 | .. Types in this section are also parsed by checkpatch. |
| 239 | .. The types are grouped into subsections based on use. |
| 240 | |
| 241 | |
| 242 | Allocation style |
| 243 | ---------------- |
| 244 | |
| 245 | **ALLOC_ARRAY_ARGS** |
| 246 | The first argument for kcalloc or kmalloc_array should be the |
| 247 | number of elements. sizeof() as the first argument is generally |
| 248 | wrong. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 249 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 250 | See: https://www.kernel.org/doc/html/latest/core-api/memory-allocation.html |
| 251 | |
| 252 | **ALLOC_SIZEOF_STRUCT** |
| 253 | The allocation style is bad. In general for family of |
| 254 | allocation functions using sizeof() to get memory size, |
| 255 | constructs like:: |
| 256 | |
| 257 | p = alloc(sizeof(struct foo), ...) |
| 258 | |
| 259 | should be:: |
| 260 | |
| 261 | p = alloc(sizeof(*p), ...) |
| 262 | |
| 263 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory |
| 264 | |
| 265 | **ALLOC_WITH_MULTIPLY** |
| 266 | Prefer kmalloc_array/kcalloc over kmalloc/kzalloc with a |
| 267 | sizeof multiply. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 268 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 269 | See: https://www.kernel.org/doc/html/latest/core-api/memory-allocation.html |
| 270 | |
| 271 | |
| 272 | API usage |
| 273 | --------- |
| 274 | |
| 275 | **ARCH_DEFINES** |
| 276 | Architecture specific defines should be avoided wherever |
| 277 | possible. |
| 278 | |
| 279 | **ARCH_INCLUDE_LINUX** |
| 280 | Whenever asm/file.h is included and linux/file.h exists, a |
| 281 | conversion can be made when linux/file.h includes asm/file.h. |
| 282 | However this is not always the case (See signal.h). |
| 283 | This message type is emitted only for includes from arch/. |
| 284 | |
| 285 | **AVOID_BUG** |
| 286 | BUG() or BUG_ON() should be avoided totally. |
| 287 | Use WARN() and WARN_ON() instead, and handle the "impossible" |
| 288 | error condition as gracefully as possible. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 289 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 290 | See: https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on |
| 291 | |
| 292 | **CONSIDER_KSTRTO** |
| 293 | The simple_strtol(), simple_strtoll(), simple_strtoul(), and |
| 294 | simple_strtoull() functions explicitly ignore overflows, which |
| 295 | may lead to unexpected results in callers. The respective kstrtol(), |
| 296 | kstrtoll(), kstrtoul(), and kstrtoull() functions tend to be the |
| 297 | correct replacements. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 298 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 299 | See: https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull |
| 300 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 301 | **CONSTANT_CONVERSION** |
| 302 | Use of __constant_<foo> form is discouraged for the following functions:: |
| 303 | |
| 304 | __constant_cpu_to_be[x] |
| 305 | __constant_cpu_to_le[x] |
| 306 | __constant_be[x]_to_cpu |
| 307 | __constant_le[x]_to_cpu |
| 308 | __constant_htons |
| 309 | __constant_ntohs |
| 310 | |
| 311 | Using any of these outside of include/uapi/ is not preferred as using the |
| 312 | function without __constant_ is identical when the argument is a |
| 313 | constant. |
| 314 | |
| 315 | In big endian systems, the macros like __constant_cpu_to_be32(x) and |
| 316 | cpu_to_be32(x) expand to the same expression:: |
| 317 | |
| 318 | #define __constant_cpu_to_be32(x) ((__force __be32)(__u32)(x)) |
| 319 | #define __cpu_to_be32(x) ((__force __be32)(__u32)(x)) |
| 320 | |
| 321 | In little endian systems, the macros __constant_cpu_to_be32(x) and |
| 322 | cpu_to_be32(x) expand to __constant_swab32 and __swab32. __swab32 |
| 323 | has a __builtin_constant_p check:: |
| 324 | |
| 325 | #define __swab32(x) \ |
| 326 | (__builtin_constant_p((__u32)(x)) ? \ |
| 327 | ___constant_swab32(x) : \ |
| 328 | __fswab32(x)) |
| 329 | |
| 330 | So ultimately they have a special case for constants. |
| 331 | Similar is the case with all of the macros in the list. Thus |
| 332 | using the __constant_... forms are unnecessarily verbose and |
| 333 | not preferred outside of include/uapi. |
| 334 | |
| 335 | See: https://lore.kernel.org/lkml/1400106425.12666.6.camel@joe-AO725/ |
| 336 | |
| 337 | **DEPRECATED_API** |
| 338 | Usage of a deprecated RCU API is detected. It is recommended to replace |
| 339 | old flavourful RCU APIs by their new vanilla-RCU counterparts. |
| 340 | |
| 341 | The full list of available RCU APIs can be viewed from the kernel docs. |
| 342 | |
| 343 | See: https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#full-list-of-rcu-apis |
| 344 | |
| 345 | **DEPRECATED_VARIABLE** |
| 346 | EXTRA_{A,C,CPP,LD}FLAGS are deprecated and should be replaced by the new |
| 347 | flags added via commit f77bf01425b1 ("kbuild: introduce ccflags-y, |
| 348 | asflags-y and ldflags-y"). |
| 349 | |
| 350 | The following conversion scheme maybe used:: |
| 351 | |
| 352 | EXTRA_AFLAGS -> asflags-y |
| 353 | EXTRA_CFLAGS -> ccflags-y |
| 354 | EXTRA_CPPFLAGS -> cppflags-y |
| 355 | EXTRA_LDFLAGS -> ldflags-y |
| 356 | |
| 357 | See: |
| 358 | |
| 359 | 1. https://lore.kernel.org/lkml/20070930191054.GA15876@uranus.ravnborg.org/ |
| 360 | 2. https://lore.kernel.org/lkml/1313384834-24433-12-git-send-email-lacombar@gmail.com/ |
| 361 | 3. https://www.kernel.org/doc/html/latest/kbuild/makefiles.html#compilation-flags |
| 362 | |
| 363 | **DEVICE_ATTR_FUNCTIONS** |
| 364 | The function names used in DEVICE_ATTR is unusual. |
| 365 | Typically, the store and show functions are used with <attr>_store and |
| 366 | <attr>_show, where <attr> is a named attribute variable of the device. |
| 367 | |
| 368 | Consider the following examples:: |
| 369 | |
| 370 | static DEVICE_ATTR(type, 0444, type_show, NULL); |
| 371 | static DEVICE_ATTR(power, 0644, power_show, power_store); |
| 372 | |
| 373 | The function names should preferably follow the above pattern. |
| 374 | |
| 375 | See: https://www.kernel.org/doc/html/latest/driver-api/driver-model/device.html#attributes |
| 376 | |
| 377 | **DEVICE_ATTR_RO** |
| 378 | The DEVICE_ATTR_RO(name) helper macro can be used instead of |
| 379 | DEVICE_ATTR(name, 0444, name_show, NULL); |
| 380 | |
| 381 | Note that the macro automatically appends _show to the named |
| 382 | attribute variable of the device for the show method. |
| 383 | |
| 384 | See: https://www.kernel.org/doc/html/latest/driver-api/driver-model/device.html#attributes |
| 385 | |
| 386 | **DEVICE_ATTR_RW** |
| 387 | The DEVICE_ATTR_RW(name) helper macro can be used instead of |
| 388 | DEVICE_ATTR(name, 0644, name_show, name_store); |
| 389 | |
| 390 | Note that the macro automatically appends _show and _store to the |
| 391 | named attribute variable of the device for the show and store methods. |
| 392 | |
| 393 | See: https://www.kernel.org/doc/html/latest/driver-api/driver-model/device.html#attributes |
| 394 | |
| 395 | **DEVICE_ATTR_WO** |
| 396 | The DEVICE_AATR_WO(name) helper macro can be used instead of |
| 397 | DEVICE_ATTR(name, 0200, NULL, name_store); |
| 398 | |
| 399 | Note that the macro automatically appends _store to the |
| 400 | named attribute variable of the device for the store method. |
| 401 | |
| 402 | See: https://www.kernel.org/doc/html/latest/driver-api/driver-model/device.html#attributes |
| 403 | |
| 404 | **DUPLICATED_SYSCTL_CONST** |
| 405 | Commit d91bff3011cf ("proc/sysctl: add shared variables for range |
| 406 | check") added some shared const variables to be used instead of a local |
| 407 | copy in each source file. |
| 408 | |
| 409 | Consider replacing the sysctl range checking value with the shared |
| 410 | one in include/linux/sysctl.h. The following conversion scheme may |
| 411 | be used:: |
| 412 | |
| 413 | &zero -> SYSCTL_ZERO |
| 414 | &one -> SYSCTL_ONE |
| 415 | &int_max -> SYSCTL_INT_MAX |
| 416 | |
| 417 | See: |
| 418 | |
| 419 | 1. https://lore.kernel.org/lkml/20190430180111.10688-1-mcroce@redhat.com/ |
| 420 | 2. https://lore.kernel.org/lkml/20190531131422.14970-1-mcroce@redhat.com/ |
| 421 | |
| 422 | **ENOSYS** |
| 423 | ENOSYS means that a nonexistent system call was called. |
| 424 | Earlier, it was wrongly used for things like invalid operations on |
| 425 | otherwise valid syscalls. This should be avoided in new code. |
| 426 | |
| 427 | See: https://lore.kernel.org/lkml/5eb299021dec23c1a48fa7d9f2c8b794e967766d.1408730669.git.luto@amacapital.net/ |
| 428 | |
| 429 | **ENOTSUPP** |
| 430 | ENOTSUPP is not a standard error code and should be avoided in new patches. |
| 431 | EOPNOTSUPP should be used instead. |
| 432 | |
| 433 | See: https://lore.kernel.org/netdev/20200510182252.GA411829@lunn.ch/ |
| 434 | |
| 435 | **EXPORT_SYMBOL** |
| 436 | EXPORT_SYMBOL should immediately follow the symbol to be exported. |
| 437 | |
| 438 | **IN_ATOMIC** |
| 439 | in_atomic() is not for driver use so any such use is reported as an ERROR. |
| 440 | Also in_atomic() is often used to determine if sleeping is permitted, |
| 441 | but it is not reliable in this use model. Therefore its use is |
| 442 | strongly discouraged. |
| 443 | |
| 444 | However, in_atomic() is ok for core kernel use. |
| 445 | |
| 446 | See: https://lore.kernel.org/lkml/20080320201723.b87b3732.akpm@linux-foundation.org/ |
| 447 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 448 | **LOCKDEP** |
| 449 | The lockdep_no_validate class was added as a temporary measure to |
| 450 | prevent warnings on conversion of device->sem to device->mutex. |
| 451 | It should not be used for any other purpose. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 452 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 453 | See: https://lore.kernel.org/lkml/1268959062.9440.467.camel@laptop/ |
| 454 | |
| 455 | **MALFORMED_INCLUDE** |
| 456 | The #include statement has a malformed path. This has happened |
| 457 | because the author has included a double slash "//" in the pathname |
| 458 | accidentally. |
| 459 | |
| 460 | **USE_LOCKDEP** |
| 461 | lockdep_assert_held() annotations should be preferred over |
| 462 | assertions based on spin_is_locked() |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 463 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 464 | See: https://www.kernel.org/doc/html/latest/locking/lockdep-design.html#annotations |
| 465 | |
| 466 | **UAPI_INCLUDE** |
| 467 | No #include statements in include/uapi should use a uapi/ path. |
| 468 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 469 | **USLEEP_RANGE** |
| 470 | usleep_range() should be preferred over udelay(). The proper way of |
| 471 | using usleep_range() is mentioned in the kernel docs. |
| 472 | |
| 473 | See: https://www.kernel.org/doc/html/latest/timers/timers-howto.html#delays-information-on-the-various-kernel-delay-sleep-mechanisms |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 474 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 475 | |
| 476 | Comments |
| 477 | -------- |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 478 | |
| 479 | **BLOCK_COMMENT_STYLE** |
| 480 | The comment style is incorrect. The preferred style for multi- |
| 481 | line comments is:: |
| 482 | |
| 483 | /* |
| 484 | * This is the preferred style |
| 485 | * for multi line comments. |
| 486 | */ |
| 487 | |
| 488 | The networking comment style is a bit different, with the first line |
| 489 | not empty like the former:: |
| 490 | |
| 491 | /* This is the preferred comment style |
| 492 | * for files in net/ and drivers/net/ |
| 493 | */ |
| 494 | |
| 495 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting |
| 496 | |
| 497 | **C99_COMMENTS** |
| 498 | C99 style single line comments (//) should not be used. |
| 499 | Prefer the block comment style instead. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 500 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 501 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting |
| 502 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 503 | **DATA_RACE** |
| 504 | Applications of data_race() should have a comment so as to document the |
| 505 | reasoning behind why it was deemed safe. |
| 506 | |
| 507 | See: https://lore.kernel.org/lkml/20200401101714.44781-1-elver@google.com/ |
| 508 | |
| 509 | **FSF_MAILING_ADDRESS** |
| 510 | Kernel maintainers reject new instances of the GPL boilerplate paragraph |
| 511 | directing people to write to the FSF for a copy of the GPL, since the |
| 512 | FSF has moved in the past and may do so again. |
| 513 | So do not write paragraphs about writing to the Free Software Foundation's |
| 514 | mailing address. |
| 515 | |
| 516 | See: https://lore.kernel.org/lkml/20131006222342.GT19510@leaf/ |
| 517 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 518 | |
| 519 | Commit message |
| 520 | -------------- |
| 521 | |
| 522 | **BAD_SIGN_OFF** |
| 523 | The signed-off-by line does not fall in line with the standards |
| 524 | specified by the community. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 525 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 526 | See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1 |
| 527 | |
| 528 | **BAD_STABLE_ADDRESS_STYLE** |
| 529 | The email format for stable is incorrect. |
| 530 | Some valid options for stable address are:: |
| 531 | |
| 532 | 1. stable@vger.kernel.org |
| 533 | 2. stable@kernel.org |
| 534 | |
| 535 | For adding version info, the following comment style should be used:: |
| 536 | |
| 537 | stable@vger.kernel.org # version info |
| 538 | |
| 539 | **COMMIT_COMMENT_SYMBOL** |
| 540 | Commit log lines starting with a '#' are ignored by git as |
| 541 | comments. To solve this problem addition of a single space |
| 542 | infront of the log line is enough. |
| 543 | |
| 544 | **COMMIT_MESSAGE** |
| 545 | The patch is missing a commit description. A brief |
| 546 | description of the changes made by the patch should be added. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 547 | |
| 548 | See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes |
| 549 | |
| 550 | **EMAIL_SUBJECT** |
| 551 | Naming the tool that found the issue is not very useful in the |
| 552 | subject line. A good subject line summarizes the change that |
| 553 | the patch brings. |
| 554 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 555 | See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes |
| 556 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 557 | **FROM_SIGN_OFF_MISMATCH** |
| 558 | The author's email does not match with that in the Signed-off-by: |
| 559 | line(s). This can be sometimes caused due to an improperly configured |
| 560 | email client. |
| 561 | |
| 562 | This message is emitted due to any of the following reasons:: |
| 563 | |
| 564 | - The email names do not match. |
| 565 | - The email addresses do not match. |
| 566 | - The email subaddresses do not match. |
| 567 | - The email comments do not match. |
| 568 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 569 | **MISSING_SIGN_OFF** |
| 570 | The patch is missing a Signed-off-by line. A signed-off-by |
| 571 | line should be added according to Developer's certificate of |
| 572 | Origin. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 573 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 574 | See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin |
| 575 | |
| 576 | **NO_AUTHOR_SIGN_OFF** |
| 577 | The author of the patch has not signed off the patch. It is |
| 578 | required that a simple sign off line should be present at the |
| 579 | end of explanation of the patch to denote that the author has |
| 580 | written it or otherwise has the rights to pass it on as an open |
| 581 | source patch. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 582 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 583 | See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin |
| 584 | |
| 585 | **DIFF_IN_COMMIT_MSG** |
| 586 | Avoid having diff content in commit message. |
| 587 | This causes problems when one tries to apply a file containing both |
| 588 | the changelog and the diff because patch(1) tries to apply the diff |
| 589 | which it found in the changelog. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 590 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 591 | See: https://lore.kernel.org/lkml/20150611134006.9df79a893e3636019ad2759e@linux-foundation.org/ |
| 592 | |
| 593 | **GERRIT_CHANGE_ID** |
| 594 | To be picked up by gerrit, the footer of the commit message might |
| 595 | have a Change-Id like:: |
| 596 | |
| 597 | Change-Id: Ic8aaa0728a43936cd4c6e1ed590e01ba8f0fbf5b |
| 598 | Signed-off-by: A. U. Thor <author@example.com> |
| 599 | |
| 600 | The Change-Id line must be removed before submitting. |
| 601 | |
| 602 | **GIT_COMMIT_ID** |
| 603 | The proper way to reference a commit id is: |
| 604 | commit <12+ chars of sha1> ("<title line>") |
| 605 | |
| 606 | An example may be:: |
| 607 | |
| 608 | Commit e21d2170f36602ae2708 ("video: remove unnecessary |
| 609 | platform_set_drvdata()") removed the unnecessary |
| 610 | platform_set_drvdata(), but left the variable "dev" unused, |
| 611 | delete it. |
| 612 | |
| 613 | See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes |
| 614 | |
| 615 | |
| 616 | Comparison style |
| 617 | ---------------- |
| 618 | |
| 619 | **ASSIGN_IN_IF** |
| 620 | Do not use assignments in if condition. |
| 621 | Example:: |
| 622 | |
| 623 | if ((foo = bar(...)) < BAZ) { |
| 624 | |
| 625 | should be written as:: |
| 626 | |
| 627 | foo = bar(...); |
| 628 | if (foo < BAZ) { |
| 629 | |
| 630 | **BOOL_COMPARISON** |
| 631 | Comparisons of A to true and false are better written |
| 632 | as A and !A. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 633 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 634 | See: https://lore.kernel.org/lkml/1365563834.27174.12.camel@joe-AO722/ |
| 635 | |
| 636 | **COMPARISON_TO_NULL** |
| 637 | Comparisons to NULL in the form (foo == NULL) or (foo != NULL) |
| 638 | are better written as (!foo) and (foo). |
| 639 | |
| 640 | **CONSTANT_COMPARISON** |
| 641 | Comparisons with a constant or upper case identifier on the left |
| 642 | side of the test should be avoided. |
| 643 | |
| 644 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 645 | Indentation and Line Breaks |
| 646 | --------------------------- |
| 647 | |
| 648 | **CODE_INDENT** |
| 649 | Code indent should use tabs instead of spaces. |
| 650 | Outside of comments, documentation and Kconfig, |
| 651 | spaces are never used for indentation. |
| 652 | |
| 653 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation |
| 654 | |
| 655 | **DEEP_INDENTATION** |
| 656 | Indentation with 6 or more tabs usually indicate overly indented |
| 657 | code. |
| 658 | |
| 659 | It is suggested to refactor excessive indentation of |
| 660 | if/else/for/do/while/switch statements. |
| 661 | |
| 662 | See: https://lore.kernel.org/lkml/1328311239.21255.24.camel@joe2Laptop/ |
| 663 | |
| 664 | **SWITCH_CASE_INDENT_LEVEL** |
| 665 | switch should be at the same indent as case. |
| 666 | Example:: |
| 667 | |
| 668 | switch (suffix) { |
| 669 | case 'G': |
| 670 | case 'g': |
| 671 | mem <<= 30; |
| 672 | break; |
| 673 | case 'M': |
| 674 | case 'm': |
| 675 | mem <<= 20; |
| 676 | break; |
| 677 | case 'K': |
| 678 | case 'k': |
| 679 | mem <<= 10; |
| 680 | fallthrough; |
| 681 | default: |
| 682 | break; |
| 683 | } |
| 684 | |
| 685 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation |
| 686 | |
| 687 | **LONG_LINE** |
| 688 | The line has exceeded the specified maximum length. |
| 689 | To use a different maximum line length, the --max-line-length=n option |
| 690 | may be added while invoking checkpatch. |
| 691 | |
| 692 | Earlier, the default line length was 80 columns. Commit bdc48fa11e46 |
| 693 | ("checkpatch/coding-style: deprecate 80-column warning") increased the |
| 694 | limit to 100 columns. This is not a hard limit either and it's |
| 695 | preferable to stay within 80 columns whenever possible. |
| 696 | |
| 697 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings |
| 698 | |
| 699 | **LONG_LINE_STRING** |
| 700 | A string starts before but extends beyond the maximum line length. |
| 701 | To use a different maximum line length, the --max-line-length=n option |
| 702 | may be added while invoking checkpatch. |
| 703 | |
| 704 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings |
| 705 | |
| 706 | **LONG_LINE_COMMENT** |
| 707 | A comment starts before but extends beyond the maximum line length. |
| 708 | To use a different maximum line length, the --max-line-length=n option |
| 709 | may be added while invoking checkpatch. |
| 710 | |
| 711 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings |
| 712 | |
| 713 | **SPLIT_STRING** |
| 714 | Quoted strings that appear as messages in userspace and can be |
| 715 | grepped, should not be split across multiple lines. |
| 716 | |
| 717 | See: https://lore.kernel.org/lkml/20120203052727.GA15035@leaf/ |
| 718 | |
| 719 | **MULTILINE_DEREFERENCE** |
| 720 | A single dereferencing identifier spanned on multiple lines like:: |
| 721 | |
| 722 | struct_identifier->member[index]. |
| 723 | member = <foo>; |
| 724 | |
| 725 | is generally hard to follow. It can easily lead to typos and so makes |
| 726 | the code vulnerable to bugs. |
| 727 | |
| 728 | If fixing the multiple line dereferencing leads to an 80 column |
| 729 | violation, then either rewrite the code in a more simple way or if the |
| 730 | starting part of the dereferencing identifier is the same and used at |
| 731 | multiple places then store it in a temporary variable, and use that |
| 732 | temporary variable only at all the places. For example, if there are |
| 733 | two dereferencing identifiers:: |
| 734 | |
| 735 | member1->member2->member3.foo1; |
| 736 | member1->member2->member3.foo2; |
| 737 | |
| 738 | then store the member1->member2->member3 part in a temporary variable. |
| 739 | It not only helps to avoid the 80 column violation but also reduces |
| 740 | the program size by removing the unnecessary dereferences. |
| 741 | |
| 742 | But if none of the above methods work then ignore the 80 column |
| 743 | violation because it is much easier to read a dereferencing identifier |
| 744 | on a single line. |
| 745 | |
| 746 | **TRAILING_STATEMENTS** |
| 747 | Trailing statements (for example after any conditional) should be |
| 748 | on the next line. |
| 749 | Statements, such as:: |
| 750 | |
| 751 | if (x == y) break; |
| 752 | |
| 753 | should be:: |
| 754 | |
| 755 | if (x == y) |
| 756 | break; |
| 757 | |
| 758 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 759 | Macros, Attributes and Symbols |
| 760 | ------------------------------ |
| 761 | |
| 762 | **ARRAY_SIZE** |
| 763 | The ARRAY_SIZE(foo) macro should be preferred over |
| 764 | sizeof(foo)/sizeof(foo[0]) for finding number of elements in an |
| 765 | array. |
| 766 | |
| 767 | The macro is defined in include/linux/kernel.h:: |
| 768 | |
| 769 | #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) |
| 770 | |
| 771 | **AVOID_EXTERNS** |
| 772 | Function prototypes don't need to be declared extern in .h |
| 773 | files. It's assumed by the compiler and is unnecessary. |
| 774 | |
| 775 | **AVOID_L_PREFIX** |
| 776 | Local symbol names that are prefixed with `.L` should be avoided, |
| 777 | as this has special meaning for the assembler; a symbol entry will |
| 778 | not be emitted into the symbol table. This can prevent `objtool` |
| 779 | from generating correct unwind info. |
| 780 | |
| 781 | Symbols with STB_LOCAL binding may still be used, and `.L` prefixed |
| 782 | local symbol names are still generally usable within a function, |
| 783 | but `.L` prefixed local symbol names should not be used to denote |
| 784 | the beginning or end of code regions via |
| 785 | `SYM_CODE_START_LOCAL`/`SYM_CODE_END` |
| 786 | |
| 787 | **BIT_MACRO** |
| 788 | Defines like: 1 << <digit> could be BIT(digit). |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 789 | The BIT() macro is defined via include/linux/bits.h:: |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 790 | |
| 791 | #define BIT(nr) (1UL << (nr)) |
| 792 | |
| 793 | **CONST_READ_MOSTLY** |
| 794 | When a variable is tagged with the __read_mostly annotation, it is a |
| 795 | signal to the compiler that accesses to the variable will be mostly |
| 796 | reads and rarely(but NOT never) a write. |
| 797 | |
| 798 | const __read_mostly does not make any sense as const data is already |
| 799 | read-only. The __read_mostly annotation thus should be removed. |
| 800 | |
| 801 | **DATE_TIME** |
| 802 | It is generally desirable that building the same source code with |
| 803 | the same set of tools is reproducible, i.e. the output is always |
| 804 | exactly the same. |
| 805 | |
| 806 | The kernel does *not* use the ``__DATE__`` and ``__TIME__`` macros, |
| 807 | and enables warnings if they are used as they can lead to |
| 808 | non-deterministic builds. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 809 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 810 | See: https://www.kernel.org/doc/html/latest/kbuild/reproducible-builds.html#timestamps |
| 811 | |
| 812 | **DEFINE_ARCH_HAS** |
| 813 | The ARCH_HAS_xyz and ARCH_HAVE_xyz patterns are wrong. |
| 814 | |
| 815 | For big conceptual features use Kconfig symbols instead. And for |
| 816 | smaller things where we have compatibility fallback functions but |
| 817 | want architectures able to override them with optimized ones, we |
| 818 | should either use weak functions (appropriate for some cases), or |
| 819 | the symbol that protects them should be the same symbol we use. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 820 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 821 | See: https://lore.kernel.org/lkml/CA+55aFycQ9XJvEOsiM3txHL5bjUc8CeKWJNR_H+MiicaddB42Q@mail.gmail.com/ |
| 822 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 823 | **DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON** |
| 824 | do {} while(0) macros should not have a trailing semicolon. |
| 825 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 826 | **INIT_ATTRIBUTE** |
| 827 | Const init definitions should use __initconst instead of |
| 828 | __initdata. |
| 829 | |
| 830 | Similarly init definitions without const require a separate |
| 831 | use of const. |
| 832 | |
| 833 | **INLINE_LOCATION** |
| 834 | The inline keyword should sit between storage class and type. |
| 835 | |
| 836 | For example, the following segment:: |
| 837 | |
| 838 | inline static int example_function(void) |
| 839 | { |
| 840 | ... |
| 841 | } |
| 842 | |
| 843 | should be:: |
| 844 | |
| 845 | static inline int example_function(void) |
| 846 | { |
| 847 | ... |
| 848 | } |
| 849 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 850 | **MISPLACED_INIT** |
| 851 | It is possible to use section markers on variables in a way |
| 852 | which gcc doesn't understand (or at least not the way the |
| 853 | developer intended):: |
| 854 | |
| 855 | static struct __initdata samsung_pll_clock exynos4_plls[nr_plls] = { |
| 856 | |
| 857 | does not put exynos4_plls in the .initdata section. The __initdata |
| 858 | marker can be virtually anywhere on the line, except right after |
| 859 | "struct". The preferred location is before the "=" sign if there is |
| 860 | one, or before the trailing ";" otherwise. |
| 861 | |
| 862 | See: https://lore.kernel.org/lkml/1377655732.3619.19.camel@joe-AO722/ |
| 863 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 864 | **MULTISTATEMENT_MACRO_USE_DO_WHILE** |
| 865 | Macros with multiple statements should be enclosed in a |
| 866 | do - while block. Same should also be the case for macros |
| 867 | starting with `if` to avoid logic defects:: |
| 868 | |
| 869 | #define macrofun(a, b, c) \ |
| 870 | do { \ |
| 871 | if (a == 5) \ |
| 872 | do_this(b, c); \ |
| 873 | } while (0) |
| 874 | |
| 875 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl |
| 876 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 877 | **PREFER_FALLTHROUGH** |
| 878 | Use the `fallthrough;` pseudo keyword instead of |
| 879 | `/* fallthrough */` like comments. |
| 880 | |
| 881 | **TRAILING_SEMICOLON** |
| 882 | Macro definition should not end with a semicolon. The macro |
| 883 | invocation style should be consistent with function calls. |
| 884 | This can prevent any unexpected code paths:: |
| 885 | |
| 886 | #define MAC do_something; |
| 887 | |
| 888 | If this macro is used within a if else statement, like:: |
| 889 | |
| 890 | if (some_condition) |
| 891 | MAC; |
| 892 | |
| 893 | else |
| 894 | do_something; |
| 895 | |
| 896 | Then there would be a compilation error, because when the macro is |
| 897 | expanded there are two trailing semicolons, so the else branch gets |
| 898 | orphaned. |
| 899 | |
| 900 | See: https://lore.kernel.org/lkml/1399671106.2912.21.camel@joe-AO725/ |
| 901 | |
| 902 | **SINGLE_STATEMENT_DO_WHILE_MACRO** |
| 903 | For the multi-statement macros, it is necessary to use the do-while |
| 904 | loop to avoid unpredictable code paths. The do-while loop helps to |
| 905 | group the multiple statements into a single one so that a |
| 906 | function-like macro can be used as a function only. |
| 907 | |
| 908 | But for the single statement macros, it is unnecessary to use the |
| 909 | do-while loop. Although the code is syntactically correct but using |
| 910 | the do-while loop is redundant. So remove the do-while loop for single |
| 911 | statement macros. |
| 912 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 913 | **WEAK_DECLARATION** |
| 914 | Using weak declarations like __attribute__((weak)) or __weak |
| 915 | can have unintended link defects. Avoid using them. |
| 916 | |
| 917 | |
| 918 | Functions and Variables |
| 919 | ----------------------- |
| 920 | |
| 921 | **CAMELCASE** |
| 922 | Avoid CamelCase Identifiers. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 923 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 924 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#naming |
| 925 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 926 | **CONST_CONST** |
| 927 | Using `const <type> const *` is generally meant to be |
| 928 | written `const <type> * const`. |
| 929 | |
| 930 | **CONST_STRUCT** |
| 931 | Using const is generally a good idea. Checkpatch reads |
| 932 | a list of frequently used structs that are always or |
| 933 | almost always constant. |
| 934 | |
| 935 | The existing structs list can be viewed from |
| 936 | `scripts/const_structs.checkpatch`. |
| 937 | |
| 938 | See: https://lore.kernel.org/lkml/alpine.DEB.2.10.1608281509480.3321@hadrien/ |
| 939 | |
| 940 | **EMBEDDED_FUNCTION_NAME** |
| 941 | Embedded function names are less appropriate to use as |
| 942 | refactoring can cause function renaming. Prefer the use of |
| 943 | "%s", __func__ to embedded function names. |
| 944 | |
| 945 | Note that this does not work with -f (--file) checkpatch option |
| 946 | as it depends on patch context providing the function name. |
| 947 | |
| 948 | **FUNCTION_ARGUMENTS** |
| 949 | This warning is emitted due to any of the following reasons: |
| 950 | |
| 951 | 1. Arguments for the function declaration do not follow |
| 952 | the identifier name. Example:: |
| 953 | |
| 954 | void foo |
| 955 | (int bar, int baz) |
| 956 | |
| 957 | This should be corrected to:: |
| 958 | |
| 959 | void foo(int bar, int baz) |
| 960 | |
| 961 | 2. Some arguments for the function definition do not |
| 962 | have an identifier name. Example:: |
| 963 | |
| 964 | void foo(int) |
| 965 | |
| 966 | All arguments should have identifier names. |
| 967 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 968 | **FUNCTION_WITHOUT_ARGS** |
| 969 | Function declarations without arguments like:: |
| 970 | |
| 971 | int foo() |
| 972 | |
| 973 | should be:: |
| 974 | |
| 975 | int foo(void) |
| 976 | |
| 977 | **GLOBAL_INITIALISERS** |
| 978 | Global variables should not be initialized explicitly to |
| 979 | 0 (or NULL, false, etc.). Your compiler (or rather your |
| 980 | loader, which is responsible for zeroing out the relevant |
| 981 | sections) automatically does it for you. |
| 982 | |
| 983 | **INITIALISED_STATIC** |
| 984 | Static variables should not be initialized explicitly to zero. |
| 985 | Your compiler (or rather your loader) automatically does |
| 986 | it for you. |
| 987 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 988 | **MULTIPLE_ASSIGNMENTS** |
| 989 | Multiple assignments on a single line makes the code unnecessarily |
| 990 | complicated. So on a single line assign value to a single variable |
| 991 | only, this makes the code more readable and helps avoid typos. |
| 992 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 993 | **RETURN_PARENTHESES** |
| 994 | return is not a function and as such doesn't need parentheses:: |
| 995 | |
| 996 | return (bar); |
| 997 | |
| 998 | can simply be:: |
| 999 | |
| 1000 | return bar; |
| 1001 | |
| 1002 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1003 | Permissions |
| 1004 | ----------- |
| 1005 | |
| 1006 | **DEVICE_ATTR_PERMS** |
| 1007 | The permissions used in DEVICE_ATTR are unusual. |
| 1008 | Typically only three permissions are used - 0644 (RW), 0444 (RO) |
| 1009 | and 0200 (WO). |
| 1010 | |
| 1011 | See: https://www.kernel.org/doc/html/latest/filesystems/sysfs.html#attributes |
| 1012 | |
| 1013 | **EXECUTE_PERMISSIONS** |
| 1014 | There is no reason for source files to be executable. The executable |
| 1015 | bit can be removed safely. |
| 1016 | |
| 1017 | **EXPORTED_WORLD_WRITABLE** |
| 1018 | Exporting world writable sysfs/debugfs files is usually a bad thing. |
| 1019 | When done arbitrarily they can introduce serious security bugs. |
| 1020 | In the past, some of the debugfs vulnerabilities would seemingly allow |
| 1021 | any local user to write arbitrary values into device registers - a |
| 1022 | situation from which little good can be expected to emerge. |
| 1023 | |
| 1024 | See: https://lore.kernel.org/linux-arm-kernel/cover.1296818921.git.segoon@openwall.com/ |
| 1025 | |
| 1026 | **NON_OCTAL_PERMISSIONS** |
| 1027 | Permission bits should use 4 digit octal permissions (like 0700 or 0444). |
| 1028 | Avoid using any other base like decimal. |
| 1029 | |
| 1030 | **SYMBOLIC_PERMS** |
| 1031 | Permission bits in the octal form are more readable and easier to |
| 1032 | understand than their symbolic counterparts because many command-line |
| 1033 | tools use this notation. Experienced kernel developers have been using |
| 1034 | these traditional Unix permission bits for decades and so they find it |
| 1035 | easier to understand the octal notation than the symbolic macros. |
| 1036 | For example, it is harder to read S_IWUSR|S_IRUGO than 0644, which |
| 1037 | obscures the developer's intent rather than clarifying it. |
| 1038 | |
| 1039 | See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/ |
| 1040 | |
| 1041 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1042 | Spacing and Brackets |
| 1043 | -------------------- |
| 1044 | |
| 1045 | **ASSIGNMENT_CONTINUATIONS** |
| 1046 | Assignment operators should not be written at the start of a |
| 1047 | line but should follow the operand at the previous line. |
| 1048 | |
| 1049 | **BRACES** |
| 1050 | The placement of braces is stylistically incorrect. |
| 1051 | The preferred way is to put the opening brace last on the line, |
| 1052 | and put the closing brace first:: |
| 1053 | |
| 1054 | if (x is true) { |
| 1055 | we do y |
| 1056 | } |
| 1057 | |
| 1058 | This applies for all non-functional blocks. |
| 1059 | However, there is one special case, namely functions: they have the |
| 1060 | opening brace at the beginning of the next line, thus:: |
| 1061 | |
| 1062 | int function(int x) |
| 1063 | { |
| 1064 | body of function |
| 1065 | } |
| 1066 | |
| 1067 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces |
| 1068 | |
| 1069 | **BRACKET_SPACE** |
| 1070 | Whitespace before opening bracket '[' is prohibited. |
| 1071 | There are some exceptions: |
| 1072 | |
| 1073 | 1. With a type on the left:: |
| 1074 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1075 | int [] a; |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1076 | |
| 1077 | 2. At the beginning of a line for slice initialisers:: |
| 1078 | |
| 1079 | [0...10] = 5, |
| 1080 | |
| 1081 | 3. Inside a curly brace:: |
| 1082 | |
| 1083 | = { [0...10] = 5 } |
| 1084 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1085 | **CONCATENATED_STRING** |
| 1086 | Concatenated elements should have a space in between. |
| 1087 | Example:: |
| 1088 | |
| 1089 | printk(KERN_INFO"bar"); |
| 1090 | |
| 1091 | should be:: |
| 1092 | |
| 1093 | printk(KERN_INFO "bar"); |
| 1094 | |
| 1095 | **ELSE_AFTER_BRACE** |
| 1096 | `else {` should follow the closing block `}` on the same line. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1097 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1098 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces |
| 1099 | |
| 1100 | **LINE_SPACING** |
| 1101 | Vertical space is wasted given the limited number of lines an |
| 1102 | editor window can display when multiple blank lines are used. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1103 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1104 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces |
| 1105 | |
| 1106 | **OPEN_BRACE** |
| 1107 | The opening brace should be following the function definitions on the |
| 1108 | next line. For any non-functional block it should be on the same line |
| 1109 | as the last construct. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1110 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1111 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces |
| 1112 | |
| 1113 | **POINTER_LOCATION** |
| 1114 | When using pointer data or a function that returns a pointer type, |
| 1115 | the preferred use of * is adjacent to the data name or function name |
| 1116 | and not adjacent to the type name. |
| 1117 | Examples:: |
| 1118 | |
| 1119 | char *linux_banner; |
| 1120 | unsigned long long memparse(char *ptr, char **retptr); |
| 1121 | char *match_strdup(substring_t *s); |
| 1122 | |
| 1123 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces |
| 1124 | |
| 1125 | **SPACING** |
| 1126 | Whitespace style used in the kernel sources is described in kernel docs. |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1127 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1128 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1129 | |
| 1130 | **TRAILING_WHITESPACE** |
| 1131 | Trailing whitespace should always be removed. |
| 1132 | Some editors highlight the trailing whitespace and cause visual |
| 1133 | distractions when editing files. |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1134 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1135 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#spaces |
| 1136 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1137 | **UNNECESSARY_PARENTHESES** |
| 1138 | Parentheses are not required in the following cases: |
| 1139 | |
| 1140 | 1. Function pointer uses:: |
| 1141 | |
| 1142 | (foo->bar)(); |
| 1143 | |
| 1144 | could be:: |
| 1145 | |
| 1146 | foo->bar(); |
| 1147 | |
| 1148 | 2. Comparisons in if:: |
| 1149 | |
| 1150 | if ((foo->bar) && (foo->baz)) |
| 1151 | if ((foo == bar)) |
| 1152 | |
| 1153 | could be:: |
| 1154 | |
| 1155 | if (foo->bar && foo->baz) |
| 1156 | if (foo == bar) |
| 1157 | |
| 1158 | 3. addressof/dereference single Lvalues:: |
| 1159 | |
| 1160 | &(foo->bar) |
| 1161 | *(foo->bar) |
| 1162 | |
| 1163 | could be:: |
| 1164 | |
| 1165 | &foo->bar |
| 1166 | *foo->bar |
| 1167 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1168 | **WHILE_AFTER_BRACE** |
| 1169 | while should follow the closing bracket on the same line:: |
| 1170 | |
| 1171 | do { |
| 1172 | ... |
| 1173 | } while(something); |
| 1174 | |
| 1175 | See: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces |
| 1176 | |
| 1177 | |
| 1178 | Others |
| 1179 | ------ |
| 1180 | |
| 1181 | **CONFIG_DESCRIPTION** |
| 1182 | Kconfig symbols should have a help text which fully describes |
| 1183 | it. |
| 1184 | |
| 1185 | **CORRUPTED_PATCH** |
| 1186 | The patch seems to be corrupted or lines are wrapped. |
| 1187 | Please regenerate the patch file before sending it to the maintainer. |
| 1188 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1189 | **CVS_KEYWORD** |
| 1190 | Since linux moved to git, the CVS markers are no longer used. |
| 1191 | So, CVS style keywords ($Id$, $Revision$, $Log$) should not be |
| 1192 | added. |
| 1193 | |
| 1194 | **DEFAULT_NO_BREAK** |
| 1195 | switch default case is sometimes written as "default:;". This can |
| 1196 | cause new cases added below default to be defective. |
| 1197 | |
| 1198 | A "break;" should be added after empty default statement to avoid |
| 1199 | unwanted fallthrough. |
| 1200 | |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1201 | **DOS_LINE_ENDINGS** |
| 1202 | For DOS-formatted patches, there are extra ^M symbols at the end of |
| 1203 | the line. These should be removed. |
| 1204 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1205 | **DT_SCHEMA_BINDING_PATCH** |
| 1206 | DT bindings moved to a json-schema based format instead of |
| 1207 | freeform text. |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1208 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1209 | See: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-schema.html |
| 1210 | |
| 1211 | **DT_SPLIT_BINDING_PATCH** |
| 1212 | Devicetree bindings should be their own patch. This is because |
| 1213 | bindings are logically independent from a driver implementation, |
| 1214 | they have a different maintainer (even though they often |
| 1215 | are applied via the same tree), and it makes for a cleaner history in the |
| 1216 | DT only tree created with git-filter-branch. |
| 1217 | |
| 1218 | See: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters |
| 1219 | |
| 1220 | **EMBEDDED_FILENAME** |
| 1221 | Embedding the complete filename path inside the file isn't particularly |
| 1222 | useful as often the path is moved around and becomes incorrect. |
| 1223 | |
| 1224 | **FILE_PATH_CHANGES** |
| 1225 | Whenever files are added, moved, or deleted, the MAINTAINERS file |
| 1226 | patterns can be out of sync or outdated. |
| 1227 | |
| 1228 | So MAINTAINERS might need updating in these cases. |
| 1229 | |
| 1230 | **MEMSET** |
| 1231 | The memset use appears to be incorrect. This may be caused due to |
| 1232 | badly ordered parameters. Please recheck the usage. |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1233 | |
| 1234 | **NOT_UNIFIED_DIFF** |
| 1235 | The patch file does not appear to be in unified-diff format. Please |
| 1236 | regenerate the patch file before sending it to the maintainer. |
| 1237 | |
| 1238 | **PRINTF_0XDECIMAL** |
| 1239 | Prefixing 0x with decimal output is defective and should be corrected. |
| 1240 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1241 | **SPDX_LICENSE_TAG** |
| 1242 | The source file is missing or has an improper SPDX identifier tag. |
| 1243 | The Linux kernel requires the precise SPDX identifier in all source files, |
| 1244 | and it is thoroughly documented in the kernel docs. |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1245 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1246 | See: https://www.kernel.org/doc/html/latest/process/license-rules.html |
Tom Rini | a5330a1 | 2021-08-03 08:31:56 -0400 | [diff] [blame] | 1247 | |
Simon Glass | d2e6dd6 | 2022-01-23 12:55:11 -0700 | [diff] [blame] | 1248 | **TYPO_SPELLING** |
| 1249 | Some words may have been misspelled. Consider reviewing them. |