Linux Kernel: Checkpatch Documentation Mentorship Tasks

I am interested in the Checkpatch Documentation mentorship program and I would like to work on the tasks for the mentee selection.

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Thanks for your interest.

The zeroth task is to learn suitable netiquette for the communication with the kernel community. Below are some basic rules and pointers for this mentorship. More information on kernel netiquette is also at
https://people.kernel.org/tglx/notes-about-netiquette.

First, please do not top-post.

A: Because we read from top to bottom, left to right.
Q: Why should I start my reply below the quoted text?

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

A: The lost context.
Q: What makes top-posted replies harder to read than bottom-posted?

A: Yes.
Q: Should I trim down the quoted part of an email to which I'm replying?

Second, please always CC: [email protected].

Third, set up your email client according to the kernel community rules.

Here is some information to that:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text
https://www.kernel.org/doc/html/latest/process/email-clients.html

Generally more information on submitting patches and responding on replies is at https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Once the zeroth task is understood, you can now move on to the first
task: Running checkpatch on a specific file

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

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.

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
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

$ ./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

============================
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

index 725e2f65c..d87dd5bae 100644
@@ -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.

index 725e2f65c..3f28dee05 100644
@@ -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.

index 725e2f65c..25c5e77e7 100644
@@ -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.

index 19753611e..2d20ac4ad 100644
@@ -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.

index 19753611e..68634276a 100644
@@ -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.

index 19753611e..0871ffeca 100644
@@ -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.

index 19753611e..0be3b5e1e 100644
@@ -1188,20 +1188,20 @@ MODULE_AUTHOR("Alain Degreffe [email protected]");
 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.

index 19753611e..f2c0dad91 100644
@@ -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.");

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

Regards,
Utkarsh Verma

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Utkarsh, the next task is to prepare some commits to the kernel repository. You can find various hints on how to create your first kernel patches on
https://www.kernel.org/doc/html/latest/process/submitting-patches.html.

Please first only send your first patches only to the linux-kernel-mentees list, Dwaipayan and me, NOT to the official linux kernel mailing lists that get_maintainers.pl suggests.

More instructions on which commits we would like to see first are below.

(snip)

Please provide some checkpatch documentation on this rule. Please include an example of what could go wrong if a trailing semicolon is used.

(snip)

But: is memset required if only one byte is to be set? Is there a more standard way in C to express setting one byte?

If so, please create a patch that uses that more standard way of setting one byte instead of memset.

(snip)

Please provide a patch for the checkpatch documentation here. Do you find a reference to some coding style guide in the kernel or some discussion on the mailing list that stated the preference you wrote here?

Also create a patch for this file for this type of warning.

(snip)

Please provide a patch for the checkpatch documentation here.

Also create a patch for this file for this type of warning.

Add a new message type SPLIT_STRING under the 'Indentation and Line Breaks' subsection. Checkpatch documentation for the splitting of quoted strings that appear in userspace, across multiple lines.

---
 Documentation/dev-tools/checkpatch.rst | 4 ++++
 1 file changed, 4 insertions(+)

index f0956e9ea..32a26a800 100644
@@ -710,6 +710,10 @@ Indentation and Line Breaks
 
     See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
 
+  **SPLIT_STRING**
+    Quoted strings that appear as messages in userspace and can be
+    grepped, should not be split across multiple lines.
+
   **TRAILING_STATEMENTS**
     Trailing statements (for example after any conditional) should be
     on the next line.

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Is this reference fine:
https://linux-kernel.vger.kernel.narkive.com/vc8OoIUF/patch-checkpatch-check-for-quoted-strings-broken-across-lines

Should I include these references in the commit description or the documentation file?

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Use Links to lore.kernel.org.

Add them in the documentation file. Then readers can follow those links if interested.

Also, if there is some general conclusion within the referenced email thread, it is good to summarize that in the documentation.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type SPLIT_STRING under the 'Indentation and Line Breaks' subsection. Checkpatch documentation for the splitting of quoted strings that appear in userspace, across multiple lines.

---
 Documentation/dev-tools/checkpatch.rst | 6 ++++++
 1 file changed, 6 insertions(+)

index f0956e9ea..b7a1288e9 100644
@@ -710,6 +710,12 @@ Indentation and Line Breaks
 
     See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
 
+  **SPLIT_STRING**
+    Quoted strings that appear as messages in userspace and can be
+    grepped, should not be split across multiple lines.
+
+    See: https://lore.kernel.org/lkml/20120203052727.GA15035@leaf/
+
   **TRAILING_STATEMENTS**
     Trailing statements (for example after any conditional) should be
     on the next line.

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Checkpatch documentation that recommends the user to use the octal permission bits instead of their symbolic macro names.

---
 Documentation/dev-tools/checkpatch.rst | 4 ++++
 1 file changed, 4 insertions(+)

index f0956e9ea..5094e3419 100644
@@ -957,6 +957,10 @@ Permissions
     Permission bits should use 4 digit octal permissions (like 0700 or 0444).
     Avoid using any other base like decimal.
 
+  **SYMBOLIC_PERMS**
+    Permission bits in the octal form are more readable and easier to
+    understand than their symbolic counterparts.
+
 
 Spacing and Brackets
 --------------------

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type TRAILING_SEMICOLON for the macro definitions that conclude with a semicolon.

---
 Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

index f0956e9ea..504edd961 100644
@@ -845,6 +845,26 @@ Macros, Attributes and Symbols
     Use the `fallthrough;` pseudo keyword instead of
     `/* fallthrough */` like comments.
 
+  **TRAILING_SEMICOLON**
+    Macro definition should not conclude with a semicolon. The macro
+    invocation should be consistent with the function call. A function
+    call concludes with a semicolon, so the macro invocation must also
+    conclude with a semicolon. Suppose if a macro MAC is defined::
+
+      #define MAC       \
+        do_something;
+
+    If this macro is used within a if else statement, like::
+
+      if(some_condition)
+              MAC;
+      else
+              do_something;
+
+    Then there would be a compilation error, because when the macro is
+    expanded there are two trailing semicolons, so the else branch gets
+    orphaned.
+
   **WEAK_DECLARATION**
     Using weak declarations like __attribute__((weak)) or __weak
     can have unintended link defects.  Avoid using them.

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type TRAILING_SEMICOLON for the macro definitions that conclude with a semicolon.

---
 Documentation/dev-tools/checkpatch.rst | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

index f0956e9ea..f2ef998ba 100644
@@ -845,6 +845,27 @@ Macros, Attributes and Symbols
     Use the `fallthrough;` pseudo keyword instead of
     `/* fallthrough */` like comments.
 
+  **TRAILING_SEMICOLON**
+    Macro definition should not conclude with a semicolon. The macro
+    invocation should be consistent with the function call. So the macro
+    invocation must conclude with a semicolon. Suppose if a macro MAC is
+    defined::
+
+      #define MAC do_something;
+
+    If this macro is used within a if else statement, like::
+
+      if (some_condition)
+              MAC;
+      else
+              do_something;
+
+    Then there would be a compilation error, because when the macro is
+    expanded there are two trailing semicolons, so the else branch gets
+    orphaned.
+
+    See: https://lore.kernel.org/lkml/1399671106.2912.21.camel@joe-AO725/
+
   **WEAK_DECLARATION**
     Using weak declarations like __attribute__((weak)) or __weak
     can have unintended link defects.  Avoid using them.

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type TRAILING_SEMICOLON for the macro definitions that conclude with a semicolon.

---
 Documentation/dev-tools/checkpatch.rst | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

index f0956e9ea2d8..30eda8f4a8bd 100644
@@ -845,6 +845,27 @@ Macros, Attributes and Symbols
     Use the `fallthrough;` pseudo keyword instead of
     `/* fallthrough */` like comments.
 
+  **TRAILING_SEMICOLON**
+    Macro definition should not end with a semicolon. The macro
+    invocation style should be consistent with function calls.
+    This can prevent any unexpected code paths::
+
+      #define MAC do_something;
+
+    If this macro is used within a if else statement, like::
+
+      if (some_condition)
+              MAC;
+
+      else
+              do_something;
+
+    Then there would be a compilation error, because when the macro is
+    expanded there are two trailing semicolons, so the else branch gets
+    orphaned.
+
+    See: https://lore.kernel.org/lkml/1399671106.2912.21.camel@joe-AO725/
+
   **WEAK_DECLARATION**
     Using weak declarations like __attribute__((weak)) or __weak
     can have unintended link defects.  Avoid using them.

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

This fixed the below checkpatch issue:
WARNING: single byte memset is suspicious. Swapped 2nd/3rd argument?

Logically the code is correct, but it is unusual to use memset for a single byte only. The same can be achieved by using a simple assignment.

---
 drivers/usb/serial/iuu_phoenix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 19753611e..6bab12f4f 100644
@@ -253,7 +253,7 @@ static int iuu_status(struct usb_serial_port *port)
 {
 	int result;
 
-	memset(port->write_urb->transfer_buffer, IUU_GET_STATE_REGISTER, 1);
+	*(char *)port->write_urb->transfer_buffer = IUU_GET_STATE_REGISTER;
 	usb_fill_bulk_urb(port->write_urb, port->serial->dev,
 			  usb_sndbulkpipe(port->serial->dev,
 					  port->bulk_out_endpointAddress),

@@ -1056,7 +1056,7 @@ static int iuu_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 	dev_dbg(dev, "%s - initialization done\n", __func__);
 
-	memset(port->write_urb->transfer_buffer, IUU_UART_RX, 1);
+	*(char *)port->write_urb->transfer_buffer = IUU_UART_RX;
 	usb_fill_bulk_urb(port->write_urb, port->serial->dev,
 			  usb_sndbulkpipe(port->serial->dev,
 					  port->bulk_out_endpointAddress),

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Fix checkpatch warning "quoted string split across lines".

---
 drivers/usb/serial/iuu_phoenix.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

index 19753611e..f2c0dad91 100644
@@ -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.");

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

You could write a line about why this change is required. And that there are no functional changes.

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

... and does this change actually increase the line length beyond 80, or even 100 characters?

So, did you run checkpatch on your patch?

If you are just swapping one violation of one rule by the violation of another rule, you will need to make a judgement which rule is more important and provide a good reasoning for that.

Other than that, it looks good to me.

Lukas

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Yes, I did run the checkpatch on my patch, and all the warnings regarding "quoted string split across lines" were removed. Although the line length increased to more than 100 characters, there were no warnings from the checkpatch.

Since there were no "LONG_LINE" warnings, so I didn't mention about it in the commit description. But will rectify this in v2.

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Fix checkpatch warning "quoted string split across lines". Although the line length increases beyond 80 characters, it makes grepping the entire string easier.

No functional change.

---
 drivers/usb/serial/iuu_phoenix.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

index 19753611e7b0..f2c0dad911c3 100644
@@ -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.");

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

This fixed the below checkpatch issue:
WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.

---
 drivers/usb/serial/iuu_phoenix.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 19753611e..0be3b5e1e 100644
@@ -1188,20 +1188,20 @@ MODULE_AUTHOR("Alain Degreffe [email protected]");
 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.");

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Use imperative; you can drop "below" here.

Usually it is Suggested-by first, then Signed-off-by at the end.

Do you find a pointer for this order of tags on the commit message in the documentation, e.g., in Documentation/process/submitting-patches.rst?

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Fix checkpatch warnings:
WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.

---
 drivers/usb/serial/iuu_phoenix.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 19753611e..0be3b5e1e 100644
@@ -1188,20 +1188,20 @@ MODULE_AUTHOR("Alain Degreffe [email protected]");
 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.");

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Concerning the subject line (commit message header prefix), check with git log --oneline drivers/usb/serial/iuu_phoenix.c for the common style of the prefix here. The prefix "USB: serial: iuu_phoenix:" looks reasonable; did you check that before or is this just a coincidence that it matches?

The patch looks good so far. Utkarsh, will you send this now to the appropriate recipients according to ./scripts/get_maintainer.pl?

Lukas

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Yes, I did check the previous commits for the appropriate subject line.

Thanks, I have sent the patch :)

Regards,
Utkarsh Verma
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

No, I didn't found any such rule in the Documentation files.

Regards,
Utkarsh Verma
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

I think you could try to create a patch to add that information on the ordering of these tags here:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

You will find the source of that documentation at
Documentation/process/submitting-patches.rst.

Do you want to give that a quick try?

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Unfortunately, I didn't find any rule for the sequence of the tags. Tried searching for discussions on the mailing lists but to no avail.

Generally, Suggested-by is first, then Signed-off-by is used, but it is not a hard and fast rule. Many patches do not follow this order, and the tags can be put in any sequence. For example:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=39652741c80bb0006214cf81ba9707d2dc372292
https://lore.kernel.org/linux-devicetree/[email protected]/

Regards,
Utkarsh Verma
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

(snip)

Yes, Linus suggested the tree-wide conversions for using octal permission.
https://lwn.net/Articles/696229/

(snip)

Regards,
Utkarsh Verma
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Well, then add that reference to your patch on checkpatch documentation. Waiting for your v2.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Checkpatch documentation that recommends the user to use the octal permission bits instead of their symbolic macro names.

---
 Documentation/dev-tools/checkpatch.rst | 6 ++++++
 1 file changed, 6 insertions(+)

index f0956e9ea..0d60f8e5e 100644
@@ -957,6 +957,12 @@ Permissions
     Permission bits should use 4 digit octal permissions (like 0700 or 0444).
     Avoid using any other base like decimal.
 
+  **SYMBOLIC_PERMS**
+    Permission bits in the octal form are more readable and easier to
+    understand than their symbolic counterparts.
+
+    See: https://lwn.net/Articles/696229/
+
 
 Spacing and Brackets
 --------------------

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Checkpatch documentation that recommends the user to use the octal permission bits instead of their symbolic macro names.

---
 Documentation/dev-tools/checkpatch.rst | 6 ++++++
 1 file changed, 6 insertions(+)

index f0956e9ea..009957cda 100644
@@ -957,6 +957,12 @@ Permissions
     Permission bits should use 4 digit octal permissions (like 0700 or 0444).
     Avoid using any other base like decimal.
 
+  **SYMBOLIC_PERMS**
+    Permission bits in the octal form are more readable and easier to
+    understand than their symbolic counterparts.
+
+    See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
+
 
 Spacing and Brackets
 --------------------

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Did you mean: Checkpatch recommends the user to use octal permission bits...?

Other than that the change itself looks fine to me.

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

No.
This patch adds documentation that explains to the user why octal permission should be used. So in the commit description, which explains my patch I wrote, "Checkpatch documentation that recommends the user ...."

If it's sounding vague, then should I change the commit description to:

"Add a new message type SYMBOLIC_PERMS under the 'Permissions'
subsection. Users should use octal permission bits instead of their symbolic macro names."

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

I think it reads better that way. Please make the change.

Cheers,
Dwaipayan
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

I think even better would be something like:

Explain to the reader why to use octal permission bits instead of their symbolic macro names.

instead of "Users should use octal permission bits instead of their symbolic macro names."

... and then actually have the documentation explain why (Linus and the rest of the community believes) octal permission bits are better than the symbolic macro names.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Users should use octal permission bits instead of their symbolic macro names.

---
 Documentation/dev-tools/checkpatch.rst | 6 ++++++
 1 file changed, 6 insertions(+)

index f0956e9ea..009957cda 100644
@@ -957,6 +957,12 @@ Permissions
     Permission bits should use 4 digit octal permissions (like 0700 or 0444).
     Avoid using any other base like decimal.
 
+  **SYMBOLIC_PERMS**
+    Permission bits in the octal form are more readable and easier to
+    understand than their symbolic counterparts.
+
+    See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
+
 
 Spacing and Brackets
 --------------------

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Like Lukas said, the reader could use a better explanation as to why octal permission bits are preferred.

However, I think this message shall be fine too as it's pretty much self-explanatory.

So, with that:

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

I think this patch could deserve another iteration addressing my comment. If that then reads worse than before, we simply take this patch, but we should at least give it a try.

The referred email discussion can be probably quickly summarized (without the more emotional aspect of the discussion).

Lukas

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Octal permission bits are easier to read and understand instead of their symbolic macro names.

---
 Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

index f0956e9ea2d8..01105e9c89de 100644
@@ -957,6 +957,17 @@ Permissions
     Permission bits should use 4 digit octal permissions (like 0700 or 0444).
     Avoid using any other base like decimal.
 
+  **SYMBOLIC_PERMS**
+    Permission bits in the octal form are more readable and easier to
+    understand than their symbolic counterparts because many command-line
+    tools use this notation only. Experienced kernel developers have been using
+    this traditional Unix permission bits for decades and so they find it
+    easier to understand the octal notation than the symbolic macros.
+    Also, it is harder to read S_IWUSR|S_IRUGO than 0644, which obscures the
+    developer's intent rather than clarifying it.
+
+    See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
+
 
 Spacing and Brackets
 --------------------

2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees