* Linux Kernel: Checkpatch Documentation Mentorship Tasks @ 2021-07-17 6:46 Utkarsh Verma 2021-07-17 7:23 ` Lukas Bulwahn 0 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-07-17 6:46 UTC (permalink / raw) To: dwaipayanray1, lukas.bulwahn, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks 2021-07-17 6:46 Linux Kernel: Checkpatch Documentation Mentorship Tasks Utkarsh Verma @ 2021-07-17 7:23 ` Lukas Bulwahn 2021-07-24 22:06 ` Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Lukas Bulwahn @ 2021-07-17 7:23 UTC (permalink / raw) To: Utkarsh Verma; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks 2021-07-17 7:23 ` Lukas Bulwahn @ 2021-07-24 22:06 ` Utkarsh Verma 2021-07-26 10:40 ` Lukas Bulwahn 0 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-07-24 22:06 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: dwaipayanray1, linux-kernel-mentees 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks 2021-07-24 22:06 ` Utkarsh Verma @ 2021-07-26 10:40 ` Lukas Bulwahn 2021-08-03 14:17 ` [PATCH] Documentation: checkpatch: Add SPLIT_STRING message Utkarsh Verma ` (6 more replies) 0 siblings, 7 replies; 43+ messages in thread From: Lukas Bulwahn @ 2021-07-26 10:40 UTC (permalink / raw) To: Utkarsh Verma; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH] Documentation: checkpatch: Add SPLIT_STRING message 2021-07-26 10:40 ` Lukas Bulwahn @ 2021-08-03 14:17 ` Utkarsh Verma 2021-08-03 14:52 ` Dwaipayan Ray 2021-08-03 14:19 ` [PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message Utkarsh Verma ` (5 subsequent siblings) 6 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 14:17 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees Add a new message type SPLIT_STRING under the 'Indentation and Line Breaks' subsection. Checkpatch documentation for the splitting of quoted strings that appear in userspace, across multiple lines. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Add SPLIT_STRING message 2021-08-03 14:17 ` [PATCH] Documentation: checkpatch: Add SPLIT_STRING message Utkarsh Verma @ 2021-08-03 14:52 ` Dwaipayan Ray 2021-08-03 18:13 ` Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-03 14:52 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Add SPLIT_STRING message 2021-08-03 14:52 ` Dwaipayan Ray @ 2021-08-03 18:13 ` Utkarsh Verma 2021-08-03 18:20 ` Lukas Bulwahn 0 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 18:13 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Add SPLIT_STRING message 2021-08-03 18:13 ` Utkarsh Verma @ 2021-08-03 18:20 ` Lukas Bulwahn 2021-08-04 18:09 ` [PATCH v2] " Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Lukas Bulwahn @ 2021-08-03 18:20 UTC (permalink / raw) To: Utkarsh Verma; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2] Documentation: checkpatch: Add SPLIT_STRING message 2021-08-03 18:20 ` Lukas Bulwahn @ 2021-08-04 18:09 ` Utkarsh Verma 0 siblings, 0 replies; 43+ messages in thread From: Utkarsh Verma @ 2021-08-04 18:09 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees Add a new message type SPLIT_STRING under the 'Indentation and Line Breaks' subsection. Checkpatch documentation for the splitting of quoted strings that appear in userspace, across multiple lines. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-07-26 10:40 ` Lukas Bulwahn 2021-08-03 14:17 ` [PATCH] Documentation: checkpatch: Add SPLIT_STRING message Utkarsh Verma @ 2021-08-03 14:19 ` Utkarsh Verma 2021-08-03 14:20 ` [PATCH] Documentation: checkpatch: Add TRAILING_SEMICOLON message Utkarsh Verma ` (4 subsequent siblings) 6 siblings, 0 replies; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 14:19 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Checkpatch documentation that recommends the user to use the octal permission bits instead of their symbolic macro names. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] Documentation: checkpatch: Add TRAILING_SEMICOLON message 2021-07-26 10:40 ` Lukas Bulwahn 2021-08-03 14:17 ` [PATCH] Documentation: checkpatch: Add SPLIT_STRING message Utkarsh Verma 2021-08-03 14:19 ` [PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message Utkarsh Verma @ 2021-08-03 14:20 ` Utkarsh Verma 2021-08-03 15:20 ` Dwaipayan Ray 2021-08-03 14:20 ` [PATCH] USB: serial: iuu_phoenix: fix checkpatch memset warning Utkarsh Verma ` (3 subsequent siblings) 6 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 14:20 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Add TRAILING_SEMICOLON message 2021-08-03 14:20 ` [PATCH] Documentation: checkpatch: Add TRAILING_SEMICOLON message Utkarsh Verma @ 2021-08-03 15:20 ` Dwaipayan Ray 2021-08-03 21:19 ` [PATCH v2] " Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-03 15:20 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2] Documentation: checkpatch: Add TRAILING_SEMICOLON message 2021-08-03 15:20 ` Dwaipayan Ray @ 2021-08-03 21:19 ` Utkarsh Verma 2021-08-05 13:49 ` Dwaipayan Ray 0 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 21:19 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2] Documentation: checkpatch: Add TRAILING_SEMICOLON message 2021-08-03 21:19 ` [PATCH v2] " Utkarsh Verma @ 2021-08-05 13:49 ` Dwaipayan Ray 2021-08-20 19:53 ` [PATCH v3] " Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-05 13:49 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v3] Documentation: checkpatch: Add TRAILING_SEMICOLON message 2021-08-05 13:49 ` Dwaipayan Ray @ 2021-08-20 19:53 ` Utkarsh Verma 2021-08-22 17:29 ` Dwaipayan Ray 0 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-20 19:53 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Documentation: checkpatch: Add TRAILING_SEMICOLON message 2021-08-20 19:53 ` [PATCH v3] " Utkarsh Verma @ 2021-08-22 17:29 ` Dwaipayan Ray 0 siblings, 0 replies; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-22 17:29 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH] USB: serial: iuu_phoenix: fix checkpatch memset warning 2021-07-26 10:40 ` Lukas Bulwahn ` (2 preceding siblings ...) 2021-08-03 14:20 ` [PATCH] Documentation: checkpatch: Add TRAILING_SEMICOLON message Utkarsh Verma @ 2021-08-03 14:20 ` Utkarsh Verma 2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines Utkarsh Verma ` (2 subsequent siblings) 6 siblings, 0 replies; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 14:20 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees This fixed the below checkpatch issue: WARNING: single byte memset is suspicious. Swapped 2nd/3rd argument? Logically the code is correct, but it is unusual to use memset for a single byte only. The same can be achieved by using a simple assignment. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines 2021-07-26 10:40 ` Lukas Bulwahn ` (3 preceding siblings ...) 2021-08-03 14:20 ` [PATCH] USB: serial: iuu_phoenix: fix checkpatch memset warning Utkarsh Verma @ 2021-08-03 14:21 ` Utkarsh Verma 2021-08-05 13:52 ` Dwaipayan Ray 2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions Utkarsh Verma 2021-08-03 16:36 ` Linux Kernel: Checkpatch Documentation Mentorship Tasks Utkarsh Verma 6 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 14:21 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines 2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines Utkarsh Verma @ 2021-08-05 13:52 ` Dwaipayan Ray 2021-08-05 14:20 ` Lukas Bulwahn 0 siblings, 1 reply; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-05 13:52 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines 2021-08-05 13:52 ` Dwaipayan Ray @ 2021-08-05 14:20 ` Lukas Bulwahn 2021-08-06 13:12 ` Utkarsh Verma 2021-08-25 8:22 ` [PATCH v2] " Utkarsh Verma 0 siblings, 2 replies; 43+ messages in thread From: Lukas Bulwahn @ 2021-08-05 14:20 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines 2021-08-05 14:20 ` Lukas Bulwahn @ 2021-08-06 13:12 ` Utkarsh Verma 2021-08-25 8:22 ` [PATCH v2] " Utkarsh Verma 1 sibling, 0 replies; 43+ messages in thread From: Utkarsh Verma @ 2021-08-06 13:12 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2] USB: serial: iuu_phoenix: fix quoted string split across lines 2021-08-05 14:20 ` Lukas Bulwahn 2021-08-06 13:12 ` Utkarsh Verma @ 2021-08-25 8:22 ` Utkarsh Verma 1 sibling, 0 replies; 43+ messages in thread From: Utkarsh Verma @ 2021-08-25 8:22 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees Fix checkpatch warning "quoted string split across lines". Although the line length increases beyond 80 characters, it makes grepping the entire string easier. No functional change. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions 2021-07-26 10:40 ` Lukas Bulwahn ` (4 preceding siblings ...) 2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines Utkarsh Verma @ 2021-08-03 14:21 ` Utkarsh Verma 2021-08-03 18:11 ` Lukas Bulwahn 2021-08-03 16:36 ` Linux Kernel: Checkpatch Documentation Mentorship Tasks Utkarsh Verma 6 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 14:21 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions 2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions Utkarsh Verma @ 2021-08-03 18:11 ` Lukas Bulwahn 2021-08-03 18:37 ` [PATCH v2] " Utkarsh Verma 2021-08-04 19:12 ` [PATCH] " Utkarsh Verma 0 siblings, 2 replies; 43+ messages in thread From: Lukas Bulwahn @ 2021-08-03 18:11 UTC (permalink / raw) To: Utkarsh Verma; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions 2021-08-03 18:11 ` Lukas Bulwahn @ 2021-08-03 18:37 ` Utkarsh Verma 2021-08-20 10:09 ` Lukas Bulwahn 2021-08-04 19:12 ` [PATCH] " Utkarsh Verma 1 sibling, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 18:37 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions 2021-08-03 18:37 ` [PATCH v2] " Utkarsh Verma @ 2021-08-20 10:09 ` Lukas Bulwahn 2021-08-20 19:08 ` Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Lukas Bulwahn @ 2021-08-20 10:09 UTC (permalink / raw) To: Utkarsh Verma; +Cc: Dwaipayan Ray, linux-kernel-mentees Concerning the subject line (commit message header prefix), check with git log --oneline drivers/usb/serial/iuu_phoenix.c for the common style of the prefix here. The prefix "USB: serial: iuu_phoenix:" looks reasonable; did you check that before or is this just a coincidence that it matches? The patch looks good so far. Utkarsh, will you send this now to the appropriate recipients according to ./scripts/get_maintainer.pl? Lukas 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions 2021-08-20 10:09 ` Lukas Bulwahn @ 2021-08-20 19:08 ` Utkarsh Verma 0 siblings, 0 replies; 43+ messages in thread From: Utkarsh Verma @ 2021-08-20 19:08 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions 2021-08-03 18:11 ` Lukas Bulwahn 2021-08-03 18:37 ` [PATCH v2] " Utkarsh Verma @ 2021-08-04 19:12 ` Utkarsh Verma 2021-08-05 9:26 ` Lukas Bulwahn 1 sibling, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-04 19:12 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions 2021-08-04 19:12 ` [PATCH] " Utkarsh Verma @ 2021-08-05 9:26 ` Lukas Bulwahn 2021-08-06 19:12 ` Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Lukas Bulwahn @ 2021-08-05 9:26 UTC (permalink / raw) To: Utkarsh Verma; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions 2021-08-05 9:26 ` Lukas Bulwahn @ 2021-08-06 19:12 ` Utkarsh Verma 0 siblings, 0 replies; 43+ messages in thread From: Utkarsh Verma @ 2021-08-06 19:12 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks 2021-07-26 10:40 ` Lukas Bulwahn ` (5 preceding siblings ...) 2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions Utkarsh Verma @ 2021-08-03 16:36 ` Utkarsh Verma 2021-08-03 18:07 ` Lukas Bulwahn 6 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 16:36 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks 2021-08-03 16:36 ` Linux Kernel: Checkpatch Documentation Mentorship Tasks Utkarsh Verma @ 2021-08-03 18:07 ` Lukas Bulwahn 2021-08-03 21:37 ` [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Lukas Bulwahn @ 2021-08-03 18:07 UTC (permalink / raw) To: Utkarsh Verma; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-03 18:07 ` Lukas Bulwahn @ 2021-08-03 21:37 ` Utkarsh Verma 2021-08-03 21:49 ` Dwaipayan Ray 0 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-03 21:37 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Checkpatch documentation that recommends the user to use the octal permission bits instead of their symbolic macro names. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-03 21:37 ` [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message Utkarsh Verma @ 2021-08-03 21:49 ` Dwaipayan Ray 2021-08-04 15:31 ` [PATCH v3] " Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-03 21:49 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-03 21:49 ` Dwaipayan Ray @ 2021-08-04 15:31 ` Utkarsh Verma 2021-08-05 13:51 ` Dwaipayan Ray 0 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-04 15:31 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Checkpatch documentation that recommends the user to use the octal permission bits instead of their symbolic macro names. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-04 15:31 ` [PATCH v3] " Utkarsh Verma @ 2021-08-05 13:51 ` Dwaipayan Ray 2021-08-06 13:46 ` Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-05 13:51 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-05 13:51 ` Dwaipayan Ray @ 2021-08-06 13:46 ` Utkarsh Verma 2021-08-06 19:56 ` Dwaipayan Ray 0 siblings, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-06 13:46 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-06 13:46 ` Utkarsh Verma @ 2021-08-06 19:56 ` Dwaipayan Ray 2021-08-06 20:03 ` Lukas Bulwahn 2021-08-06 20:10 ` [PATCH v4] " Utkarsh Verma 0 siblings, 2 replies; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-06 19:56 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-06 19:56 ` Dwaipayan Ray @ 2021-08-06 20:03 ` Lukas Bulwahn 2021-08-06 20:10 ` [PATCH v4] " Utkarsh Verma 1 sibling, 0 replies; 43+ messages in thread From: Lukas Bulwahn @ 2021-08-06 20:03 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-06 19:56 ` Dwaipayan Ray 2021-08-06 20:03 ` Lukas Bulwahn @ 2021-08-06 20:10 ` Utkarsh Verma 2021-08-22 17:52 ` Dwaipayan Ray 1 sibling, 1 reply; 43+ messages in thread From: Utkarsh Verma @ 2021-08-06 20:10 UTC (permalink / raw) To: Lukas Bulwahn, Dwaipayan Ray; +Cc: linux-kernel-mentees Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Users should use octal permission bits instead of their symbolic macro names. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-06 20:10 ` [PATCH v4] " Utkarsh Verma @ 2021-08-22 17:52 ` Dwaipayan Ray 2021-08-23 7:18 ` Lukas Bulwahn 0 siblings, 1 reply; 43+ messages in thread From: Dwaipayan Ray @ 2021-08-22 17:52 UTC (permalink / raw) To: Utkarsh Verma; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-22 17:52 ` Dwaipayan Ray @ 2021-08-23 7:18 ` Lukas Bulwahn 2021-08-24 22:13 ` [PATCH v5] " Utkarsh Verma 0 siblings, 1 reply; 43+ messages in thread From: Lukas Bulwahn @ 2021-08-23 7:18 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5] Documentation: checkpatch: Add SYMBOLIC_PERMS message 2021-08-23 7:18 ` Lukas Bulwahn @ 2021-08-24 22:13 ` Utkarsh Verma 0 siblings, 0 replies; 43+ messages in thread From: Utkarsh Verma @ 2021-08-24 22:13 UTC (permalink / raw) To: Lukas Bulwahn, Dwaipayan Ray; +Cc: linux-kernel-mentees Add a new message type SYMBOLIC_PERMS under the 'Permissions' subsection. Octal permission bits are easier to read and understand instead of their symbolic macro names. 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
end of thread, other threads:[~2021-08-25 8:23 UTC | newest] Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-17 6:46 Linux Kernel: Checkpatch Documentation Mentorship Tasks Utkarsh Verma 2021-07-17 7:23 ` Lukas Bulwahn 2021-07-24 22:06 ` Utkarsh Verma 2021-07-26 10:40 ` Lukas Bulwahn 2021-08-03 14:17 ` [PATCH] Documentation: checkpatch: Add SPLIT_STRING message Utkarsh Verma 2021-08-03 14:52 ` Dwaipayan Ray 2021-08-03 18:13 ` Utkarsh Verma 2021-08-03 18:20 ` Lukas Bulwahn 2021-08-04 18:09 ` [PATCH v2] " Utkarsh Verma 2021-08-03 14:19 ` [PATCH] Documentation: checkpatch: Add SYMBOLIC_PERMS message Utkarsh Verma 2021-08-03 14:20 ` [PATCH] Documentation: checkpatch: Add TRAILING_SEMICOLON message Utkarsh Verma 2021-08-03 15:20 ` Dwaipayan Ray 2021-08-03 21:19 ` [PATCH v2] " Utkarsh Verma 2021-08-05 13:49 ` Dwaipayan Ray 2021-08-20 19:53 ` [PATCH v3] " Utkarsh Verma 2021-08-22 17:29 ` Dwaipayan Ray 2021-08-03 14:20 ` [PATCH] USB: serial: iuu_phoenix: fix checkpatch memset warning Utkarsh Verma 2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: fix quoted string split across lines Utkarsh Verma 2021-08-05 13:52 ` Dwaipayan Ray 2021-08-05 14:20 ` Lukas Bulwahn 2021-08-06 13:12 ` Utkarsh Verma 2021-08-25 8:22 ` [PATCH v2] " Utkarsh Verma 2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions Utkarsh Verma 2021-08-03 18:11 ` Lukas Bulwahn 2021-08-03 18:37 ` [PATCH v2] " Utkarsh Verma 2021-08-20 10:09 ` Lukas Bulwahn 2021-08-20 19:08 ` Utkarsh Verma 2021-08-04 19:12 ` [PATCH] " Utkarsh Verma 2021-08-05 9:26 ` Lukas Bulwahn 2021-08-06 19:12 ` Utkarsh Verma 2021-08-03 16:36 ` Linux Kernel: Checkpatch Documentation Mentorship Tasks Utkarsh Verma 2021-08-03 18:07 ` Lukas Bulwahn 2021-08-03 21:37 ` [PATCH v2] Documentation: checkpatch: Add SYMBOLIC_PERMS message Utkarsh Verma 2021-08-03 21:49 ` Dwaipayan Ray 2021-08-04 15:31 ` [PATCH v3] " Utkarsh Verma 2021-08-05 13:51 ` Dwaipayan Ray 2021-08-06 13:46 ` Utkarsh Verma 2021-08-06 19:56 ` Dwaipayan Ray 2021-08-06 20:03 ` Lukas Bulwahn 2021-08-06 20:10 ` [PATCH v4] " Utkarsh Verma 2021-08-22 17:52 ` Dwaipayan Ray 2021-08-23 7:18 ` Lukas Bulwahn 2021-08-24 22:13 ` [PATCH v5] " Utkarsh Verma
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.