-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Openthread border router fix socket close #99495
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
Openthread border router fix socket close #99495
Conversation
modules/openthread/platform/trel.c
Outdated
| if (!net_mgmt(NET_REQUEST_WIFI_IFACE_STATUS, ail_iface_ptr, | ||
| &status, sizeof(status))) { | ||
| if (status.state == WIFI_STATE_COMPLETED) { | ||
| (void)trel_plat_init(ot_instance_ptr, ail_iface_ptr); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this make a hardcoded assumption that the backbone link is Wi-Fi? Please correct me if I'm wrong, but I think this module should also work with other interface types like Ethernet?
A generic way to check if the interface is ready to transmit data would be to check net_if_is_up() - for Wi-Fi interfaces, this should only return true if the interface is connected to a Wi-Fi network.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTBR is planned to support both interfaces, Wi-FI or Ethernet, you are right. It would be a better idea to stick with your approach.
2523147 to
7311d26
Compare
7311d26 to
6de7a35
Compare
|
@Cristib05 you need to rebase to latest main, the network APIs were name spaced by #99169 and there is now conflict with this PR. |
6de7a35 to
66ff636
Compare
66ff636 to
aaa4f94
Compare
| entry = net_route_mcast_add(ail_iface_ptr, &addr, 16); | ||
| if (entry != NULL) { | ||
| mcast_addr = net_if_ipv6_maddr_add(ail_iface_ptr, | ||
| (const struct in6_addr *)&addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typecast here should also be updated.
| net_if_ipv6_maddr_join(ail_iface_ptr, mcast_addr); | ||
| net_if_ipv6_maddr_leave(ail_iface_ptr, mcast_addr); | ||
| net_if_ipv6_maddr_rm(ail_iface_ptr, | ||
| (const struct in6_addr *)&addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
aaa4f94 to
452c5be
Compare
| net_ipv6_addr_create(&mreq_v6.ipv6mr_multiaddr, 0xff02, 0, 0, 0, 0, 0, 0, 0x00fb); | ||
| VerifyOrExit(zsock_setsockopt(mdns_sock_v6, NET_IPPROTO_IPV6, ZSOCK_IPV6_ADD_MEMBERSHIP, | ||
| &mreq_v6, sizeof(mreq_v6)) == 0, | ||
| mcast_join_ret = zsock_setsockopt(mdns_sock_v6, IPPROTO_IPV6, IPV6_ADD_MEMBERSHIP, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also these needs to be changed
IPPROTO_IPV6 -> NET_IPPROTO_IPV6
IPV6_ADD_MEMBERSHIP -> ZSOCK_IPV6_ADD_MEMBERSHIP
| VerifyOrExit(zsock_setsockopt(mdns_sock_v4, NET_IPPROTO_IP, ZSOCK_IP_ADD_MEMBERSHIP, | ||
| &mreq_v4, sizeof(mreq_v4)) == 0, | ||
| mreq_v4.imr_multiaddr.s_addr = htonl(0xE00000FB); | ||
| mcast_join_ret = zsock_setsockopt(mdns_sock_v4, IPPROTO_IP, IP_ADD_MEMBERSHIP, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar issue here
|
I will investigate why the CI did not fail because some symbols were not converted, probably openthread was not part of |
| mreq_v4.imr_multiaddr.s_addr = net_htonl(0xE00000FB); | ||
| VerifyOrExit(zsock_setsockopt(mdns_sock_v4, NET_IPPROTO_IP, ZSOCK_IP_ADD_MEMBERSHIP, | ||
| &mreq_v4, sizeof(mreq_v4)) == 0, | ||
| mreq_v4.imr_multiaddr.s_addr = htonl(0xE00000FB); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also htonl() -> net_htonl()
452c5be to
082b945
Compare
Rebased and added changes, but having tests enabled would definitely ease the process. Hopefully I haven't missed much this time |
|
If needed, you can try things by setting |
082b945 to
4744add
Compare
OpenThread interface is initialized beform Wi-Fi interface. `otPlatTrelEnable` is called by OpenThread stack when it's interface is being initialized. Given this scenario, socket operation, like `bind`, will fail. There is also no mean to get a valid pointer to backbone interface. This is why, `trel_plat_init` was declared and called when backbone interface reported connectivity. This commit handles the `ot trel disable/ot trel enable` scenario that can be initialized from CLI. `otPlatTrelEnable` will be called, but `trel_plat_init` will not be called anymore, leaving trel socket without any options set, and `net_socket_service_register` is not called anymore, to handle incoming traffic. Now, when `otPlatTrelEnable` is called, it will verify if Wi-Fi interface is connected and will call `trel_plat_init` if needed. Signed-off-by: Cristian Bulacu <cristian.bulacu@nxp.com>
In this commit, `net_socket_service_register` is called when a platform module that has sockets is deinitialized, and it's socket/sockets are closed. This commit also handles a case when a module tries to join a multicast group and a subscription, from another module, is already present. Also, covered network namespace changes done in zephyrproject-rtos#99169 Signed-off-by: Cristian Bulacu <cristian.bulacu@nxp.com>
This commit deinitializes platform modules when external network interface is brought down. It also delets the multicast routes that are added for OpenThread interface. Signed-off-by: Cristian Bulacu <cristian.bulacu@nxp.com>
4744add to
4c2b107
Compare
|
|
Rebased to include #99593 |
| struct net_sockaddr_in6 dest_sock_addr = {.sin6_family = NET_AF_INET6, | ||
| .sin6_port = net_htons(DHCPV6_SERVER_PORT), | ||
| .sin6_addr = NET_IN6ADDR_ANY_INIT, | ||
| .sin6_scope_id = 0}; | ||
| .sin6_port = net_htons(DHCPV6_SERVER_PORT), | ||
| .sin6_addr = NET_IN6ADDR_ANY_INIT, | ||
| .sin6_scope_id = 0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking here, if you'd add a trailing comma to the last item, it would be formatted like:
struct net_sockaddr_in6 dest_sock_addr = {
.sin6_family = NET_AF_INET6,
.sin6_port = net_htons(DHCPV6_SERVER_PORT),
.sin6_addr = NET_IN6ADDR_ANY_INIT,
.sin6_scope_id = 0,
};Which is a best nicer IMO.
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, single empty line is sufficient



Main goal of this PR is to handle a WI-Fi connect/disconnect or link lost and link re-established scenario.
OpenThread's platform code has been enhanced to handle those events.
Border Router application code has been updated to de-initialize platform modules and remove its multicast routes.
TREL code has been updated to accommodate user enable/disable interaction from CLI.