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)
> -   

Re: [lng-odp] [PATCH 1/2] example: ipsec: fix endianness of IP address checks

[Ion Grigore]

Hi,

The conversion must be done when the ETTPE field is updated :

eth->type = odp_cpu_to_be_16(ODPH_ETHTYPE_IPV4);

Thanks,
Grig


From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Bill 
Fischofer
Sent: Thursday, January 07, 2016 2:09 AM
To: Stuart Haslam 
Cc: LNG ODP Mailman List 
Subject: Re: [lng-odp] [PATCH 1/2] example: ipsec: fix endianness of IP address 
checks



On Wed, Jan 6, 2016 at 5:33 AM, Stuart Haslam 
> wrote:
Convert IP addresses in packet header from network to CPU byte order
before comparing with policy cache.

Signed-off-by: Stuart Haslam 
>

Reviewed-by: Bill Fischofer 
>

---
 example/ipsec/odp_ipsec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c
index fab1035..45892f9 100644
--- a/example/ipsec/odp_ipsec.c
+++ b/example/ipsec/odp_ipsec.c
@@ -771,8 +771,8 @@ pkt_disposition_e do_ipsec_in_finish(odp_packet_t pkt,
ip = (odph_ipv4hdr_t *)odp_packet_l3_ptr(pkt, NULL);

/* Check inbound policy */
-   if ((ip->src_addr != ctx->ipsec.src_ip ||
-ip->dst_addr != ctx->ipsec.dst_ip))
+   if ((odp_be_to_cpu_32(ip->src_addr) != ctx->ipsec.src_ip ||
+odp_be_to_cpu_32(ip->dst_addr) != ctx->ipsec.dst_ip))
return PKT_DROP;

return PKT_CONTINUE;
--
2.1.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 2/2] example: ipsec: initialise packet length and tailroom correctly

[Ion Grigore]

Another possible bug fix is to allocate a packet of an well known length :
pkt = odp_packet_alloc(pkt_pool, ODPH_ETHHDR_LEN + 
ODPH_IPV4HDR_LEN +
   ODPH_ICMPHDR_LEN + 
sizeof(stream_pkt_hdr_t) +
   stream->length);
and then the length of the packet is updated as supplementary headers are added.

Of course one may compute the length of packet before allocation.

Thanks,
Grig

From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Bill 
Fischofer
Sent: Thursday, January 07, 2016 12:10 AM
To: Stuart Haslam 
Cc: LNG ODP Mailman List 
Subject: Re: [lng-odp] [PATCH 2/2] example: ipsec: initialise packet length and 
tailroom correctly



On Wed, Jan 6, 2016 at 5:33 AM, Stuart Haslam 
> wrote:
The example scripts run_esp_out, run_ah_out and run_both_out are failing
due to test packet meta data being initialised incorrectly.

Signed-off-by: Stuart Haslam 
>
---
This is a minimal change to fix the bug, but I'm not convinced the way
packets are created here is valid. It calls odp_packet_alloc() with a
length of 0, then starts writing directly to the odp_packet_data()
pointer. This works with linux-generic because allocating a packet of
length 0 gets a packet with the data length initialised to the length
specified in odp_pool_create(), but this behaviour isn't defined.

That is erroneous behavior.  If you allocate a packet of length 0 then 
odp_packet_len() will report 0 so the code would be writing beyond the end of 
the packet.  Such behavior is, by definition, undefined.  If you alloc a packet 
with length 0 then you need to first call odp_packet_push_tail() to sweep out 
the number of bytes you want to be able to write.  Of course, the RC for this 
must be checked since the push may or may not be successful.


 example/ipsec/odp_ipsec_stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/example/ipsec/odp_ipsec_stream.c b/example/ipsec/odp_ipsec_stream.c
index f750e18..4d908e4 100644
--- a/example/ipsec/odp_ipsec_stream.c
+++ b/example/ipsec/odp_ipsec_stream.c
@@ -356,7 +356,7 @@ odp_packet_t create_ipv4_packet(stream_db_entry_t *stream,
}

/* Correct set packet length offsets */
-   odp_packet_push_tail(pkt, data - base);
+   odp_packet_pull_tail(pkt, odp_packet_len(pkt) - (data - base));
odp_packet_l2_offset_set(pkt, (uint8_t *)eth - base);
odp_packet_l3_offset_set(pkt, (uint8_t *)ip - base);
odp_packet_l4_offset_set(pkt, ((uint8_t *)ip - base) + sizeof(*ip));
--
2.1.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

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

[Ion Grigore]

Ping

-Original Message-
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Ion Grigore
Sent: Monday, January 04, 2016 2:54 PM
To: Maxim Uvarov <maxim.uva...@linaro.org>; Ilya Maximets 
<i.maxim...@samsung.com>
Cc: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCHv7] helper : Fix UDP checksum computation

Done, as it was suggested.

Thanks,
Grig


-Original Message-
From: Maxim Uvarov [mailto:maxim.uva...@linaro.org]
Sent: Thursday, December 31, 2015 10:07 AM
To: Ilya Maximets <i.maxim...@samsung.com>; Ion Grigore 
<ion.grig...@freescale.com>; lng-odp@lists.linaro.org
Subject: Re: [PATCHv7] helper : Fix UDP checksum computation

On 12/31/2015 08:26, Ilya Maximets wrote:
> This version almost good.
> 2 more comments:
>   * no need to remove blank lines at the beginning of the file.

that is ok. Our checkpatch did not capture double empty lines at the beginning, 
now it does.
So this patch just removes that checkpatch warning.

>   * Why checksum is computed in the CPU endianness? There is no
> reason to do so. I can't imagine case where checksum will be
> needed in CPU endiannes. IMO, it's better not to convert it
> on return:
> + /* Return checksum in BE endianness */
> + return (sum == 0x0) ? 0x : sum;
> And remove conversion to BE in all other files:
> $ git grep odph_ipv4_udp_chksum
> example/generator/odp_generator.c:  udp->chksum = 
> odp_cpu_to_be_16(odph_ipv4_udp_chksum(pkt));
> helper/include/odp/helper/udp.h:static inline uint16_t 
> odph_ipv4_udp_chksum(odp_packet_t pkt)
> helper/test/odp_chksum.c:   udp->chksum = 
> odp_cpu_to_be_16(odph_ipv4_udp_chksum(test_packet));
> And modify only checksum value in helper/test/odp_chksum.c.
+1

>
> Best regards, Ilya Maximets.
>
> On 30.12.2015 18:15, Maxim Uvarov wrote:
>> Ilya is that version good for you? Do you want to add your Review-by?
>>
>> Maxim.
>>
>> On 12/17/2015 19:36, Ion Grigore wrote:
>>> Ping
>>>
>>> -Original Message-
>>> From: ion.grig...@freescale.com [mailto:ion.grig...@freescale.com]
>>> Sent: Monday, December 14, 2015 12:52 PM
>>> To: lng-odp@lists.linaro.org
>>> Cc: Grigore Ion-B17953 <ion.grig...@freescale.com>
>>> Subject: [PATCHv7] helper : Fix UDP checksum computation
>>>
>>> From: Grigore Ion <ion.grig...@freescale.com>
>>>
>>> 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 <ion.grig...@freescale.com>
>>> ---
>>>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)
>>>- Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
>>>
>>>helper/include/odp/helper/udp.h | 65 
>>> +
>>>helper/test/odp_chksum.c|  4 +--
>>>2 files changed, 35 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/helper/include/odp/helper/udp.h 
>>> b/helper/include/odp/helper/udp.h index 06c439b..f43fa54 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
>>> *
>

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

[Ion Grigore]

Done, as it was suggested.

Thanks,
Grig


-Original Message-
From: Maxim Uvarov [mailto:maxim.uva...@linaro.org] 
Sent: Thursday, December 31, 2015 10:07 AM
To: Ilya Maximets <i.maxim...@samsung.com>; Ion Grigore 
<ion.grig...@freescale.com>; lng-odp@lists.linaro.org
Subject: Re: [PATCHv7] helper : Fix UDP checksum computation

On 12/31/2015 08:26, Ilya Maximets wrote:
> This version almost good.
> 2 more comments:
>   * no need to remove blank lines at the beginning of the file.

that is ok. Our checkpatch did not capture double empty lines at the beginning, 
now it does.
So this patch just removes that checkpatch warning.

>   * Why checksum is computed in the CPU endianness? There is no
> reason to do so. I can't imagine case where checksum will be
> needed in CPU endiannes. IMO, it's better not to convert it
> on return:
> + /* Return checksum in BE endianness */
> + return (sum == 0x0) ? 0x : sum;
> And remove conversion to BE in all other files:
> $ git grep odph_ipv4_udp_chksum
> example/generator/odp_generator.c:  udp->chksum = 
> odp_cpu_to_be_16(odph_ipv4_udp_chksum(pkt));
> helper/include/odp/helper/udp.h:static inline uint16_t 
> odph_ipv4_udp_chksum(odp_packet_t pkt)
> helper/test/odp_chksum.c:   udp->chksum = 
> odp_cpu_to_be_16(odph_ipv4_udp_chksum(test_packet));
> And modify only checksum value in helper/test/odp_chksum.c.
+1

>
> Best regards, Ilya Maximets.
>
> On 30.12.2015 18:15, Maxim Uvarov wrote:
>> Ilya is that version good for you? Do you want to add your Review-by?
>>
>> Maxim.
>>
>> On 12/17/2015 19:36, Ion Grigore wrote:
>>> Ping
>>>
>>> -Original Message-
>>> From: ion.grig...@freescale.com [mailto:ion.grig...@freescale.com]
>>> Sent: Monday, December 14, 2015 12:52 PM
>>> To: lng-odp@lists.linaro.org
>>> Cc: Grigore Ion-B17953 <ion.grig...@freescale.com>
>>> Subject: [PATCHv7] helper : Fix UDP checksum computation
>>>
>>> From: Grigore Ion <ion.grig...@freescale.com>
>>>
>>> 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 <ion.grig...@freescale.com>
>>> ---
>>>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)
>>>- Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
>>>
>>>helper/include/odp/helper/udp.h | 65 
>>> +
>>>helper/test/odp_chksum.c|  4 +--
>>>2 files changed, 35 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/helper/include/odp/helper/udp.h 
>>> b/helper/include/odp/helper/udp.h index 06c439b..f43fa54 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
&

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

[Ion Grigore]

Ping

-Original Message-
From: ion.grig...@freescale.com [mailto:ion.grig...@freescale.com] 
Sent: Monday, December 14, 2015 12:52 PM
To: lng-odp@lists.linaro.org
Cc: Grigore Ion-B17953 
Subject: [PATCHv7] helper : Fix UDP checksum computation

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 
---
 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)
 - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)

 helper/include/odp/helper/udp.h | 65 +
 helper/test/odp_chksum.c|  4 +--
 2 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/helper/include/odp/helper/udp.h b/helper/include/odp/helper/udp.h 
index 06c439b..f43fa54 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 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))
+   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 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
+++ b/helper/test/odp_chksum.c
@@ -189,14 +189,14 @@ int main(int argc TEST_UNUSED, char *argv[] TEST_UNUSED)

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

[Ion Grigore]

Hi Ilya,

I checked the changes you suggested. It's Ok.  I sent a new patch(v7).

Tkanks,
Grig

-Original Message-
From: Grigore Ion-B17953 
Sent: Friday, December 11, 2015 5:57 PM
To: 'Ilya Maximets' <i.maxim...@samsung.com>; Maxim Uvarov 
<maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
Subject: RE: [PATCHv6] helper : Fix UDP checksum computation

Ok. I'll check your proposal on BE/LE.

-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
Sent: Friday, December 11, 2015 5:54 PM
To: Grigore Ion-B17953 <ion.grig...@freescale.com>; Maxim Uvarov 
<maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
Subject: Re: [PATCHv6] helper : Fix UDP checksum computation

On 11.12.2015 18:47, Ion Grigore wrote:
> A more precise question. Did you check on a LE platform ?

Yes. On my LE i7.

>>>>> - /* 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) +
>>>>> + u8_pad_to_u16(iph->proto, 1) + udph->length;
>>>> I meant something like:
>>>>
>>>> union {
>>>>uint8_t v8[2];
>>>>uint16_t v16;
>>>> } val;
>>>>
>>>> val.v8[0] = 0;
>>>> val.v8[1] = iph->proto;
>>>>
>>>
>>> The val.val16 must be swapped on LE platforms. 
>>
>> NO.
>>
>> Did you checked this ?
> 
> And you?
> 
>>
>>>
>>>
>>>> sum = (iph->src_addr & 0x) + (iph->src_addr >> 16) +
>>>>(iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
>>>>val.v16 + udph->length;
>>>>
>>>>> + 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 += u8_pad_to_u16(*buf, 0);
>>>> val.v8[0] = *buf;
>>>> val.v8[1] = 0;
>>>>
>>>> sum += val.v16;
>>>>
>>>> Isn't it more understandable, then artificial byte swapping?
>>>>
>>>
>>> The val.val16 must be swapped on LE platforms.
>>
>> NO.
>>
>> Did you checked this ?
> 
> And you?
> 
> P.S. Just to be honest, I checked this a few minutes ago.
>  Helper test below works.
> 
> Yes on BE/LE platforms, but not with the change you proposed.


So why you talking that val.val16 must be swapped if you do not understand how 
it works and you have not tested it?
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

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

[Ion Grigore]

Comments inlined

-Original Message-
From: Maxim Uvarov [mailto:maxim.uva...@linaro.org] 
Sent: Friday, December 11, 2015 11:20 AM
To: Ilya Maximets ; Grigore Ion-B17953 
; lng-odp@lists.linaro.org
Subject: Re: [PATCHv6] helper : Fix UDP checksum computation

On 12/11/2015 10:39, Ilya Maximets wrote:
> Comments inlined.
>
> On 10.12.2015 12: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 
>> ---
>>   v6:
>>   - Make code more understandable (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)
>>   - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
>>
>>   helper/include/odp/helper/udp.h | 81 
>> +
>>   helper/test/odp_chksum.c|  4 +-
>>   2 files changed, 51 insertions(+), 34 deletions(-)
>>
>> diff --git a/helper/include/odp/helper/udp.h 
>> b/helper/include/odp/helper/udp.h index 06c439b..d0599b1 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
>>*  @{
>>*/
>> @@ -39,51 +37,70 @@ typedef struct ODP_PACKED {
>>   } odph_udphdr_t;
>>   
>>   /**
>> + * Perform byte swap required by UDP checksum computation algorithm  
>> +*/ static inline uint16_t csum_bswap(uint16_t val) { #if 
>> +ODP_BYTE_ORDER == ODP_LITTLE_ENDIAN
>> +val = __odp_builtin_bswap16((__odp_force uint16_t)val); #endif
>> +return val;
>> +}
> Please, don't duplicate the code.
>
>> +
>> +/**
>> + * Pad a byte value to the left or to the right as required by UDP 
>> +checksum
>> + * computation algorithm and convert the result to CPU native uint16_t.
>> + * Left padding is performed for the IP protocol field in the UDP
>> + * pseudo-header (RFC 768). Right padding is performed in the case 
>> +of the odd
>> + * byte in a UDP packet having the length not a 2 multiple.
>> + */
>> +static inline uint16_t u8_pad_to_u16(uint8_t val, odp_bool_t left) {
>> +uint16_tret;
>> +
>> +ret = (left) ? val : val << 8;
>> +return csum_bswap(ret);
>> +}
>> +
>> +/**
>>* UDP checksum
>>*
>>* 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) +
>> +u8_pad_to_u16(iph->proto, 1) + 

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

[Ion Grigore]

Comments inlined

-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
Sent: Friday, December 11, 2015 5:40 PM
To: Grigore Ion-B17953 <ion.grig...@freescale.com>; Maxim Uvarov 
<maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
Subject: Re: [PATCHv6] helper : Fix UDP checksum computation

On 11.12.2015 18:28, Ion Grigore wrote:
> 
> Comments inlined
> 
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> Sent: Friday, December 11, 2015 5:24 PM
> To: Grigore Ion-B17953 <ion.grig...@freescale.com>; Maxim Uvarov 
> <maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
> Subject: Re: [PATCHv6] helper : Fix UDP checksum computation
> 
> On 11.12.2015 17:07, Ion Grigore wrote:
>> Comments inlined
>>
>> -Original Message-
>> From: Maxim Uvarov [mailto:maxim.uva...@linaro.org]
>> Sent: Friday, December 11, 2015 11:20 AM
>> To: Ilya Maximets <i.maxim...@samsung.com>; Grigore Ion-B17953 
>> <ion.grig...@freescale.com>; lng-odp@lists.linaro.org
>> Subject: Re: [PATCHv6] helper : Fix UDP checksum computation
>>
>> On 12/11/2015 10:39, Ilya Maximets wrote:
>>> Comments inlined.
>>>
>>> On 10.12.2015 12:59, ion.grig...@freescale.com wrote:
>>>> From: Grigore Ion <ion.grig...@freescale.com>
>>>>
>>>> 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 <ion.grig...@freescale.com>
>>>> ---
>>>>   v6:
>>>>   - Make code more understandable (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)
>>>>   - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
>>>>
>>>>   helper/include/odp/helper/udp.h | 81 
>>>> +
>>>>   helper/test/odp_chksum.c|  4 +-
>>>>   2 files changed, 51 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/helper/include/odp/helper/udp.h 
>>>> b/helper/include/odp/helper/udp.h index 06c439b..d0599b1 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
>>>>*  @{
>>>>*/
>>>> @@ -39,51 +37,70 @@ typedef struct ODP_PACKED {
>>>>   } odph_udphdr_t;
>>>>   
>>>>   /**
>>>> + * Perform byte swap required by UDP checksum computation 
>>>> +algorithm */ static inline uint16_t csum_bswap(uint16_t val) { #if 
>>>> +ODP_BYTE_ORDER == ODP_LITTLE_ENDIAN
>>>> +  val = __odp_builtin_bswap16((__odp_force uint16_t)val); #endif
>>>> +  return val;
>>>> +}
>>> Please, don't duplicate the code.
>>>
>>>> +
>>>> +/**
>>>> + * Pad a byte value to the left or to the right as required by UDP 
>>>> +checksum
>>>> + * computation algorithm and convert the result to CPU native uint16_t.
>>>> + * Left padding is performed for the IP protocol field in the UDP
>>>> + * pseudo-header

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

[Ion Grigore]

A more precise question. Did you check on a LE platform ?

-Original Message-
From: Grigore Ion-B17953 
Sent: Friday, December 11, 2015 5:44 PM
To: 'Ilya Maximets' <i.maxim...@samsung.com>; Maxim Uvarov 
<maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
Subject: RE: [PATCHv6] helper : Fix UDP checksum computation

Comments inlined

-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com]
Sent: Friday, December 11, 2015 5:40 PM
To: Grigore Ion-B17953 <ion.grig...@freescale.com>; Maxim Uvarov 
<maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
Subject: Re: [PATCHv6] helper : Fix UDP checksum computation

On 11.12.2015 18:28, Ion Grigore wrote:
> 
> Comments inlined
> 
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> Sent: Friday, December 11, 2015 5:24 PM
> To: Grigore Ion-B17953 <ion.grig...@freescale.com>; Maxim Uvarov 
> <maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
> Subject: Re: [PATCHv6] helper : Fix UDP checksum computation
> 
> On 11.12.2015 17:07, Ion Grigore wrote:
>> Comments inlined
>>
>> -Original Message-
>> From: Maxim Uvarov [mailto:maxim.uva...@linaro.org]
>> Sent: Friday, December 11, 2015 11:20 AM
>> To: Ilya Maximets <i.maxim...@samsung.com>; Grigore Ion-B17953 
>> <ion.grig...@freescale.com>; lng-odp@lists.linaro.org
>> Subject: Re: [PATCHv6] helper : Fix UDP checksum computation
>>
>> On 12/11/2015 10:39, Ilya Maximets wrote:
>>> Comments inlined.
>>>
>>> On 10.12.2015 12:59, ion.grig...@freescale.com wrote:
>>>> From: Grigore Ion <ion.grig...@freescale.com>
>>>>
>>>> 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 <ion.grig...@freescale.com>
>>>> ---
>>>>   v6:
>>>>   - Make code more understandable (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)
>>>>   - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
>>>>
>>>>   helper/include/odp/helper/udp.h | 81 
>>>> +
>>>>   helper/test/odp_chksum.c|  4 +-
>>>>   2 files changed, 51 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/helper/include/odp/helper/udp.h 
>>>> b/helper/include/odp/helper/udp.h index 06c439b..d0599b1 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
>>>>*  @{
>>>>*/
>>>> @@ -39,51 +37,70 @@ typedef struct ODP_PACKED {
>>>>   } odph_udphdr_t;
>>>>   
>>>>   /**
>>>> + * Perform byte swap required by UDP checksum computation 
>>>> +algorithm */ static inline uint16_t csum_bswap(uint16_t val) { #if 
>>>> +ODP_BYTE_ORDER == ODP_LITTLE_ENDIAN
>>>> +  val = __odp_builtin_bswap16((__odp_force uint16_t)val); #endif
>>>> +  return val;
>>>> +}
>>> Please, don't duplicate the code.
>>>
>>>> +
&

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

