Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 5 Apr 2020 03:36:18 -0700
From:   Matthew Wilcox <willy@infradead.org>
To:     Dexuan Cui <decui@microsoft.com>
Cc:     netdev@vger.kernel.org, davem@davemloft.net, willemb@google.com,
        kuba@kernel.org, simon.horman@netronome.com, sdf@google.com,
        john.hurley@netronome.com, edumazet@google.com, fw@strlen.de,
        jonathan.lemon@gmail.com, pablo@netfilter.org,
        rdunlap@infradead.org, jeremy@azazel.net, pabeni@redhat.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] skbuff.h: Improve the checksum related comments
Message-ID: <20200405103618.GV21484@bombadil.infradead.org>
References: <1586071063-51656-1-git-send-email-decui@microsoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1586071063-51656-1-git-send-email-decui@microsoft.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Apr 05, 2020 at 12:17:43AM -0700, Dexuan Cui wrote:
>   * CHECKSUM_COMPLETE:
>   *
> - *   This is the most generic way. The device supplied checksum of the _whole_
> - *   packet as seen by netif_rx() and fills out in skb->csum. Meaning, the
> + *   This is the most generic way. The device supplies checksum of the _whole_
> + *   packet as seen by netif_rx() and fills out in skb->csum. This means the

I think both 'supplies' and 'supplied' are correct in this sentence.  The
nuances are slightly different, but the meaning is the same in this instance.

You missed a mistake in the second line though, it should be either 'fills
out' or 'fills in'.  I think we tend to prefer 'fills in'.

>   * CHECKSUM_COMPLETE:
>   *   Not used in checksum output. If a driver observes a packet with this value
> - *   set in skbuff, if should treat as CHECKSUM_NONE being set.
> + *   set in skbuff, the driver should treat it as CHECKSUM_NONE being set.

I would go with "it should treat the packet as if CHECKSUM_NONE were set."

> @@ -211,7 +211,7 @@
>   * is implied by the SKB_GSO_* flags in gso_type. Most obviously, if the
>   * gso_type is SKB_GSO_TCPV4 or SKB_GSO_TCPV6, TCP checksum offload as
>   * part of the GSO operation is implied. If a checksum is being offloaded
> - * with GSO then ip_summed is CHECKSUM_PARTIAL, csum_start and csum_offset
> + * with GSO then ip_summed is CHECKSUM_PARTIAL AND csum_start and csum_offset
>   * are set to refer to the outermost checksum being offload (two offloaded
>   * checksums are possible with UDP encapsulation).

Why the capitalisation of 'AND'?

Thanks for the improvements,

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>