Hi, I just sent the patch.
Thanks, Grig -----Original Message----- From: Maxim Uvarov [mailto:[email protected]] Sent: Monday, December 07, 2015 3:52 PM To: Grigore Ion-B17953 <[email protected]>; [email protected] Cc: Grigore Sebastian-SGRIGOR1 <[email protected]> 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 : > > 0000 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:[email protected]] On Behalf Of > Maxim Uvarov > Sent: Monday, December 07, 2015 1:09 PM > To: [email protected] > 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, <[email protected] >> <mailto:[email protected]>> wrote: >> >> From: Grigore Ion <[email protected] >> <mailto:[email protected]>> >> >> 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 <[email protected] >> <mailto:[email protected]>> >> >> >> Reviewed-by: Bill Fischofer <[email protected] >> <mailto:[email protected]>> >> >> --- >> 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 & 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; >> - } >> + 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 & 0xFFFF) + (sum >> 16); >> + sum = (sum & 0xFFFF) + (sum >> 16); >> + 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 */ >> -- >> 1.7.3.4 >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] <mailto:[email protected]> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] >> https://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ > lng-odp mailing list > [email protected] > https://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