[Ion Grigore]

Comments inlined

-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
Sent: Friday, December 11, 2015 5:24 PM
To: Grigore Ion-B17953 <ion.grig...@freescale.com>; Maxim Uvarov 
<maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
Subject: Re: [PATCHv6] helper : Fix UDP checksum computation

On 11.12.2015 17:07, Ion Grigore wrote:
> Comments inlined
> 
> -Original Message-
> From: Maxim Uvarov [mailto:maxim.uva...@linaro.org]
> Sent: Friday, December 11, 2015 11:20 AM
> To: Ilya Maximets <i.maxim...@samsung.com>; Grigore Ion-B17953 
> <ion.grig...@freescale.com>; lng-odp@lists.linaro.org
> Subject: Re: [PATCHv6] helper : Fix UDP checksum computation
> 
> On 12/11/2015 10:39, Ilya Maximets wrote:
>> Comments inlined.
>>
>> On 10.12.2015 12:59, ion.grig...@freescale.com wrote:
>>> From: Grigore Ion <ion.grig...@freescale.com>
>>>
>>> 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 <ion.grig...@freescale.com>
>>> ---
>>>   v6:
>>>   - Make code more understandable (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)
>>>   - Modify check of odp_packet_l4_offset() return. (Bill Fischofer)
>>>
>>>   helper/include/odp/helper/udp.h | 81 
>>> +
>>>   helper/test/odp_chksum.c|  4 +-
>>>   2 files changed, 51 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/helper/include/odp/helper/udp.h 
>>> b/helper/include/odp/helper/udp.h index 06c439b..d0599b1 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
>>>*  @{
>>>*/
>>> @@ -39,51 +37,70 @@ typedef struct ODP_PACKED {
>>>   } odph_udphdr_t;
>>>   
>>>   /**
>>> + * Perform byte swap required by UDP checksum computation algorithm 
>>> +*/ static inline uint16_t csum_bswap(uint16_t val) { #if 
>>> +ODP_BYTE_ORDER == ODP_LITTLE_ENDIAN
>>> +   val = __odp_builtin_bswap16((__odp_force uint16_t)val); #endif
>>> +   return val;
>>> +}
>> Please, don't duplicate the code.
>>
>>> +
>>> +/**
>>> + * Pad a byte value to the left or to the right as required by UDP 
>>> +checksum
>>> + * computation algorithm and convert the result to CPU native uint16_t.
>>> + * Left padding is performed for the IP protocol field in the UDP
>>> + * pseudo-header (RFC 768). Right padding is performed in the case 
>>> +of the odd
>>> + * byte in a UDP packet having the length not a 2 multiple.
>>> + */
>>> +static inline uint16_t u8_pad_to_u16(uint8_t val, odp_bool_t left) {
>>> +   uint16_tret;
>>> +
>>> +   ret = (left) ? val : val << 8;
>>> +   return csum_bswap(ret);
>>> +}
>>> +
>>> +/**
>>>* UDP checksum
>>>*
>>>* This function uses odp packet to calc checksum
>>>*
>>>* @param pkt  calculate chksum for pkt
>>> - * @return  checksum value
>>> + * @return  checksum value in CPU endianness
>>>*/
>>>   st

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

[Ion Grigore]

Ok. I'll check your proposal on BE/LE.

-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
Sent: Friday, December 11, 2015 5:54 PM
To: Grigore Ion-B17953 <ion.grig...@freescale.com>; Maxim Uvarov 
<maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
Subject: Re: [PATCHv6] helper : Fix UDP checksum computation

On 11.12.2015 18:47, Ion Grigore wrote:
> A more precise question. Did you check on a LE platform ?

Yes. On my LE i7.

>>>>> - /* 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) +
>>>>> + u8_pad_to_u16(iph->proto, 1) + udph->length;
>>>> I meant something like:
>>>>
>>>> union {
>>>>uint8_t v8[2];
>>>>uint16_t v16;
>>>> } val;
>>>>
>>>> val.v8[0] = 0;
>>>> val.v8[1] = iph->proto;
>>>>
>>>
>>> The val.val16 must be swapped on LE platforms. 
>>
>> NO.
>>
>> Did you checked this ?
> 
> And you?
> 
>>
>>>
>>>
>>>> sum = (iph->src_addr & 0x) + (iph->src_addr >> 16) +
>>>>(iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
>>>>val.v16 + udph->length;
>>>>
>>>>> + 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 += u8_pad_to_u16(*buf, 0);
>>>> val.v8[0] = *buf;
>>>> val.v8[1] = 0;
>>>>
>>>> sum += val.v16;
>>>>
>>>> Isn't it more understandable, then artificial byte swapping?
>>>>
>>>
>>> The val.val16 must be swapped on LE platforms.
>>
>> NO.
>>
>> Did you checked this ?
> 
> And you?
> 
> P.S. Just to be honest, I checked this a few minutes ago.
>  Helper test below works.
> 
> Yes on BE/LE platforms, but not with the change you proposed.


So why you talking that val.val16 must be swapped if you do not understand how 
it works and you have not tested it?
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

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

[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 <ion.grig...@freescale.com>; 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 <ion.grig...@freescale.com>; 
> 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 <ion.grig...@freescale.com>
>>
>> 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 <ion.grig...@freescale.com>
>> ---
>> 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-&g

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

[Ion Grigore]

Resent with the fix inside.

Thanks,
Grig

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

It's a completely the same patch as v3.
Only commit message changed.

Best regards, Ilya Maximets.

On 08.12.2015 19:58, 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 
> ---
> 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;
> + 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);
> + /* 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..2b32111 100644
> --- a/helper/test/odp_chksum.c
> +++ b/helper/test/odp_chksum.c
> @@ -189,14 +189,14 @@ int main(int argc TEST_UNUSED, char *argv[] TEST_UNUSED)
>   udp->dst_port = 0;
>   udp->length = odp_cpu_to_be_16(udat_size + 

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

[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 ?

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

[Ion Grigore]

 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..2b32111 100644
> --- a/helper/test/odp_chksum.c
> +++ b/helper/test/odp_chksum.c
> @@ -189,14 +189,14 @@ int main(int argc TEST_UNUSED, char *argv[] TEST_UNUSED)
>   udp->dst_port = 0;
>   udp->length = odp_cpu_to_be_16(udat_size + ODPH_UDPHDR_LEN);
>   udp->chksum = 0;
> - udp->chksum = odph_ipv4_udp_chksum(test_packet);
> + udp->chksum = odp_cpu_to_be_16(odph_ipv4_udp_chksum(test_packet));
>  
>   if (udp->chksum == 0)
>   return -1;
>  
>   printf("chksum = 0x%x\n", udp->chksum);
>  
> - if (udp->chksum != 0xab2d)
> + if (udp->chksum != odp_be_to_cpu_16(0x7e5a))

0x7e5a is in CPU endianness at compile time. You're using odp_be_to_cpu_16() 
that leads to converting that digit to BE endianness. That is why that code 
works. It works because
udp->chksum is in BE and resulting digit in right side is also in BE.

But, that is illogical to use function odp_be_to_cpu_16() to convert from CPU 
to BE.

On 07.12.2015 19:27, Ion Grigore wrote:
> 
> The comparison is done in the CPU endianness.

NO, comparison is done in BE endianness.


Best regards, Ilya Maximets.
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

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

[Ion Grigore]

Hi Maxim,

I noticed you merged this patch and then you reverted it because the validation 
test fails.
The problem is in the test not in the patch.

odp_checksum.c  from line 192 
udp->chksum = odp_cpu_to_be_16(odph_ipv4_udp_chksum(test_packet));

if (udp->chksum == 0)
return -1;

printf("chksum = 0x%x\n", udp->chksum);

if (udp->chksum != odp_be_to_cpu_16(0x7e5a))
status = -1;

Here is the packet used in the test :

 fe 0f 97 c9 e0 44 fe 0f 97 c9 e0 44 08 00 45 00
0010 00 34 00 01 00 00 00 11 39 65 c0 a8 00 01 c0 a8
0020 00 02 00 00 00 00 00 20 7e 5a 00 00 00 00 00 00
0030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0040 00 00

The UDP checksum is 0x7e5a (in BE) and this is confirmed by Wireshark.

Thanks,
Grig

-Original Message-
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Maxim 
Uvarov
Sent: Monday, December 07, 2015 1:09 PM
To: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCHv2] helper : Fix UDP checksum computation

Merged,
Maxim.

On 12/04/2015 18:20, Bill Fischofer wrote:
>
>
> On Thu, Oct 22, 2015 at 7:02 AM,  > 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.
>
> Signed-off-by: Grigore Ion  >
>
>
> Reviewed-by: Bill Fischofer  >
>
> ---
>  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 |   32
> +---
>  1 files changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/helper/include/odp/helper/udp.h
> b/helper/include/odp/helper/udp.h
> index 06c439b..6fce3f2 100644
> --- a/helper/include/odp/helper/udp.h
> +++ b/helper/include/odp/helper/udp.h
> @@ -44,20 +44,16 @@ 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;
> +   uint32_t sum;
> odph_udphdr_t *udph;
> odph_ipv4hdr_t *iph;
> -   uint16_t udplen;
> -   uint8_t *buf;
> +   uint16_t udplen, *buf;
>
> -   if (!odp_packet_l3_offset(pkt))
> -   return 0;
> -
> -   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);
> @@ -67,23 +63,21 @@ static inline uint16_t
> odph_ipv4_udp_chksum(odp_packet_t pkt)
> /* 32-bit sum of all 16-bit words covered by UDP chksum */
> 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;
> -   }
> + odp_be_to_cpu_16(iph->proto) + udph->length;
> +   for (buf = (uint16_t *)((void *)udph); udplen > 1; udplen
> -= 2)
> +   sum += *buf++;
> +   if (udplen) /* If length is not a multiple of 2 bytes */
> +   sum += odp_be_to_cpu_16(*((uint8_t *)buf) << 8);
>
> /* Fold sum to 16 bits: add carrier to result */
> -   while (sum >> 16)
> -   sum = (sum & 0x) + (sum >> 16);
> +   sum = (sum & 0x) + (sum >> 16);
> +   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);
>  }
>
>  /** 

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

[Ion Grigore]

Hi,

I just sent the patch.

Thanks,
Grig

-Original Message-
From: Maxim Uvarov [mailto:maxim.uva...@linaro.org] 
Sent: Monday, December 07, 2015 3:52 PM
To: Grigore Ion-B17953 <ion.grig...@freescale.com>; lng-odp@lists.linaro.org
Cc: Grigore Sebastian-SGRIGOR1 <sebastian.grig...@freescale.com>
Subject: Re: [lng-odp] [PATCHv2] helper : Fix UDP checksum computation

On 12/07/2015 16:35, Ion Grigore wrote:
> Hi Maxim,
>
> I noticed you merged this patch and then you reverted it because the 
> validation test fails.
> The problem is in the test not in the patch.
>
> odp_checksum.c  from line 192
>   udp->chksum = odp_cpu_to_be_16(odph_ipv4_udp_chksum(test_packet));
>
>   if (udp->chksum == 0)
>   return -1;
>
>   printf("chksum = 0x%x\n", udp->chksum);
>
>   if (udp->chksum != odp_be_to_cpu_16(0x7e5a))
>   status = -1;
>
> Here is the packet used in the test :
>
>  fe 0f 97 c9 e0 44 fe 0f 97 c9 e0 44 08 00 45 00
> 0010 00 34 00 01 00 00 00 11 39 65 c0 a8 00 01 c0 a8
> 0020 00 02 00 00 00 00 00 20 7e 5a 00 00 00 00 00 00
> 0030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0040 00 00
>
> The UDP checksum is 0x7e5a (in BE) and this is confirmed by Wireshark.
>
> Thanks,
> Grig

Yes, odp_be_to_cpu_16(0x7e5a) is logical, but change from 0xab2d to 0x7e5a 
means what we had really bad code before.

Grig, all patches should pass validation test. Can you please send updated 
version with test case fixes included. You can also add Bills review for it as 
he did it for main patch.

Thanks,
Maxim.


>
> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of 
> Maxim Uvarov
> Sent: Monday, December 07, 2015 1:09 PM
> To: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCHv2] helper : Fix UDP checksum computation
>
> Merged,
> Maxim.
>
> On 12/04/2015 18:20, Bill Fischofer wrote:
>>
>> On Thu, Oct 22, 2015 at 7:02 AM, <ion.grig...@freescale.com 
>> <mailto:ion.grig...@freescale.com>> wrote:
>>
>>  From: Grigore Ion <ion.grig...@freescale.com
>>  <mailto:ion.grig...@freescale.com>>
>>
>>  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.
>>
>>  Signed-off-by: Grigore Ion <ion.grig...@freescale.com
>>  <mailto:ion.grig...@freescale.com>>
>>
>>
>> Reviewed-by: Bill Fischofer <bill.fischo...@linaro.org 
>> <mailto:bill.fischo...@linaro.org>>
>>
>>  ---
>>   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 |   32
>>  +---
>>   1 files changed, 13 insertions(+), 19 deletions(-)
>>
>>  diff --git a/helper/include/odp/helper/udp.h
>>  b/helper/include/odp/helper/udp.h
>>  index 06c439b..6fce3f2 100644
>>  --- a/helper/include/odp/helper/udp.h
>>  +++ b/helper/include/odp/helper/udp.h
>>  @@ -44,20 +44,16 @@ 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;
>>  +   uint32_t sum;
>>  odph_udphdr_t *udph;
>>  odph_ipv4hdr_t *iph;
>>  -   uint16_t udplen;
>>  -   uint8_t *buf;
>>  +   uint16_t udplen, *buf;
>>
>>  -   if (!odp_packet_l3_offset(pkt))
>>  -   return 0;
>>  -
>>  -   if (!odp_packet_l4_offset(pkt))
>>  +   if (odp_packet_l4_offset(pkt) == ODP_PACKET_OFFSET_INVALID)
>>  return 0;
>>

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

[Ion Grigore]

Done. Patch v3

Grig


-Original Message-
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Maxim 
Uvarov
Sent: Monday, December 07, 2015 4:50 PM
To: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCHv1] helper:test: Fix UDP checksum computation

I tried to say in previous message to merge that chunk to previous patch.
So it will be 1 patch which fixes both tests and validation. Just to avoid 
PASS->FAILED->PASS test sequence doing bisecting.

Maxim.

On 12/07/2015 17:29, ion.grig...@freescale.com wrote:
> From: Grigore Ion 
>
> The UDP 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.
>
> Signed-off-by: Grigore Ion 
> ---
> v1:
> Fix UDP checksum computation
>
>   helper/test/odp_chksum.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/helper/test/odp_chksum.c b/helper/test/odp_chksum.c index 
> 1d417a8..2b32111 100644
> --- a/helper/test/odp_chksum.c
> +++ b/helper/test/odp_chksum.c
> @@ -189,14 +189,14 @@ int main(int argc TEST_UNUSED, char *argv[] TEST_UNUSED)
>   udp->dst_port = 0;
>   udp->length = odp_cpu_to_be_16(udat_size + ODPH_UDPHDR_LEN);
>   udp->chksum = 0;
> - udp->chksum = odph_ipv4_udp_chksum(test_packet);
> + udp->chksum = odp_cpu_to_be_16(odph_ipv4_udp_chksum(test_packet));
>   
>   if (udp->chksum == 0)
>   return -1;
>   
>   printf("chksum = 0x%x\n", udp->chksum);
>   
> - if (udp->chksum != 0xab2d)
> + if (udp->chksum != odp_be_to_cpu_16(0x7e5a))
>   status = -1;
>   
>   odp_packet_free(test_packet);

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

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

[Ion Grigore]

Hi,

What is the status of this patch ?

Example or test applications using odph_ipv4_udp_chksum function
may fail on LE platforms.

Thanks,
Grig

-Original Message-
From: ion.grig...@freescale.com [mailto:ion.grig...@freescale.com] 
Sent: Thursday, October 22, 2015 3:03 PM
To: lng-odp@lists.linaro.org
Cc: Grigore Ion-B17953 
Subject: [PATCHv2] helper : Fix UDP checksum computation

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.

Signed-off-by: Grigore Ion 
---
 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 |   32 +---
 1 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/helper/include/odp/helper/udp.h b/helper/include/odp/helper/udp.h 
index 06c439b..6fce3f2 100644
--- a/helper/include/odp/helper/udp.h
+++ b/helper/include/odp/helper/udp.h
@@ -44,20 +44,16 @@ 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;
+   uint32_t sum;
odph_udphdr_t *udph;
odph_ipv4hdr_t *iph;
-   uint16_t udplen;
-   uint8_t *buf;
+   uint16_t udplen, *buf;
 
-   if (!odp_packet_l3_offset(pkt))
-   return 0;
-
-   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); @@ -67,23 +63,21 
@@ static inline uint16_t odph_ipv4_udp_chksum(odp_packet_t pkt)
/* 32-bit sum of all 16-bit words covered by UDP chksum */
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;
-   }
+ odp_be_to_cpu_16(iph->proto) + udph->length;
+   for (buf = (uint16_t *)((void *)udph); udplen > 1; udplen -= 2)
+   sum += *buf++;
+   if (udplen) /* If length is not a multiple of 2 bytes */
+   sum += odp_be_to_cpu_16(*((uint8_t *)buf) << 8);
 
