[U-Boot] ARM: net.c: UDP Checksum code failing every packet

[U-Boot] ARM: net.c: UDP Checksum code failing every packet

[Tom Evans]

UDP Checksumming is enabled with the configuration variable
CONFIG_UDP_CHECKSUM. This is only enabled in 7 out of 437
include/configs/*.h files. Enabling UDP checksumming can be useful, as 
it allows any errors (in downloading via TFTP) to be retried rather than 
resulting in an image CRC error when the whole download has finished. 
The download isn't meant to fail, but I'm seeing intermittent corrupted 
downloads currently.

If CONFIG_UDP_CHECKSUM is enabled on an ARM core, and the CPU is prior 
to ARM Core Version 6, all packets are reported as having checksum errors.

This is due to the ARM's 32-bit alignment requirements and the following 
four lines of code in net/net.c::NetReceive()

     xsum += (ntohl(ip->ip_src) >> 16) & 0x0000ffff;
     xsum += (ntohl(ip->ip_src) >>  0) & 0x0000ffff;
     xsum += (ntohl(ip->ip_dst) >> 16) & 0x0000ffff;
     xsum += (ntohl(ip->ip_dst) >>  0) & 0x0000ffff;

The original Ethernet packet is (usually, this is not controlled) 32-bit 
aligned, the Ethernet header is 14 bytes long, so by design the aligned 
fields in the IP (and other) headers are all misaligned, at least with 
the NE2000 driver we're using.

ARM (prior to V6) "silently corrupts" misaligned reads. For 32-bit reads 
offset by 16 bits it reads 32-bits from the aligned address two bytes 
below that requested, and then swaps the words.

If the above lines are changed to match other related ARM-related 
modifications as follows, then the UDP checksum code works:

     tmp = NetReadIP(&ip->ip_src);
     xsum += (ntohl(tmp) >> 16) & 0x0000ffff;
     xsum += (ntohl(tmp) >>  0) & 0x0000ffff;
     tmp = NetReadIP(&ip->ip_dst);
     xsum += (ntohl(tmp) >> 16) & 0x0000ffff;
     xsum += (ntohl(tmp) >>  0) & 0x0000ffff;

I'm afraid I can't generate a patch to do this. Could someone else 
please incorporate this change if required?

----

Tom Evans

[U-Boot] ARM: net.c: UDP Checksum code failing every packet

[Ben Warren]

Hi Tom,

Tom Evans wrote:
> UDP Checksumming is enabled with the configuration variable
> CONFIG_UDP_CHECKSUM. This is only enabled in 7 out of 437
> include/configs/*.h files. Enabling UDP checksumming can be useful, as 
> it allows any errors (in downloading via TFTP) to be retried rather than 
> resulting in an image CRC error when the whole download has finished. 
> The download isn't meant to fail, but I'm seeing intermittent corrupted 
> downloads currently.
>
> If CONFIG_UDP_CHECKSUM is enabled on an ARM core, and the CPU is prior 
> to ARM Core Version 6, all packets are reported as having checksum errors.
>
> This is due to the ARM's 32-bit alignment requirements and the following 
> four lines of code in net/net.c::NetReceive()
>
>      xsum += (ntohl(ip->ip_src) >> 16) & 0x0000ffff;
>      xsum += (ntohl(ip->ip_src) >>  0) & 0x0000ffff;
>      xsum += (ntohl(ip->ip_dst) >> 16) & 0x0000ffff;
>      xsum += (ntohl(ip->ip_dst) >>  0) & 0x0000ffff;
>
> The original Ethernet packet is (usually, this is not controlled) 32-bit 
> aligned, the Ethernet header is 14 bytes long, so by design the aligned 
> fields in the IP (and other) headers are all misaligned, at least with 
> the NE2000 driver we're using.
>
> ARM (prior to V6) "silently corrupts" misaligned reads. For 32-bit reads 
> offset by 16 bits it reads 32-bits from the aligned address two bytes 
> below that requested, and then swaps the words.
>
> If the above lines are changed to match other related ARM-related 
> modifications as follows, then the UDP checksum code works:
>
>      tmp = NetReadIP(&ip->ip_src);
>      xsum += (ntohl(tmp) >> 16) & 0x0000ffff;
>      xsum += (ntohl(tmp) >>  0) & 0x0000ffff;
>      tmp = NetReadIP(&ip->ip_dst);
>      xsum += (ntohl(tmp) >> 16) & 0x0000ffff;
>      xsum += (ntohl(tmp) >>  0) & 0x0000ffff;
>
> I'm afraid I can't generate a patch to do this. Could someone else 
> please incorporate this change if required?
>
>   
Everything seems logical until this point.  Why can't you create a patch?

regards,
Ben

[U-Boot] ARM: net.c: UDP Checksum code failing every packet

[Ben Warren]

Tom Evans wrote:
> Ben Warren wrote:
>> Hi Tom,
>>
>> Tom Evans wrote:
> >> ...
>>> I'm afraid I can't generate a patch to do this. Could someone else 
>>> please incorporate this change if required?
>>>
>>>   
>> Everything seems logical until this point.  Why can't you create a 
>> patch?
>
> Because I'm NOT working from a copy of the "official git archive".
>
> We've bought a package (u-boot, linux kernel and root filesystem) for 
> a CPU and board that isn't currently supported in u-boot. Our 
> suppliers aren't taking the time and effort required to feed the 
> changes back into the u-boot master copies, so I'm working from a 
> "non-branch" that is somewhat elderly.
>
> I've only just worked out how to fetch the current "master" using git, 
> just to see that this bug is still current.
>
> Besides which, I've never made a patch before, and couldn't find any 
> instructions on how to do so.
>
There is good documentation out there, just not where you looked.  
People are always glad to help in this area anyway.  There are a small 
number of steps:
1. $ git clone <repo>  You've done this already
2. <edit/test>
3. $ git commit -a -s (this assumes git knows your name and e-mail.  
That's easy to set up - the easiest is by editing the config file in the 
.git directory of your repository)
4. $ git format-patch origin
5. $ git send-email (preferred) or paste in e-mail

You may find this helpful.  I have:
http://www.kernel.org/pub/software/scm/git/docs/everyday.html

> Besides which, I'm not meant to be doing this (working on u-boot or 
> sending bug reports back) at all.
>
Well, but you are anyway, and we're thankful for that.  I'm sure it's 
pointless, but you have a pretty good defense that your employer is 
getting a lot of sophisticated software for free, and all that's asked 
for in return are the legalities of the GPL and the softer concept of 
helping out when and if you can.
> It is only six lines of code in one file.
>
True, but you found the bug and made the fix.  There's no way I'm going 
to post this patch without having the hardware to test it on.  Others 
may, but it's better if you do it.

regards,
Ben

[U-Boot] ARM: net.c: UDP Checksum code failing every packet

[Ben Warren]

Tom Evans wrote:
> Ben Warren wrote:
>> Tom Evans wrote:
>>> Ben Warren wrote:
>> number of steps:
>> 1. $ git clone <repo>  You've done this already
>> 2. <edit/test>
>
> That's the bit I can't do. The u-boot distribution DOESN'T support our 
> hardware. I can only test it on our "variant" version of u-boot.
>
Right, but as you said, this block of code hasn't changed.  Testing what 
you can, and posting a patch relative to top-of-tree is OK.
>> 3. $ git commit -a -s (this assumes git knows your name and e-mail.  
> > 4. $ git format-patch origin
>
> I could change it in my version (which I have), paste the changes 
> untested into the official one then generate patches, but it's an 
> untested change.
Yes and no.  The code block is tested.
>
> It can be tested by anyone working from the official distribution, 
> turning CONFIG_UDP_CHECKSUM on for their board and then running tftp.
>
> Which isn't me.
>
If you post a good patch and it's pulled in, somebody will test this 
configuration.
>> True, but you found the bug and made the fix.  There's
> > no way I'm going to post this patch without having the
> > hardware to test it on.  Others
>> may, but it's better if you do it.
>
> I don't have the hardware.
>
> I think it is worth sending a summary of our discussion back to the 
> list (excluding the "I'm not meant to be doing this" bit). Have you 
> sent anything yet?
Yes, my other response and this one are CC'd to the list, to be archived 
in perpetuity and looked at by an infinitesimally small number of people. :)

cheers,
Ben