Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks

[Utkarsh Verma <utkarshverma294@gmail.com>]

On Sat, Jul 17, 2021 at 09:23:19AM +0200, Lukas Bulwahn wrote:
> Get a clone of the Linux kernel repository. The script checkpatch.pl
> is under the scripts directory.
> 
> Then, the first task is to run checkpatch.pl on a few files below and
> share the results:
> 
> drivers/acpi/acpica/acapps.h
> drivers/usb/serial/iuu_phoenix.c 

$ ./scripts/checkpatch.pl --file --terse drivers/acpi/acpica/acapps.h
drivers/acpi/acpica/acapps.h:50: ERROR: Use of the '__DATE__' macro makes the build non-deterministic
drivers/acpi/acpica/acapps.h:50: ERROR: Use of the '__TIME__' macro makes the build non-deterministic
drivers/acpi/acpica/acapps.h:54: ERROR: Macros with multiple statements should be enclosed in a do - while loop
drivers/acpi/acpica/acapps.h:54: WARNING: macros should not use a trailing semicolon
drivers/acpi/acpica/acapps.h:55: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:57: ERROR: Macros with multiple statements should be enclosed in a do - while loop
drivers/acpi/acpica/acapps.h:57: WARNING: macros should not use a trailing semicolon
drivers/acpi/acpica/acapps.h:58: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:60: ERROR: Macros with multiple statements should be enclosed in a do - while loop
drivers/acpi/acpica/acapps.h:60: WARNING: macros should not use a trailing semicolon
drivers/acpi/acpica/acapps.h:61: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:65: ERROR: Macros starting with if should be enclosed by a do - while loop to avoid possible if/else logic defects
drivers/acpi/acpica/acapps.h:66: ERROR: that open brace { should be on the previous line
drivers/acpi/acpica/acapps.h:68: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:69: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:74: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:74: ERROR: Macros with multiple statements should be enclosed in a do - while loop
drivers/acpi/acpica/acapps.h:74: WARNING: macros should not use a trailing semicolon
drivers/acpi/acpica/acapps.h:88: ERROR: "foo * bar" should be "foo *bar"
drivers/acpi/acpica/acapps.h:90: ERROR: "foo * bar" should be "foo *bar"
total: 10 errors, 10 warnings, 154 lines checked


$ ./scripts/checkpatch.pl --file --terse drivers/usb/serial/iuu_phoenix.c
drivers/usb/serial/iuu_phoenix.c:256: WARNING: single byte memset is suspicious. Swapped 2nd/3rd argument?
drivers/usb/serial/iuu_phoenix.c:313: WARNING: Missing a blank line after declarations
drivers/usb/serial/iuu_phoenix.c:363: ERROR: space prohibited before that ',' (ctx:WxE)
drivers/usb/serial/iuu_phoenix.c:383: ERROR: space prohibited before that ',' (ctx:WxE)
drivers/usb/serial/iuu_phoenix.c:451: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:454: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:456: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:458: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:464: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:583: WARNING: braces {} are not necessary for single statement blocks
drivers/usb/serial/iuu_phoenix.c:648: WARNING: Missing a blank line after declarations
drivers/usb/serial/iuu_phoenix.c:773: WARNING: Missing a blank line after declarations
drivers/usb/serial/iuu_phoenix.c:978: ERROR: trailing statements should be on next line
drivers/usb/serial/iuu_phoenix.c:1059: WARNING: single byte memset is suspicious. Swapped 2nd/3rd argument?
drivers/usb/serial/iuu_phoenix.c:1191: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1194: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1197: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1199: WARNING: quoted string split across lines
drivers/usb/serial/iuu_phoenix.c:1201: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1203: WARNING: quoted string split across lines
drivers/usb/serial/iuu_phoenix.c:1205: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1207: WARNING: quoted string split across lines
total: 3 errors, 19 warnings, 1207 lines checked



> Which information on these rules that checkpatch warns about is
> available in the Checkpatch Documentation?
> 
> If documentation of these rules is available, explain your
> understanding of the rules in your own words.
> If no information is available in the documentation, explain your
> understanding of the rule.
> 
> In any case, explain the violation that is checked and raised in the
> checkpatch script, i.e., what is implemented in checkpatch to check
> the rule and possible violations. Which code in the checkpatch script
> is raising the warning? What does it check and how is that
> implemented?
> 
> Explain how to possibly fix this code in the files above with regards
> to that violation.



============================
drivers/acpi/acpica/acapps.h
============================



drivers/acpi/acpica/acapps.h:50: ERROR: Use of the '__DATE__' macro makes the build non-deterministic
drivers/acpi/acpica/acapps.h:50: ERROR: Use of the '__TIME__' macro makes the build non-deterministic

The __DATE__ and __TIME__ macros give the date and the time at which the
file was compiled. The Linux Kernel should be reproducible, so that no
vulnerabilities or backdoors are introduced. It should always generate
same binaries when a particular version of it is compiled using same set
of tools. The __DATE__ and __TIME__ macro does not result in reproducible
builds, since its value will change over time. Building the kernel at
different time will result in different macro value and thus will give
different binaries each time. Also the GCC version 4.9 and newer gives an
error on using these macros. So the checkpatch.pl produces an ERROR
message when it encounters the usage of __DATE__ and __TIME__ macros.

Proper explanation for this message is available in the checkpatch.rst
documentation file.

This line in the checkpatch.pl file checks for the the usage of __DATE__
or __TIME__ or __TIMESTAMP__ in the input file.

scripts/checkpatch.pl:7098:
		while ($line =~ /\b(__(?:DATE|TIME|TIMESTAMP)__)\b/g) {

If it finds the usage of any such macro, then these lines print the
appropriate ERROR message.

scripts/checkpatch.pl:7099:
			ERROR("DATE_TIME",
			      "Use of the '$1' macro makes the build non-deterministic\n" . $herecurr);

The fix is to not to use these macros and remove its usage in the kernel.


--------------------------------------------------------------------------


drivers/acpi/acpica/acapps.h:54: ERROR: Macros with multiple statements should be enclosed in a do - while loop
drivers/acpi/acpica/acapps.h:57: ERROR: Macros with multiple statements should be enclosed in a do - while loop
drivers/acpi/acpica/acapps.h:60: ERROR: Macros with multiple statements should be enclosed in a do - while loop
drivers/acpi/acpica/acapps.h:74: ERROR: Macros with multiple statements should be enclosed in a do - while loop

Macros that consist of multilple statements should be enclosed in a
do - while loop, because if macros are invoked from inside of a if-else
statement without using the braces, then it may cause some unpredictable
behaviour. For example if we have a macro PRT,

#define PRT \
	printf("First Line\n"); \
	printf("Second Line\n")

if this macro is invoked from inside of a if statement like,

if (<condition>)
	PRT;

then if the condition is true then the first printf inside PRT will be
called, while the second printf is always called irrespective of the
condition. It is because the statements are not inside the curly braces.
One can put the curly braces at the place of macro invocation but it is
avoided, because function-style-macros should be uniform with the use of
ordinary functions in all contexts. It makes the switch between function
call and macro invocation convenient.

Most of the time this error pops up, when when the curly braces are
used instead of do - while loop in macro definition. Curly braces should
not be used because macros should expand into a regular statement and not
in a compound one. For example if we have a macro PRT,

#define PRT \
{ \
	printf("First Line\n"); \
	printf("Second Line\n"); \
}

if this macro is invoked from inside of a if statement like,

if(<condition>)
	PRT;
else
	some_function();

then there would be a compilation error of: ‘else’ without a previous ‘if’
When the macro from the if statement is invoked then it is replaced by a
compound statement after which a semicolon is present, so the else branch
would get orphaned (thus a compilation error).
One can omit the semicolon after the macro invocation but that is not
elegant and defies the uniformity between the function call and the macro
invocation. By using the do - while loop instead of the curly braces,
the compound statement converts to a regular one and so one may use the
function call and macro invocation interchangeably.

Proper explanation for this message is available in the checkpatch.rst
documentation file.

In the checkpatch.pl file, these lines check for the usage of "#define"
keyword, so as to search for macros.

scripts/checkpatch.pl:5716
		if ($realfile !~ m@/vmlinux.lds.h$@ &&
		    $line =~ /^.\s*\#\s*define\s*$Ident(\()?/) {

The lines of code from scripts/checkpatch.pl:5785 check if the statements
inside the define follow proper style, and also if they are enclosed
within the do while loop or not. If the statements are not following the
proper coding style then it further checks, if the statements is using a
semicolon or not. If a semicolon is used, it means that the macro has
multiple statements inside it and not enclosed in the do while loop. Then
it also prints the error message,

scripts/checkpatch.pl:5804
				} elsif ($dstat =~ /;/) {
					ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
					      "Macros with multiple statements should be enclosed in a do - while loop\n" . "$herectx");

But in drivers/acpi/acpica/acapps.h file case all the 4 error messages
produced by checkpatch.pl are wrong, beacause in all the error cases there
is only one statement defined in the macro. So it is not necessary to
enclose that statement inside the do while loop. Although there should not
be any semicolon at the end of the macro defintion, whose WARNING messages
follow next.

Since the error messages are wrong so there is no need to fix the file.
Although one can enclose the statement inside macro in a do {} while(0)
loop, to prevent the error message, but it is not necessary.


--------------------------------------------------------------------------


drivers/acpi/acpica/acapps.h:54: WARNING: macros should not use a trailing semicolon
drivers/acpi/acpica/acapps.h:57: WARNING: macros should not use a trailing semicolon
drivers/acpi/acpica/acapps.h:60: WARNING: macros should not use a trailing semicolon
drivers/acpi/acpica/acapps.h:74: WARNING: macros should not use a trailing semicolon

Macro defintions should never conclude with a semicolon. It is because
when the macro is invoked and expanded the semicolon can unexpectedly
change the control flow of the program. Also to be consistent with the
ordinary function calls, macros defintion should not conclude with a
semicolon.

Explanation for this warning message is not in the Checkpatch
documentation. But the message is self explanatory.

In the checkpatch.pl file, these lines check for the usage of define
keyword

scripts/checkpatch.pl:5878
		if ($perl_version_ok &&
		    $realfile !~ m@/vmlinux.lds.h$@ &&
		    $line =~ /^.\s*\#\s*define\s+$Ident(\()?/) {

If it evaluates to true, then it further checks if the lines start with
a define keyword and end with a semicolon.

scripts/checkpatch.pl:5909
			} elsif ($dstat =~ /^\+\s*#\s*define\s+$Ident.*;\s*$/) {

If this also evaluates to true, then a trailing semicolon is used by the
macro, so it prints the appropriate warning message.

scripts/checkpatch.pl:5914
				WARN("TRAILING_SEMICOLON",
				     "macros should not use a trailing semicolon\n" . "$herectx");

To fix the warning message, simply remove the semicolon present at the end
of the macro defintion.


--------------------------------------------------------------------------


drivers/acpi/acpica/acapps.h:55: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:58: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:61: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:68: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:69: WARNING: space prohibited between function name and open parenthesis '('
drivers/acpi/acpica/acapps.h:74: WARNING: space prohibited between function name and open parenthesis '('

According to the Linux Kernel coding style there should not be a space
between the function name and the opening parenthesis either in
declaration, defintion, or function calling. Although logically it is not
wrong.

For this warning message the checkpatch documentation gives the link to
the coding style documentation, which clearly explains this warning.

The checkpatch.pl file scans for all the function like expressions and
checks if they are on of those directives which can be ignored,

scripts/checkpatch.pl:4873
		if ($name =~ /^(?:
				if|for|while|switch|return|case|
				volatile|__volatile__|
				__attribute__|format|__extension__|
				asm|__asm__)$/x)

it also checks and ignores spaces for the #define statements, elif
statements and for typedefs

scripts/checkpatch.pl:4882
			} elsif ($ctx_before =~ /^.\s*\#\s*define\s*$/) {

scripts/checkpatch.pl:4885
			} elsif ($ctx =~ /^.\s*\#\s*elif\s*$/) {

scripts/checkpatch.pl:4889
			} elsif ($ctx =~ /$Type$/) {

finally if all the above conditions are false, then its likely a function
only, so it prints the warning message,

scripts/checkpatch.pl:4892
				if (WARN("SPACING",
					 "space prohibited between function name and open parenthesis '('\n" . $herecurr) &&

To fix the WARNING message, simply remove the spacing between the function
name the opening parenthesis at the places specified by the checkpatch

diff --git a/drivers/acpi/acpica/acapps.h b/drivers/acpi/acpica/acapps.h
index 725e2f65c..d87dd5bae 100644
--- a/drivers/acpi/acpica/acapps.h
+++ b/drivers/acpi/acpica/acapps.h
@@ -52,26 +52,26 @@
 /* Macros for usage messages */
 
 #define ACPI_USAGE_HEADER(usage) \
-       printf ("Usage: %s\nOptions:\n", usage);
+       printf("Usage: %s\nOptions:\n", usage);
 
 #define ACPI_USAGE_TEXT(description) \
-       printf (description);
+       printf(description);
 
 #define ACPI_OPTION(name, description) \
-       printf ("  %-20s%s\n", name, description);
+       printf("  %-20s%s\n", name, description);
 
 /* Check for unexpected exceptions */
 
 #define ACPI_CHECK_STATUS(name, status, expected) \
        if (status != expected) \
        { \
-               acpi_os_printf ("Unexpected %s from %s (%s-%d)\n", \
-                       acpi_format_exception (status), #name, _acpi_module_name, __LINE__); \
+               acpi_os_printf("Unexpected %s from %s (%s-%d)\n", \
+                       acpi_format_exception(status), #name, _acpi_module_name, __LINE__); \
        }
 
 /* Check for unexpected non-AE_OK errors */
 
-#define ACPI_CHECK_OK(name, status)   ACPI_CHECK_STATUS (name, status, AE_OK);
+#define ACPI_CHECK_OK(name, status)   ACPI_CHECK_STATUS(name, status, AE_OK);


--------------------------------------------------------------------------


drivers/acpi/acpica/acapps.h:65: ERROR: Macros starting with if should be enclosed by a do - while loop to avoid possible if/else logic defects

As previously mentioned, all the statements inside the macro defintion
should be enclosed within a do {} while(0)
Incase of macros having if/else statement, nesting the macro inside
if/else statement and using a semicolon after it can cause dangling else
problem. Macros invocation should be like function calls and by using the
do - while loop in macro defintion, it ensures that they always work the
same way, and the semicolon after a macro has the same effect, regardless
of how the macro is used (particularly on the issue of nesting the macro
in an if/else statement without curly brackets).

The explanation of this error is given in the checkpatch documentation.

The checkpatch.pl first searches for the macros, by scanning for the
"#define" keyword.

scripts/checkpatch.pl:5716
		if ($realfile !~ m@/vmlinux.lds.h$@ &&
		    $line =~ /^.\s*\#\s*define\s*$Ident(\()?/) {

Then the lines of code from scripts/checkpatch.pl:5785 check if the
statements inside the macro follow proper coding style or not and if they
are enclosed withing the do while loop, if not then it checks whether
there is an if statement used or not, by searching for the "if" keyword.
If the "if" statement is used, then it finally prints the error message.

scripts/checkpatch.pl:5801
				if ($dstat =~ /^\s*if\b/) {
					ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
					      "Macros starting with if should be enclosed by a do - while loop to avoid possible if/else logic defects\n" . "$herectx");

To fix the error message, simply put all the statements inside macro
defintion in a do {} part of a do - while loop. The do part ensures that
the logic inside the curly braces executes, and the while(0) part ensures
that the execution happens only once.

diff --git a/drivers/acpi/acpica/acapps.h b/drivers/acpi/acpica/acapps.h
index 725e2f65c..3f28dee05 100644
--- a/drivers/acpi/acpica/acapps.h
+++ b/drivers/acpi/acpica/acapps.h
@@ -63,11 +63,13 @@
 /* Check for unexpected exceptions */
 
 #define ACPI_CHECK_STATUS(name, status, expected) \
-       if (status != expected) \
-       { \
-               acpi_os_printf ("Unexpected %s from %s (%s-%d)\n", \
-                       acpi_format_exception (status), #name, _acpi_module_name, __LINE__); \
-       }
+       do { \
+               if (status != expected) \
+               { \
+                       acpi_os_printf ("Unexpected %s from %s (%s-%d)\n", \
+                               acpi_format_exception (status), #name, _acpi_module_name, __LINE__); \
+               } \
+       } while (0)
 
 /* Check for unexpected non-AE_OK errors */


--------------------------------------------------------------------------


drivers/acpi/acpica/acapps.h:66: ERROR: that open brace { should be on the previous line

This is a wrong error message.
According to Kernighan and Ritchie, the opening brace should be on the
same line and at the end of the line, while the closing brace should be
on a new line and at the beginning.

if (<condition>) {
	fun1();
	fun2();
}

This applies to all non-function statement blocks (if, switch, for, while,
do).
However for a single statement the braces should not be used. Thus, the
above error message is wrong and instead it should give a warning of not
using braces at all.

Proper explanation for this error message is given in the checkpatch
documentation.

The checkpatch.pl first checks for the usage of if/while/etc keywords,
because for these statements the opening curly brace should be on the same
line.

scripts/checkpatch.pl:4137
		if ($line =~ /(.*)\b((?:if|while|for|switch|(?:[a-z_]+|)for_each[a-z_]+)\s*\(|do\b|else\b)/ && $line !~ /^.\s*\#/) {

If it finds one, then if checks for for the placement of the curly brace.
If the opening curly brace is not on the same line as the statement and
is on the next line, then it prints the error message.

scripts/checkpatch.pl:4164
			if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
				ERROR("OPEN_BRACE",
				      "that open brace { should be on the previous line\n" .

Although there is coding style error in using the braces, but not the one
given by the checkpatch.pl. To remove the error simply delete the opening
and closing curly braces from the if block statement.


--------------------------------------------------------------------------


drivers/acpi/acpica/acapps.h:88: ERROR: "foo * bar" should be "foo *bar"
drivers/acpi/acpica/acapps.h:90: ERROR: "foo * bar" should be "foo *bar"

According to the Linux Kernel coding style, the asterisk "*" used in
pointers should be adjacent to the pointer name name and not the pointer
type name. Correct usage is:

char *linux_banner;
unsigned long long memparse(char *ptr, char **retptr);
char *match_strdup(substring_t *s);

Proper explanation about this message is given in the checkpatch
documentation.

The checkpatch.pl checks for the usage of pointers by searching for the
asterisk symbol along wth other checks.

scripts/checkpatch.pl:4622
		while ($line =~ m{(\b$NonptrType(\s*(?:$Modifier\b\s*|\*\s*)+)($Ident))}g) {

If it finds one then it checks the pointer declaration style with the 
correct coding style. If both of them do not match, then it prints the
error message.

scripts/checkpatch.pl:4637
			if ($from ne $to && $ident !~ /^$Modifier$/) {
				if (ERROR("POINTER_LOCATION",
					  "\"foo${from}bar\" should be \"foo${to}bar\"\n" .  $herecurr) &&

To fix the error message simply remove the space between the "*" and the
pointer name.

diff --git a/drivers/acpi/acpica/acapps.h b/drivers/acpi/acpica/acapps.h
index 725e2f65c..25c5e77e7 100644
--- a/drivers/acpi/acpica/acapps.h
+++ b/drivers/acpi/acpica/acapps.h
@@ -85,9 +85,9 @@ ac_get_all_tables_from_file(char *filename,
 
 void ac_delete_table_list(struct acpi_new_table_desc *list_head);
 
-u8 ac_is_file_binary(FILE * file);
+u8 ac_is_file_binary(FILE *file);
 
-acpi_status ac_validate_table_header(FILE * file, long table_offset);
+acpi_status ac_validate_table_header(FILE *file, long table_offset);
 
 /* Values for get_only_aml_tables */


--------------------------------------------------------------------------

================================
drivers/usb/serial/iuu_phoenix.c
================================



drivers/usb/serial/iuu_phoenix.c:256: WARNING: single byte memset is suspicious. Swapped 2nd/3rd argument?
drivers/usb/serial/iuu_phoenix.c:1059: WARNING: single byte memset is suspicious. Swapped 2nd/3rd argument?

memset is a function defined in include/linux/string.h file.

void *memset(void *s, int c, size_t n);

The memset() function fills the first n bytes of the memory area pointed
to by s with the constant byte c. It returns a pointer to the memory area
s.
Calling the memset with n value of 1, is likely to be unintended and maybe
by mistake (swapped with the int c value). It is more likely to fill a
memory region with all ones, than to only fill a single byte in a
memory region.
So this message warns about the calling of memset with n value equal to 1
and also hints about the swapping of c and n values.

The checkpatch documentation explains about this warning.

The checkpatch.pl first scans for the usage of memset, by searching for
the "memset" keyword.

scripts/checkpatch.pl:6738
		if ($perl_version_ok &&
		    defined $stat &&
		    $stat =~ /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*$FuncArg\s*\)/) {

Then it extracts the arguments passed inside the memset function call.

scripts/checkpatch.pl:6742
			my $ms_addr = $2;
			my $ms_val = $7;
			my $ms_size = $12;

Then it finally checks the third argument, if it is either 0 or 1.
If that happens to be true then it finally prints the warning message.

scripts/checkpatch.pl:6749
			} elsif ($ms_size =~ /^(0x|)1$/i) {
				WARN("MEMSET",
				     "single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . "$here\n$stat\n");

At both the places in drivers/usb/serial/iuu_phoenix.c file, when this
warning appears, the module author intentionally called the memset with
3rd argument equal to 1. So this warning can be safely ignored.


--------------------------------------------------------------------------


drivers/usb/serial/iuu_phoenix.c:313: WARNING: Missing a blank line after declarations
drivers/usb/serial/iuu_phoenix.c:648: WARNING: Missing a blank line after declarations
drivers/usb/serial/iuu_phoenix.c:773: WARNING: Missing a blank line after declarations

Linux Kernel coding style prefers to have a blank line after the variable
declarations in a function. Example:

void foo(int bar)
{
	int baz;

	code ...
}

and not this:

void foo(int bar)
{
	int baz;
	code ...
}

In the checkpatch documentation, link to the Kernel coding style is
mentioned. But unfortunately nothing specfic to having a blank line after
the variable declarations in a function is given in the documentation.
But the warning message is sufficient enough and self explanatory.

The checkpatch.pl performs various checks to test whether there are blank
lines after declaration or not. But the condition due to which the above
warning appears is,

scripts/checkpatch.pl:3921
			if (($pl =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||

it searches for the declarations on previous lines and it also performs
other checks like if the current line also has some declaration or not, or
the previous lines has some "for", "if", "else" keyword and etc.

After performing the various checks, if the previous line is actually some
type of declaration and the current line is not a declaration, then it
finally prints the warning message,

scripts/checkpatch.pl:3950
				if (WARN("LINE_SPACING",
					 "Missing a blank line after declarations\n" . $hereprev) &&

To fix the warning messages, simply put a blank line after the declaration
at the places mentioned in the message.

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e..2d20ac4ad 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -310,6 +310,7 @@ static int iuu_led(struct usb_serial_port *port, unsigned int R,
 {
        int status;
        u8 *buf;
+
        buf = kmalloc(8, GFP_KERNEL);
        if (!buf)
                return -ENOMEM;
@@ -645,6 +646,7 @@ static void iuu_uart_read_callback(struct urb *urb)
        int status = urb->status;
        int len = 0;
        unsigned char *data = urb->transfer_buffer;
+
        priv->poll++;
 
        if (status) {
@@ -770,6 +772,7 @@ static int iuu_uart_off(struct usb_serial_port *port)
 {
        int status;
        u8 *buf;
+
        buf = kmalloc(1, GFP_KERNEL);
        if (!buf)
                return -ENOMEM;


--------------------------------------------------------------------------


drivers/usb/serial/iuu_phoenix.c:363: ERROR: space prohibited before that ',' (ctx:WxE)
drivers/usb/serial/iuu_phoenix.c:383: ERROR: space prohibited before that ',' (ctx:WxE)

According to the Linux Kernel coding style, there should not be a space
before the comma, but a space after the comma should be present. Example:
foo, bar

and not
foo , bar

In the checkpatch documentation, link to the Kernel coding style
documentation is mentioned. But there is no style mentioned for the comma
use. But the error from the checkpatch.pl is self explanatory.

The checkpatch.pl searches for the comma usage

scripts/checkpatch.pl:5024
				} elsif ($op eq ',') {

if it finds one then it searches whether the comma has a space before it
or not. If that also evaluates to true, then it finally prints the error
message.

scripts/checkpatch.pl:5027
					if ($ctx =~ /Wx./) {
						if (ERROR("SPACING",
							  "space prohibited before that '$op' $at\n" . $hereptr)) {

To fix the error message, simply remove the space before the comma at
the places mentioned by the checkpatch.

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e..68634276a 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -360,7 +360,7 @@ static void iuu_led_activity_on(struct urb *urb)
        usb_fill_bulk_urb(port->write_urb, port->serial->dev,
                          usb_sndbulkpipe(port->serial->dev,
                                          port->bulk_out_endpointAddress),
-                         port->write_urb->transfer_buffer, 8 ,
+                         port->write_urb->transfer_buffer, 8,
                          iuu_rxcmd, port);
        usb_submit_urb(port->write_urb, GFP_ATOMIC);
 }
@@ -380,7 +380,7 @@ static void iuu_led_activity_off(struct urb *urb)
        usb_fill_bulk_urb(port->write_urb, port->serial->dev,
                          usb_sndbulkpipe(port->serial->dev,
                                          port->bulk_out_endpointAddress),
-                         port->write_urb->transfer_buffer, 8 ,
+                         port->write_urb->transfer_buffer, 8,
                          iuu_rxcmd, port);
        usb_submit_urb(port->write_urb, GFP_ATOMIC);
 }


--------------------------------------------------------------------------


drivers/usb/serial/iuu_phoenix.c:451: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:454: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:456: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:458: WARNING: Too many leading tabs - consider code refactoring
drivers/usb/serial/iuu_phoenix.c:464: WARNING: Too many leading tabs - consider code refactoring

According to the checkpatch documentation, if 6 or more tabs are used in
the block of statements, then the code is overly indented and it should be
refactored.

Information about this warning is available in the checkpatch
documentation.

The checkpatch.pl checks for the usage of "if", "else", "for", "do",
"while", "for_each", or "switch" keywords.

scripts/checkpatch.pl:4137
		if ($line =~ /(.*)\b((?:if|while|for|switch|(?:[a-z_]+|)for_each[a-z_]+)\s*\(|do\b|else\b)/ && $line !~ /^.\s*\#/) {

If it finds one, then it checks if there are a minimum of 6 tab spaces or
not. If there is 6 or more than 6 tab spaces, then it prints the
appropriate warning message.

scripts.checkpatch.pl:4142
			if ($line =~ /^\+\t{6,}/) {
				WARN("DEEP_INDENTATION",
				     "Too many leading tabs - consider code refactoring\n" . $herecurr);

To fix the warning message, the flow of the code should be changed and
it should be written in a more simpler manner, so that, so  many for loops
and if statements are not required.
Although if the code is simpler to understand, the warning can be ignored.


--------------------------------------------------------------------------


drivers/usb/serial/iuu_phoenix.c:583: WARNING: braces {} are not necessary for single statement blocks

According to the Linux Kernel coding style, it is not necessary to put the
curly braces for single statements inside the non-functional blocks like
if, else, for, etc. Example:

if (condition)
        do_this();
else
        do_that();

In the checkpatch deocumentation, link to the Kernel coding style
documentation is mentioned, which clearly specifies this rule. 

The checkpatch.pl file checks for if the usage of "if", "while", "for",
or "else" keywords, because for these statements there is no need to put
the braces for single statements inside them.

scripts/checkpatch.pl:5982
		if (!defined $suppress_ifbraces{$linenr - 1} &&
					$line =~ /\b(if|while|for|else)\b/) {

If it finds the usage of above keywords, then it checks the number of
lines are 1 or not (single statement) and also checks for the usage of
opening curly brace.

scripts/checkpatch.pl:6024
			if ($level == 0 && $block =~ /^\s*\{/ && !$allowed) {

If it finds one, then it is obviously a single statement with the curly
braces, so it prints the appropriate warning message.

scripts/checkpatch.pl:6028
				WARN("BRACES",
				     "braces {} are not necessary for single statement blocks\n" . $herectx);

To fix the warning message, simply remove the the curly braces from the if
statement. In the drivers/usb/serial/iuu_phoenix.c:583 there is only a
comment inside the if block, so the author may be specifying some future
work that needs to be done.


--------------------------------------------------------------------------


drivers/usb/serial/iuu_phoenix.c:978: ERROR: trailing statements should be on next line

According to the checkpatch documentation, after the use of any
non-functional blocks like if/else or while loop, the trailing statement
should be on the next line.

TRAILING_STATEMENTS error is documented in the checkpatch documentation.

The checkpatch.pl file checks by if a trailing statement is present or
not, by the following code,

scripts/checkpatch.pl:5541
			if (length($c) && $s !~ /^\s*{?\s*\\*\s*$/ &&
			    $c !~ /}\s*while\s*/)

If it finds one, then it prints the appropriate message 

scripts/checkpatch.pl:5555
				ERROR("TRAILING_STATEMENTS",
				      "trailing statements should be on next line\n" . $herecurr . $stat_real);

However for the iuu_phoenix.c file the error is raised when the do while
loop is inside a macro. So it can be ignored, but to remove the warning
message, simply put the closing curly brace and the while statement on a
new line, although it is not necessary.

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e..0871ffeca 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -975,7 +975,8 @@ static int iuu_open(struct tty_struct *tty, struct usb_serial_port *port)
        result = usb_control_msg(port->serial->dev,     \
                                usb_sndctrlpipe(port->serial->dev, 0),  \
                                b, a, c, d, NULL, 0, 1000); \
-       dev_dbg(dev, "0x%x:0x%x:0x%x:0x%x  %d\n", a, b, c, d, result); } while (0)
+       dev_dbg(dev, "0x%x:0x%x:0x%x:0x%x  %d\n", a, b, c, d, result); \
+       } while (0)
 
        /*  This is not UART related but IUU USB driver related or something */
        /*  like that. Basically no IUU will accept any commands from the USB */


--------------------------------------------------------------------------


drivers/usb/serial/iuu_phoenix.c:1191: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1194: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1197: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1201: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
drivers/usb/serial/iuu_phoenix.c:1205: WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.

The file permission macros used in the kernel code are defined in
<linux/stat.h> like S_IRUGO and friends. But it is better to use their
octal equivalent instead, because it easier for Linux users to
understand 0444 instead of S_IRUGO.

There is no checkpatch documentation for this warning message, but the
message is self explanatory.

The checkpatch.pl file checks for the usage of "S_<file_permissions>" like
S_IRUGO and friends.

scripts/checkpatch.pl:7348
		while ($line =~ m{\b($multi_mode_perms_string_search)\b}g) {

if it finds one, then it prints the appropriate message,

scripts.checkpatch.pl:7351
			if (WARN("SYMBOLIC_PERMS",
				 "Symbolic permissions '$oval' are not preferred. Consider using octal permissions '$octal'.\n" . $herecurr) &&

To fix the warning message, simply use the octal equivalent instead of
the macros, as mentioned in the warning message.

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e..0be3b5e1e 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -1188,20 +1188,20 @@ MODULE_AUTHOR("Alain Degreffe eczema@ecze.com");
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL");
 
-module_param(xmas, bool, S_IRUGO | S_IWUSR);
+module_param(xmas, bool, 0644);
 MODULE_PARM_DESC(xmas, "Xmas colors enabled or not");
 
-module_param(boost, int, S_IRUGO | S_IWUSR);
+module_param(boost, int, 0644);
 MODULE_PARM_DESC(boost, "Card overclock boost (in percent 100-500)");
 
-module_param(clockmode, int, S_IRUGO | S_IWUSR);
+module_param(clockmode, int, 0644);
 MODULE_PARM_DESC(clockmode, "Card clock mode (1=3.579 MHz, 2=3.680 MHz, "
                "3=6 Mhz)");
 
-module_param(cdmode, int, S_IRUGO | S_IWUSR);
+module_param(cdmode, int, 0644);
 MODULE_PARM_DESC(cdmode, "Card detect mode (0=none, 1=CD, 2=!CD, 3=DSR, "
                 "4=!DSR, 5=CTS, 6=!CTS, 7=RING, 8=!RING)");
 
-module_param(vcc_default, int, S_IRUGO | S_IWUSR);
+module_param(vcc_default, int, 0644);
 MODULE_PARM_DESC(vcc_default, "Set default VCC (either 3 for 3.3V or 5 "
                "for 5V). Default to 5.");


--------------------------------------------------------------------------


drivers/usb/serial/iuu_phoenix.c:1199: WARNING: quoted string split across lines
drivers/usb/serial/iuu_phoenix.c:1203: WARNING: quoted string split across lines
drivers/usb/serial/iuu_phoenix.c:1207: WARNING: quoted string split across lines

Quoted strings should not be splitted across multiple lines, this is done
so that the strings that appear as messages in the userspace can be
grepped.

The checkpatch documentation has no information about this warning.

The checkpatch.pl checks for the usage of strings, if the previous line
used a double quotes, and this line is also inside the quotes, then the
string is split across multiple lines. But it makes exceptions when the
previous string ends in a newline or '\t', '\r', ';', or '{' or is a octal
or hexadecimal value.

scripts/checkpatch.pl:6066
		if ($line =~ /^\+\s*$String/ &&
		    $prevline =~ /"\s*$/ &&
		    $prevrawline !~ /(?:\\(?:[ntr]|[0-7]{1,3}|x[0-9a-fA-F]{1,2})|;\s*|\{\s*)"\s*$/) {

If the condition is true it prints the appropriate message,

scripts/checkpatch.pl:6069
			if (WARN("SPLIT_STRING",
				 "quoted string split across lines\n" . $hereprev) &&

To resolve the warning, make the whole string fit in a single line only.

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e..f2c0dad91 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -1195,13 +1195,10 @@ module_param(boost, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(boost, "Card overclock boost (in percent 100-500)");
 
 module_param(clockmode, int, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(clockmode, "Card clock mode (1=3.579 MHz, 2=3.680 MHz, "
-               "3=6 Mhz)");
+MODULE_PARM_DESC(clockmode, "Card clock mode (1=3.579 MHz, 2=3.680 MHz, 3=6 Mhz)");
 
 module_param(cdmode, int, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(cdmode, "Card detect mode (0=none, 1=CD, 2=!CD, 3=DSR, "
-                "4=!DSR, 5=CTS, 6=!CTS, 7=RING, 8=!RING)");
+MODULE_PARM_DESC(cdmode, "Card detect mode (0=none, 1=CD, 2=!CD, 3=DSR, 4=!DSR, 5=CTS, 6=!CTS, 7=RING, 8=!RING)");
 
 module_param(vcc_default, int, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(vcc_default, "Set default VCC (either 3 for 3.3V or 5 "
-               "for 5V). Default to 5.");
+MODULE_PARM_DESC(vcc_default, "Set default VCC (either 3 for 3.3V or 5 for 5V). Default to 5.");


--------------------------------------------------------------------------


> Once you succeed on this first task, we inform you about the further
> second and third task. If you fail on any of those tasks, you are out
> of the selection process.
> 
> The selection of the mentee will happen according to schedule, at
> earliest on August 12th and at latest at the end of August. More
> information is available at
> https://docs.linuxfoundation.org/lfx/mentorship/mentorship-program-timelines.
> 
> 
> Lukas


Regards,
Utkarsh Verma

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees