Linux Kernel: Checkpatch Documentation Mentorship Tasks

Linux Kernel: Checkpatch Documentation Mentorship Tasks

[Utkarsh Verma]

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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks

[Lukas Bulwahn]

On Sat, Jul 17, 2021 at 8:47 AM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> I am interested in the Checkpatch Documentation mentorship program and I would like to work on the tasks for the mentee selection.
>

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: linux-kernel-mentees@lists.linuxfoundation.org.

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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks

[Utkarsh Verma]

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

Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks

[Lukas Bulwahn]

On Sun, Jul 25, 2021 at 12:06 AM Utkarsh Verma
<utkarshverma294@gmail.com> wrote:
>
> 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
>

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)

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

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

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

(snip)

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

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)

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

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.

> 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.");
>
>

(snip)

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

Please provide a patch for the checkpatch documentation here.

Also create a patch for this file for this type of 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.");
>

Overall, we expect you to provide a patch series that adds some more
checkpatch Documentation and some first changes to the file you
investigated. Remember each patch should do one thing.

We will review your patch, provide you feedback and once we are all
happy about the patches, we will let you know to send it to the
larger-audience kernel lists.

Good luck; we are looking forward to your patch series.

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

[PATCH] Documentation: checkpatch: Add SPLIT_STRING message

[Utkarsh Verma]

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.

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea..32a26a800 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] Documentation: checkpatch: Add SPLIT_STRING message

[Dwaipayan Ray]

On Tue, Aug 3, 2021 at 7:49 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> 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.
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea..32a26a800 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -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.
> +

Is there any reference you can point to for this?
Usually LKML links are okay. So you can try adding some discussion
link which helps know more about why this message is important.

>    **TRAILING_STATEMENTS**
>      Trailing statements (for example after any conditional) should be
>      on the next line.
> --
> 2.17.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] Documentation: checkpatch: Add SPLIT_STRING message

[Utkarsh Verma]

On Tue, Aug 03, 2021 at 08:22:39PM +0530, Dwaipayan Ray wrote:
> On Tue, Aug 3, 2021 at 7:49 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > 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.
> >
> > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> >  Documentation/dev-tools/checkpatch.rst | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > index f0956e9ea..32a26a800 100644
> > --- a/Documentation/dev-tools/checkpatch.rst
> > +++ b/Documentation/dev-tools/checkpatch.rst
> > @@ -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.
> > +
> 
> Is there any reference you can point to for this?
> Usually LKML links are okay. So you can try adding some discussion
> link which helps know more about why this message is important.
>

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?

> >    **TRAILING_STATEMENTS**
> >      Trailing statements (for example after any conditional) should be
> >      on the next line.
> > --
> > 2.17.1
> >
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] Documentation: checkpatch: Add SPLIT_STRING message

[Lukas Bulwahn]

On Tue, Aug 3, 2021 at 8:14 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> On Tue, Aug 03, 2021 at 08:22:39PM +0530, Dwaipayan Ray wrote:
> > On Tue, Aug 3, 2021 at 7:49 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> > >
> > > 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.
> > >
> > > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > ---
> > >  Documentation/dev-tools/checkpatch.rst | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > > index f0956e9ea..32a26a800 100644
> > > --- a/Documentation/dev-tools/checkpatch.rst
> > > +++ b/Documentation/dev-tools/checkpatch.rst
> > > @@ -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.
> > > +
> >
> > Is there any reference you can point to for this?
> > Usually LKML links are okay. So you can try adding some discussion
> > link which helps know more about why this message is important.
> >
>
> Is this reference fine:
> https://linux-kernel.vger.kernel.narkive.com/vc8OoIUF/patch-checkpatch-check-for-quoted-strings-broken-across-lines
>

Use Links to lore.kernel.org.

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

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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH v2] Documentation: checkpatch: Add SPLIT_STRING message

[Utkarsh Verma]

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.

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea..b7a1288e9 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Utkarsh Verma]

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.

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea..5094e3419 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH] Documentation: checkpatch: Add TRAILING_SEMICOLON message

[Utkarsh Verma]

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

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea..504edd961 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] Documentation: checkpatch: Add TRAILING_SEMICOLON message

[Dwaipayan Ray]

