Submission of task 1 for checkpatch documentation mentorship program

Rahul Balaji rahulbalaji78 at gmail.com
Sat Jul 31 20:37:54 UTC 2021


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.

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/
#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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxfoundation.org/pipermail/linux-kernel-mentees/attachments/20210801/81608f04/attachment-0001.html>


More information about the Linux-kernel-mentees mailing list