cpu/stm32/periph/eth: Disable hardware checksums

[yarrick]

lwIP will fill them in already.

Having this enabled causes empty checksums to be sent: #19853

Contribution description

Testing procedure

Test pinging the stm32 board using both GNRC and lwIP running.

For lwip:

$ LWIP_IPV4=1 make -C tests/pkg/lwip flash

Not tested yet since I don't have the hardware.

Issues/PRs references

Tries to fix underlying issue in #19853

[yarrick]

@krzysztof-cabaj does this make your board work?

[yarrick]

The TX_DESC_STAT_CIC macro is not very well named, there should be three different macros instead for setting the non-disabled checksum modes.

[krzysztof-cabaj]

I tested this PR on nucleo-f207zg and it works!

However, I have some doubts if disabling hardware chsum calculation is the best idea. Please see most recent info at PR #19853.

[yarrick]

Every single network interface would have to support and use hardware checksums for the software to not fill it in. Since we know that lwIP does fill it there is no issue turning it off - calculating it again inverts the value and makes it zero as you have seen.

@maribu do you know why this hw checksum mode was chosen for the stm eth?

We could maybe switch to just doing IP checksums (and not for payload) in hardware, since that seems to not be broken when the checksum is already calculated by the stack.

[yarrick]

@miri64 Does gnrc have any optimizations for hardware which can compute checksums?

[maribu]

IMO we should just disable hardware checksuming altogether. There is no reason to compute them twice.

I think the radio HAL does provide a way to tell the network stack if hardware checksum computation is supported, but Ethernet won't fit that radio HAL well :) If the netdev API would be extended such that hardware checksum computation can be integrated into the network stack, we could re-enable it again.

@maribu do you know why this hw checksum mode was chosen for the stm eth?

No idea. Likely a leftover from before I refactored the driver. To be honest, I was unaware that hardware checksum computation is actually enabled. Likely, because of the non-obvious macro naming 😅 Feel free to piggy-back a fix for that, if you want.

[maribu]

Feel free to piggy pack something like

diff --git a/cpu/stm32/include/periph/cpu_eth.h b/cpu/stm32/include/periph/cpu_eth.h
index e3c1db6890..7ef4abc7ca 100644
--- a/cpu/stm32/include/periph/cpu_eth.h
+++ b/cpu/stm32/include/periph/cpu_eth.h
@@ -140,7 +140,10 @@ typedef struct eth_dma_desc {
  * | `0b10` | Calculate and insert IPv4 checksum, insert pre-calculated payload checksum    |
  * | `0b11  | Calculated and insert both IPv4 and payload checksum                          |
  */
-#define TX_DESC_STAT_CIC        (BIT22 | BIT23)
+#define TX_DESC_STAT_CIC                    (BIT22 | BIT23)
+#define TX_DESC_STAT_CIC_NO_HW_CHECKSUM     (0)             /**< Do not compute checksums in hardware */
+#define TX_DESC_STAT_CIC_HW_CHECKSUM_IPV4   (BIT22)         /**< Compute the IPv4 header checksum in hardware */
+#define TX_DESC_STAT_CIC_HW_CHECKSUM_BOTH   (BIT22 | BIT32) /**< Compute the IPv4 header and payload checksum in hardware */
 #define TX_DESC_STAT_TTSE       (BIT25) /**< If set, an PTP timestamp is added to the descriptor after TX completed */
 #define TX_DESC_STAT_FS         (BIT28) /**< If set, buffer contains first segment of frame to transmit */
 #define TX_DESC_STAT_LS         (BIT29) /**< If set, buffer contains last segment of frame to transmit */

If you do so, the ACK remains valid. If not, I could do so as a follow PR just as well.

[riot-ci]

Murdock results

✔️ PASSED

1986b5e cpu/stm32/periph/eth: Disable hardware checksums

Success	Failures	Total	Runtime
7937	0	7937	15m:32s

Artifacts

• Documentation preview

[maribu]

bors merge

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• Murdock
• static-tests
• tools-build-success

[yarrick]

Thanks for merging. If you update the header file, BIT23 was misspelled in your example. Also there is a mode for just BIT23, see https://github.com/STMicroelectronics/STM32CubeH7/blob/dd1b1d7144e6c61f995a368bbbeaad1936d60cd1/Drivers/STM32H7xx_HAL_Driver/Inc/Legacy/stm32h7xx_hal_eth_legacy.h#L607C1-L613C146

[krzysztof-cabaj]

I test this PR using nucleo-f429zi and it works perfect. There are some bigger problems with nucleo-f767zi - it did not successfully acquire address via DHCP - i see some queries but process is not finalized.