On Tue, Aug 3, 2021 at 7:50 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Add a new message type TRAILING_SEMICOLON for the macro definitions
> that conclude with a semicolon.
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea..504edd961 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -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::

The objective is really to avoid unexpected code paths. The explanation
seems correct but can be shortened a bit?

These two sentences can be combined:
The macro invocation should be consistent with the function call. A function
call concludes with a semicolon (We already know this right?), so the
macro invocation
must also conclude with a semicolon.

Also let's add link to the discussion which introduced it:
https://lore.kernel.org/lkml/alpine.DEB.2.02.1405092345480.6261@ionos.tec.linutronix.de/


> +
> +      #define MAC       \
> +        do_something;

combine it in one line?

> +
> +    If this macro is used within a if else statement, like::
> +
> +      if(some_condition)

space after if

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

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

[PATCH v2] Documentation: checkpatch: Add TRAILING_SEMICOLON message

[Utkarsh Verma]

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

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea..f2ef998ba 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v2] Documentation: checkpatch: Add TRAILING_SEMICOLON message

[Dwaipayan Ray]

On Wed, Aug 4, 2021 at 2:50 AM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Add a new message type TRAILING_SEMICOLON for the macro definitions
> that conclude with a semicolon.
>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea..f2ef998ba 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -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::

Maybe:

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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH v3] Documentation: checkpatch: Add TRAILING_SEMICOLON message

[Utkarsh Verma]

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

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..30eda8f4a8bd 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v3] Documentation: checkpatch: Add TRAILING_SEMICOLON message

[Dwaipayan Ray]

On Sat, Aug 21, 2021 at 1:23 AM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Add a new message type TRAILING_SEMICOLON for the macro definitions
> that conclude with a semicolon.
>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..30eda8f4a8bd 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -845,6 +845,27 @@ Macros, Attributes and Symbols
>      Use the `fallthrough;` pseudo keyword instead of
>      `/* fallthrough */` like comments.
>

I think this is good enough.

Reviewed-by: Dwaipayan Ray <dwaipayanray1@gmail.com>

Lukas, any inputs?

Thanks,
Dwaipayan.

> +  **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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH] USB: serial: iuu_phoenix: fix checkpatch memset warning

[Utkarsh Verma]

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.

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 drivers/usb/serial/iuu_phoenix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e..6bab12f4f 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines

[Utkarsh Verma]

Fix checkpatch warning "quoted string split across lines".

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 drivers/usb/serial/iuu_phoenix.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

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.");
-- 
2.17.1

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

Re: [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines

[Dwaipayan Ray]

On Tue, Aug 3, 2021 at 7:51 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Fix checkpatch warning "quoted string split across lines".
>

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

> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
>  drivers/usb/serial/iuu_phoenix.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> 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.");
> --
> 2.17.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines

[Lukas Bulwahn]

On Thu, Aug 5, 2021 at 3:53 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> On Tue, Aug 3, 2021 at 7:51 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > Fix checkpatch warning "quoted string split across lines".
> >
>
> You could write a line about why this change is required.
> And that there are no functional changes.
>

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

> > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> >  drivers/usb/serial/iuu_phoenix.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > 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.");
> > --
> > 2.17.1
> >
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines

[Utkarsh Verma]

On Thu, Aug 05, 2021 at 04:20:56PM +0200, Lukas Bulwahn wrote:
> On Thu, Aug 5, 2021 at 3:53 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
> >
> > On Tue, Aug 3, 2021 at 7:51 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> > >
> > > Fix checkpatch warning "quoted string split across lines".
> > >
> >
> > You could write a line about why this change is required.
> > And that there are no functional changes.
> >
> 
> ... and does this change actually increase the line length beyond 80,
> or even 100 characters?
> 
> So, did you run checkpatch on your patch?
> 

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.

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

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

> Other than that, it looks good to me.
> 
> Lukas
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH v2] USB: serial: iuu_phoenix: fix quoted string split across lines

[Utkarsh Verma]

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.

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 drivers/usb/serial/iuu_phoenix.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e7b0..f2c0dad911c3 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.");
-- 
2.17.1

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

[PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions

[Utkarsh Verma]

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

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 drivers/usb/serial/iuu_phoenix.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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.");
-- 
2.17.1

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

Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions

[Lukas Bulwahn]

On Tue, Aug 3, 2021 at 4:21 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> This fixed the below checkpatch issue:

Use imperative; you can drop "below" here.

> WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> Consider using octal permissions '0644'.
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>

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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH v2] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions

[Utkarsh Verma]

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

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 drivers/usb/serial/iuu_phoenix.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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.");
-- 
2.17.1

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

Re: [PATCH v2] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions

[Lukas Bulwahn]

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

On Tue, Aug 3, 2021 at 8:38 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Fix checkpatch warnings:
> WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> Consider using octal permissions '0644'.
>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  drivers/usb/serial/iuu_phoenix.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> 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.");
> --
> 2.17.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v2] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions

[Utkarsh Verma]

On Fri, Aug 20, 2021 at 12:09:22PM +0200, Lukas Bulwahn wrote:
> 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?
>

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

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

Thanks, I have sent the patch :)

> Lukas
> 
> On Tue, Aug 3, 2021 at 8:38 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > Fix checkpatch warnings:
> > WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> > Consider using octal permissions '0644'.
> >
> > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > ---
> >  drivers/usb/serial/iuu_phoenix.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > 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.");
> > --
> > 2.17.1
> >

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

Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions

[Utkarsh Verma]

On Tue, Aug 03, 2021 at 08:11:28PM +0200, Lukas Bulwahn wrote:
> On Tue, Aug 3, 2021 at 4:21 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > This fixed the below checkpatch issue:
> 
> Use imperative; you can drop "below" here.
> 
> > WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> > Consider using octal permissions '0644'.
> >
> > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> 
> 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?

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


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

Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions

[Lukas Bulwahn]

On Wed, Aug 4, 2021 at 9:12 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> On Tue, Aug 03, 2021 at 08:11:28PM +0200, Lukas Bulwahn wrote:
> > On Tue, Aug 3, 2021 at 4:21 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> > >
> > > This fixed the below checkpatch issue:
> >
> > Use imperative; you can drop "below" here.
> >
> > > WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> > > Consider using octal permissions '0644'.
> > >
> > > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> >
> > 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?
>
> No, I didn't found any such rule in the Documentation files.
>

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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions

[Utkarsh Verma]

On Thu, Aug 05, 2021 at 11:26:49AM +0200, Lukas Bulwahn wrote:
> On Wed, Aug 4, 2021 at 9:12 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > On Tue, Aug 03, 2021 at 08:11:28PM +0200, Lukas Bulwahn wrote:
> > > On Tue, Aug 3, 2021 at 4:21 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> > > >
> > > > This fixed the below checkpatch issue:
> > >
> > > Use imperative; you can drop "below" here.
> > >
> > > > WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> > > > Consider using octal permissions '0644'.
> > > >
> > > > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > > > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > >
> > > 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?
> >
> > No, I didn't found any such rule in the Documentation files.
> >
> 
> 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?
> 

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/20210221174930.27324-10-nramas@linux.microsoft.com/


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

Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks

[Utkarsh Verma]

On Mon, Jul 26, 2021 at 12:40:48PM +0200, Lukas Bulwahn wrote:
>
> On Sun, Jul 25, 2021 at 12:06 AM Utkarsh Verma
> <utkarshverma294@gmail.com> wrote:
> >
> > 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.
> > >

(snip)

> >
> > 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.
> >
> 
> 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?
> 

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

(snip)

> 
> Good luck; we are looking forward to your patch series.
> 
> Lukas


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

Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks

[Lukas Bulwahn]

On Tue, Aug 3, 2021 at 6:36 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> On Mon, Jul 26, 2021 at 12:40:48PM +0200, Lukas Bulwahn wrote:
> >
> > On Sun, Jul 25, 2021 at 12:06 AM Utkarsh Verma
> > <utkarshverma294@gmail.com> wrote:
> > >
> > > 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.
> > > >
>
> (snip)
>
> > >
> > > 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.
> > >
> >
> > 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?
> >
>
> Yes, Linus suggested the tree-wide conversions for using octal permission.
> https://lwn.net/Articles/696229/
>

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

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

[PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Utkarsh Verma]

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.

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea..0d60f8e5e 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Dwaipayan Ray]

On Wed, Aug 4, 2021 at 3:07 AM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> 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.
>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea..0d60f8e5e 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -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/