/* Fold sum to 16 bits: add carrier to result */
-   while (sum >> 16)
-   sum = (sum & 0x) + (sum >> 16);
+   sum = (sum & 0x) + (sum >> 16);
+   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 */
--
1.7.3.4

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCHv2 1/2] example:generator : Fix data race condition

[Ion Grigore]

Correction:
Excepting example-generetaor-Fix-UDP-checksum-computation.patch

-Original Message-
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Ion Grigore
Sent: Thursday, October 22, 2015 5:12 PM
To: Maxim Uvarov <maxim.uva...@linaro.org>
Cc: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCHv2 1/2] example:generator : Fix data race condition

I resent all patches updated to the latest master, excepting 
0001-validation-pktio-Fix-UDP-checksum-computation.patch
that was merged.

Thanks,
Grig

-Original Message-
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Maxim 
Uvarov
Sent: Monday, October 05, 2015 2:32 PM
To: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCHv2 1/2] example:generator : Fix data race condition

please update patch to the latest master.

Thanks,
Maxim.

On 10/02/15 13:39, ion.grig...@freescale.com wrote:
> From: Grigore Ion <ion.grig...@freescale.com>
>
> The counters.seq counter is used to check if the configured number of 
> packets was processed. There is a race condition between the counter 
> incrementation time and its value testing time. If code is running on 
> multiple CPUs it is possible the application send more packets than 
> expected (with number of CPUs - 1). A separate counter must be used 
> for the processed packets.
>
> Signed-off-by: Grigore Ion <ion.grig...@freescale.com>
> ---
>   v2:
>   - Add PATCH version information (Maxim Uvarov)
>   - Check patch with checkpatch script. (Bill Fischofer)
>
>   example/generator/odp_generator.c |   19 ++-
>   1 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/example/generator/odp_generator.c
> b/example/generator/odp_generator.c
> index d6ec758..4af82a9 100644
> --- a/example/generator/odp_generator.c
> +++ b/example/generator/odp_generator.c
> @@ -66,6 +66,7 @@ static struct {
>   odp_atomic_u64_t ip;/**< ip packets */
>   odp_atomic_u64_t udp;   /**< udp packets */
>   odp_atomic_u64_t icmp;  /**< icmp packets */
> + odp_atomic_u64_t cnt;   /**< sent packets*/
>   } counters;
>   
>   /** * Thread specific arguments
> @@ -395,6 +396,11 @@ static void *gen_send_thread(void *arg)
>   for (;;) {
>   int err;
>   
> + if (args->appl.number != -1 &&
> + odp_atomic_fetch_add_u64(, 1) >=
> + (unsigned int)args->appl.number)
> + break;
> +
>   if (args->appl.mode == APPL_MODE_UDP)
>   pkt = pack_udp_pkt(thr_args->pool);
>   else if (args->appl.mode == APPL_MODE_PING) @@ -426,11 +432,6 
> @@ 
> static void *gen_send_thread(void *arg)
>  thr_args->tmo_ev);
>   
>   }
> - if (args->appl.number != -1 &&
> - odp_atomic_load_u64()
> - >= (unsigned int)args->appl.number) {
> - break;
> - }
>   }
>   
>   /* receive number of reply pks until timeout */ @@ -450,11 +451,10 
> @@ static void *gen_send_thread(void *arg)
>   
>   /* print info */
>   if (args->appl.mode == APPL_MODE_UDP) {
> - printf("  [%02i] total send: %ju\n",
> -thr, odp_atomic_load_u64());
> + printf("  [%02i] total send: %d\n", thr, args->appl.number);
>   } else if (args->appl.mode == APPL_MODE_PING) {
> - printf("  [%02i] total send: %ju total receive: %ju\n",
> -thr, odp_atomic_load_u64(),
> + printf("  [%02i] total send: %d total receive: %ju\n",
> +thr, args->appl.number,
>  odp_atomic_load_u64());
>   }
>   return arg;
> @@ -610,6 +610,7 @@ int main(int argc, char *argv[])
>   odp_atomic_init_u64(, 0);
>   odp_atomic_init_u64(, 0);
>   odp_atomic_init_u64(, 0);
> + odp_atomic_init_u64(, 0);
>   
>   /* Reserve memory for args from shared mem */
>   shm = odp_shm_reserve("shm_args", sizeof(args_t),

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] validation:pktio : Fix UDP checksum computation

[Ion Grigore]

-Original Message-
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Maxim 
Uvarov
Sent: Wednesday, September 23, 2015 8:37 PM
To: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCH] validation:pktio : Fix UDP checksum computation



On 09/21/15 08:57, ion.grig...@freescale.com wrote:
> From: Grigore Ion 
>
> The UDP checksum is computed in the CPU endianess. The returned result 
> must be converted to the BE ordering when it is used to update the UDP 
> checksum in a packet.
>
> Signed-off-by: Grigore Ion 
> ---
>   test/validation/pktio/pktio.c |2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/test/validation/pktio/pktio.c 
> b/test/validation/pktio/pktio.c index d62f18d..604ac89 100644
> --- a/test/validation/pktio/pktio.c
> +++ b/test/validation/pktio/pktio.c
> @@ -195,7 +195,7 @@ static int pktio_fixup_checksums(odp_packet_t pkt)
>   ip->chksum = 0;
>   odph_ipv4_csum_update(pkt);
>   udp->chksum = 0;
> - udp->chksum = odph_ipv4_udp_chksum(pkt);
> + udp->chksum = odp_cpu_to_be_16(odph_ipv4_udp_chksum(pkt));
>   

odph_ipv4_udp_chksum() should return checksum in right bite order. Converting 
it each time like that complicates code.

Maxim.

The chksum field is declared as uint16be_t.  In my opinion, applications 
updating the xxxbe_t fields should call the 
odp_cpu_to_be_xx function to update such fields (mapped on data in a BE 
packet).   From this point of view,
odph_ipv4_udp_chksum() returns the checksum in the CPU byte order.

