Skip to content

Conversation

@JordanYates
Copy link
Contributor

@JordanYates JordanYates commented Apr 11, 2025

Transition net_mgmt event codes to be bit enumerations instead of integer enumerations. This is required in order for the standard usage (and documented behaviour) of ORing together events for net_mgmt_init_event_callback to work properly.

Since mgmt_run_slist_callbacks runs the event handler if ANY bits between the occuring command code and the registered event command bits match, ORing together integers results in event being delivered to handlers that never asked for them.

This requires shifting bits from the layer code to the command mask, since both WiFi and IPv6 both have more than 16 unique events.

NOTE: IPv6 actually has more than the 20 bits currently allocated, so I have left it out of the change for now. The options I see:

  1. Split some IPv6 events into a second layer
  2. Expand the event mask into 64 bits

As a concrete example of what this change is fixing, on current main, running this function:

net_mgmt_init_event_callback(&cb, wifi_mgmt_event_handler, NET_EVENT_WIFI_CMD_AP_STA_CONNECTED);

Will actually result in wifi_mgmt_event_handler being called for all of the following events:

NET_EVENT_WIFI_CMD_SCAN_RESULT
NET_EVENT_WIFI_CMD_SCAN_DONE
NET_EVENT_WIFI_CMD_CONNECT_RESULT
NET_EVENT_WIFI_CMD_DISCONNECT_RESULT
NET_EVENT_WIFI_CMD_IFACE_STATUS
NET_EVENT_WIFI_CMD_TWT
NET_EVENT_WIFI_CMD_TWT_SLEEP_STATE
NET_EVENT_WIFI_CMD_RAW_SCAN_RESULT
NET_EVENT_WIFI_CMD_DISCONNECT_COMPLETE
NET_EVENT_WIFI_CMD_SIGNAL_CHANGE
NET_EVENT_WIFI_CMD_NEIGHBOR_REP_RECEIVED
NET_EVENT_WIFI_CMD_NEIGHBOR_REP_COMPLETE
NET_EVENT_WIFI_CMD_AP_ENABLE_RESULT
NET_EVENT_WIFI_CMD_AP_DISABLE_RESULT
NET_EVENT_WIFI_CMD_AP_STA_CONNECTED

Since NET_EVENT_WIFI_CMD_AP_STA_CONNECTED (15) shares bits with all of the above.

Compress the network management layer codes down to a sequential range
starting at 1.

Signed-off-by: Jordan Yates <jordan@embeint.com>
Shift 8 bits assigned to the layer code to the command mask instead.
Leaving 7 bits (128 options) for the layer mask is plenty of space
considering that there are less than 16 layers defined in-tree.

On the other hand, the Wi-Fi management layer already has more than 16
unique events that can be generated. Allowing each event to be assigned
a bit requires more than the 16 bits currently available.

Signed-off-by: Jordan Yates <jordan@embeint.com>
@pdgendt
Copy link
Contributor

pdgendt commented Apr 11, 2025

This needs a migration note as this is a public facing API.

@JordanYates
Copy link
Contributor Author

This needs a migration note as this is a public facing API.

Of course, I am waiting to check there is consensus and CI doesn't throw up roadblocks first

Transition net_mgmt event codes to be bit enumerations instead of
integer enumerations. This is required in order for the standard
usage (and documented behaviour) of ORing together events for
`net_mgmt_init_event_callback` to work properly.

Since `mgmt_run_slist_callbacks` runs the event handler if *ANY* bits
between the occuring command code and the registered event command bits
match, ORing together integers results in event being delivered to
handlers that never asked for them.

Signed-off-by: Jordan Yates <jordan@embeint.com>
@JordanYates JordanYates force-pushed the 250411_net_mgmt_bit branch from ba7936f to e42db1b Compare April 11, 2025 12:42
@JordanYates
Copy link
Contributor Author

This needs a migration note as this is a public facing API.

More importantly, there needs to be a solution for the IPv6 events as well if we are fixing this, which might affect the migration notes.

@rlubos
Copy link
Contributor

rlubos commented Apr 11, 2025

Expand the event mask into 64 bits

If we want to fix this long standing awkwardness of the net_mgmt subsys, I'd be rather leaning towards this option, let's do it once and right. Yes, this will be pretty painful API change (every net_mgmt handler would need to be aligned), however,

  1. I don't see a good way to split IPv6 layer that wouldn't look artificial,
  2. Some other layers are already getting dangerously close to the current limit.

@jukkar
Copy link
Member

jukkar commented Apr 11, 2025

Hold on gents. I think we cannot change the event numbering to be bits as we will run out of bits eventually even if allocating 64 bits for these. For the layer code, we could probably use some sanitation. I am not sure why those were "random" values. As the layers need to be unique, not sure how we can guarantee that there is no conflict, so should we have a registry of those values?

@JordanYates
Copy link
Contributor Author

Hold on gents. I think we cannot change the event numbering to be bits as we will run out of bits eventually even if allocating 64 bits for these. For the layer code, we could probably use some sanitation. I am not sure why those were "random" values. As the layers need to be unique, not sure how we can guarantee that there is no conflict, so should we have a registry of those values?

If splitting IPv6 is a bad idea and 64 bits may not be enough in the future, the only remaining solution I see is changing the semantics of net_mgmt_init_event_callback so that you can only subscribe at the layer level (e.g. _NET_WIFI_BASE). This would let us leave the event definitions as they are, and rely on the user callbacks to filter out the desired sub-events.

The current behaviour is nonsensical.

@JordanYates
Copy link
Contributor Author

The problem with subscribing at the base level and leaving the event defines as integers is that several current API functions are not possible to implement properly (and are therefore currently broken).

e.g. net_mgmt_event_wait cannot currently work

@JordanYates
Copy link
Contributor Author

I have created a dedicated issue discussing the problem and possible solutions: #88534

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jun 12, 2025
@github-actions github-actions bot closed this Jun 26, 2025
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.

4 participants