Please use lore.kernel.org links wherever possible. That article
should be also there in the list.

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

[PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Utkarsh Verma]

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.

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea..009957cda 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Dwaipayan Ray]

On Wed, Aug 4, 2021 at 9:02 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> subsection. Checkpatch documentation that recommends the user to use

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

Other than that the change itself looks fine to me.

> the octal permission bits instead of their symbolic macro names.
>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea..009957cda 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Utkarsh Verma]

On Thu, Aug 05, 2021 at 07:21:50PM +0530, Dwaipayan Ray wrote:
> On Wed, Aug 4, 2021 at 9:02 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> > subsection. Checkpatch documentation that recommends the user to use
> 
> Did you mean: Checkpatch recommends the user to use octal
> permission bits...?
> 

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."

> Other than that the change itself looks fine to me.
> 
> > the octal permission bits instead of their symbolic macro names.
> >
> > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > ---
> >  Documentation/dev-tools/checkpatch.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > index f0956e9ea..009957cda 100644
> > --- a/Documentation/dev-tools/checkpatch.rst
> > +++ b/Documentation/dev-tools/checkpatch.rst
> > @@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Dwaipayan Ray]

On Fri, Aug 6, 2021 at 7:16 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> On Thu, Aug 05, 2021 at 07:21:50PM +0530, Dwaipayan Ray wrote:
> > On Wed, Aug 4, 2021 at 9:02 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> > >
> > > Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> > > subsection. Checkpatch documentation that recommends the user to use
> >
> > Did you mean: Checkpatch recommends the user to use octal
> > permission bits...?
> >
>
> 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."
>

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

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

Re: [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Lukas Bulwahn]

On Fri, Aug 6, 2021 at 9:56 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 7:16 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > On Thu, Aug 05, 2021 at 07:21:50PM +0530, Dwaipayan Ray wrote:
> > > On Wed, Aug 4, 2021 at 9:02 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> > > >
> > > > Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> > > > subsection. Checkpatch documentation that recommends the user to use
> > >
> > > Did you mean: Checkpatch recommends the user to use octal
> > > permission bits...?
> > >
> >
> > 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."
> >
>
> I think it reads better that way. Please make the change.
>

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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH v4] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Utkarsh Verma]

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

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea..009957cda 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v4] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Dwaipayan Ray]

On Sat, Aug 7, 2021 at 1:41 AM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> subsection. Users should use octal permission bits instead of their
> symbolic macro names.
>

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:

Reviewed-by: Dwaipayan Ray <dwaipayanray1@gmail.com>

> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea..009957cda 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v4] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Lukas Bulwahn]

On Sun, Aug 22, 2021 at 7:52 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> On Sat, Aug 7, 2021 at 1:41 AM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > Add a new message type SYMBOLIC_PERMS under the 'Permissions'
> > subsection. Users should use octal permission bits instead of their
> > symbolic macro names.
> >
>
> Like Lukas said, the reader could use a better explanation as to why octal
> permission bits are preferred.
>

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

> However, I think this message shall be fine too as it's pretty much
> self-explanatory.
>
> So, with that:
>
> Reviewed-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
>
> > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > ---
> >  Documentation/dev-tools/checkpatch.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > index f0956e9ea..009957cda 100644
> > --- a/Documentation/dev-tools/checkpatch.rst
> > +++ b/Documentation/dev-tools/checkpatch.rst
> > @@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH v5] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Utkarsh Verma]

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.

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..01105e9c89de 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Utkarsh Verma]

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.

Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..01105e9c89de 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -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.25.1

Re: [PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message

[Lukas Bulwahn]

On Sat, Sep 4, 2021 at 9:30 AM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> 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.
>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..01105e9c89de 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -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.

Just a quick stylistic nit:

s/Also, it is harder to read /For example, it is harder to read/

Other than that:

Acked-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Reviewed-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>

Feel free to send a quick v2 for my nitpicking, and apply the tags
from this email.

Lukas

> +
> +    See: https://lore.kernel.org/lkml/CA+55aFw5v23T-zvDZp-MmD_EYxF8WbafwwB59934FV7g21uMGQ@mail.gmail.com/
> +
>
>  Spacing and Brackets
>  --------------------
> --
> 2.25.1
>