buglwIP - A Lightweight TCP/IP stack - Bugs: bug #29763, CHECKSUM_GEN_IP_INLINE - AVR32

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #29763: CHECKSUM_GEN_IP_INLINE - AVR32

Submitter:  juan schiavoni <jjsch>
Submitted:  Mon 03 May 2010 11:22:50 PM UTC
   
 
Category:  IPv4 Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Thu 06 May 2010 02:55:42 PM UTC, comment #12: 

I download the source code from cvs, and the patch work fine with  avr32.

Thanks a lot
Juan

juan schiavoni <jjsch>
Wed 05 May 2010 12:42:51 PM UTC, comment #11: 

If htons is a macro creating C code, the compiler will constant fold the redundant shift/ORs away. The advantage of #if/#else/#endif is there is nothing for the compiler to do.  If htons generates inline assembly then there is waste because the compiler will be forced to generate the shift/OR and then the inline assembly which for little endian has to undo the shift/OR.

The following is legit too and a conforming compiler MUST not generate the test or code for the false half:

chk_sum = (BYTE_ORDER == LITTLE_ENDIAN) ? (proto << 8) | ttl : (ttl << 8) | proto;

Bill Auerbach <billauerbach>
Wed 05 May 2010 04:42:33 AM UTC, comment #10: 


> I guess too it simply could be chk_sum = htons( (proto << 8) | ttl );


That would work, too, but for little endian, it would be shifting in the wrong direction and then shifting back again in htons(), wouldn't it?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 04 May 2010 08:32:50 PM UTC, comment #9: 


>That's what I meant with the "patch didn't apply to CVS HEAD" :-)


Yeah, I saw what that meant pretty quickly :-)

>Fixed by adding an endian-dependent macro to create an u16_t in def.h.


I guess too it simply could be chk_sum = htons( (proto << 8) | ttl );

Bill Auerbach <billauerbach>
Tue 04 May 2010 07:47:52 PM UTC, comment #8: 


> When you do the patch, I download the source code from cvs to
> test it in the avr32.


When? It's already done in CVS.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 04 May 2010 07:25:01 PM UTC, comment #7: 

Hi Simon,

When you do the patch, I download the source code from cvs to test it in the avr32.

Regards,
Juan

juan schiavoni <jjsch>
Tue 04 May 2010 07:00:01 PM UTC, comment #6: 


> The field _ttl_prot, don't exist in the struct ip_hdr, it has
> two 8 bits field: (proto, and ttl).


That's what I meant with the "patch didn't apply to CVS HEAD" :-) And, no, I was testing it with little endian (x86), and it worked.

Fixed by adding an endian-dependent macro to create an u16_t in def.h.

Thanks for reporting and testing!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 04 May 2010 05:42:58 PM UTC, comment #5: 

Ok, thanks Juan.

Simon, I think we need:

#if BYTE_ORDER == LITTLE_ENDIAN
chk_sum = (proto << 8) | ttl;
#else
chk_sum = (ttl<<8) | proto;
#endif

Bill Auerbach <billauerbach>
Tue 04 May 2010 04:48:48 PM UTC, comment #4: 

Hi Bill,

The avr32 is big endian cpu.

The field _ttl_prot, don't exist in the struct ip_hdr, it has two 8 bits field: (proto, and ttl).

I change the order of [chk_sum = (proto << 8) | ttl;] for [chk_sum = (ttl<<8) | proto;], and work fine.

Thanks a lot.


juan schiavoni <jjsch>
Tue 04 May 2010 04:28:01 PM UTC, comment #3: 

chk_sum = iphdr->_ttl_proto;

is correct for big and little endian - I tested both.  What was checked in [chk_sum = (proto << 8) | ttl; ] apparently does not work for little endian?  Is Avr32 little endian?

Simon, I assumed your changes where for #define names and format and other consistency things - I didn't check the actual C code.

Juan, can you plug in the line above [chk_sum = iphdr->_ttl_proto;] and take out your += 0x7F8 and see how it goes?

Bill Auerbach <billauerbach>
Tue 04 May 2010 03:58:32 PM UTC, comment #2: 

That's because the original patch didn't apply to CVS HEAD and I seem to have forgotten about big-endian as it works for me??

Simon Goldschmidt <goldsimon>
Group administrator
Tue 04 May 2010 01:14:46 PM UTC, comment #1: 

The code in CVS HEAD is not quite the same as the original patch.  On line 600 (CVS HEAD version) it had:

    chk_sum = iphdr->_ttl_proto;

It's checked in as:

    chk_sum = (proto << 8) | ttl;

Your += 0x7F8 looks like it might be compensating for this difference.

Bill Auerbach <billauerbach>
Mon 03 May 2010 11:22:50 PM UTC, original submission:  

Hi, I download the source code of lwip from the cvs, and port to the avr32 framework of Atmel, and found that checksum of IP header has wrong values when the CHECKSUM_GEN_IP_INLINE is defined to one. If I do iphdr->_chksum += 0x7F8; the checksum is good.
Thanks in advance,
Juan

juan schiavoni <jjsch>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by billauerbach (Posted a comment)
  • -email is unavailable- added by jjsch (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-05-04 goldsimon Open/ClosedOpen Closed
    2010-05-04 goldsimon StatusNone Fixed
        Assigned toNone goldsimon

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code