This version almost good.
2 more comments:
* no need to remove blank lines at the beginning of the file.
* 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) ? 0xFFFF : 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.
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 <odp/debug.h>
>> #include <odp/byteorder.h>
>> -
>> /** @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_t sum;
>> + uint16_t udplen, *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 & 0xFFFF) + (iph->src_addr >> 16) +
>> - (iph->dst_addr & 0xFFFF) + (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 & 0xFFFF) + (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 & 0xFFFF) + (sum >> 16);
>> -
>> + /* Fold sum to 16 bits */
>> + sum = (sum & 0xFFFF) + (sum >> 16);
>> + /* Add carrier (0/1) to result */
>> + sum += (sum >> 16);
>> /* 1's complement */
>> sum = ~sum;
>> -
>> - /* set computation result */
>> - sum = (sum == 0x0) ? 0xFFFF : sum;
>> -
>> - return sum;
>> + /* Return checksum in CPU endianness */
>> + return (sum == 0x0) ? 0xFFFF : 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)
>> 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 (odp_be_to_cpu_16(udp->chksum) != 0x7e5a)
>> status = -1;
>> odp_packet_free(test_packet);
>> --
>> 1.9.3
>>
>
>
>
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
