Fixed the punctuation and some typos.
Improved a few sentences with minor changes.
No change to the semantics or the code.
Signed-off-by: Dexuan Cui <[email protected]>
---
English is not my mother tongue, so I may not be making the best changes
here. I'm happy to post a v2 if necessary. Looking forward to your comments!
include/linux/skbuff.h | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 28b1a2b..746049c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -47,8 +47,8 @@
* A. IP checksum related features
*
* Drivers advertise checksum offload capabilities in the features of a device.
- * From the stack's point of view these are capabilities offered by the driver,
- * a driver typically only advertises features that it is capable of offloading
+ * From the stack's point of view these are capabilities offered by the driver.
+ * A driver typically only advertises features that it is capable of offloading
* to its device.
*
* The checksum related features are:
@@ -63,7 +63,7 @@
* TCP or UDP packets over IPv4. These are specifically
* unencapsulated packets of the form IPv4|TCP or
* IPv4|UDP where the Protocol field in the IPv4 header
- * is TCP or UDP. The IPv4 header may contain IP options
+ * is TCP or UDP. The IPv4 header may contain IP options.
* This feature cannot be set in features for a device
* with NETIF_F_HW_CSUM also set. This feature is being
* DEPRECATED (see below).
@@ -79,13 +79,13 @@
* DEPRECATED (see below).
*
* NETIF_F_RXCSUM - Driver (device) performs receive checksum offload.
- * This flag is used only used to disable the RX checksum
+ * This flag is only used to disable the RX checksum
* feature for a device. The stack will accept receive
* checksum indication in packets received on a device
* regardless of whether NETIF_F_RXCSUM is set.
*
* B. Checksumming of received packets by device. Indication of checksum
- * verification is in set skb->ip_summed. Possible values are:
+ * verification is set in skb->ip_summed. Possible values are:
*
* CHECKSUM_NONE:
*
@@ -115,16 +115,16 @@
* the packet minus one that have been verified as CHECKSUM_UNNECESSARY.
* For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet
* and a device is able to verify the checksums for UDP (possibly zero),
- * GRE (checksum flag is set), and TCP-- skb->csum_level would be set to
+ * GRE (checksum flag is set) and TCP, skb->csum_level would be set to
* two. If the device were only able to verify the UDP checksum and not
- * GRE, either because it doesn't support GRE checksum of because GRE
+ * GRE, either because it doesn't support GRE checksum or because GRE
* checksum is bad, skb->csum_level would be set to zero (TCP checksum is
* not considered in this case).
*
* 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
* hardware doesn't need to parse L3/L4 headers to implement this.
*
* Notes:
@@ -153,8 +153,8 @@
* from skb->csum_start up to the end, and to record/write the checksum at
* offset skb->csum_start + skb->csum_offset. A driver may verify that the
* csum_start and csum_offset values are valid values given the length and
- * offset of the packet, however they should not attempt to validate that the
- * checksum refers to a legitimate transport layer checksum-- it is the
+ * offset of the packet, but it should not attempt to validate that the
+ * checksum refers to a legitimate transport layer checksum -- it is the
* purview of the stack to validate that csum_start and csum_offset are set
* correctly.
*
@@ -178,18 +178,18 @@
*
* CHECKSUM_UNNECESSARY:
*
- * This has the same meaning on as CHECKSUM_NONE for checksum offload on
+ * This has the same meaning as CHECKSUM_NONE for checksum offload on
* output.
*
* 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.
*
* D. Non-IP checksum (CRC) offloads
*
* NETIF_F_SCTP_CRC - This feature indicates that a device is capable of
* offloading the SCTP CRC in a packet. To perform this offload the stack
- * will set set csum_start and csum_offset accordingly, set ip_summed to
+ * will set csum_start and csum_offset accordingly, set ip_summed to
* CHECKSUM_PARTIAL and set csum_not_inet to 1, to provide an indication in
* the skbuff that the CHECKSUM_PARTIAL refers to CRC32c.
* A driver that supports both IP checksum offload and SCTP CRC32c offload
@@ -200,10 +200,10 @@
* NETIF_F_FCOE_CRC - This feature indicates that a device is capable of
* offloading the FCOE CRC in a packet. To perform this offload the stack
* will set ip_summed to CHECKSUM_PARTIAL and set csum_start and csum_offset
- * accordingly. Note the there is no indication in the skbuff that the
- * CHECKSUM_PARTIAL refers to an FCOE checksum, a driver that supports
+ * accordingly. Note that there is no indication in the skbuff that the
+ * CHECKSUM_PARTIAL refers to an FCOE checksum, so a driver that supports
* both IP checksum offload and FCOE CRC offload must verify which offload
- * is configured for a packet presumably by inspecting packet headers.
+ * is configured for a packet, presumably by inspecting packet headers.
*
* E. Checksumming on output with GSO.
*
@@ -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).
*/
--
1.8.3.1
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) <[email protected]>
> From: Matthew Wilcox <[email protected]>
> Sent: Sunday, April 5, 2020 3:36 AM
> To: Dexuan Cui <[email protected]>
>
> 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.
I see. So let me rever back to "supplied".
> 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'.
Thanks! Will use "fills in" in v2.
> > * 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."
Thanks. Will use this version.
> > @@ -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'?
The current text without the patch is:
* 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
* are set to refer to the outermost checksum being offload (two offloaded
* checksums are possible with UDP encapsulation).
The comma after the "CHECKSUM_PARTIAL" seems suspicious to me. I feel we
should add an "and" after the comma, or replace the comma with "and", but
either way we'll have "... and csum_start and csum_offset...", which seems a little
unnatural to me since we have 2 'and's here... So I tried to make it a little natural
by replacing the first 'and' with 'AND', which obviously causes confusion to you.
Please suggest the best change here. Thanks!
> Thanks for the improvements,
>
> Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
Thanks for the comments! I'll wait for your suggestion on the 'AND' and post
a v2.
Thanks,
-- Dexuan
On 4/5/20 9:33 AM, Dexuan Cui wrote:
>> From: Matthew Wilcox <[email protected]>
>> Sent: Sunday, April 5, 2020 3:36 AM
>> To: Dexuan Cui <[email protected]>
>>
>> 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.
>
> I see. So let me rever back to "supplied".
>
>> 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'.
>
> Thanks! Will use "fills in" in v2.
>
>>> * 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."
>
> Thanks. Will use this version.
>
>>> @@ -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'?
>
> The current text without the patch is:
> * 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
> * are set to refer to the outermost checksum being offload (two offloaded
> * checksums are possible with UDP encapsulation).
>
> The comma after the "CHECKSUM_PARTIAL" seems suspicious to me. I feel we
> should add an "and" after the comma, or replace the comma with "and", but
> either way we'll have "... and csum_start and csum_offset...", which seems a little
> unnatural to me since we have 2 'and's here... So I tried to make it a little natural
> by replacing the first 'and' with 'AND', which obviously causes confusion to you.
maybe "both csum_start and csum_offset are set to refer to".
> Please suggest the best change here. Thanks!
>
>> Thanks for the improvements,
>>
>> Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
>
> Thanks for the comments! I'll wait for your suggestion on the 'AND' and post
> a v2.
--
~Randy
> From: Randy Dunlap <[email protected]>
> Sent: Sunday, April 5, 2020 9:41 AM
> To: Dexuan Cui <[email protected]>; Matthew Wilcox
> >> Why the capitalisation of 'AND'?
> > ...
> > The comma after the "CHECKSUM_PARTIAL" seems suspicious to me. I feel
> > we should add an "and" after the comma, or replace the comma with "and",
> > but either way we'll have "... and csum_start and csum_offset...", which
> > seems a little unnatural to me since we have 2 'and's here... So I tried to
> > make it a little natural by replacing the first 'and' with 'AND', which
> > obviously causes confusion to you.
>
> maybe "both csum_start and csum_offset are set to refer to".
> ~Randy
Looks good. I'll post a v2 shortly. Thank you both!
Thanks,
-- Dexuan