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

sys: make auto init default with DHCPv6 client #21178

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

OlegHahm
Copy link
Member

Contribution description

The DHCPv6 client uses an event queue that needs to get initialized. The auto_init code does that and, hence, should be used by default.

Testing procedure

Build the gnrc_networking example with DHCPv6 client support:

USEMODULE=gnrc_dhcpv6_client make -C examples/gnrc_networking all term

Result

2025-01-30 23:50:21,735 # All up, running the shell now
> 2025-01-30 23:50:22,631 # sys/event/event.c:39 => *** RIOT kernel panic:
2025-01-30 23:50:22,632 # FAILED ASSERTION.
2025-01-30 23:50:22,632 # 
2025-01-30 23:50:22,633 # 	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
2025-01-30 23:50:22,634 # 	 - | isr_stack            | -        - |   - |   8192 (   -1) ( 8193) |  0x80898e0 |  0x80898e0
2025-01-30 23:50:22,634 # 	 1 | idle                 | pending  Q |  15 |   8192 (  436) ( 7756) |  0x80842e0 |  0x808613c 
2025-01-30 23:50:22,635 # 	 2 | main                 | running  Q |   7 |  12288 ( 1664) (10624) |  0x80862e0 |  0x808913c 
2025-01-30 23:50:22,635 # 	 3 | pktdump              | bl rx    _ |   6 |  12224 (  896) (11328) |  0x8091f80 |  0x8094d9c 
2025-01-30 23:50:22,636 # 	 4 | ipv6                 | bl rx    _ |   4 |   8128 ( 1776) ( 6352) |  0x808c1c0 |  0x808dfdc 
2025-01-30 23:50:22,636 # 	 5 | udp                  | bl rx    _ |   5 |   4032 (  960) ( 3072) |  0x8097220 |  0x809803c 
2025-01-30 23:50:22,637 # 	 6 | gnrc_netdev_tap      | bl anyfl _ |   2 |   8064 ( 2264) ( 5800) |  0x808e760 |  0x809053c 
2025-01-30 23:50:22,637 # 	 7 | RPL                  | bl rx    _ |   5 |   8192 (  896) ( 7296) |  0x80951c0 |  0x809701c 
2025-01-30 23:50:22,637 # 	   | SUM                  |            |     |  69312 ( 8892) (60420)
2025-01-30 23:50:22,638 # 
2025-01-30 23:50:22,638 # *** halted.

Expected result
DHCP client works, RIOT doesn't crash.

@github-actions github-actions bot added the Area: sys Area: System label Jan 30, 2025
@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Jan 30, 2025
@OlegHahm OlegHahm enabled auto-merge February 2, 2025 17:19
@OlegHahm OlegHahm added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: ready for merge train 🚃 PR is ready to be merged and awaiting the next merge train labels Feb 3, 2025
@riot-ci
Copy link

riot-ci commented Feb 3, 2025

Murdock results

✔️ PASSED

9201073 sys: net: gnrc: set simple_pd specific dhcpv6 hook

Success Failures Total Runtime
10271 0 10271 10m:40s

Artifacts

@miri64
Copy link
Member

miri64 commented Feb 3, 2025

tests/net/gnrc_dhcpv6_client_6lbr needs a DISABLE_MODULE for this test, I think.

@benpicco
Copy link
Contributor

benpicco commented Feb 4, 2025

examples/gnrc_border_router too if you set UPLINK=ethernet - which makes me wonder if this is the right thing to do.
We also don't default to including auto_init_gnrc_netif if netdev_default is used.

@miri64
Copy link
Member

miri64 commented Feb 4, 2025

examples/gnrc_border_router too if you set UPLINK=ethernet

Huh? Why wouldn't you want the DHCPv6 client auto-initialized with Ethernet?

@benpicco
Copy link
Contributor

benpicco commented Feb 4, 2025

Why wouldn't you want the DHCPv6 client auto-initialized with Ethernet?

Because gnrc_dhcpv6_client_simple_pd already takes care of it

/home/benpicco/dev/RIOT/sys/net/gnrc/application_layer/dhcpv6/client_simple_pd.c:28:2: error: #error "Module `gnrc_dhcpv6_client_simple_pd` is mutually exclusive to `auto_init_dhcpv6_client`"
   28 | #error "Module `gnrc_dhcpv6_client_simple_pd` is mutually exclusive to \
      |  ^~~~~
make[4]: *** [/home/benpicco/dev/RIOT/Makefile.base:149: /home/benpicco/dev/RIOT/examples/gnrc_border_router/bin/native/gnrc_dhcpv6/client_simple_pd.o] Error 1
make[3]: *** [/home/benpicco/dev/RIOT/Makefile.base:31: ALL--/home/benpicco/dev/RIOT/sys/net/gnrc/application_layer/dhcpv6] Error 2

Maybe add gnrc_dhcpv6_client_simple_ia ?

@OlegHahm
Copy link
Member Author

OlegHahm commented Feb 4, 2025

examples/gnrc_border_router too if you set UPLINK=ethernet - which makes me wonder if this is the right thing to do. We also don't default to including auto_init_gnrc_netif if netdev_default is used.

Because you can use netdev without gnrc.

Isn't it the default for modules any more that the corresponding auto_init is enabled?

@OlegHahm
Copy link
Member Author

OlegHahm commented Feb 4, 2025

I wonder why this error is raised in the C file instead of modelling this check directly in the auto_init().

@OlegHahm
Copy link
Member Author

OlegHahm commented Feb 4, 2025

Wouln't it make sense to consolidate the auto_init code from dhcp6_client with the simple_pd code? There seems to be quite some duplicated code here.

@miri64
Copy link
Member

miri64 commented Feb 4, 2025

Wouln't it make sense to consolidate the auto_init code from dhcp6_client with the simple_pd code? There seems to be quite some duplicated code here.

They do mainly different configuration, IIRC.

@OlegHahm
Copy link
Member Author

OlegHahm commented Feb 4, 2025

I'll come up with a consolidation proposal.

@OlegHahm
Copy link
Member Author

OlegHahm commented Feb 4, 2025

I moved the former simple_pd init code into the regular auto_init code.

@OlegHahm OlegHahm force-pushed the pr/bug/dhcp6_client_auto_init branch from 94fef5b to e89d3d9 Compare February 4, 2025 20:10
@OlegHahm OlegHahm added this pull request to the merge queue Feb 4, 2025
@OlegHahm OlegHahm removed this pull request from the merge queue due to a manual request Feb 4, 2025
The DHCPv6 client uses an event queue that needs to get initialized. The
auto_init code does that and, hence, should be used by default.
Allows to set and execute a hook during auto_init of the DHCPv6 client.
Sets a hook to configure the DHCPv6 client for simple PD.
@OlegHahm OlegHahm force-pushed the pr/bug/dhcp6_client_auto_init branch from e89d3d9 to 9201073 Compare February 5, 2025 23:22
@OlegHahm
Copy link
Member Author

OlegHahm commented Feb 5, 2025

I reverted my previous idea because it was leaking gnrc code into the generic DHCPv6 client code. This new proposal now solves this via a generic hook that can be set during auto_init and is used by the gnrc simple_pd module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: ready for merge train 🚃 PR is ready to be merged and awaiting the next merge train Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants