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) ? 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. +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 <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 _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
