net: ethernet: Only try ARP for IP packets #84158
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pdgendt
previously approved these changes
Jan 17, 2025
rlubos
previously approved these changes
Jan 17, 2025
The work in Ethernet send in commit 2f10d7d ("net: ethernet: Set the ptype by the caller in send") introduced a bug because we could try to do ARP resolving for ARP packets too. This is clearly a wrong thing to do. So before trying to do ARP resolving, make sure the the packet type is IP packet. Signed-off-by: Jukka Rissanen <[email protected]>
Print more data / debug information for ARP messages. Also remove unnecessary "&" when printing IPv4 address. Signed-off-by: Jukka Rissanen <[email protected]>
It was incorrectly introduced earlier and not really needed because the test will pass without it. Signed-off-by: Jukka Rissanen <[email protected]>
The ARP code has set the protocol type of the packet to 0x806, so do not change it when preparing to send to 0x800 which is the IP protocol type. Lets trust the previously called functions to set the ptype correctly and do not set it here. Signed-off-by: Jukka Rissanen <[email protected]>
jukkar
force-pushed
the
fix/ethenet-ptype
branch
from
cbb4209 to
86048ad
Compare
|
pdgendt
previously approved these changes
Jan 20, 2025
rlubos
previously approved these changes
Jan 20, 2025
Trust that the protocol type is set correctly by functions called before this one. We should not set the protocol type blindly in this generic function. Signed-off-by: Jukka Rissanen <[email protected]>
The fragmented packet should inherit the protocol type of the original packet. Signed-off-by: Jukka Rissanen <[email protected]>
Remove the build assert from NET_L3_REGISTER() macro as that
is causing an issue with llvm. Add runtime check of the handler
pointer value.
subsys/net/l2/ethernet/arp.c:1044:1: error: static_assert expression
is not an integral constant expression
ETH_NET_L3_REGISTER(ARP, NET_ETH_PTYPE_ARP, arp_recv);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/zephyr/net/ethernet.h:1272:2: note: expanded from
macro 'ETH_NET_L3_REGISTER'
NET_L3_REGISTER(&NET_L2_GET_NAME(ETHERNET), name, ptype, handler)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/zephyr/net/net_core.h:190:2: note: expanded from
macro 'NET_L3_REGISTER'
BUILD_ASSERT((_handler) != NULL, "Handler is not defined")
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/zephyr/toolchain/gcc.h:87:51: note: expanded from
macro 'BUILD_ASSERT'
define BUILD_ASSERT(EXPR, MSG...) _Static_assert((EXPR), "" MSG)
^~~~~~
subsys/net/l2/ethernet/arp.c:1044:1: note: cast from 'void *' is not
allowed in a constant expression
include/zephyr/net/ethernet.h:1272:2: note: expanded from
macro 'ETH_NET_L3_REGISTER'
NET_L3_REGISTER(&NET_L2_GET_NAME(ETHERNET), name, ptype, handler)
^
include/zephyr/net/net_core.h:190:29: note: expanded from
macro 'NET_L3_REGISTER'
BUILD_ASSERT((_handler) != NULL, "Handler is not defined")
^
/usr/lib/llvm-14/lib/clang/14.0.0/include/stddef.h:89:16: note: expanded
from macro 'NULL'
define NULL ((void*)0)
Signed-off-by: Jukka Rissanen <[email protected]>
|
rlubos
approved these changes
Jan 21, 2025
clamattia
reviewed
Jan 21, 2025
| @@ -186,8 +186,7 @@ struct net_l3_register { | |||
| .handler = _handler, \ | |||
| .name = STRINGIFY(_name), \ | |||
| .l2 = _l2_type, \ | |||
| }; \ | |||
| BUILD_ASSERT((_handler) != NULL, "Handler is not defined") | |||
There was a problem hiding this comment.
Did you intentionally remove this?
There was a problem hiding this comment.
Yes, there was a compile error with llvm. I replaced it with runtime check.
There was a problem hiding this comment.
Kind of surprised this isn't an issue for other instances of BUILD_ASSERT?
|
ping @pdgendt, please take a look if you have time, there should be no more changes to this one. |
pdgendt
approved these changes
Jan 21, 2025
| @@ -186,8 +186,7 @@ struct net_l3_register { | |||
| .handler = _handler, \ | |||
| .name = STRINGIFY(_name), \ | |||
| .l2 = _l2_type, \ | |||
| }; \ | |||
| BUILD_ASSERT((_handler) != NULL, "Handler is not defined") | |||
There was a problem hiding this comment.
Kind of surprised this isn't an issue for other instances of BUILD_ASSERT?
Yes, the errors reported by llvm were weird (the output is in the commit msg). Also why this was not reported in the PR that introduced the build assert. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The work in Ethernet send in commit 2f10d7d ("net: ethernet: Set the ptype by the caller in send") introduced a bug because we could try to do ARP resolving for ARP packets too. This is clearly a wrong thing to do. So before trying to do ARP resolving, make sure the the packet type is IP packet.