Added and documented the following new message types:
- JIFFIES_COMPARISON
- LONG_UDELAY
- MSLEEP
- INDENTED_LABEL
- IF_0
- IF_1
- MISORDERED_TYPE
- UNNECESSARY_BREAK
- UNNECESSARY_ELSE
- UNNECESSARY_INT
- UNSPECIFIED_INT
Signed-off-by: Utkarsh Verma <utkarshverma294@xxxxxxxxx>
---
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
See: https://lore.kernel.org/lkml/20080320201723.b87b3732.akpm@xxxxxxxxxxxxxxxxxxxx/
+ **JIFFIES_COMPARISON**
+ 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.
+
**LOCKDEP**
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
See: https://lore.kernel.org/lkml/1268959062.9440.467.camel@laptop/
+ **LONG_UDELAY**
+ 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: https://www.kernel.org/doc/html/latest/timers/timers-howto.html
+
**MALFORMED_INCLUDE**
The #include statement has a malformed path. This has happened
because the author has included a double slash "//" in the pathname
accidentally.
+ **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
+ (https://lore.kernel.org/all/15327.1186166232@xxxxxxx/). 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. https://lore.kernel.org/all/alpine.DEB.2.22.394.2110171140040.3026@hadrien/
+ 2. https://lore.kernel.org/linux-staging/260b38b8-6f3f-f6cc-0388-09a269ead507@xxxxxxxx/
+ 3. https://lore.kernel.org/all/1357253791.2685.48.camel@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
+
**USE_LOCKDEP**
lockdep_assert_held() annotations should be preferred over
assertions based on spin_is_locked()
@@ -661,6 +746,30 @@ Indentation and Line Breaks
See: https://lore.kernel.org/lkml/1328311239.21255.24.camel@joe2Laptop/
+ **INDENTED_LABEL**
+ goto labels either should not have any indentation or only a single
+ space indentation (https://lore.kernel.org/all/20070527171817.4ce9d40d.akpm@xxxxxxxxxxxxxxxxxxxx/).
+ 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_CASE_INDENT_LEVEL**
switch should be at the same indent as case.
Example::
@@ -823,6 +932,19 @@ Macros, Attributes and Symbols
**DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON**
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
+
**INIT_ATTRIBUTE**
Const init definitions should use __initconst instead of
__initdata.
@@ -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.
+ **MISORDERED_TYPE**
+ 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 |
+ +-----------------------------------------------------------------------+
+
**NOT_UNIFIED_DIFF**
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
**TYPO_SPELLING**
Some words may have been misspelled. Consider reviewing them.
+
+ **UNNECESSARY_BREAK**
+ 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
+ (https://lore.kernel.org/lkml/18981cad4ac27b4a22b2e38d40bd112432d4a4e7.camel@xxxxxxxxxxx/).
+
+ 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.
+
+ **UNNECESSARY_ELSE**
+ 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: https://lore.kernel.org/lkml/20140925032215.GK7996@xxxxxxxxxxxxxxxxxx/
+
+ **UNNECESSARY_INT**
+ 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: https://lore.kernel.org/all/7bbd97dc0a1e5896a0251fada7bb68bb33643f77.camel@xxxxxxxxxxx/T/#m1fa34198ce2bd088b3520b74326468a2ab314ce7
+
+ **UNSPECIFIED_INT**
+ "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".
--
2.25.1
