Linux Kernel: Checkpatch Documentation Mentorship Tasks

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Jul 26 10:40:48 UTC 2021


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


More information about the Linux-kernel-mentees mailing list