search

FreeRTOS / FreeRTOS-Plus-TCP

FreeRTOS-Plus-TCP library repository. +TCP files only. Submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
MIT License
152 stars 163 forks source link

Set IP Header Checksum and Protocol Header Checksum to zero in case of EMAC peripheral Checksum calculation #1085

Closed microcris closed 9 months ago

microcris commented 9 months ago

Description

In case of "ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM = 1", it is needed to initialize the IP Header Checksum and the Protocol Header Checksum with zero. I'm using the NXP1060 NetworkInterface and it fails to (automatically) calculate the correct IP Header Checksum if it is not nulled beforehand.

Test Steps

For NXP1060 NetworkInterface 1 - Enable IP header checksum and Protocol checksum insertion config.txAccelerConfig = kENET_TxAccelIpCheckEnabled | kENET_TxAccelProtoCheckEnabled; config.macSpecialConfig &= ~(kENET_ControlStoreAndFwdDisable); 2 - #define ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM 1 3 - Run the code and let the DHCP client to get an address 4 - ping the given address and see the it failing to ping the address. 5 - "sniff" some packets with wireshark. In the ping response packet, the IP Header Checksum is set to 0xFFFF 6 - set pxIPHeader->usHeaderChecksum = 0x00U in prvProcessICMPEchoRequest 7 - Run the code and let the DHCP client to get an address 8 - ping the given address and see the ping response arriving correctly 9 - "sniff" some packets with wireshark. In the ping response packet, the IP Header Checksum is calculated correctly

Checklist:

Related Issue

https://forums.freertos.org/t/tx-checksum-offloading-and-icmp/11161

In this case only the protocol checksum was addressed

The NXP1060 has a separated configuration for Automatic IP Header Checksum and Protocol Header Checksum calculation

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

paulbartell commented 9 months ago

/bot run formatting

paulbartell commented 9 months ago

@microcris Thanks for the PR. I will get it over to one of the FreeRTOS+TCP maintainers for review.

htibosch commented 9 months ago

I think that this new code should be located in the porting layer: the driver.

Up until now I have never seen an EMAC that needs the UDP checksum be zero in order to calculate it correctly. And therefor, I would prefer to see this code in the NXP1060 driver.

Here two examples of code that clears the protocol checksum in the driver:

In STM32Fx, the ICMP checksum must be cleared.

In Zynq the ICMP checksums are calculated manually

And there are more variations. Some EMAC's are perfectly able to "offload" any type of checksum without clearing any field.

microcris commented 9 months ago

My "way of thinking" was something like: -let me check where ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM flag is being "verified" and if a checksum was (when the flag is false) being calculated I'm going to clear (when the flag is true) that checksum variable.

But I agree that the ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM flag should only enable or disable the FreeRTOS checksum calculation mechanism. Specific driver stuff that needs to be handled when the ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM flag is true must go to the driver file.

Regarding the NXP1060 ENET, we have this:

ENETx_TACC field descriptions
Field 4 PROCHK:
Enables insertion of protocol checksum.
0 - Checksum not inserted.
1 - If an IP frame with a known protocol is transmitted, the checksum is inserted automatically into the
frame. The checksum field must be cleared. The other frames are not modified.

Field 3 IPCHK:
Enables insertion of IP header checksum.
0 - Checksum is not inserted.
1 - If an IP frame is transmitted, the checksum is inserted automatically. The IP header checksum field
must be cleared. If a non-IP frame is transmitted the frame is not modified.
htibosch commented 9 months ago

@microcris wrote:

But I agree that the ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM flag should only enable or disable the FreeRTOS checksum calculation mechanism. Specific driver stuff that needs to be handled when the ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM flag is true must go to the driver file.

And the reason is to keep the core clean. Do not add/execute code just because one out of 20 drivers need that particular action. Already in the past we have move checksum code from the IP-task to the drivers.

In most cases when checksum offloading is available, ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM will be disabled. And if there are shortcomings in the EMAC, the corrections will be made in the driver.

I think that my proposed change is not too difficult. I showed some example in this PR.

Anyway, thanks a lot for your PR!

microcris commented 9 months ago

Ok, I'm going to do what needs to be done in the driver side. Thank you for the comments/guidance :)