Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: ethernet: Set the ptype by the caller in send #83228

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

jukkar
Copy link
Member

@jukkar jukkar commented Dec 19, 2024

Instead of setting the upper protocol type in ethernet_send() by checking the protocol type bits, use the ptype that is already set by the caller. This allows new protocol types to be supported and makes the system extensible

@jukkar jukkar marked this pull request as draft December 19, 2024 15:18
@jukkar jukkar requested a review from rlubos December 19, 2024 15:18
Copy link
Contributor

@clamattia clamattia left a comment

Choose a reason for hiding this comment

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

Nice, thanks jukkar.

Maybe in a next step custom bit-flags like net_pkt_set_lldp and similar could be removed.

@@ -270,6 +270,9 @@ static inline struct net_pkt *arp_prepare(struct net_if *iface,
return NULL;
}

net_pkt_set_ll_proto_type(pkt, NET_ETH_PTYPE_ARP);
Copy link
Contributor

Choose a reason for hiding this comment

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

In ARP context, arp_gratuitous_send() needs an update too.

@jukkar
Copy link
Member Author

jukkar commented Dec 19, 2024

Maybe in a next step custom bit-flags like net_pkt_set_lldp and similar could be removed.

Yes, absolutely. There needs to be a cleanup before this is ready for merge. This is currently a draft so people could try it without it getting merged accidentally.

@jukkar jukkar force-pushed the devel/l2-send-refactoring branch from 80dbfd8 to ff75c8a Compare December 20, 2024 09:49
@jukkar
Copy link
Member Author

jukkar commented Dec 20, 2024

  • Fixed the gratuitous ARP ptype missing
  • Fixed various tests

@jukkar jukkar force-pushed the devel/l2-send-refactoring branch 2 times, most recently from d6c0a85 to 772623d Compare January 14, 2025 09:49
@jukkar jukkar marked this pull request as ready for review January 14, 2025 11:10
@jukkar
Copy link
Member Author

jukkar commented Jan 14, 2025

Maybe in a next step custom bit-flags like net_pkt_set_lldp and similar could be removed.

Yes, absolutely. There needs to be a cleanup before this is ready for merge. This is currently a draft so people could try it without it getting merged accidentally.

@clamattia I will do a separate PR for cleaning up fields in net_pkt that are not needed any more.

@jukkar jukkar requested review from clamattia and rlubos January 14, 2025 13:04
Copy link
Contributor

@clamattia clamattia left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for adjusting. As long as it is possible to use ethernet_send with some custom ptype set, I am happy. I do not understand some of the remaining special handling in said function.

Instead of setting the upper protocol type in ethernet_send()
by checking the protocol type bits, use the ptype that is already
set by the caller. This allows new protocol types to be supported
and makes the system extensible.

Signed-off-by: Jukka Rissanen <[email protected]>
Use the 192.0.2/24 address space reserved for documentation.
No functional changes by this commit.

Signed-off-by: Jukka Rissanen <[email protected]>
@kartben kartben force-pushed the devel/l2-send-refactoring branch from 772623d to a839d07 Compare January 16, 2025 09:39
@kartben kartben merged commit 71911eb into zephyrproject-rtos:main Jan 16, 2025
26 checks passed
@jukkar jukkar deleted the devel/l2-send-refactoring branch January 17, 2025 06:34
@jukkar
Copy link
Member Author

jukkar commented Jan 17, 2025

While testing things, I noticed some issues with this. For example ping does not work to the device. If I am not able to figure out the culprit soon, I will send a revert for this.

@jukkar
Copy link
Member Author

jukkar commented Jan 17, 2025

The ARP issue I encountered is fixed by #84158

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.

6 participants