Re: Submission of task 1 for checkpatch documentation mentorship program | linux-kernel-mentees

quoted

Rahul, please see below the further tasks to continue.

On Sat, Jul 31, 2021 at 10:38 PM Rahul Balaji [off-list ref] wrote:
Respected mentor,

Thank you for giving me the first task.

The results after running checkpatch.pl on each of the files given is as below.
I will do my best to explain each of the information presented in the result.

https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html -- link to documentation used.

As per my understanding, there are 3 different levels for the information generated.
They are explained below from highest to lowest level of strictness.

ERROR : requires immediate attention. They are most likely wrong.
WARNING : requires careful review, but not as serious issue compared to ERROR.
CHECK : requires a look, but it is of the mildest level.
-----------------------------------------------------------------------------
RESULT (1):

./scripts/checkpatch.pl drivers/mtd/ubi/block.c

WARNING: quoted string split across lines
#354: FILE: drivers/mtd/ubi/block.c:354:
+ pr_warn("UBI: block: volume size is not a multiple of 512, "
+ "last %llu bytes are ignored!\n",

message not present in documentation.
this warning, as stated, is generated because of the quoted strings being split across lines.
we can solve this by just keeping them in a single line.
if the last line ends with a quote and the next line also begins with a quote, then this issue will arise.
WARNING: braces {} are not necessary for single statement blocks
#374: FILE: drivers/mtd/ubi/block.c:374:
+ if (ret) {
+ return ret;
+ }

proper way to use braces is explained in the documentation, which is related to the message here.
single statement blocks don't require brackets, removing them will solve the issue.
proper way to use braces is explained in the documentation.
I believe it checks whether there is only one statement written between the braces. if condition is true, then warn the user.
WARNING: Statements terminations use 1 semicolon
#408: FILE: drivers/mtd/ubi/block.c:408:
+ goto out_free_dev;;

message not present in documentation.
using two semicolons resulted in this warning, can be solved by [ goto out_free_dev; ].
it checks if there is another semicolon (;) immediately after one, so if there is a ";;" anywhere, then it issues a warning.
We identified 13 violations of this rule (ONE_SEMICOLON) in the kernel
source code:

drivers/mtd/ubi/block.c:408: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon

drivers/gpu/drm/nouveau/nouveau_gem.c:342: WARNING:ONE_SEMICOLON:
Statements terminations use 1 semicolon

fs/block_dev.c:1269: WARNING:ONE_SEMICOLON: Statements terminations
use 1 semicolon

include/trace/events/iocost.h:163: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon

net/sched/cls_rsvp.h:231: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon
net/sched/cls_u32.c:782: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon
drivers/net/ethernet/mediatek/mtk_star_emac.c:1089:
WARNING:ONE_SEMICOLON: Statements terminations use 1 semicolon

arch/ia64/include/asm/mca_asm.h:139: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon
arch/ia64/include/asm/mca_asm.h:213: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon
arch/ia64/include/asm/native/inst.h:79: WARNING:ONE_SEMICOLON:
Statements terminations use 1 semicolon
arch/ia64/kernel/minstate.h:14: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon
arch/ia64/kernel/minstate.h:151: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon
arch/ia64/kernel/minstate.h:213: WARNING:ONE_SEMICOLON: Statements
terminations use 1 semicolon

Your task is:
  - to identify if these violations are true positives and can be
fixed as you suggested or not.
  - if they can be fixed as you suggested, provide a patch fixing this
issue to the corresponding maintainer.

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.

I suggest you start with a patch to drivers/mtd/ubi/block.c, and then
we see how to continue with the next patch.

Lukas

total: 0 errors, 3 warnings, 697 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

drivers/mtd/ubi/block.c has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

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

RESULT (2):

./scripts/checkpatch.pl drivers/net/ethernet/amazon/ena/ena_netdev.c

CHECK: multiple assignments should be avoided
#282: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:282:
+ ena_tx_ctx->num_bufs = tx_info->num_of_bufs = 1;

message not in documentation.
this warning was triggered because it devtected the assigning of values to multiple variables in same statement using "=".
this can be solved by changing it to this instead,

ena_tx_ctx->num_bufs = 1;
tx_info->num_of_bufs = 1;
WARNING: nested (un)?likely() calls, IS_ERR already uses unlikely() internally
#1025: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1025:
+ if (unlikely(IS_ERR(page)))

message not in documentation.
the error could have been caused because the "IS_ERR" function already
calls the "unlikely" function internally, and we pass the "IS_ERR" function through
"unlikely" function again, resulting in nested calls of the "unlikely" function.
this warning can be removed if the nested calls can be avoided.
CHECK: Alignment should match open parenthesis
#1533: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1533:
+static void ena_rx_checksum(struct ena_ring *rx_ring,
+   struct ena_com_rx_ctx *ena_rx_ctx,

message not in documentation.
this was caused because the 2nd line "struct ena_com_rx_ctx *ena_rx_ctx,"
was not aligned with the opening parenthesis of the first line "(".
aligning it should remove the message.
CHECK: Unnecessary parentheses around 'ena_rx_ctx->l3_proto == ENA_ETH_IO_L3_PROTO_IPV4'
#1549: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1549:
+ if (unlikely((ena_rx_ctx->l3_proto == ENA_ETH_IO_L3_PROTO_IPV4) &&
+     (ena_rx_ctx->l3_csum_err))) {

CHECK: Unnecessary parentheses around 'ena_rx_ctx->l3_csum_err'
#1549: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1549:
+ if (unlikely((ena_rx_ctx->l3_proto == ENA_ETH_IO_L3_PROTO_IPV4) &&
+     (ena_rx_ctx->l3_csum_err))) {

message explained in documentation.
caused by unnecessary parentheses.
message can be removed by changing to,

if (unlikely(ena_rx_ctx->l3_proto == ENA_ETH_IO_L3_PROTO_IPV4 &&
    ena_rx_ctx->l3_csum_err)) {
CHECK: Unnecessary parentheses around 'ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_TCP'
#1561: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1561:
+ if (likely((ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_TCP) ||
+   (ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_UDP))) {

CHECK: Unnecessary parentheses around 'ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_UDP'
#1561: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1561:
+ if (likely((ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_TCP) ||
+   (ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_UDP))) {

unnecessary parentheses, as explained in the previous example.
CHECK: Blank lines aren't necessary before a close brace '}'
#1587: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1587:
+
+}

proper usage of braces is explained in the documentation, which is related to this message.
as stated, it is caused due to the blank line before the closing brace.
removing the blank line should close the message.
CHECK: Unnecessary parentheses around 'ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_TCP'
#1596: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1596:
+ if (likely((ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_TCP) ||
+   (ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_UDP)))

unnecessary parentheses, as explained in the previous example.
CHECK: Unnecessary parentheses around 'ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_UDP'
#1596: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1596:
+ if (likely((ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_TCP) ||
+   (ena_rx_ctx->l4_proto == ENA_ETH_IO_L4_PROTO_UDP)))

unnecessary parentheses, as explained in the previous example.
CHECK: Please use a blank line after function/struct/union/enum declarations
#1636: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1636:
+}
+/* ena_clean_rx_irq - Cleanup RX irq

message not present in docs for checkpatch.
we should use a blank line after declaring a function/struct/enum.
adding a blank line after the closing brace should remove the message.
CHECK: Alignment should match open parenthesis
#1820: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1820:
+static void ena_unmask_interrupt(struct ena_ring *tx_ring,
+ struct ena_ring *rx_ring)

the 2nd line not aligned properly with the opening parenthesis of the first.
explained in a previous example.
CHECK: Alignment should match open parenthesis
#1852: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:1852:
+static void ena_update_ring_numa_node(struct ena_ring *tx_ring,
+     struct ena_ring *rx_ring)

the 2nd line not aligned properly with the opening parenthesis of the first.
explained in a previous example.
WARNING: Unnecessary ftrace-like logging - prefer using ftrace
#2621: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:2621:
+ netif_dbg(adapter, ifup, adapter->netdev, "%s\n", __func__);

WARNING: Unnecessary ftrace-like logging - prefer using ftrace
#2683: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:2683:
+ netif_info(adapter, ifdown, adapter->netdev, "%s\n", __func__);

WARNING: Unnecessary ftrace-like logging - prefer using ftrace
#2771: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:2771:
+ netif_dbg(adapter, ifdown, netdev, "%s\n", __func__);

no explanation for this message was available in the documentation for checkpatch.
To my understanding, ftrace is a function that is used in development of
linux kernel to understand and analyse any function that takes place out of user-space.
so here, i believe that netif_dbg, netif_info, and netif_dbg are functions that
carry out a task that can also be done by ftrace, which could be even more efficient.
so usage of ftrace is preferred so that we get better information of that function which is analysed.
CHECK: Unnecessary parentheses around 'skb->ip_summed == CHECKSUM_PARTIAL'
#2855: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:2855:
+ if ((skb->ip_summed == CHECKSUM_PARTIAL) || mss) {

unnecessary parentheses, as explained in the previous example.
CHECK: Unnecessary parentheses around 'num_frags == tx_ring->sgl_size'
#2912: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:2912:
+ if ((num_frags == tx_ring->sgl_size) &&
+    (header_len < tx_ring->tx_max_header_size))

unnecessary parentheses, as explained in the previous example.
CHECK: Unnecessary parentheses around 'header_len < tx_ring->tx_max_header_size'
#2912: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:2912:
+ if ((num_frags == tx_ring->sgl_size) &&
+    (header_len < tx_ring->tx_max_header_size))

unnecessary parentheses, as explained in the previous example.
WARNING: LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged
#3168: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3168:
+ host_info->kernel_ver = LINUX_VERSION_CODE;

an exact number of the kernel version to which it will be merged
should be assigned to host_info->kernel_ver rather than another variable.
WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/ (local)
#3169: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3169:
+ strlcpy(host_info->kernel_ver_str, utsname()->version,

strlcpy() function limits the destination size, and is depreciated.
using strscpy() should remove the warning.
CHECK: line length of 101 exceeds 100 columns
#3365: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3365:
+   struct ena_llq_configurations *llq_default_configurations)

line length exceeds 100 columns. message will be removed if it is split to multiple lines.
explained in the docs.
CHECK: Alignment should match open parenthesis
#3373: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3373:
+ dev_warn(&pdev->dev,
+ "LLQ is not supported Fallback to host mode policy.\n");

the 2nd line not aligned properly with the opening parenthesis of the first.
explained in a previous example.
WARNING: memory barrier without comment
#3698: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3698:
+ smp_mb__before_atomic();

WARNING: memory barrier without comment
#3737: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3737:
+ smp_mb__before_atomic();

no documentation is given for this message in checkpatch.
to my understanding, this is caused because there is no
proper comment to this function call giving the context on why it is called in that line.
adding a comment that gives proper context should remove the message.
CHECK: line length of 119 exceeds 100 columns
#3747: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3747:
+ time_since_last_napi = jiffies_to_usecs(jiffies - tx_ring->tx_stats.last_napi_jiffies);

CHECK: line length of 105 exceeds 100 columns
#3748: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3748:
+ missing_tx_comp_to = jiffies_to_msecs(adapter->missing_tx_completion_to);

CHECK: line length of 104 exceeds 100 columns
#3751: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:3751:
+     tx_ring->qid, i, time_since_last_napi, missing_tx_comp_to);

line length exceeds 100 columns. message will be removed if it is split to multiple lines.
CHECK: Please don't use multiple blank lines
#4139: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:4139:
+
+

multiple blank lines caused the message. removing them should do it.
WARNING: Unnecessary ftrace-like logging - prefer using ftrace
#4228: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:4228:
+ dev_dbg(&pdev->dev, "%s\n", __func__);

using ftrace is encouraged instead of the above fn. explained in an example before.
CHECK: Unnecessary parentheses around 'adapter->msix_vecs >= 1'
#4464: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:4464:
+ if ((adapter->msix_vecs >= 1) && (netdev->rx_cpu_rmap)) {

CHECK: Unnecessary parentheses around 'netdev->rx_cpu_rmap'
#4464: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:4464:
+ if ((adapter->msix_vecs >= 1) && (netdev->rx_cpu_rmap)) {

discussed in a previous example.
WARNING: Unnecessary ftrace-like logging - prefer using ftrace
#4612: FILE: drivers/net/ethernet/amazon/ena/ena_netdev.c:4612:
+ netif_dbg(adapter, ifup, adapter->netdev, "%s\n", __func__);

discussed in a previous example.
total: 0 errors, 10 warnings, 23 checks, 4689 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

drivers/net/ethernet/amazon/ena/ena_netdev.c has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS

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

I hope I have answered all the questions asked as per the first task. If there is any clarification or further explanation required,
please do let me know so that i can expand further on this answer. Thank you for this opportunity. I eagerly await my second task.

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help