* [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions
@ 2021-08-20 19:03 Utkarsh Verma
2021-08-24 13:55 ` Johan Hovold
0 siblings, 1 reply; 11+ messages in thread
From: Utkarsh Verma @ 2021-08-20 19:03 UTC (permalink / raw)
To: Johan Hovold
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Lukas Bulwahn,
Utkarsh Verma
This fixed the below checkpatch issue:
WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
Consider using octal permissions '0644'.
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
drivers/usb/serial/iuu_phoenix.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e7b0..0be3b5e1eaf3 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions
2021-08-20 19:03 [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions Utkarsh Verma
@ 2021-08-24 13:55 ` Johan Hovold
2021-08-24 19:15 ` Utkarsh Verma
0 siblings, 1 reply; 11+ messages in thread
From: Johan Hovold @ 2021-08-24 13:55 UTC (permalink / raw)
To: Utkarsh Verma; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Lukas Bulwahn
On Sat, Aug 21, 2021 at 12:33:06AM +0530, Utkarsh Verma wrote:
> This fixed the below checkpatch issue:
> WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> Consider using octal permissions '0644'.
Please do not run checkpatch.pl on code that's already in the tree. Use
it for your own patches before submitting them and always use your own
judgement when considering its suggestions.
This code does not need to be changed.
Johan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions
2021-08-24 13:55 ` Johan Hovold
@ 2021-08-24 19:15 ` Utkarsh Verma
2021-08-25 9:24 ` Johan Hovold
0 siblings, 1 reply; 11+ messages in thread
From: Utkarsh Verma @ 2021-08-24 19:15 UTC (permalink / raw)
To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Lukas Bulwahn
On Tue, Aug 24, 2021 at 03:55:41PM +0200, Johan Hovold wrote:
> On Sat, Aug 21, 2021 at 12:33:06AM +0530, Utkarsh Verma wrote:
> > This fixed the below checkpatch issue:
> > WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> > Consider using octal permissions '0644'.
>
> Please do not run checkpatch.pl on code that's already in the tree. Use
> it for your own patches before submitting them and always use your own
> judgement when considering its suggestions.
>
Okay, I will not run checkpatch on the code that's already in the tree.
> This code does not need to be changed.
>
But using the octal permission bits makes the code more readable. So I
made the change.
But if you don't want such changes, I will refrain from doing it in the
future.
Anyway thanks for the review.
Regards,
Utkarsh Verma
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions
2021-08-24 19:15 ` Utkarsh Verma
@ 2021-08-25 9:24 ` Johan Hovold
2021-08-25 16:23 ` [PATCH v2] USB: serial: " Utkarsh Verma
0 siblings, 1 reply; 11+ messages in thread
From: Johan Hovold @ 2021-08-25 9:24 UTC (permalink / raw)
To: Utkarsh Verma; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Lukas Bulwahn
On Wed, Aug 25, 2021 at 12:45:37AM +0530, Utkarsh Verma wrote:
> On Tue, Aug 24, 2021 at 03:55:41PM +0200, Johan Hovold wrote:
> > On Sat, Aug 21, 2021 at 12:33:06AM +0530, Utkarsh Verma wrote:
> > > This fixed the below checkpatch issue:
> > > WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred.
> > > Consider using octal permissions '0644'.
> >
> > Please do not run checkpatch.pl on code that's already in the tree. Use
> > it for your own patches before submitting them and always use your own
> > judgement when considering its suggestions.
> >
>
> Okay, I will not run checkpatch on the code that's already in the tree.
>
> > This code does not need to be changed.
>
> But using the octal permission bits makes the code more readable. So I
> made the change.
Then put that in the commit message since that may be a valid motivation
for the change (unlike shutting up checkpatch.pl).
But if you want to do this then do it subsystem wide in one patch rather
than change only one of the seven usb-serial drivers that use the
permission macros.
Johan
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] USB: serial: Replace symbolic permissions by octal permissions
2021-08-25 9:24 ` Johan Hovold
@ 2021-08-25 16:23 ` Utkarsh Verma
2021-08-26 7:51 ` Johan Hovold
0 siblings, 1 reply; 11+ messages in thread
From: Utkarsh Verma @ 2021-08-25 16:23 UTC (permalink / raw)
To: Johan Hovold
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Lukas Bulwahn,
Utkarsh Verma
Replace symbolic permission macros with octal permission numbers
because, octal permission numbers are easier to read and understand
instead of their symbolic macro names.
No functional change.
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
drivers/usb/serial/cypress_m8.c | 6 +++---
drivers/usb/serial/ftdi_sio.c | 2 +-
drivers/usb/serial/garmin_gps.c | 2 +-
drivers/usb/serial/io_ti.c | 4 ++--
drivers/usb/serial/ipaq.c | 4 ++--
drivers/usb/serial/iuu_phoenix.c | 10 +++++-----
drivers/usb/serial/sierra.c | 2 +-
7 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c
index 6b18990258c3..6924fa95f6bd 100644
--- a/drivers/usb/serial/cypress_m8.c
+++ b/drivers/usb/serial/cypress_m8.c
@@ -1199,9 +1199,9 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
-module_param(stats, bool, S_IRUGO | S_IWUSR);
+module_param(stats, bool, 0644);
MODULE_PARM_DESC(stats, "Enable statistics or not");
-module_param(interval, int, S_IRUGO | S_IWUSR);
+module_param(interval, int, 0644);
MODULE_PARM_DESC(interval, "Overrides interrupt interval");
-module_param(unstable_bauds, bool, S_IRUGO | S_IWUSR);
+module_param(unstable_bauds, bool, 0644);
MODULE_PARM_DESC(unstable_bauds, "Allow unstable baud rates");
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 33bbb3470ca3..99d19828dae6 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2938,5 +2938,5 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
-module_param(ndi_latency_timer, int, S_IRUGO | S_IWUSR);
+module_param(ndi_latency_timer, int, 0644);
MODULE_PARM_DESC(ndi_latency_timer, "NDI device latency timer override");
diff --git a/drivers/usb/serial/garmin_gps.c b/drivers/usb/serial/garmin_gps.c
index 756d1ac7e96f..e5c75944ebb7 100644
--- a/drivers/usb/serial/garmin_gps.c
+++ b/drivers/usb/serial/garmin_gps.c
@@ -1444,5 +1444,5 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
-module_param(initial_mode, int, S_IRUGO);
+module_param(initial_mode, int, 0444);
MODULE_PARM_DESC(initial_mode, "Initial mode");
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 84b848d2794e..a7b3c15957ba 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -2746,9 +2746,9 @@ MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
MODULE_FIRMWARE("edgeport/down3.bin");
-module_param(ignore_cpu_rev, bool, S_IRUGO | S_IWUSR);
+module_param(ignore_cpu_rev, bool, 0644);
MODULE_PARM_DESC(ignore_cpu_rev,
"Ignore the cpu revision when connecting to a device");
-module_param(default_uart_mode, int, S_IRUGO | S_IWUSR);
+module_param(default_uart_mode, int, 0644);
MODULE_PARM_DESC(default_uart_mode, "Default uart_mode, 0=RS232, ...");
diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
index f81746c3c26c..e11441bac44f 100644
--- a/drivers/usb/serial/ipaq.c
+++ b/drivers/usb/serial/ipaq.c
@@ -599,10 +599,10 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
-module_param(connect_retries, int, S_IRUGO|S_IWUSR);
+module_param(connect_retries, int, 0644);
MODULE_PARM_DESC(connect_retries,
"Maximum number of connect retries (one second each)");
-module_param(initial_wait, int, S_IRUGO|S_IWUSR);
+module_param(initial_wait, int, 0644);
MODULE_PARM_DESC(initial_wait,
"Time to wait before attempting a connection (in seconds)");
diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 19753611e7b0..0be3b5e1eaf3 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.");
diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
index 4b49b7c33364..9d56138133a9 100644
--- a/drivers/usb/serial/sierra.c
+++ b/drivers/usb/serial/sierra.c
@@ -1056,5 +1056,5 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL v2");
-module_param(nmea, bool, S_IRUGO | S_IWUSR);
+module_param(nmea, bool, 0644);
MODULE_PARM_DESC(nmea, "NMEA streaming");
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] USB: serial: Replace symbolic permissions by octal permissions
2021-08-25 16:23 ` [PATCH v2] USB: serial: " Utkarsh Verma
@ 2021-08-26 7:51 ` Johan Hovold
0 siblings, 0 replies; 11+ messages in thread
From: Johan Hovold @ 2021-08-26 7:51 UTC (permalink / raw)
To: Utkarsh Verma; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Lukas Bulwahn
On Wed, Aug 25, 2021 at 09:53:02PM +0530, Utkarsh Verma wrote:
> Replace symbolic permission macros with octal permission numbers
> because, octal permission numbers are easier to read and understand
> instead of their symbolic macro names.
>
> No functional change.
>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
Thanks for the update. Now applied.
Johan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Linux Kernel: Checkpatch Documentation Mentorship Tasks
@ 2021-07-26 10:40 Lukas Bulwahn
2021-08-03 14:21 ` [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions Utkarsh Verma
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions
2021-07-26 10:40 Linux Kernel: Checkpatch Documentation Mentorship Tasks Lukas Bulwahn
@ 2021-08-03 14:21 ` Utkarsh Verma
2021-08-03 18:11 ` Lukas Bulwahn
0 siblings, 1 reply; 11+ 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] 11+ 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-04 19:12 ` Utkarsh Verma
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions
2021-08-03 18:11 ` Lukas Bulwahn
@ 2021-08-04 19:12 ` Utkarsh Verma
2021-08-05 9:26 ` Lukas Bulwahn
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions
2021-08-04 19:12 ` Utkarsh Verma
@ 2021-08-05 9:26 ` Lukas Bulwahn
2021-08-06 19:12 ` Utkarsh Verma
0 siblings, 1 reply; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread
end of thread, other threads:[~2021-08-26 7:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20 19:03 [PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions Utkarsh Verma
2021-08-24 13:55 ` Johan Hovold
2021-08-24 19:15 ` Utkarsh Verma
2021-08-25 9:24 ` Johan Hovold
2021-08-25 16:23 ` [PATCH v2] USB: serial: " Utkarsh Verma
2021-08-26 7:51 ` Johan Hovold
-- strict thread matches above, loose matches on Subject: below --
2021-07-26 10:40 Linux Kernel: Checkpatch Documentation Mentorship Tasks Lukas Bulwahn
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-04 19:12 ` Utkarsh Verma
2021-08-05 9:26 ` Lukas Bulwahn
2021-08-06 19:12 ` 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.