Skip to content

Conversation

@WorldofJARcraft
Copy link
Contributor

The Xilinx AXI Ethernet subsystem is capable of RX/Tx checksum offloading. While it supports computing IP and UDP/TCP checksums, it does not support computing ICMP checksums and only computes IP checksums for ICMP messages. Thus, this patch adds an default-disabled option for always computing the ICMP checksum in software while relying on hardware to compute the remaining checksums.

Copy link
Contributor

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a driver capability, not a global setting. If multiple drivers are used, each should work properly.

@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch 2 times, most recently from 211c1a5 to 9f988f5 Compare June 10, 2024 14:05
@zephyrbot zephyrbot requested a review from decsny June 10, 2024 14:05
@rlubos
Copy link
Contributor

rlubos commented Jun 10, 2024

Please check compliance results, there are missing spaces etc.

@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch 2 times, most recently from f5a9774 to e6ee845 Compare June 10, 2024 15:57
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the compliance report.
Also the commit title could be updated, the changeset is no longer specific to ICMP.

@decsny
Copy link
Member

decsny commented Jun 11, 2024

This is convenient, I was planning to look into something like this after LTS because NXP ENET has the same limitation, it only doesn't accelerate ICMPv6 checksum

@WorldofJARcraft
Copy link
Contributor Author

WorldofJARcraft commented Jun 11, 2024

Please check the compliance report. Also the commit title could be updated, the changeset is no longer specific to ICMP.

Some of the complaints in the compliance report refer to code that I did not change. I could run the corresponding files through clang-format or manually fix the violations, but this would bloat the PR.
I have just pushed a new version that passes checkpatch locally:

git diff HEAD~1 HEAD | ./scripts/checkpatch.pl -

@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch from e6ee845 to 66d30ba Compare June 11, 2024 13:38
@WorldofJARcraft WorldofJARcraft changed the title subsys: net: Option to force compute ICMP checksum net: Support partial checksum offloading Jun 11, 2024
@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch 7 times, most recently from f1abf1e to 90402bf Compare June 11, 2024 14:25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Please align with the opening bracket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Btw., the return type was also incorrect, this is fixed as well.

@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch from 90402bf to 4eed806 Compare June 11, 2024 14:37
Comment on lines 2664 to 2665
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
NET_IF_CHECKSUM_IPV4_UDP = NET_IF_CHECKSUM_IPV4_HEADER_BIT|
NET_IF_CHECKSUM_UDP_BIT,
NET_IF_CHECKSUM_IPV4_UDP = NET_IF_CHECKSUM_IPV4_HEADER_BIT |
NET_IF_CHECKSUM_UDP_BIT,

etc, just reads better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rlubos
Copy link
Contributor

rlubos commented Jun 11, 2024

This is convenient, I was planning to look into something like this after LTS because NXP ENET has the same limitation, it only doesn't accelerate ICMPv6 checksum

@decsny Good to hear that - feel free to participate in the review, to make sure those changes fit your HW needs as well.

@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch from 4eed806 to ec6dcc7 Compare June 11, 2024 15:11
@WorldofJARcraft
Copy link
Contributor Author

This is convenient, I was planning to look into something like this after LTS because NXP ENET has the same limitation, it only doesn't accelerate ICMPv6 checksum

@decsny Good to hear that - feel free to participate in the review, to make sure those changes fit your HW needs as well.

I have a draft implementation for the Xilinx AXI Ethernet subsystem, which is where I initially came across this issue, here: #73986

@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch from ec6dcc7 to 39db211 Compare June 12, 2024 06:35
@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch from 39db211 to 0213c5a Compare June 12, 2024 08:36
tbursztyka
tbursztyka previously approved these changes Jun 12, 2024
jukkar
jukkar previously approved these changes Jun 12, 2024
The Xilinx AXI Ethernet subsystem is capable of RX/Tx checksum
offloading. While it supports computing IP and UDP/TCP checksums, it
does not support computing ICMP checksums and only computes IP checksums
for ICMP messages. Thus, this patch adds an additional configuration for
ethernet drivers that indicates for which protocols checksum offloading
is (to be) supported. This flag is then considered by the IP subsystem
in determining when flags need to be computed in software.

Signed-off-by: Eric Ackermann <[email protected]>
@WorldofJARcraft WorldofJARcraft dismissed stale reviews from jukkar and tbursztyka via 5114fd4 June 12, 2024 09:48
@WorldofJARcraft WorldofJARcraft force-pushed the kernel_add_option_to_force_icmp_checksum branch from 0213c5a to 5114fd4 Compare June 12, 2024 09:48
@decsny
Copy link
Member

decsny commented Jun 12, 2024

It feels kind of strange for this to come from the ethernet config, since this is not really a configuration but rather a hardware capability? why not come from the get_capabilities function?

decsny
decsny previously requested changes Jun 12, 2024
Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking until question is answered

@WorldofJARcraft
Copy link
Contributor Author

It feels kind of strange for this to come from the ethernet config, since this is not really a configuration but rather a hardware capability? why not come from the get_capabilities function?

The original motivation was to save bits in the existing hardware capabilities enum.
However, I think including checksum offloading in the configuration API actually makes more sense: For some devices, checksum offloading is a run-time configuration. For example, for the Xilinx AXI Ethernet, TX checksum offloading must be requested for each packet via the control stream / the "userapp" fields in the AXI DMA SG descriptor. If TX offloading is not requested, the NIC will gladly send a packet with no checksum. Integrating checksum offloading with the configuration interfaces makes it possible to enable and disable the feature and query whether it is currently active.

@jukkar jukkar requested a review from decsny June 14, 2024 08:45
@decsny decsny dismissed their stale review June 14, 2024 20:39

not going to block this but I will want to revisit this from the perspective of determining hardware supported features and not just software requests to enable/disable

@nashif nashif merged commit 4f72df4 into zephyrproject-rtos:main Jun 14, 2024
@github-actions
Copy link

Hi @WorldofJARcraft!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants