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

Submitted by:  juan schiavoni <jjsch>
Submitted on:  Mon 03 May 2010 11:22:50 PM UTC  
 
Category: IPv4Severity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 04 May 2010 07:00:13 PM UTCgoldsimonOpen/ClosedOpen=>Closed
    Tue 04 May 2010 07:00:01 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon

    Back to the top


    Powered by Savane 3.1-cleanup1