Re: [lng-odp] [PATCHv8] helper : Fix UDP checksum computation

[Ilya Maximets]

On 04.01.2016 15:48, ion.grig...@freescale.com wrote:
> From: Grigore Ion 
> 
> This patch fixes the following problems:
> - checksum computation for LE platforms
> - checksum computation for packets having the UDP length not a
> multiple of 2
> - checksum computation in the test and the example applications
> 
> Signed-off-by: Grigore Ion 
> ---
> v8:
> - Checksum in BE endianness (Ilya Maximets)
> v7:
> - Make code simpler and more understandable as it was
> sugessted by Ilya Maximets
> v6:
> - Make code more understandable. Not done as it was
> sugessted by Ilya Maximets.
> v5:
> - Checksum in CPU endianness fix added (Ilya Maximets)
> v4:
> - Verify checksum in CPU endianness in the associated test
> (Ilya Maximets)
> v3:
> - fix the UDP checksum computation in the associated test
> (Maxim Uvarov)
> v2:
> - patch updated to the last master (Maxim Uvarov)
> v1:
> - Move variables declaration on top of block. (Maxim Uvarov)
> - Check patch with checkpatch script.  (Maxim Uvarov)
> - L3 header presence is tested twice. (Alexandru Badicioiu)
> - Remove unnecessary check for L3 header presence. (Bill Fischofer)
> 
>  example/generator/odp_generator.c |  2 +-
>  helper/include/odp/helper/udp.h   | 65 
> ---
>  helper/test/odp_chksum.c  |  4 +--
>  test/validation/pktio/pktio.c |  2 +-
>  4 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/example/generator/odp_generator.c 
> b/example/generator/odp_generator.c
> index cdbc761..757dc54 100644
> --- a/example/generator/odp_generator.c
> +++ b/example/generator/odp_generator.c
> @@ -242,7 +242,7 @@ static odp_packet_t pack_udp_pkt(odp_pool_t pool)
>   udp->dst_port = 0;
>   udp->length = odp_cpu_to_be_16(args->appl.payload + ODPH_UDPHDR_LEN);
>   udp->chksum = 0;
> - udp->chksum = odp_cpu_to_be_16(odph_ipv4_udp_chksum(pkt));
> + udp->chksum = odph_ipv4_udp_chksum(pkt);
>  
>   return pkt;
>  }
> diff --git a/helper/include/odp/helper/udp.h b/helper/include/odp/helper/udp.h
> index 06c439b..afdb4fc 100644
> --- a/helper/include/odp/helper/udp.h
> +++ b/helper/include/odp/helper/udp.h
> @@ -4,7 +4,6 @@
>   * SPDX-License-Identifier: BSD-3-Clause
>   */
>  
> -
>  /**
>   * @file
>   *
> @@ -22,7 +21,6 @@ extern "C" {
>  #include 
>  #include 
>  
> -
>  /** @addtogroup odph_header ODPH HEADER
>   *  @{
>   */
> @@ -44,46 +42,49 @@ typedef struct ODP_PACKED {
>   * This function uses odp packet to calc checksum
>   *
>   * @param pkt  calculate chksum for pkt
> - * @return  checksum value
> + * @return  checksum value in BE endianness
>   */
>  static inline uint16_t odph_ipv4_udp_chksum(odp_packet_t pkt)
>  {
> - uint32_t sum = 0;
> - odph_udphdr_t *udph;
> - odph_ipv4hdr_t *iph;
> - uint16_t udplen;
> - uint8_t *buf;
> -
> - if (!odp_packet_l3_offset(pkt))
> + odph_ipv4hdr_t  *iph;
> + odph_udphdr_t   *udph;
> + uint32_tsum;
> + uint16_tudplen, *buf;
> + union {
> + uint8_t v8[2];
> + uint16_t v16;
> + } val;
> +
> + if (odp_packet_l4_offset(pkt) == ODP_PACKET_OFFSET_INVALID)
>   return 0;
> -
> - if (!odp_packet_l4_offset(pkt))
> - return 0;
> -
>   iph = (odph_ipv4hdr_t *)odp_packet_l3_ptr(pkt, NULL);
>   udph = (odph_udphdr_t *)odp_packet_l4_ptr(pkt, NULL);
> - udplen = odp_be_to_cpu_16(udph->length);
> -
> - /* 32-bit sum of all 16-bit words covered by UDP chksum */
> + /* 32-bit sum of UDP pseudo-header */

I think, it should be specified, that it's a sum of 16-bit words.

>   sum = (iph->src_addr & 0x) + (iph->src_addr >> 16) +
> -   (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
> -   (uint16_t)iph->proto + udplen;
> - for (buf = (uint8_t *)udph; udplen > 1; udplen -= 2) {
> - sum += ((*buf << 8) + *(buf + 1));
> - buf += 2;
> + (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
> + udph->length;
> + val.v8[0] = 0;
> + val.v8[1] = iph->proto;
> + sum += val.v16;
> + /* 32-bit sum of UDP header (checksum field cleared) and UDP data */

Same here.

> + udplen = odp_be_to_cpu_16(udph->length);
> + buf = (uint16_t *)((void *)udph);
> + for ( ; udplen > 1; udplen -= 2)
> + sum += *buf++;
> + /* Length is not a multiple of 2 bytes */
> + if (udplen) {
> + val.v8[0] = *buf;
> + val.v8[1] = 0;
> + sum += val.v16;
>   }
> -
> - /* Fold sum to 16 bits: add carrier to result */
> - while (sum >> 16)
> - sum = (sum & 0x) + (sum >> 16);
> -
> + /* Fold sum to 16 bits */
> + sum = (sum & 0x) + (sum >> 16);
> + /* Add carrier (0/1) to result */
> + sum += (sum >> 16);
>   /* 1's complement */
>   sum = ~sum;
> -
> - /* set computation result */
> - 

Re: [lng-odp] [PATCHv8] helper : Fix UDP checksum computation

[Ion Grigore]

Done

-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
Sent: Monday, January 11, 2016 10:14 AM
To: ion.grig...@freescale.com; lng-odp@lists.linaro.org
Cc: Maxim Uvarov 
Subject: Re: [PATCHv8] helper : Fix UDP checksum computation

On 04.01.2016 15:48, ion.grig...@freescale.com wrote:
> From: Grigore Ion 
> 
> This patch fixes the following problems:
> - checksum computation for LE platforms
> - checksum computation for packets having the UDP length not a 
> multiple of 2
> - checksum computation in the test and the example applications
> 
> Signed-off-by: Grigore Ion 
> ---
> v8:
> - Checksum in BE endianness (Ilya Maximets)
> v7:
> - Make code simpler and more understandable as it was sugessted by 
> Ilya Maximets
> v6:
> - Make code more understandable. Not done as it was sugessted by Ilya 
> Maximets.
> v5:
> - Checksum in CPU endianness fix added (Ilya Maximets)
> v4:
> - Verify checksum in CPU endianness in the associated test (Ilya 
> Maximets)
> v3:
> - fix the UDP checksum computation in the associated test (Maxim 
> Uvarov)
> v2:
> - patch updated to the last master (Maxim Uvarov)
> v1:
> - Move variables declaration on top of block. (Maxim Uvarov)
> - Check patch with checkpatch script.  (Maxim Uvarov)
> - L3 header presence is tested twice. (Alexandru Badicioiu)
> - Remove unnecessary check for L3 header presence. (Bill Fischofer)
> 
>  example/generator/odp_generator.c |  2 +-
>  helper/include/odp/helper/udp.h   | 65 
> ---
>  helper/test/odp_chksum.c  |  4 +--
>  test/validation/pktio/pktio.c |  2 +-
>  4 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/example/generator/odp_generator.c 
> b/example/generator/odp_generator.c
> index cdbc761..757dc54 100644
> --- a/example/generator/odp_generator.c
> +++ b/example/generator/odp_generator.c
> @@ -242,7 +242,7 @@ static odp_packet_t pack_udp_pkt(odp_pool_t pool)
>   udp->dst_port = 0;
>   udp->length = odp_cpu_to_be_16(args->appl.payload + ODPH_UDPHDR_LEN);
>   udp->chksum = 0;
> - udp->chksum = odp_cpu_to_be_16(odph_ipv4_udp_chksum(pkt));
> + udp->chksum = odph_ipv4_udp_chksum(pkt);
>  
>   return pkt;
>  }
> diff --git a/helper/include/odp/helper/udp.h 
> b/helper/include/odp/helper/udp.h index 06c439b..afdb4fc 100644
> --- a/helper/include/odp/helper/udp.h
> +++ b/helper/include/odp/helper/udp.h
> @@ -4,7 +4,6 @@
>   * SPDX-License-Identifier: BSD-3-Clause
>   */
>  
> -
>  /**
>   * @file
>   *
> @@ -22,7 +21,6 @@ extern "C" {
>  #include 
>  #include 
>  
> -
>  /** @addtogroup odph_header ODPH HEADER
>   *  @{
>   */
> @@ -44,46 +42,49 @@ typedef struct ODP_PACKED {
>   * This function uses odp packet to calc checksum
>   *
>   * @param pkt  calculate chksum for pkt
> - * @return  checksum value
> + * @return  checksum value in BE endianness
>   */
>  static inline uint16_t odph_ipv4_udp_chksum(odp_packet_t pkt)  {
> - uint32_t sum = 0;
> - odph_udphdr_t *udph;
> - odph_ipv4hdr_t *iph;
> - uint16_t udplen;
> - uint8_t *buf;
> -
> - if (!odp_packet_l3_offset(pkt))
> + odph_ipv4hdr_t  *iph;
> + odph_udphdr_t   *udph;
> + uint32_tsum;
> + uint16_tudplen, *buf;
> + union {
> + uint8_t v8[2];
> + uint16_t v16;
> + } val;
> +
> + if (odp_packet_l4_offset(pkt) == ODP_PACKET_OFFSET_INVALID)
>   return 0;
> -
> - if (!odp_packet_l4_offset(pkt))
> - return 0;
> -
>   iph = (odph_ipv4hdr_t *)odp_packet_l3_ptr(pkt, NULL);
>   udph = (odph_udphdr_t *)odp_packet_l4_ptr(pkt, NULL);
> - udplen = odp_be_to_cpu_16(udph->length);
> -
> - /* 32-bit sum of all 16-bit words covered by UDP chksum */
> + /* 32-bit sum of UDP pseudo-header */

I think, it should be specified, that it's a sum of 16-bit words.

>   sum = (iph->src_addr & 0x) + (iph->src_addr >> 16) +
> -   (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
> -   (uint16_t)iph->proto + udplen;
> - for (buf = (uint8_t *)udph; udplen > 1; udplen -= 2) {
> - sum += ((*buf << 8) + *(buf + 1));
> - buf += 2;
> + (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
> + udph->length;
> + val.v8[0] = 0;
> + val.v8[1] = iph->proto;
> + sum += val.v16;
> + /* 32-bit sum of UDP header (checksum field cleared) and UDP data */

Same here.

> + udplen = odp_be_to_cpu_16(udph->length);
> + buf = (uint16_t *)((void *)udph);
> + for ( ; udplen > 1; udplen -= 2)
> + sum += *buf++;
> + /* Length is not a multiple of 2 bytes */
> + if (udplen) {
> + val.v8[0] = *buf;
> + val.v8[1] = 0;
> + sum += val.v16;
>   }
> -
> - /* Fold sum to 16 bits: add carrier to result */
> - while (sum >> 16)
> -   

[lng-odp] [PATCHv8] helper : Fix UDP checksum computation

[ion.grigore]

From: Grigore Ion 

This patch fixes the following problems:
- checksum computation for LE platforms
- checksum computation for packets having the UDP length not a
multiple of 2
- checksum computation in the test and the example applications

Signed-off-by: Grigore Ion 
---
v8:
- Checksum in BE endianness (Ilya Maximets)
v7:
- Make code simpler and more understandable as it was
sugessted by Ilya Maximets
v6:
- Make code more understandable. Not done as it was
sugessted by Ilya Maximets.
v5:
- Checksum in CPU endianness fix added (Ilya Maximets)
v4:
- Verify checksum in CPU endianness in the associated test
(Ilya Maximets)
v3:
- fix the UDP checksum computation in the associated test
(Maxim Uvarov)
v2:
- patch updated to the last master (Maxim Uvarov)
v1:
- Move variables declaration on top of block. (Maxim Uvarov)
- Check patch with checkpatch script.  (Maxim Uvarov)
- L3 header presence is tested twice. (Alexandru Badicioiu)
- Remove unnecessary check for L3 header presence. (Bill Fischofer)

 example/generator/odp_generator.c |  2 +-
 helper/include/odp/helper/udp.h   | 65 ---
 helper/test/odp_chksum.c  |  4 +--
 test/validation/pktio/pktio.c |  2 +-
 4 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/example/generator/odp_generator.c 
b/example/generator/odp_generator.c
index cdbc761..757dc54 100644
--- a/example/generator/odp_generator.c
+++ b/example/generator/odp_generator.c
@@ -242,7 +242,7 @@ static odp_packet_t pack_udp_pkt(odp_pool_t pool)
udp->dst_port = 0;
udp->length = odp_cpu_to_be_16(args->appl.payload + ODPH_UDPHDR_LEN);
udp->chksum = 0;
-   udp->chksum = odp_cpu_to_be_16(odph_ipv4_udp_chksum(pkt));
+   udp->chksum = odph_ipv4_udp_chksum(pkt);
 
return pkt;
 }
diff --git a/helper/include/odp/helper/udp.h b/helper/include/odp/helper/udp.h
index 06c439b..afdb4fc 100644
--- a/helper/include/odp/helper/udp.h
+++ b/helper/include/odp/helper/udp.h
@@ -4,7 +4,6 @@
  * SPDX-License-Identifier: BSD-3-Clause
  */
 
-
 /**
  * @file
  *
@@ -22,7 +21,6 @@ extern "C" {
 #include 
 #include 
 
-
 /** @addtogroup odph_header ODPH HEADER
  *  @{
  */
@@ -44,46 +42,49 @@ typedef struct ODP_PACKED {
  * This function uses odp packet to calc checksum
  *
  * @param pkt  calculate chksum for pkt
- * @return  checksum value
+ * @return  checksum value in BE endianness
  */
 static inline uint16_t odph_ipv4_udp_chksum(odp_packet_t pkt)
 {
-   uint32_t sum = 0;
-   odph_udphdr_t *udph;
-   odph_ipv4hdr_t *iph;
-   uint16_t udplen;
-   uint8_t *buf;
-
-   if (!odp_packet_l3_offset(pkt))
+   odph_ipv4hdr_t  *iph;
+   odph_udphdr_t   *udph;
+   uint32_tsum;
+   uint16_tudplen, *buf;
+   union {
+   uint8_t v8[2];
+   uint16_t v16;
+   } val;
+
+   if (odp_packet_l4_offset(pkt) == ODP_PACKET_OFFSET_INVALID)
return 0;
-
-   if (!odp_packet_l4_offset(pkt))
-   return 0;
-
iph = (odph_ipv4hdr_t *)odp_packet_l3_ptr(pkt, NULL);
udph = (odph_udphdr_t *)odp_packet_l4_ptr(pkt, NULL);
-   udplen = odp_be_to_cpu_16(udph->length);
-
-   /* 32-bit sum of all 16-bit words covered by UDP chksum */
+   /* 32-bit sum of UDP pseudo-header */
sum = (iph->src_addr & 0x) + (iph->src_addr >> 16) +
- (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
- (uint16_t)iph->proto + udplen;
-   for (buf = (uint8_t *)udph; udplen > 1; udplen -= 2) {
-   sum += ((*buf << 8) + *(buf + 1));
-   buf += 2;
+   (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
+   udph->length;
+   val.v8[0] = 0;
+   val.v8[1] = iph->proto;
+   sum += val.v16;
+   /* 32-bit sum of UDP header (checksum field cleared) and UDP data */
+   udplen = odp_be_to_cpu_16(udph->length);
+   buf = (uint16_t *)((void *)udph);
+   for ( ; udplen > 1; udplen -= 2)
+   sum += *buf++;
+   /* Length is not a multiple of 2 bytes */
+   if (udplen) {
+   val.v8[0] = *buf;
+   val.v8[1] = 0;
+   sum += val.v16;
}
-
-   /* Fold sum to 16 bits: add carrier to result */
-   while (sum >> 16)
-   sum = (sum & 0x) + (sum >> 16);
-
+   /* Fold sum to 16 bits */
+   sum = (sum & 0x) + (sum >> 16);
+   /* Add carrier (0/1) to result */
+   sum += (sum >> 16);
/* 1's complement */
sum = ~sum;
-
-   /* set computation result */
-   sum = (sum == 0x0) ? 0x : sum;
-
-   return sum;
+   /* Return checksum in BE endianness */
+   return (sum == 0x0) ? 0x : sum;
 }
 
 /** @internal Compile time assert */
diff --git a/helper/test/odp_chksum.c b/helper/test/odp_chksum.c
index 1d417a8..b71fabc 100644
---