[lkml]   [2022]   [Aug]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
Subject[PATCH] docs: checkpatch: add some new checkpatch documentation messages
Added and documented the following new message types:
- IF_0
- IF_1

Signed-off-by: Utkarsh Verma <>
Documentation/dev-tools/checkpatch.rst | 300 +++++++++++++++++++++++++
1 file changed, 300 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index b52452bc2963..78abcadb5228 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -445,6 +445,34 @@ API usage


+ Doing time comparison by directly using relational operators with jiffies
+ or get_jiffies_64() is wrong. Example::
+ if (jiffies > timeout)
+ do_something;
+ The above code is wrong because when the jiffies value reaches its maximum
+ limit, then it overflows and wraps around zero, so the above code will
+ not work as intended. To correctly handle this jiffies overflow condition,
+ the kernel provides some helper macros defined in <linux/jiffies.h>.
+ Some of the important macros are::
+ int time_after(unsigned long a, unsigned long b);
+ int time_before(unsigned long a, unsigned long b);
+ int time_after_eq(unsigned long a, unsigned long b);
+ int time_before_eq(unsigned long a, unsigned long b);
+ So the above code can be corrected by::
+ if (time_after(jiffies, timeout))
+ do_something;
+ Also, by using these macros, the code is easier to maintain and
+ future-proof because if the timer wrap changes in the future, then there
+ will be no need to alter the driver code. So it is strongly recommended
+ to always use these macros instead of direct comparison with jiffies.
The lockdep_no_validate class was added as a temporary measure to
prevent warnings on conversion of device->sem to device->mutex.
@@ -452,11 +480,68 @@ API usage


+ Passing large arguments to udelay() results in integer overflow in the
+ internal loop calculation of udelay() and the driver code will display
+ an error of unresolved symbol, __bad_udelay.
+ Different CPU architectures have different threshold values for udelay()
+ after which they call __bad_udelay(). For ARM architecture, the threshold
+ value is 2000us. For the exact maths behind this limit see
+ arch/arm/include/asm/delay.h
+ This checkpatch warning is triggered when the argument passed to udelay()
+ is greater than 2000us. Though for some architectures this threshold
+ value is more than 2000us, but as a general rule if the wanted delay is
+ in thousand of micro seconds, then use mdelay().
+ mdelay() is a wrapper around udelay() that accounts for the overflow
+ condition when passing large arguments to udelay(). Also, note that the
+ udelay() and mdelay() are busy-waiting, other tasks cannot run during
+ that time. So if the hardware supports hrtimers, then a better approach
+ would be to use usleep_range() function. For delaying 10us - 20ms,
+ usleep_range() is the recommended API. But make a change to usleep_range()
+ only if the hardware supports hrtimers, along with proper testing on a
+ real hardware.
+ If the delay is more than 20ms then use msleep(). msleep() is the
+ recommended API for delaying more than 20ms because it is implemented
+ using the jiffies timer and does not involve busy-waiting.
+ See:
The #include statement has a malformed path. This has happened
because the author has included a double slash "//" in the pathname

+ **MSLEEP**
+ msleep() is implemented using the jiffies counter. When msleep() is
+ called it stops for a minimum of 2 jiffies. So, depending on the value
+ of HZ the mleep will atleast stop for 1-20ms. In worst case, if HZ=100
+ then msleep() will delay for a minimum of 20ms
+ ( So for
+ sleeping for 10us-20ms it is recommended to use usleep_range() and not
+ msleep().
+ But ignore this warning, if the change to usleep_range() cannot be tested
+ on a real hardware.
+ usleep_range() is implemented using the hrtimer. Some hardware doesn't
+ support hrtimer, so no sense in making the change to usleep_range().
+ Also, the min and max value in usleep_range(unsigned long min, unsigned
+ long max) must be selected by understanding the driver code, and the
+ hardware with lots of testing and verification. One can see these timing
+ changes that others have made to get some approximate min and max values,
+ and then test to get the best possible values.
+ See:
+ 1.
+ 2.
+ 3.
lockdep_assert_held() annotations should be preferred over
assertions based on spin_is_locked()
@@ -661,6 +746,30 @@ Indentation and Line Breaks


+ goto labels either should not have any indentation or only a single
+ space indentation (
+ The Linux Kernel Coding Style does not indent the goto labels. Since
+ the program control can directly jump from one part of the program to a
+ pre-defined goto label, so making these labels easy to spot is important.
+ By flushing the goto labels to the left, these labels are more visible
+ and easier to identify.
+ Since the checkpatch only uses the regular expressions for finding the
+ possible coding style violation in a patch, and does not track the logic
+ or flow of the program, so there can be some false positives. Though for
+ this rule it is rare, still do not blindly follow the checkpatch advice.
+ Suppose if there is a conditional (ternary) operator across multiple
+ lines and there is no space around the “:” then this warning can be
+ triggered. Example::
+ return_value = (function1(value1) && function2(value2)) ?
+ NULL: some_other_value;
+ Here the checkpatch will give this label warning along with the spacing
+ warning.
switch should be at the same indent as case.
@@ -823,6 +932,19 @@ Macros, Attributes and Symbols
do {} while(0) macros should not have a trailing semicolon.

+ **IF_0**
+ The code enclosed within #if 0 and #endif is not executed and is used
+ for temporarily removing the segments of code with the intention of
+ using it in the future, much like comments. But comments cannot be
+ nested, so #if 0 is preferred. But if the code inside #if 0 and #endif
+ doesn't seem to be anymore required then remove it.
+ **IF_1**
+ The code enclosed within #if 1 and #endif is always executed, so the
+ #if 1 and #endif statements are redundant, thus remove it.
+ It is only useful for debugging purposes, it can quickly disable the
+ code enclosed within itself by changing #if 1 to #if 0
Const init definitions should use __initconst instead of
@@ -1231,6 +1353,49 @@ Others
The memset use appears to be incorrect. This may be caused due to
badly ordered parameters. Please recheck the usage.

+ According to section “6.7.2 Type Specifiers” in C90 standard, “type
+ specifiers may occur in any order.” This means that "signed long long
+ int" is same as "long long int signed". But to avoid confusion and make
+ the code easier to read, the declaration type should use the following
+ format::
+ [[un]signed] [short|int|long|long long]
+ Below is the list of standard integer types. Each row lists all the
+ different ways of specifying a particular type delimited by commas.
+ Note: Also include all the permutations of a particular type
+ on the left column delimited by comma. For example, the permutations
+ for "signed long int" are "signed int long", "long signed int",
+ "long int signed", "int signed long", and "int long signed".
+ +--------------------------------------------------+--------------------+
+ | Types | Recommended Way |
+ +=======================================================================+
+ | char | char |
+ +-----------------------------------------------------------------------+
+ | signed char | signed char |
+ +-----------------------------------------------------------------------+
+ | unsigned char | unsigned char |
+ +-----------------------------------------------------------------------+
+ | signed, int, signed int | int |
+ +-----------------------------------------------------------------------+
+ | unsigned, unsigned int | unsigned int |
+ +-----------------------------------------------------------------------+
+ | short, signed short, short int, signed short int | short |
+ +-----------------------------------------------------------------------+
+ | unsigned short, unsigned short int | unsigned short |
+ +-----------------------------------------------------------------------+
+ | long, signed long, long int, signed long int | long |
+ +-----------------------------------------------------------------------+
+ | unsigned long, unsigned long int | unsigned long |
+ +-----------------------------------------------------------------------|
+ | long long, signed long long, long long int, | long long |
+ | signed long long int | |
+ +-----------------------------------------------------------------------+
+ | unsigned long long, unsigned long long int | unsigned long long |
+ +-----------------------------------------------------------------------+
The patch file does not appear to be in unified-diff format. Please
regenerate the patch file before sending it to the maintainer.
@@ -1247,3 +1412,138 @@ Others

Some words may have been misspelled. Consider reviewing them.
+ Using break statement just after a goto, return or break is unnecessary.
+ For example::
+ switch (foo) {
+ case 1:
+ goto err;
+ break;
+ }
+ Here, the break statement is completely unnecessary, because it will
+ never be executed. So it is better to remove it.
+ It is not a bug or syntactically incorrect, but it should be removed
+ because it is an unreachable statement that will never be executed
+ (
+ Note there can be some false positives, which happen because of the way
+ this rule is implemented in the checkpatch script. The checkpatch script
+ throws this warning message if it finds a break statement and the line
+ above it is a goto/return/break statement with the same indentation
+ (same number of tabs). It only relies on the same indentation and does
+ not care about the logic of the code. For example::
+ if (foo)
+ break;
+ break;
+ Here the checkpatch will throw this warning message, because both the
+ break statements are at the same indentation. The second break statement
+ is unnecessarily indented, which causes this false positive. So do not
+ blindly follow the checkpatch advice here, instead consider the logic of
+ the code before making the changes. In the above example, correct the
+ indentation of the second break statement instead of following the
+ checkpatch advice.
+ Using an else statement just after a return/break/continue statement is
+ unnecessary. For example::
+ if (foo)
+ break;
+ else
+ usleep(1);
+ is generally better written as::
+ if (foo)
+ break;
+ usleep(1);
+ It helps to reduce the indentation and removes the unnecessary else
+ statement. But do not blindly follow checkpatch's advice here, as blind
+ changes due to this rule have already caused some disturbance, see commit
+ 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else after return
+ statement."). That commit made it to the mainline which had to be
+ reverted and fixed.
+ These false positives happen because of the way this rule is implemented
+ in the checkpatch script. The checkpatch script throws this warning
+ message if it finds an else statement and the line above it is a
+ break/continue/return statement indented at one tab more than the else
+ statement. So there can be some false positives like::
+ int n = 15;
+ if (n > 10)
+ n--;
+ else if (n == 10)
+ return 0;
+ else
+ n++;
+ Now the checkpatch will give a warning for the use of else after return
+ statement. If the else statement is removed then::
+ int n = 15;
+ if (n > 10)
+ n--;
+ else if (n == 10)
+ return 0;
+ n++;
+ Now both the n-- and n++ statements will be executed which is different
+ from the logic in the first case. As the if block doesn't have a return
+ statement, so removing the else statement is wrong. So always check the
+ previous if/else if blocks, for break/continue/return statements, before
+ following this rule.
+ Also, do not change the code if there is only a single return statement
+ inside if-else block, like::
+ if (a > b)
+ return a;
+ else
+ return b;
+ now if the else statement is removed::
+ if (a > b)
+ return a;
+ return b;
+ there is no considerable increase in the readability and one can argue
+ that the first form is more readable because of the indentation. So
+ do not remove the else statement in case of a single return statement
+ inside the if-else block.
+ See:
+ On Sun, 2018-08-05 at 08:52 -0700, Linus Torvalds wrote:
+ > "long unsigned int" isn't _technically_ wrong. But we normally
+ > call that type "unsigned long".
+ Using "int" type-specifier with "short", "long", and "long long" is
+ optional. So "short int" is same as "short", "long int" is same as
+ "long", and "long long int" is same as "long long". Similary for
+ their unsigned counterparts also.
+ To avoid confusion so that people do not interpret these synonymous
+ types as different types, and also to make the code uniform, clean and
+ more readable usage of "int" should be avoided with "short", "long",
+ and "long long".
+ See:
+ "signed", "signed int", and "int" are all the same types. Similarly,
+ "unsigned" and "unsigned int" are also the same.
+ So using type-specifier "signed" and "unsigned" with or without "int"
+ is the same type only. But to avoid confusion so that people do not
+ interpret these synonymous types as different types, and also to make
+ the code uniform, clean, and more readable prefer using the "signed int"
+ or "int" in place of "signed" and "unsigned int" in place of "unsigned".
 \ /
  Last update: 2022-08-02 07:57    [W:0.071 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site