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 <odp/debug.h> >> #include <odp/byteorder.h> >> >> - >> /** @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_t sum; >> + uint16_t udplen, *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 & 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; >> - } >> - >> - /* Fold sum to 16 bits: add carrier to result */ >> - while (sum >> 16) >> - sum = (sum & 0xFFFF) + (sum >> 16); >> - >> + (iph->dst_addr & 0xFFFF) + (iph->dst_addr >> 16) + >> + odp_be_to_cpu_16(iph->proto) + udph->length; > > This line is also strange. You're using odp_be_to_cpu_16 for 8-bit digit. > At this point, iph->proto firstly will be extended to u16 in CPU endianness > and after that odp_be_to_cpu_16() will return it to BE. You will have right > position of all bytes (8 zero bits + 8 bits of iph->proto), but the way how > this done is wrong. > > [IGR] Here the proto field is added as the LSB in a 16-bit BE value. Do you > think the following code clearer ? > > uint16 val; > ... > val = iph->proto; > sum += odp_be_to_cpu_16(val); No, because 'val' is in CPU endianness and you're using odp_be_to_cpu_16() for variable that is already in CPU endianness. > >> + udplen = odp_be_to_cpu_16(udph->length); >> + buf = (uint16_t *)((void *)udph); >> + /* 32-bit sum of UDP header (checksum field cleared) and UDP data */ >> + for ( ; udplen > 1; udplen -= 2) >> + sum += *buf++; >> + /* Length is not a multiple of 2 bytes */ >> + if (udplen) >> + sum += odp_be_to_cpu_16(*((uint8_t *)buf) << 8); > > Same at this point. You're 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 ? > if (udplen) { > val = *((uint8_t *)buf) << 8; > sum += odp_be_to_cpu_16(val); > } Same. > >> + /* 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; >> + /* Set computation result 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); >> _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
