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

2015-12-10 Thread Ion Grigore 
Hi,

I sent a new patch(V6) making the code more understandable.

Thanks,
Grig


-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
Sent: Wednesday, December 09, 2015 2:52 PM
To: Grigore Ion-B17953 ; lng-odp@lists.linaro.org
Subject: Re: [PATCHv5] helper : Fix UDP checksum computation



On 09.12.2015 15:26, Ion Grigore wrote:
> See inline comments.
> 
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> Sent: Wednesday, December 09, 2015 12:04 PM
> To: Grigore Ion-B17953 ; 
> lng-odp@lists.linaro.org
> Subject: Re: [PATCHv5] helper : Fix UDP checksum computation
> 
> Some comments below.
> 
> On 09.12.2015 11:59, ion.grig...@freescale.com wrote:
>> From: Grigore Ion 
>>
>> This patch fixes the following problems:
>> - checksum computation for LE platforms
>> - checksum is computed in the CPU endianness. The returned result 
>> must be converted to the BE ordering when it is used to update the 
>> UDP checksum in a packet.
>> - checksum computation for packets having the UDP length not a 
>> multiple of 2
>> - fixes the UDP checksum associated validation test
>>
>> Signed-off-by: Grigore Ion 
>> ---
>> 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)
>> - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
>>
>>  helper/include/odp/helper/udp.h | 55 
>> +
>>  helper/test/odp_chksum.c|  4 +--
>>  2 files changed, 25 insertions(+), 34 deletions(-)
>>
>> diff --git a/helper/include/odp/helper/udp.h 
>> b/helper/include/odp/helper/udp.h index 06c439b..9e7256d 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,39 @@ 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 CPU 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))
>> -return 0;
>> +odph_ipv4hdr_t  *iph;
>> +odph_udphdr_t   *udph;
>> +uint32_tsum;
>> +uint16_tudplen, *buf;
>>  
>> -if (!odp_packet_l4_offset(pkt))
>> +if (odp_packet_l4_offset(pkt) == ODP_PACKET_OFFSET_INVALID)
>>  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;
>> -}
>> -
>> -/* Fold sum to 16 bits: add carrier to result */
>> -while (sum >> 16)
>> -sum = (sum & 0x) + (sum >> 16);
>> -
>> +(iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
>> +odp_be_to_cpu_16(iph->proto) + udph->length;
> 
> This line is also strange. You're using odp_be_to_cpu_16 for 8-bit digit.
> At this point, iph->proto firstly will be extended to u16 in CPU endianness 
> and after that odp_be_to_cpu_16() will return it to BE. You will have right 
> position of all bytes (8 zero bits + 8 bits of iph->proto), but the way how 
> this done is wrong.
> 
> [IGR] Here the proto field is added as the LSB in a 16-bit BE value. Do you 
> think the following code clearer ?
> 
>   uint16 val;
>   ...
>   val = iph->proto;
>   sum += odp_be_to_cpu_16(val);

No, because 'val' is in CPU endianness and you're using odp_be_to_cpu_16() for 
variable that is already in CPU endianness.

> 
>> +udplen = odp_be_to_cpu_16(udph->length);
>> +buf =

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

2015-12-09 Thread Ilya Maximets 
Some comments below.

On 09.12.2015 11:59, ion.grig...@freescale.com wrote:
> From: Grigore Ion 
> 
> This patch fixes the following problems:
> - checksum computation for LE platforms
> - checksum is computed in the CPU endianness. The returned result
> must be converted to the BE ordering when it is used to update
> the UDP checksum in a packet.
> - checksum computation for packets having the UDP length not a
> multiple of 2
> - fixes the UDP checksum associated validation test
> 
> Signed-off-by: Grigore Ion 
> ---
> 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)
> - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
> 
>  helper/include/odp/helper/udp.h | 55 
> +
>  helper/test/odp_chksum.c|  4 +--
>  2 files changed, 25 insertions(+), 34 deletions(-)
> 
> diff --git a/helper/include/odp/helper/udp.h b/helper/include/odp/helper/udp.h
> index 06c439b..9e7256d 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,39 @@ 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 CPU 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))
> - return 0;
> + odph_ipv4hdr_t  *iph;
> + odph_udphdr_t   *udph;
> + uint32_tsum;
> + uint16_tudplen, *buf;
>  
> - if (!odp_packet_l4_offset(pkt))
> + if (odp_packet_l4_offset(pkt) == ODP_PACKET_OFFSET_INVALID)
>   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;
> - }
> -
> - /* Fold sum to 16 bits: add carrier to result */
> - while (sum >> 16)
> - sum = (sum & 0x) + (sum >> 16);
> -
> + (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
> + odp_be_to_cpu_16(iph->proto) + udph->length;

This line is also strange. You're using odp_be_to_cpu_16 for 8-bit digit.
At this point, iph->proto firstly will be extended to u16 in CPU endianness
and after that odp_be_to_cpu_16() will return it to BE. You will have
right position of all bytes (8 zero bits + 8 bits of iph->proto), but
the way how this done is wrong.

> + udplen = odp_be_to_cpu_16(udph->length);
> + buf = (uint16_t *)((void *)udph);
> + /* 32-bit sum of UDP header (checksum field cleared) and UDP data */
> + for ( ; udplen > 1; udplen -= 2)
> + sum += *buf++;
> + /* Length is not a multiple of 2 bytes */
> + if (udplen)
> + sum += odp_be_to_cpu_16(*((uint8_t *)buf) << 8);

Same at this point. You're using odp_be_to_cpu_16() to perform opposite 
operation.

May be, better to use byte arrays here to avoid that strange endianness and 
compiler
aware operations.

> + /* 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;
> + /* Set computation result in CPU endianness */
> + return (sum == 0x0) ? 0x : odp_be_to_cpu_16(sum);
>  }
>  
>  /** @internal Compile time assert */
> diff --git a/helper/test/odp_chksum.c b/helper/test/odp_chksum.c
> index 1d417a8..152018a 100644
> --- a/helper/test/odp_chksum.c
> +++

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