The same is done in the function odph_ipv4_csum_update().

Grig

>   return 0;
>   }

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] example:generator : Fix data race condition

[Ion Grigore]

Pong.

-Original Message-
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Maxim 
Uvarov
Sent: Thursday, October 01, 2015 1:37 PM
To: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCH] example:generator : Fix data race condition

ping.

On 09/22/15 17:19, ion.grig...@freescale.com wrote:
> From: Grigore Ion 
>
> The counters.seq counter is used to check if the configured number of 
> packets was processed. There is a race condition between the counter 
> incrementation time and its value testing time. If code is running on 
> multiple CPUs it is possible the application send more packets than 
> expected (with number of CPUs - 1). A separate counter must be used 
> for the processed packets.
>
> Signed-off-by: Grigore Ion 
> ---
>   example/generator/odp_generator.c |   21 -
>   1 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/example/generator/odp_generator.c 
> b/example/generator/odp_generator.c
> index e281b27..2dc1801 100644
> --- a/example/generator/odp_generator.c
> +++ b/example/generator/odp_generator.c
> @@ -66,6 +66,7 @@ static struct {
>   odp_atomic_u64_t ip;/**< ip packets */
>   odp_atomic_u64_t udp;   /**< udp packets */
>   odp_atomic_u64_t icmp;  /**< icmp packets */
> + odp_atomic_u64_t cnt;   /**< sent packets*/
>   } counters;
>   
>   /** * Thread specific arguments
> @@ -228,6 +229,7 @@ static odp_packet_t pack_udp_pkt(odp_pool_t pool)
>  ODPH_IPV4HDR_LEN);
>   ip->proto = ODPH_IPPROTO_UDP;
>   seq = odp_atomic_fetch_add_u64(, 1) % 0x;
> +
>   ip->id = odp_cpu_to_be_16(seq);
>   ip->chksum = 0;
>   odph_ipv4_csum_update(pkt);
> @@ -395,6 +397,12 @@ static void *gen_send_thread(void *arg)
>   for (;;) {
>   int err;
>   
> + if (args->appl.number != -1 &&
> + odp_atomic_fetch_add_u64(, 1) >=
> + (unsigned int)args->appl.number) {
> + break;
> + }
> +
>   if (args->appl.mode == APPL_MODE_UDP)
>   pkt = pack_udp_pkt(thr_args->pool);
>   else if (args->appl.mode == APPL_MODE_PING) @@ -426,11 +434,6 
> @@ 
> static void *gen_send_thread(void *arg)
>  thr_args->tmo_ev);
>   
>   }
> - if (args->appl.number != -1 &&
> - odp_atomic_load_u64()
> - >= (unsigned int)args->appl.number) {
> - break;
> - }
>   }
>   
>   /* receive number of reply pks until timeout */ @@ -450,11 +453,10 
> @@ static void *gen_send_thread(void *arg)
>   
>   /* print info */
>   if (args->appl.mode == APPL_MODE_UDP) {
> - printf("  [%02i] total send: %ju\n",
> -thr, odp_atomic_load_u64());
> + printf("  [%02i] total send: %d\n", thr, args->appl.number);
>   } else if (args->appl.mode == APPL_MODE_PING) {
> - printf("  [%02i] total send: %ju total receive: %ju\n",
> -thr, odp_atomic_load_u64(),
> + printf("  [%02i] total send: %d total receive: %ju\n",
> +thr, args->appl.number,
>  odp_atomic_load_u64());
>   }
>   return arg;
> @@ -610,6 +612,7 @@ int main(int argc, char *argv[])
>   odp_atomic_init_u64(, 0);
>   odp_atomic_init_u64(, 0);
>   odp_atomic_init_u64(, 0);
> + odp_atomic_init_u64(, 0);
>   
>   /* Reserve memory for args from shared mem */
>   shm = odp_shm_reserve("shm_args", sizeof(args_t),

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] example:generator : Fix data race condition

[Ion Grigore]

Hi

I superseded the old patch and I resent a new one with the “empty line” removed.

Thanks,
Grig


From: Ola Liljedahl [mailto:ola.liljed...@linaro.org]
Sent: Thursday, October 01, 2015 4:05 PM
To: Grigore Ion-B17953 <ion.grig...@freescale.com>
Cc: Maxim Uvarov <maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCH] example:generator : Fix data race condition

I think Maxim expects you to send an updated patch with the following change 
reversed:

seq = odp_atomic_fetch_add_u64(, 1) % 0x;
+
ip->id = odp_cpu_to_be_16(seq);

Unfortunately Maxim only responded to the ODP list and not to you directly. 
Perhaps you are not on the list?

-- Ola


On 1 October 2015 at 13:04, Ion Grigore 
<ion.grig...@freescale.com<mailto:ion.grig...@freescale.com>> wrote:
Pong.

-Original Message-
From: lng-odp 
[mailto:lng-odp-boun...@lists.linaro.org<mailto:lng-odp-boun...@lists.linaro.org>]
 On Behalf Of Maxim Uvarov
Sent: Thursday, October 01, 2015 1:37 PM
To: lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>
Subject: Re: [lng-odp] [PATCH] example:generator : Fix data race condition

ping.

On 09/22/15 17:19, ion.grig...@freescale.com<mailto:ion.grig...@freescale.com> 
wrote:
> From: Grigore Ion 
> <ion.grig...@freescale.com<mailto:ion.grig...@freescale.com>>
>
> The counters.seq counter is used to check if the configured number of
> packets was processed. There is a race condition between the counter
> incrementation time and its value testing time. If code is running on
> multiple CPUs it is possible the application send more packets than
> expected (with number of CPUs - 1). A separate counter must be used
> for the processed packets.
>
> Signed-off-by: Grigore Ion 
> <ion.grig...@freescale.com<mailto:ion.grig...@freescale.com>>
> ---
>   example/generator/odp_generator.c |   21 -
>   1 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/example/generator/odp_generator.c
> b/example/generator/odp_generator.c
> index e281b27..2dc1801 100644
> --- a/example/generator/odp_generator.c
> +++ b/example/generator/odp_generator.c
> @@ -66,6 +66,7 @@ static struct {
>   odp_atomic_u64_t ip;/**< ip packets */
>   odp_atomic_u64_t udp;   /**< udp packets */
>   odp_atomic_u64_t icmp;  /**< icmp packets */
> + odp_atomic_u64_t cnt;   /**< sent packets*/
>   } counters;
>
>   /** * Thread specific arguments
> @@ -228,6 +229,7 @@ static odp_packet_t pack_udp_pkt(odp_pool_t pool)
>  ODPH_IPV4HDR_LEN);
>   ip->proto = ODPH_IPPROTO_UDP;
>   seq = odp_atomic_fetch_add_u64(, 1) % 0x;
> +
>   ip->id = odp_cpu_to_be_16(seq);
>   ip->chksum = 0;
>   odph_ipv4_csum_update(pkt);
> @@ -395,6 +397,12 @@ static void *gen_send_thread(void *arg)
>   for (;;) {
>   int err;
>
> + if (args->appl.number != -1 &&
> + odp_atomic_fetch_add_u64(, 1) >=
> + (unsigned int)args->appl.number) {
> + break;
> + }
> +
>   if (args->appl.mode == APPL_MODE_UDP)
>   pkt = pack_udp_pkt(thr_args->pool);
>   else if (args->appl.mode == APPL_MODE_PING) @@ -426,11 +434,6 @@
> static void *gen_send_thread(void *arg)
>  thr_args->tmo_ev);
>
>   }
> - if (args->appl.number != -1 &&
> - odp_atomic_load_u64()
> - >= (unsigned int)args->appl.number) {
> - break;
> - }
>   }
>
>   /* receive number of reply pks until timeout */ @@ -450,11 +453,10
> @@ static void *gen_send_thread(void *arg)
>
>   /* print info */
>   if (args->appl.mode == APPL_MODE_UDP) {
> - printf("  [%02i] total send: %ju\n",
> -thr, odp_atomic_load_u64());
> + printf("  [%02i] total send: %d\n", thr, args->appl.number);
>   } else if (args->appl.mode == APPL_MODE_PING) {
> - printf("  [%02i] total send: %ju total receive: %ju\n",
> -thr, odp_atomic_load_u64(),
> + printf("  [%02i] total send: %d total receive: %ju\n",
> +thr, args->appl.number,
>  odp_atomic_load_u64());
>   }
>   return arg;
> @@ -610,6 +612,7 @@ int main(int argc, char *argv[])
>   odp_atomic_init_u64(, 0);
>   odp_atomic_init_u64(, 0);
>   odp_atomic_init_u64(, 0);
> + odp_atomic_init_

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

[Ion Grigore]

Hi,

Now all variables are declared on top of block.
The patch passed checkpatch verification.

I superseded the old patch.

Thanks,
Grig

-Original Message-
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Maxim 
Uvarov
Sent: Wednesday, September 23, 2015 8:34 PM
To: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCH] helper : Fix UDP checksum computation



On 09/21/15 08:45, 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 endianess. 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.
>
> Signed-off-by: Grigore Ion 
> ---
>   helper/include/odp/helper/udp.h |   54 
> ++-
>   1 files changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/helper/include/odp/helper/udp.h 
> b/helper/include/odp/helper/udp.h index 06c439b..74fad3c 100644
> --- a/helper/include/odp/helper/udp.h
> +++ b/helper/include/odp/helper/udp.h
> @@ -22,7 +22,6 @@ extern "C" {
>   #include 
>   #include 
>   
> -
>   /** @addtogroup odph_header ODPH HEADER
>*  @{
>*/
> @@ -44,46 +43,43 @@ 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 endianess
>*/
>   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))
> + if (!odp_packet_l3_offset(pkt) || !odp_packet_l3_offset(pkt))
that looks strange :)
>   return 0;
>   
> - if (!odp_packet_l4_offset(pkt))
> - return 0;
> + odph_ipv4hdr_t *iph = (odph_ipv4hdr_t *)odp_packet_l3_ptr(pkt, NULL);
> + odph_udphdr_t *udph = (odph_udphdr_t *)odp_packet_l4_ptr(pkt, NULL);
iph and udph should be defined on top of the block. Interesting if checkpatch 
did not said about that.
The same for all other variables. So just not delete them from top and run 
./script/checkpatch.pl before sending.

Thanks,
Maxim.

> +
> + /* 32-bit sum of UDP pseudo-header */
> + uint32_t sum = (iph->src_addr & 0x) + (iph->src_addr >> 16) +
> + (iph->dst_addr & 0x) + (iph->dst_addr >> 16) +
> + odp_be_to_cpu_16(iph->proto) + udph->length;
>   
> - 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);
> + uint16_t udplen = odp_be_to_cpu_16(udph->length);
> + uint16_t *buf = (uint16_t *)((void *)udph);
>   
> - /* 32-bit sum of all 16-bit words covered by UDP chksum */
> - 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;
> - }
> + /* 32-bit sum of UDP header (checksum field cleared) and UDP data */
> + for ( ; udplen > 1; udplen -= 2)
> + sum += *buf++;
>   
> - /* Fold sum to 16 bits: add carrier to result */
> - while (sum >> 16)
> - sum = (sum & 0x) + (sum >> 16);
> + /* Length is not a multiple of 2 bytes */
> + if (udplen)
> + sum += odp_be_to_cpu_16(*((uint8_t *)buf) << 8);
> +
> + /* 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 */
> + return (sum == 0x0) ? 0x : odp_be_to_cpu_16(sum);
>   }
>   
>   /** @internal Compile time assert */

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp