STM32 MAC Checksum offload bug + PTP Topic is solved

[drizzi]

Hi,
I've been working with the STM32F4 MAC lately, and I have came up with a bug. ChibiOS 16.1.x.
When the checksum offload is enabled (IPCO=1, MACCR register bit 10), enhanced descriptor RDES4 must be used to check if the received frame checksum was OK, as the reference manual says (page 1185 of RM0090).

I have also PTP enabled the MAC: the code style is a little bit off the standard (some cut and paste from ST HAL), but the code works reliably.

I would be happy to provide the patches!

0001-Added-PTP-Fixed-Checksum-offload.patch.gz

Patch

(4.91 KiB) Downloaded 610 times

Ciao

[Giovanni]

bump

[steved]

I've been looking at this from the perspective of MAC Checksum offload. Couldn't apply the patch (seems to be git-specific), but looked through the relevant changes, and they make sense to me.
I've also tried the existing code with lwIP 2.0.2, and it appears to work fine without extended DMA descriptors, although I have no means of generating checksum errors.
While RM0090 clearly states that extended DMA descriptors must be enabled when offloading checksums, it doesn't appear to cause a problem in practice, and the definitions of the status flags suggest that the extended descriptors are not essential

This link suggests that maybe there are situations (such as fragmented packets) where the chacksum calculation should not be offloaded.

[steved]

Another small comment on the patch - it introduces a new define STM32_MAC_USE_ENHANCED_DMA_DESCRIPTORS. Personally I would prefer that to be automatically defined if STM32_MAC_ENABLE_PTP or STM32_MAC_IP_CHECKSUM_OFFLOAD is defined

And assuming MAC_USE_PTP is a 'capabilities' flag, I didn't come across an error message if MAC_USE_PTP is FALSE and STM32_MAC_ENABLE_PTP is defined.

[rasm]

I'm using the Olimex STM32-E407 board and when STM32_MAC_IP_CHECKSUM_OFFLOAD is enabled and the lwIP options CHECKSUM_GEN_IP, CHECKSUM_GEN_TCP, and CHECKSUM_GEN_UDP are disabled I can't even establish a TCP connection to the STM32-E407 board. Applying this patch (only the checksum offload part) solved it for me. Should I send a patch with only drizzi's checksum offload fix ?

[steved]

I found that there are a few more things to configure than are documented or obvious.

In mcuconf.h:

Code: Select all

#define STM32_MAC_IP_CHECKSUM_OFFLOAD       3

In lwipopts.h:

Code: Select all

#define CHECKSUM_GEN_ICMP               0
#define CHECKSUM_GEN_IP                 0
#define CHECKSUM_GEN_TCP                0
#define CHECKSUM_GEN_UDP                0
#define CHECKSUM_CHECK_ICMP             0
#define CHECKSUM_CHECK_IP               0
#define CHECKSUM_CHECK_TCP              0
#define CHECKSUM_CHECK_UDP              0

Without some of those, things did indeed lock up.

This is the configuration I am currently running - only on my development target ATM, but no noticeable problems.

Having said that, an update which conforms to the letter of the manual would be useful.

[As an aside, personally I prefer modified complete files rather than patches against a specific file version; I tend to do a file compare locally to look at the differences. I can see that patches can be advantageous when modifying a large codebase, but when it's a handful of files, less so, IMO.]

[rasm]

Thank you for the information steved. Setting STM32_MAC_IP_CHECKSUM_OFFLOAD to 3 does indeed fix the issue I was having, my bad for not reading the hal_mac_lld.h header. I sent several ICMP packets with invalid checksums to test both implementations (mainline, drizzi's) and both are able to detect invalid IP checksums when checksum offload is enabled. By reading the manual I believe that both implementations are correct:

- ChibiOS 16.1.x does not enable enhanced descriptors and therefore the bit IPHCE signals a checksum error.
- drizzi's patch enables enhanced descriptors because of PTP but in this scenario the IPHCE bit takes on the function of TSV (time stamp valid), only in this case RDES4 must be read to check for checksum errors.

With that said I don't think the mainline code has to be fixed until the PTP code is merged.

[steved]

Thank you for the information steved. Setting STM32_MAC_IP_CHECKSUM_OFFLOAD to 3 does indeed fix the issue I was having

Been there too!

I sent several ICMP packets with invalid checksums to test both implementations (mainline, drizzi's) and both are able to detect invalid IP checksums when checksum offload is enabled

Good that were able to test. As a matter of interest, how did you generate the faulty packets?

By reading the manual I believe that both implementations are correct:

- ChibiOS 16.1.x does not enable enhanced descriptors and therefore the bit IPHCE signals a checksum error.
- drizzi's patch enables enhanced descriptors because of PTP but in this scenario the IPHCE bit takes on the function of TSV (time stamp valid), only in this case RDES4 must be read to check for checksum errors.

That was the impression I got; good that someone else thinks the same.

With that said I don't think the mainline code has to be fixed until the PTP code is merged.

I agree

[rasm]

Good that were able to test. As a matter of interest, how did you generate the faulty packets?

I used the hping3 tool with the -b flag (git://git.debian.org/collab-maint/hping3.git)

[Giovanni]

bump