2015-12-09 Thread Ilya Maximets 


On 09.12.2015 15:26, Ion Grigore wrote:
> See inline comments.
> 
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
> Sent: Wednesday, December 09, 2015 12:04 PM
> To: Grigore Ion-B17953 ; lng-odp@lists.linaro.org
> Subject: Re: [PATCHv5] helper : Fix UDP checksum computation
> 
> Some comments below.
> 
> On 09.12.2015 11:59, ion.grig...@freescale.com wrote:
>> From: Grigore Ion 
>>
>> This patch fixes the following problems:
>> - checksum computation for LE platforms
>> - checksum is computed in the CPU endianness. The returned result must 
>> be converted to the BE ordering when it is used to update the UDP 
>> checksum in a packet.
>> - checksum computation for packets having the UDP length not a 
>> multiple of 2
>> - fixes the UDP checksum associated validation test
>>
>> Signed-off-by: Grigore Ion 
>> ---
>> 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)
>> - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
>>
>>  helper/include/odp/helper/udp.h | 55 
>> +
>>  helper/test/odp_chksum.c|  4 +--
>>  2 files changed, 25 insertions(+), 34 deletions(-)
>>
>> diff --git a/helper/include/odp/helper/udp.h 
>> b/helper/include/odp/helper/udp.h index 06c439b..9e7256d 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,39 @@ 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 CPU 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))
>> -return 0;
>> +odph_ipv4hdr_t  *iph;
>> +odph_udphdr_t   *udph;
>> +uint32_tsum;
>> +uint16_tudplen, *buf;
>>  
>> -if (!odp_packet_l4_offset(pkt))
>> +if (odp_packet_l4_offset(pkt) == ODP_PACKET_OFFSET_INVALID)
>>  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;
>> -}
>> -
>> -/* Fold sum to 16 bits: add carrier to result */
>> -while (sum >> 16)
>> -sum = (sum & 0x) + (sum >> 16);
>> -
>> +(iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
>> +odp_be_to_cpu_16(iph->proto) + udph->length;
> 
> This line is also strange. You're using odp_be_to_cpu_16 for 8-bit digit.
> At this point, iph->proto firstly will be extended to u16 in CPU endianness 
> and after that odp_be_to_cpu_16() will return it to BE. You will have right 
> position of all bytes (8 zero bits + 8 bits of iph->proto), but the way how 
> this done is wrong.
> 
> [IGR] Here the proto field is added as the LSB in a 16-bit BE value. Do you 
> think the following code clearer ?
> 
>   uint16 val;
>   ...
>   val = iph->proto;
>   sum += odp_be_to_cpu_16(val);

No, because 'val' is in CPU endianness and you're using odp_be_to_cpu_16()
for variable that is already in CPU endianness.

> 
>> +udplen = odp_be_to_cpu_16(udph->length);
>> +buf = (uint16_t *)((void *)udph);
>> +/* 32-bit sum of UDP header (checksum field cleared) and UDP data */
>> +for ( ; udplen > 1; udplen -= 2)
>> +sum += *buf++;
>> +/* Length is not a multiple of 2 bytes */
>> +if (udplen)
>> +sum += odp_be_to_cpu_16(*((uint8_t *)buf) << 8);
> 
> Same at this point. You're

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

2015-12-09 Thread Ion Grigore 
See inline comments.

-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
Sent: Wednesday, December 09, 2015 12:04 PM
To: Grigore Ion-B17953 ; lng-odp@lists.linaro.org
Subject: Re: [PATCHv5] helper : Fix UDP checksum computation

Some comments below.

On 09.12.2015 11:59, ion.grig...@freescale.com wrote:
> From: Grigore Ion 
> 
> This patch fixes the following problems:
> - checksum computation for LE platforms
> - checksum is computed in the CPU endianness. The returned result must 
> be converted to the BE ordering when it is used to update the UDP 
> checksum in a packet.
> - checksum computation for packets having the UDP length not a 
> multiple of 2
> - fixes the UDP checksum associated validation test
> 
> Signed-off-by: Grigore Ion 
> ---
> 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)
> - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
> 
>  helper/include/odp/helper/udp.h | 55 
> +
>  helper/test/odp_chksum.c|  4 +--
>  2 files changed, 25 insertions(+), 34 deletions(-)
> 
> diff --git a/helper/include/odp/helper/udp.h 
> b/helper/include/odp/helper/udp.h index 06c439b..9e7256d 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,39 @@ 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 CPU 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))
> - return 0;
> + odph_ipv4hdr_t  *iph;
> + odph_udphdr_t   *udph;
> + uint32_tsum;
> + uint16_tudplen, *buf;
>  
> - if (!odp_packet_l4_offset(pkt))
> + if (odp_packet_l4_offset(pkt) == ODP_PACKET_OFFSET_INVALID)
>   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;
> - }
> -
> - /* Fold sum to 16 bits: add carrier to result */
> - while (sum >> 16)
> - sum = (sum & 0x) + (sum >> 16);
> -
> + (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
> + odp_be_to_cpu_16(iph->proto) + udph->length;

This line is also strange. You're using odp_be_to_cpu_16 for 8-bit digit.
At this point, iph->proto firstly will be extended to u16 in CPU endianness and 
after that odp_be_to_cpu_16() will return it to BE. You will have right 
position of all bytes (8 zero bits + 8 bits of iph->proto), but the way how 
this done is wrong.

[IGR] Here the proto field is added as the LSB in a 16-bit BE value. Do you 
think the following code clearer ?

uint16 val;
...
val = iph->proto;
sum += odp_be_to_cpu_16(val);

> + udplen = odp_be_to_cpu_16(udph->length);
> + buf = (uint16_t *)((void *)udph);
> + /* 32-bit sum of UDP header (checksum field cleared) and UDP data */
> + for ( ; udplen > 1; udplen -= 2)
> + sum += *buf++;
> + /* Length is not a multiple of 2 bytes */
> + if (udplen)
> + sum += odp_be_to_cpu_16(*((uint8_t *)buf) << 8);

Same at this point. You're using odp_be_to_cpu_16() to perform opposite 
operation.

May be, better to use byte arrays here to avoid that strange endianness and 
compiler aware operations.

[IGR] Here the last byte is added as the MSB in a 16-bit BE value. Do you think 
the following code clearer ?